Fix scrollTo for destination START and END and add regression testing. (#11707)

- Initial implementation erroneously assumed that
ScrollDestination.START would only be used for scrolling up and
ScrollDestination.END for scrolling down. That's obviously not what they
are for, otherwise everyone would be using ScrollDestination.ANY.
- Moved actual scrolling to within the helper method that originally
only calculated the new scroll position. Parent method became too long
otherwise.

Fixes #11706

client/src/main/java/com/vaadin/client/widgets/Escalator.java

@@ -5102,14 +5102,18 @@ public class Escalator extends Widget
             case START:
                 // target row at the top of the viewport, include buffer
                 // row if there is room for one
-                logicalTargetIndex = Math
+                newTopRowLogicalIndex = Math
                         .max(firstVisibleIndexIfScrollingUp - 1, 0);
-                newTopRowLogicalIndex = logicalTargetIndex;
+                if (getVisibleRowRange().contains(newTopRowLogicalIndex)) {
+                    logicalTargetIndex = oldTopRowLogicalIndex
+                            + visualRangeLength;
+                } else {
+                    logicalTargetIndex = newTopRowLogicalIndex;
+                }
                 break;
             default:
-                throw new IllegalArgumentException(
-                        "Internal: Unsupported ScrollDestination: "
-                                + destination.name());
+                String msg = "Internal: Unsupported ScrollDestination: ";
+                throw new IllegalArgumentException(msg + destination.name());
             }
 
             // adjust visual range if necessary
@@ -5125,12 +5129,8 @@ public class Escalator extends Widget
             boolean rowsWereMoved = newTopRowLogicalIndex != oldTopRowLogicalIndex;
 
             // update scroll position if necessary
-            double scrollTop = calculateScrollPositionForScrollToRowSpacerOrBoth(
-                    targetRowIndex, destination, padding, scrollType);
-            if (scrollTop != getScrollTop()) {
-                setScrollTop(scrollTop);
-                setBodyScrollPosition(tBodyScrollLeft, scrollTop);
-            }
+            adjustScrollPositionForScrollToRowSpacerOrBoth(targetRowIndex,
+                    destination, padding, scrollType);
 
             if (rowsWereMoved) {
                 fireRowVisibilityChangeEvent();
@@ -5217,7 +5217,7 @@ public class Escalator extends Widget
         }
 
         /**
-         * Calculates scroll position for
+         * Adjusts scroll position for
          * {@link #scrollToRowSpacerOrBoth(int, ScrollDestination, double, boolean, boolean)},
          * reuse at your own peril.
          *
@@ -5225,9 +5225,8 @@ public class Escalator extends Widget
          * @param destination
          * @param padding
          * @param scrollType
-         * @return expected scroll position
          */
