Improvements to ScrollDestination sanity checks (#11772) (#11776)8.9.2

- The new top row logical index should always be within the logical
range and high enough up to avoid leaving a gap if possible.
- Added regression testing for using the different scroll destination
types for scrolling to the top and to the bottom by index.

Fixes #11732

2 files changed, 118 insertions, 8 deletions

diff --git a/client/src/main/java/com/vaadin/client/widgets/Escalator.java b/client/src/main/java/com/vaadin/client/widgets/Escalator.java
index 5c3bf593fb..e98ae2c578 100644
--- a/client/src/main/java/com/vaadin/client/widgets/Escalator.java
+++ b/client/src/main/java/com/vaadin/client/widgets/Escalator.java
@@ -5059,9 +5059,10 @@ public class Escalator extends Widget
                 break;
             case END:
                 // target row at the bottom of the viewport
-                newTopRowLogicalIndex = Math.min(
-                        lastVisibleIndexIfScrollingDown + 1, getRowCount() - 1)
+                newTopRowLogicalIndex = lastVisibleIndexIfScrollingDown + 1
                         - visualRangeLength + 1;
+                newTopRowLogicalIndex = ensureTopRowLogicalIndexSanity(
+                        newTopRowLogicalIndex);
                 if ((newTopRowLogicalIndex > oldTopRowLogicalIndex)
                         && (newTopRowLogicalIndex
                                 - oldTopRowLogicalIndex < visualRangeLength)) {
@@ -5078,10 +5079,8 @@ public class Escalator extends Widget
                 // target row at the middle of the viewport, padding has to be
                 // zero or we never would have reached this far
                 newTopRowLogicalIndex = targetRowIndex - visualRangeLength / 2;
-                // ensure we don't attempt to go beyond the bottom row
-                if (newTopRowLogicalIndex + visualRangeLength > getRowCount()) {
-                    newTopRowLogicalIndex = getRowCount() - visualRangeLength;
-                }
+                newTopRowLogicalIndex = ensureTopRowLogicalIndexSanity(
+                        newTopRowLogicalIndex);
                 if (newTopRowLogicalIndex < oldTopRowLogicalIndex) {
                     logicalTargetIndex = newTopRowLogicalIndex;
                 } else if (newTopRowLogicalIndex > oldTopRowLogicalIndex) {
@@ -5102,8 +5101,9 @@ public class Escalator extends Widget
             case START:
                 // target row at the top of the viewport, include buffer
                 // row if there is room for one
-                newTopRowLogicalIndex = Math
-                        .max(firstVisibleIndexIfScrollingUp - 1, 0);
+                newTopRowLogicalIndex = firstVisibleIndexIfScrollingUp - 1;
+                newTopRowLogicalIndex = ensureTopRowLogicalIndexSanity(
+                        newTopRowLogicalIndex);
                 if (getVisibleRowRange().contains(newTopRowLogicalIndex)) {
                     logicalTargetIndex = oldTopRowLogicalIndex
                             + visualRangeLength;
@@ -5141,6 +5141,24 @@ public class Escalator extends Widget
         }
 
         /**
+         * Modifies the proposed top row logical index to fit within the logical
+         * range and to not leave gaps if it is avoidable.
+         *
+         * @param proposedTopRowLogicalIndex
+         * @return an adjusted index, or the original if no changes were
+         *         necessary
+         */
+        private int ensureTopRowLogicalIndexSanity(
+                int proposedTopRowLogicalIndex) {
+            int newTopRowLogicalIndex = Math.max(proposedTopRowLogicalIndex, 0);
+            int visualRangeLength = visualRowOrder.size();
+            if (newTopRowLogicalIndex + visualRangeLength > getRowCount()) {
+                newTopRowLogicalIndex = getRowCount() - visualRangeLength;
+            }
+            return newTopRowLogicalIndex;
+        }
+
+        /**
          * Checks that scrolling is allowed and resets the scroll position if
          * it's not.
          *
diff --git a/uitest/src/test/java/com/vaadin/tests/components/grid/GridScrollDestinationTest.java b/uitest/src/test/java/com/vaadin/tests/components/grid/GridScrollDestinationTest.java
index 67a831557d..302ea741a4 100644
--- a/uitest/src/test/java/com/vaadin/tests/components/grid/GridScrollDestinationTest.java
+++ b/uitest/src/test/java/com/vaadin/tests/components/grid/GridScrollDestinationTest.java
@@ -15,10 +15,12 @@ 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.testbench.elements.TextFieldElement;
 import com.vaadin.tests.tb3.SingleBrowserTest;
 
 public class GridScrollDestinationTest extends SingleBrowserTest {
 
+    private TextFieldElement textField;
     private ButtonElement button;
     private GridElement grid;
     private TestBenchElement header;
@@ -28,6 +30,7 @@ public class GridScrollDestinationTest extends SingleBrowserTest {
     public void setup() throws Exception {
         super.setup();
         openTestURL();
+        textField = $(TextFieldElement.class).first();
         button = $(ButtonElement.class).first();
         grid = $(GridElement.class).first();
         header = grid.getHeader();
@@ -110,6 +113,28 @@ public class GridScrollDestinationTest extends SingleBrowserTest {
         rows = grid.getBody().findElements(By.className("v-grid-row"));
         row = rows.get(6);
         assertEquals("50", row.getText());
+
+        // scroll to beginning using scroll destination
+        textField.setValue("0");
+        button.click();
+
+        // expect to be scrolled all the way up
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(0);
+        assertEquals("0", row.getText());
+
+        assertElementAtTop(row);
+
+        // scroll to end using scroll destination
+        textField.setValue("99");
+        button.click();
+
+        // expect to be scrolled all the way down
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(rows.size() - 1);
+        assertEquals("99", row.getText());
+
+        assertElementAtBottom(row);
     }
 
     @Test
@@ -168,6 +193,29 @@ public class GridScrollDestinationTest extends SingleBrowserTest {
         assertEquals("50", row.getText());
 
         assertElementAtBottom(row);
+
+        // scroll to beginning using scroll destination
+        textField.setValue("0");
+        button.click();
+        button.click();
+
+        // expect to be scrolled all the way up
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(0);
+        assertEquals("0", row.getText());
+
+        assertElementAtTop(row);
+
+        // scroll to end using scroll destination
+        textField.setValue("99");
+        button.click();
+
+        // expect to be scrolled all the way down
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(rows.size() - 1);
+        assertEquals("99", row.getText());
+
+        assertElementAtBottom(row);
     }
 
     @Test
@@ -226,6 +274,28 @@ public class GridScrollDestinationTest extends SingleBrowserTest {
         assertEquals("50", row.getText());
 
         assertElementAtTop(row);
+
+        // scroll to beginning using scroll destination
+        textField.setValue("0");
+        button.click();
+
+        // expect to be scrolled all the way up
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(0);
+        assertEquals("0", row.getText());
+
+        assertElementAtTop(row);
+
+        // scroll to end using scroll destination
+        textField.setValue("99");
+        button.click();
+
+        // expect to be scrolled all the way down
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(rows.size() - 1);
+        assertEquals("99", row.getText());
+
+        assertElementAtBottom(row);
     }
 
     @Test
@@ -287,5 +357,27 @@ public class GridScrollDestinationTest extends SingleBrowserTest {
         assertEquals("50", row.getText());
 
         assertElementAtMiddle(row);
+
+        // scroll to beginning using scroll destination
+        textField.setValue("0");
+        button.click();
+
+        // expect to be scrolled all the way up
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(0);
+        assertEquals("0", row.getText());
+
+        assertElementAtTop(row);
+
+        // scroll to end using scroll destination
+        textField.setValue("99");
+        button.click();
+
+        // expect to be scrolled all the way down
+        rows = grid.getBody().findElements(By.className("v-grid-row"));
+        row = rows.get(rows.size() - 1);
+        assertEquals("99", row.getText());
+
+        assertElementAtBottom(row);
     }
 }