-        private double calculateScrollPositionForScrollToRowSpacerOrBoth(
+        private void adjustScrollPositionForScrollToRowSpacerOrBoth(
                 int targetRowIndex, ScrollDestination destination,
                 double padding, ScrollType scrollType) {
             /*
@@ -5281,10 +5280,7 @@ public class Escalator extends Widget
                             - sectionHeight + padding;
                     // ensure that we don't overshoot beyond bottom
                     scrollTop = Math.min(scrollTop,
-                            getRowTop(getRowCount() - 1) + getDefaultRowHeight()
-                                    + spacerContainer
-                                            .getSpacerHeight(getRowCount() - 1)
-                                    - sectionHeight);
+                            getRowTop(getRowCount()) - sectionHeight);
                     // if padding is set we want to overshoot or undershoot,
                     // otherwise make sure the top of the row or spacer is
                     // in view
@@ -5304,26 +5300,15 @@ public class Escalator extends Widget
             case END:
                 if (ScrollType.ROW.equals(scrollType)
                         && rowTop + getDefaultRowHeight()
-                                + padding > getScrollTop() + sectionHeight) {
-                    // within visual range but end of row below the viewport
-                    // or not enough padding, shift a little
+                                + padding != getScrollTop() + sectionHeight) {
+                    // row should be at the bottom of the viewport
                     scrollTop = rowTop + getDefaultRowHeight() - sectionHeight
                             + padding;
-                    // ensure that we don't overshoot beyond bottom
-                    scrollTop = Math.min(scrollTop,
-                            getRowTop(getRowCount() - 1) + getDefaultRowHeight()
-                                    + spacerContainer
-                                            .getSpacerHeight(getRowCount() - 1)
-                                    - sectionHeight);
                 } else if (rowTop + getDefaultRowHeight() + spacerHeight
-                        + padding > getScrollTop() + sectionHeight) {
-                    // within visual range but end of spacer below the viewport
-                    // or not enough padding, shift a little
+                        + padding != getScrollTop() + sectionHeight) {
+                    // spacer should be at the bottom of the viewport
                     scrollTop = rowTop + getDefaultRowHeight() + spacerHeight
                             - sectionHeight + padding;
-                    // ensure that we don't overshoot beyond bottom
-                    scrollTop = Math.min(scrollTop,
-                            getRowTop(getRowCount()) - sectionHeight);
                 } else {
                     // we are fine where we are
                     scrollTop = getScrollTop();
@@ -5344,26 +5329,16 @@ public class Escalator extends Widget
                             + (spacerHeight / 2.0);
                 }
                 scrollTop = center - Math.ceil(sectionHeight / 2.0);
-                // ensure that we don't overshoot beyond bottom
-                scrollTop = Math.min(scrollTop,
-                        getRowTop(getRowCount() - 1) + getDefaultRowHeight()
-                                + spacerContainer
-                                        .getSpacerHeight(getRowCount() - 1)
-                                - sectionHeight);
-                // ensure that we don't overshoot beyond top
-                scrollTop = Math.max(0, scrollTop);
                 break;
             case START:
                 if (!ScrollType.SPACER.equals(scrollType)
-                        && Math.max(rowTop - padding, 0) < getScrollTop()) {
-                    // row top above the viewport or not enough padding, shift a
-                    // little
+                        && Math.max(rowTop - padding, 0) != getScrollTop()) {
+                    // row should be at the top of the viewport
                     scrollTop = Math.max(rowTop - padding, 0);
                 } else if (ScrollType.SPACER.equals(scrollType)
                         && Math.max(rowTop + getDefaultRowHeight() - padding,
-                                0) < getScrollTop()) {
-                    // spacer top above the viewport or not enough padding,
-                    // shift a little
+                                0) != getScrollTop()) {
+                    // spacer should be at the top of the viewport
                     scrollTop = Math
                             .max(rowTop + getDefaultRowHeight() - padding, 0);
                 } else {
@@ -5373,7 +5348,16 @@ public class Escalator extends Widget
             default:
                 scrollTop = getScrollTop();
             }
-            return scrollTop;
+            // ensure that we don't overshoot beyond bottom
+            scrollTop = Math.min(scrollTop,
+                    getRowTop(getRowCount()) - sectionHeight);
+            // ensure that we don't overshoot beyond top
+            scrollTop = Math.max(0, scrollTop);
+
+            if (scrollTop != getScrollTop()) {
+                setScrollTop(scrollTop);
+                setBodyScrollPosition(tBodyScrollLeft, scrollTop);
+            }
         }
 
         @Override

uitest/src/main/java/com/vaadin/tests/components/grid/GridScrollDestination.java

@@ -0,0 +1,33 @@
+package com.vaadin.tests.components.grid;
+
+import java.util.Arrays;
+import java.util.stream.IntStream;
+
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.shared.ui.grid.ScrollDestination;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Grid;
+import com.vaadin.ui.NativeSelect;
+import com.vaadin.ui.TextField;
+
+public class GridScrollDestination extends AbstractTestUI {
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        Grid<Integer> grid = new Grid<>();
+        grid.addColumn(Integer::intValue).setCaption("number");
+        grid.setItems(IntStream.range(0, 100).boxed());
+        TextField tf = new TextField("row index", "50");
+        NativeSelect<ScrollDestination> ns = new NativeSelect<>(
+                "scroll destination",
+                Arrays.asList(ScrollDestination.values()));
+        ns.setValue(ScrollDestination.ANY);
+        Button button = new Button("Scroll to above row index", (event) -> {
+            int rowIndex = Integer.parseInt(tf.getValue());
+            grid.scrollTo(rowIndex, ns.getValue());
+        });
+        addComponents(tf, ns, button, grid);
+    }
+
+}

uitest/src/test/java/com/vaadin/tests/components/grid/GridScrollDestinationTest.java

@@ -0,0 +1,291 @@
+package com.vaadin.tests.components.grid;
+
+import static org.hamcrest.number.IsCloseTo.closeTo;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
+
+import java.util.List;
+
+import org.junit.Test;
+import org.openqa.selenium.WebElement;
+
+import com.vaadin.shared.ui.grid.ScrollDestination;
+import com.vaadin.testbench.By;
+import com.vaadin.testbench.TestBenchElement;
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.testbench.elements.GridElement;
+import com.vaadin.testbench.elements.NativeSelectElement;
+import com.vaadin.tests.tb3.SingleBrowserTest;
+
+public class GridScrollDestinationTest extends SingleBrowserTest {
+
+    private ButtonElement button;
+    private GridElement grid;
+    private TestBenchElement header;
+    private TestBenchElement tableWrapper;
+
+    @Override
+    public void setup() throws Exception {
+        super.setup();
+        openTestURL();
+        button = $(ButtonElement.class).first();
+        grid = $(GridElement.class).first();
+        header = grid.getHeader();
+        tableWrapper = grid.getTableWrapper();
+    }
+
+    private void assertElementAtTop(WebElement row) {
+        assertThat((double) row.getLocation().getY(), closeTo(
+                header.getLocation().getY() + header.getSize().getHeight(),
+                1d));
+    }
+
+    private void assertElementAtBottom(WebElement row) {
+        assertThat(
+                (double) row.getLocation().getY() + row.getSize().getHeight(),
+                closeTo((double) tableWrapper.getLocation().getY()
+                        + tableWrapper.getSize().getHeight(), 1d));
+    }
+
+    private void assertElementAtMiddle(WebElement row) {
+        assertThat((double) row.getLocation()
+                .getY() + (row.getSize().getHeight() / 2), closeTo(
+                        (double) tableWrapper.getLocation().getY()
+                                + header.getSize().getHeight()
+                                + ((tableWrapper.getSize().getHeight()
+                                        - header.getSize().getHeight()) / 2),
+                        1d));
+    }
+
+    @Test
+    public void destinationAny() {
+        // ScrollDestination.ANY selected by default
+
+        // scroll down
+        button.click();
+
+        // expect the row at the bottom of the viewport
+        List<WebElement> rows = grid.getBody()
+                .findElements(By.className("v-grid-row"));
+        // last rendered row is a buffer row, inspect second to last
+        WebElement row = rows.get(rows.size() - 2);
+        assertEquals("50", row.getText());
+
+        assertElementAtBottom(row);
+
+        // scroll to end
+        grid.scrollToRow((int) grid.getRowCount() - 1);
+
+        // ensure row 50 is out of visual range, first two rows are out of view
+        // and getText can't find the contents so inspect the third row
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(2);
+
+        assertGreater(row.getText() + " is not greater than 52",
+                Integer.valueOf(row.getText()), 52);
+
+        // scroll up
+        button.click();
+
+        // expect the row at the top of the viewport
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        // first rendered row is a buffer row, inspect second
+        row = rows.get(1);
+        assertEquals("50", row.getText());
+
+        assertElementAtTop(row);
+
+        // scroll up by a few rows
+        grid.scrollToRow(45);
+
+        // refresh row references
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(6);
+        assertEquals("50", row.getText());
+
+        // scroll while already within viewport
+        button.click();
+
+        // expect no change since the row is still within viewport
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(6);
+        assertEquals("50", row.getText());
+    }
+
+    @Test
+    public void destinationEnd() {
+        $(NativeSelectElement.class).first()
+                .selectByText(ScrollDestination.END.name());
+
+        // scroll down
+        button.click();
+
+        // expect the row at the bottom of the viewport
+        List<WebElement> rows = grid.getBody()
+                .findElements(By.className("v-grid-row"));
+        // last rendered row is a buffer row, inspect second to last
+        WebElement row = rows.get(rows.size() - 2);
+        assertEquals("50", row.getText());
+
+        assertElementAtBottom(row);
+
+        // scroll to end
+        grid.scrollToRow((int) grid.getRowCount() - 1);
+
+        // ensure row 50 is out of visual range, first two rows are out of view
+        // and getText can't find the contents so inspect the third row
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(2);
+
+        assertGreater(row.getText() + " is not greater than 52",
+                Integer.valueOf(row.getText()), 52);
+
+        // scroll up
+        button.click();
+
+        // expect the row at the bottom of the viewport
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        // last rendered row is a buffer row, inspect second to last
+        row = rows.get(rows.size() - 2);
+        assertEquals("50", row.getText());
+
+        assertElementAtBottom(row);
+
+        // scroll down by a few rows
+        grid.scrollToRow(55);
+
+        // refresh row references
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(rows.size() - 7);
+        assertEquals("50", row.getText());
+
+        // scroll while already within viewport
+        button.click();
+
+        // expect the row at the bottom of the viewport again
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(rows.size() - 2);
+        assertEquals("50", row.getText());
+
+        assertElementAtBottom(row);
+    }
+
+    @Test
+    public void destinationStart() {
+        $(NativeSelectElement.class).first()
+                .selectByText(ScrollDestination.START.name());
+
+        // scroll down
+        button.click();
+
+        // expect the row at the top of the viewport
+        List<WebElement> rows = grid.getBody()
+                .findElements(By.className("v-grid-row"));
+        // first rendered row is a buffer row, inspect second
+        WebElement row = rows.get(1);
+        assertEquals("50", row.getText());
+
+        assertElementAtTop(row);
+
+        // scroll to end
+        grid.scrollToRow((int) grid.getRowCount() - 1);
+
+        // ensure row 50 is out of visual range, first two rows are out of view
+        // and getText can't find the contents so inspect the third row
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(2);
+
+        assertGreater(row.getText() + " is not greater than 52",
+                Integer.valueOf(row.getText()), 52);
+
+        // scroll up
+        button.click();
+
+        // expect the row at the top of the viewport
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        // first rendered row is a buffer row, inspect second
+        row = rows.get(1);
+        assertEquals("50", row.getText());
+
+        assertElementAtTop(row);
+
+        // scroll up by a few rows
+        grid.scrollToRow(45);
+
+        // refresh row references
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(6);
+        assertEquals("50", row.getText());
+
+        // scroll while already within viewport
+        button.click();
+
+        // expect the row at the top of the viewport again
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(1);
+        assertEquals("50", row.getText());
+
+        assertElementAtTop(row);
+    }
+
+    @Test
+    public void destinationMiddle() {
+        NativeSelectElement destinationSelect = $(NativeSelectElement.class)
+                .first();
+        destinationSelect.selectByText(ScrollDestination.MIDDLE.name());
+
+        // scroll down
+        button.click();
+
+        // expect the row at the middle of the viewport
+        List<WebElement> rows = grid.getBody()
+                .findElements(By.className("v-grid-row"));
+        // inspect the middle row
+        WebElement row = rows.get(rows.size() / 2);
+        assertEquals("50", row.getText());
+
+        assertElementAtMiddle(row);
+
+        // scroll to end
+        grid.scrollToRow((int) grid.getRowCount() - 1);
+
+        // ensure row 50 is out of visual range, first two rows are out of view
+        // and getText can't find the contents so inspect the third row
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(2);
+
+        assertGreater(row.getText() + " is not greater than 52",
+                Integer.valueOf(row.getText()), 52);
+
+        // scroll up
+        button.click();
+
+        // expect the row at the middle of the viewport
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        // first rendered row is a buffer row, inspect second
+        row = rows.get(rows.size() / 2);
+        assertEquals("50", row.getText());
+
+        assertElementAtMiddle(row);
+
+        // scroll down by a few rows
+        destinationSelect.selectByText(ScrollDestination.START.name());
+        button.click();
+
+        // refresh row references
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(1);
+        assertEquals("50", row.getText());
+
+        // scroll while already within viewport
+        destinationSelect.selectByText(ScrollDestination.MIDDLE.name());
+        button.click();
+
+        // expect the row at the top of the viewport again
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(rows.size() / 2);
+        assertEquals("50", row.getText());
+
+        assertElementAtMiddle(row);
+    }
+}