Client Grid.scrollToRow takes details into account (#17270)

Change-Id: I8a63f51f3b444c3c4342c46b7b58ad152f4bd0e1

7 files changed, 258 insertions, 40 deletions

diff --git a/client/src/com/vaadin/client/widgets/Escalator.java b/client/src/com/vaadin/client/widgets/Escalator.java
index 2b3a254b52..e02b4e9f83 100644
--- a/client/src/com/vaadin/client/widgets/Escalator.java
+++ b/client/src/com/vaadin/client/widgets/Escalator.java
@@ -5859,7 +5859,8 @@ public class Escalator extends Widget implements RequiresResize,
      *             column
      * @throws IllegalArgumentException
      *             if {@code destination} is {@link ScrollDestination#MIDDLE}
-     *             and padding is nonzero, or if the indicated column is frozen
+     *             and padding is nonzero; or if the indicated column is frozen;
+     *             or if {@code destination == null}
      */
     public void scrollToColumn(final int columnIndex,
             final ScrollDestination destination, final int padding)
@@ -5900,7 +5901,9 @@ public class Escalator extends Widget implements RequiresResize,
      *             if {@code rowIndex} is not a valid index for an existing row
      * @throws IllegalArgumentException
      *             if {@code destination} is {@link ScrollDestination#MIDDLE}
-     *             and padding is nonzero
+     *             and padding is nonzero; or if {@code destination == null}
+     * @see #scrollToRowAndSpacer(int, ScrollDestination, int)
+     * @see #scrollToSpacer(int, ScrollDestination, int)
      */
     public void scrollToRow(final int rowIndex,
             final ScrollDestination destination, final int padding)
@@ -5921,7 +5924,7 @@ public class Escalator extends Widget implements RequiresResize,
     /**
      * Scrolls the body vertically so that the spacer at the given row index is
      * visible and there is at least {@literal padding} pixesl to the given
-     * scroll destination
+     * scroll destination.
      * 
      * @since
      * @param spacerIndex
@@ -5932,9 +5935,11 @@ public class Escalator extends Widget implements RequiresResize,
      *            the number of pixels to place between the scrolled-to spacer
      *            and the viewport edge
      * @throws IllegalArgumentException
-     *             if {@code spacerIndex} is not an opened spacer if
+     *             if {@code spacerIndex} is not an opened spacer; or if
      *             {@code destination} is {@link ScrollDestination#MIDDLE} and
-     *             padding is nonzero
+     *             padding is nonzero; or if {@code destination == null}
+     * @see #scrollToRow(int, ScrollDestination, int)
+     * @see #scrollToRowAndSpacer(int, ScrollDestination, int)
      */
     public void scrollToSpacer(final int spacerIndex,
             ScrollDestination destination, final int padding)
@@ -5943,8 +5948,87 @@ public class Escalator extends Widget implements RequiresResize,
         body.scrollToSpacer(spacerIndex, destination, padding);
     }
 
+    /**
+     * Scrolls vertically to a row and the spacer below it.
+     * <p>
+     * If a spacer is not open at that index, this method behaves like
+     * {@link #scrollToRow(int, ScrollDestination, int)}
+     * 
+     * @since
+     * @param rowIndex
+     *            the index of the logical row to scroll to. -1 takes the
+     *            topmost spacer into account as well.
+     * @param destination
+     *            where the row should be aligned visually after scrolling
+     * @param padding
+     *            the number pixels to place between the scrolled-to row and the
+     *            viewport edge.
+     * @see #scrollToRow(int, ScrollDestination, int)
+     * @see #scrollToSpacer(int, ScrollDestination, int)
+     * @throws IllegalArgumentException
+     *             if {@code destination} is {@link ScrollDestination#MIDDLE}
+     *             and {@code padding} is not zero; or if {@code rowIndex} is
+     *             not a valid row index, or -1; or if
+     *             {@code destination == null}; or if {@code rowIndex == -1} and
+     *             there is no spacer open at that index.
+     */
+    public void scrollToRowAndSpacer(int rowIndex,
+            ScrollDestination destination, int padding)
+            throws IllegalArgumentException {
+        validateScrollDestination(destination, padding);
+        if (rowIndex != -1) {
+            verifyValidRowIndex(rowIndex);
+        }
+
+        // row range
+        final Range rowRange;
+        if (rowIndex != -1) {
+            int rowTop = (int) Math.floor(body.getRowTop(rowIndex));
+            int rowHeight = (int) Math.ceil(body.getDefaultRowHeight());
+            rowRange = Range.withLength(rowTop, rowHeight);
+        } else {
+            rowRange = Range.withLength(0, 0);
+        }
+
+        // get spacer
+        final SpacerContainer.SpacerImpl spacer = body.spacerContainer
+                .getSpacer(rowIndex);
+
+        if (rowIndex == -1 && spacer == null) {
+            throw new IllegalArgumentException("Cannot scroll to row index "
+                    + "-1, as there is no spacer open at that index.");
+        }
+
+        // make into target range
+        final Range targetRange;
+        if (spacer != null) {
+            final int spacerTop = (int) Math.floor(spacer.getTop());
+            final int spacerHeight = (int) Math.ceil(spacer.getHeight());
+            Range spacerRange = Range.withLength(spacerTop, spacerHeight);
+
+            targetRange = rowRange.combineWith(spacerRange);
+        } else {
+            targetRange = rowRange;
+        }
+
+        // get params
+        int targetStart = targetRange.getStart();
+        int targetEnd = targetRange.getEnd();
+        double viewportStart = getScrollTop();
+        double viewportEnd = viewportStart + body.getHeightOfSection();
+
+        double scrollPos = getScrollPos(destination, targetStart, targetEnd,
+                viewportStart, viewportEnd, padding);
+
+        setScrollTop(scrollPos);
+    }
+
     private static void validateScrollDestination(
             final ScrollDestination destination, final int padding) {
+        if (destination == null) {
+            throw new IllegalArgumentException("Destination cannot be null");
+        }
+
         if (destination == ScrollDestination.MIDDLE && padding != 0) {
             throw new IllegalArgumentException(
                     "You cannot have a padding with a MIDDLE destination");
diff --git a/client/src/com/vaadin/client/widgets/Grid.java b/client/src/com/vaadin/client/widgets/Grid.java
index 8316cb8b45..9fcdd20296 100644
--- a/client/src/com/vaadin/client/widgets/Grid.java
+++ b/client/src/com/vaadin/client/widgets/Grid.java
@@ -5887,6 +5887,9 @@ public class Grid<T> extends ResizeComposite implements
 
     /**
      * Scrolls to a certain row, using {@link ScrollDestination#ANY}.
+     * <p>
+     * If the details for that row are visible, those will be taken into account
+     * as well.
      * 
      * @param rowIndex
      *            zero-based index of the row to scroll to.
@@ -5901,6 +5904,9 @@ public class Grid<T> extends ResizeComposite implements
 
     /**
      * Scrolls to a certain row, using user-specified scroll destination.
+     * <p>
+     * If the details for that row are visible, those will be taken into account
+     * as well.
      * 
      * @param rowIndex
      *            zero-based index of the row to scroll to.
@@ -5920,6 +5926,9 @@ public class Grid<T> extends ResizeComposite implements
 
     /**
      * Scrolls to a certain row using only user-specified parameters.
+     * <p>
+     * If the details for that row are visible, those will be taken into account
+     * as well.
      * 
      * @param rowIndex
      *            zero-based index of the row to scroll to.
@@ -5949,7 +5958,7 @@ public class Grid<T> extends ResizeComposite implements
                     + ") is above maximum (" + maxsize + ")!");
         }
 
-        escalator.scrollToRow(rowIndex, destination, paddingPx);
+        escalator.scrollToRowAndSpacer(rowIndex, destination, paddingPx);
     }
 
     /**
@@ -7686,5 +7695,4 @@ public class Grid<T> extends ResizeComposite implements
     private Sidebar getSidebar() {
         return sidebar;
     }
-
 }
diff --git a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/EscalatorBasicClientFeaturesTest.java b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/EscalatorBasicClientFeaturesTest.java
index de254b4deb..9037b29bb1 100644
--- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/EscalatorBasicClientFeaturesTest.java
+++ b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/EscalatorBasicClientFeaturesTest.java
@@ -78,6 +78,7 @@ public abstract class EscalatorBasicClientFeaturesTest extends MultiBrowserTest
     protected static final String SET_100PX = "Set 100px";
     protected static final String SPACERS = "Spacers";
     protected static final String SCROLL_HERE_ANY_0PADDING = "Scroll here (ANY, 0)";
+    protected static final String SCROLL_HERE_SPACERBELOW_ANY_0PADDING = "Scroll here row+spacer below (ANY, 0)";
     protected static final String REMOVE = "Remove";
 
     protected static final String ROW_MINUS1 = "Row -1";
diff --git a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/client/GridDetailsClientTest.java b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/client/GridDetailsClientTest.java
index d32b731cf1..5ab0c238e5 100644
--- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/client/GridDetailsClientTest.java
+++ b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/client/GridDetailsClientTest.java
@@ -28,12 +28,17 @@ import org.junit.Test;
 import org.openqa.selenium.NoSuchElementException;
 import org.openqa.selenium.WebElement;
 
+import com.vaadin.shared.ui.grid.Range;
+import com.vaadin.shared.ui.grid.ScrollDestination;
 import com.vaadin.testbench.By;
 import com.vaadin.testbench.ElementQuery;
 import com.vaadin.testbench.TestBenchElement;
+import com.vaadin.testbench.annotations.RunLocally;
 import com.vaadin.testbench.elements.NotificationElement;
+import com.vaadin.testbench.parallel.Browser;
 import com.vaadin.tests.components.grid.basicfeatures.GridBasicClientFeaturesTest;
 
+@RunLocally(Browser.PHANTOMJS)
 public class GridDetailsClientTest extends GridBasicClientFeaturesTest {
 
     private static final String[] SET_GENERATOR = new String[] { "Component",
@@ -42,10 +47,6 @@ public class GridDetailsClientTest extends GridBasicClientFeaturesTest {
             "Component", "Row details", "Set faulty generator" };
     private static final String[] SET_EMPTY_GENERATOR = new String[] {
             "Component", "Row details", "Set empty generator" };
-    private static final String[] TOGGLE_DETAILS_FOR_ROW_1 = new String[] {
-            "Component", "Row details", "Toggle details for row 1" };
-    private static final String[] TOGGLE_DETAILS_FOR_ROW_100 = new String[] {
-            "Component", "Row details", "Toggle details for row 100" };
 
     @Before
     public void setUp() {
@@ -61,7 +62,7 @@ public class GridDetailsClientTest extends GridBasicClientFeaturesTest {
 
     @Test
     public void nullRendererShowsDetailsPlaceholder() {
-        selectMenuPath(TOGGLE_DETAILS_FOR_ROW_1);
+        toggleDetailsFor(1);
         TestBenchElement details = getGridElement().getDetails(1);
         assertNotNull("details for row 1 should not exist at the start",
                 details);
@@ -72,7 +73,7 @@ public class GridDetailsClientTest extends GridBasicClientFeaturesTest {
     @Test
     public void applyRendererThenOpenDetails() {
         selectMenuPath(SET_GENERATOR);
-        selectMenuPath(TOGGLE_DETAILS_FOR_ROW_1);
+        toggleDetailsFor(1);
 
         TestBenchElement details = getGridElement().getDetails(1);
         assertTrue("Unexpected details content",
@@ -81,7 +82,7 @@ public class GridDetailsClientTest extends GridBasicClientFeaturesTest {
 
     @Test
     public void openDetailsThenAppyRenderer() {
-        selectMenuPath(TOGGLE_DETAILS_FOR_ROW_1);
+        toggleDetailsFor(1);
         selectMenuPath(SET_GENERATOR);
 
         TestBenchElement details = getGridElement().getDetails(1);
@@ -99,7 +100,7 @@ public class GridDetailsClientTest extends GridBasicClientFeaturesTest {
         }
 
         selectMenuPath(SET_GENERATOR);
-        selectMenuPath(TOGGLE_DETAILS_FOR_ROW_100);
+        toggleDetailsFor(100);
 
         // scroll a bit beyond so we see below.
         getGridElement().scrollToRow(101);
@@ -114,7 +115,7 @@ public class GridDetailsClientTest extends GridBasicClientFeaturesTest {
         assertFalse("No notifications should've been at the start",
                 $(NotificationElement.class).exists());
 
-        selectMenuPath(TOGGLE_DETAILS_FOR_ROW_1);
+        toggleDetailsFor(1);
         selectMenuPath(SET_FAULTY_GENERATOR);
 
         ElementQuery<NotificationElement> notification = $(NotificationElement.class);
@@ -128,7 +129,7 @@ public class GridDetailsClientTest extends GridBasicClientFeaturesTest {
 
     @Test
     public void updaterStillWorksAfterError() {
-        selectMenuPath(TOGGLE_DETAILS_FOR_ROW_1);
+        toggleDetailsFor(1);
 
         selectMenuPath(SET_FAULTY_GENERATOR);
         $(NotificationElement.class).first().close();
@@ -142,7 +143,7 @@ public class GridDetailsClientTest extends GridBasicClientFeaturesTest {
     @Test
     public void updaterRendersExpectedWidgets() {
         selectMenuPath(SET_GENERATOR);
-        selectMenuPath(TOGGLE_DETAILS_FOR_ROW_1);
+        toggleDetailsFor(1);
 
         TestBenchElement detailsElement = getGridElement().getDetails(1);
         assertNotNull(detailsElement.findElement(By.className("gwt-Label")));
@@ -152,7 +153,7 @@ public class GridDetailsClientTest extends GridBasicClientFeaturesTest {
     @Test
     public void widgetsInUpdaterWorkAsExpected() {
         selectMenuPath(SET_GENERATOR);
-        selectMenuPath(TOGGLE_DETAILS_FOR_ROW_1);
+        toggleDetailsFor(1);
 
         TestBenchElement detailsElement = getGridElement().getDetails(1);
         WebElement button = detailsElement.findElement(By
@@ -167,7 +168,7 @@ public class GridDetailsClientTest extends GridBasicClientFeaturesTest {
     @Test
     public void emptyGenerator() {
         selectMenuPath(SET_EMPTY_GENERATOR);
-        selectMenuPath(TOGGLE_DETAILS_FOR_ROW_1);
+        toggleDetailsFor(1);
 
         assertEquals("empty generator did not produce an empty details row",
                 "", getGridElement().getDetails(1).getText());
@@ -176,9 +177,54 @@ public class GridDetailsClientTest extends GridBasicClientFeaturesTest {
     @Test(expected = NoSuchElementException.class)
     public void removeDetailsRow() {
         selectMenuPath(SET_GENERATOR);
-        selectMenuPath(TOGGLE_DETAILS_FOR_ROW_1);
-        selectMenuPath(TOGGLE_DETAILS_FOR_ROW_1);
+        toggleDetailsFor(1);
+        toggleDetailsFor(1);
 
         getGridElement().getDetails(1);
     }
+
+    @Test
+    public void scrollDownToRowWithDetails() {
+        toggleDetailsFor(100);
+        scrollToRow(100, ScrollDestination.ANY);
+
+        Range validScrollRange = Range.between(1700, 1715);
+        assertTrue(validScrollRange.contains(getGridVerticalScrollPos()));
+    }
+
+    @Test
+    public void scrollUpToRowWithDetails() {
+        toggleDetailsFor(100);
+        scrollGridVerticallyTo(999999);
+        scrollToRow(100, ScrollDestination.ANY);
+
+        Range validScrollRange = Range.between(1990, 2010);
+        assertTrue(validScrollRange.contains(getGridVerticalScrollPos()));
+    }
+
+    @Test
+    public void cannotScrollBeforeTop() {
+        toggleDetailsFor(1);
+        scrollToRow(0, ScrollDestination.END);
+        assertEquals(0, getGridVerticalScrollPos());
+    }
+
+    @Test
+    public void cannotScrollAfterBottom() {
+        toggleDetailsFor(999);
+        scrollToRow(999, ScrollDestination.START);
+
+        Range expectedRange = Range.withLength(19680, 20);
+        assertTrue(expectedRange.contains(getGridVerticalScrollPos()));
+    }
+
+    private void scrollToRow(int rowIndex, ScrollDestination destination) {
+        selectMenuPath(new String[] { "Component", "State", "Scroll to...",
+                "Row " + rowIndex + "...", "Destination " + destination });
+    }
+
+    private void toggleDetailsFor(int rowIndex) {
+        selectMenuPath(new String[] { "Component", "Row details",
+                "Toggle details for...", "Row " + rowIndex });
+    }
 }
diff --git a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/escalator/EscalatorSpacerTest.java b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/escalator/EscalatorSpacerTest.java
index a0fa961ad2..ce8fcd233e 100644
--- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/escalator/EscalatorSpacerTest.java
+++ b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/escalator/EscalatorSpacerTest.java
@@ -332,10 +332,46 @@ public class EscalatorSpacerTest extends EscalatorBasicClientFeaturesTest {
         scrollVerticallyTo(1000);
         selectMenuPath(FEATURES, SPACERS, ROW_50, SCROLL_HERE_ANY_0PADDING);
 
-        // Browsers might vary with a few pixels.
         assertEquals(getScrollTop(), 1000);
     }
 
+    @Test
+    public void scrollToRowAndSpacerFromAbove() throws Exception {
+        selectMenuPath(FEATURES, SPACERS, ROW_50, SET_100PX);
+        selectMenuPath(FEATURES, SPACERS, ROW_50,
+                SCROLL_HERE_SPACERBELOW_ANY_0PADDING);
+
+        // Browsers might vary with a few pixels.
+        Range allowableScrollRange = Range.between(765, 780);
+        int scrollTop = (int) getScrollTop();
+        assertTrue("Scroll position was not " + allowableScrollRange + ", but "
+                + scrollTop, allowableScrollRange.contains(scrollTop));
+    }
+
+    @Test
+    public void scrollToRowAndSpacerFromBelow() throws Exception {
+        selectMenuPath(FEATURES, SPACERS, ROW_50, SET_100PX);
+        scrollVerticallyTo(999999);
+        selectMenuPath(FEATURES, SPACERS, ROW_50,
+                SCROLL_HERE_SPACERBELOW_ANY_0PADDING);
+
+        // Browsers might vary with a few pixels.
+        Range allowableScrollRange = Range.between(995, 1005);
+        int scrollTop = (int) getScrollTop();
+        assertTrue("Scroll position was not " + allowableScrollRange + ", but "
+                + scrollTop, allowableScrollRange.contains(scrollTop));
+    }
+
+    @Test
+    public void scrollToRowAndSpacerAlreadyInViewport() throws Exception {
+        selectMenuPath(FEATURES, SPACERS, ROW_50, SET_100PX);
+        scrollVerticallyTo(950);
+        selectMenuPath(FEATURES, SPACERS, ROW_50,
+                SCROLL_HERE_SPACERBELOW_ANY_0PADDING);
+
+        assertEquals(getScrollTop(), 950);
+    }
+
     private static double[] getElementDimensions(WebElement element) {
         /*
          * we need to parse the style attribute, since using getCssValue gets a
diff --git a/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorBasicClientFeaturesWidget.java b/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorBasicClientFeaturesWidget.java
index 4cf4726c28..ec9f214748 100644
--- a/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorBasicClientFeaturesWidget.java
+++ b/uitest/src/com/vaadin/tests/widgetset/client/grid/EscalatorBasicClientFeaturesWidget.java
@@ -698,6 +698,14 @@ public class EscalatorBasicClientFeaturesWidget extends
                 escalator.scrollToSpacer(rowIndex, ScrollDestination.ANY, 0);
             }
         }, menupath);
+        addMenuCommand("Scroll here row+spacer below (ANY, 0)",
+                new ScheduledCommand() {
+                    @Override
+                    public void execute() {
+                        escalator.scrollToRowAndSpacer(rowIndex,
+                                ScrollDestination.ANY, 0);
+                    }
+                }, menupath);
     }
 
     private void insertRows(final RowContainer container, int offset, int number) {
diff --git a/uitest/src/com/vaadin/tests/widgetset/client/grid/GridBasicClientFeaturesWidget.java b/uitest/src/com/vaadin/tests/widgetset/client/grid/GridBasicClientFeaturesWidget.java
index 1fd926d726..196428822c 100644
--- a/uitest/src/com/vaadin/tests/widgetset/client/grid/GridBasicClientFeaturesWidget.java
+++ b/uitest/src/com/vaadin/tests/widgetset/client/grid/GridBasicClientFeaturesWidget.java
@@ -80,6 +80,7 @@ import com.vaadin.client.widgets.Grid.Column;
 import com.vaadin.client.widgets.Grid.FooterRow;
 import com.vaadin.client.widgets.Grid.HeaderRow;
 import com.vaadin.client.widgets.Grid.SelectionMode;
+import com.vaadin.shared.ui.grid.ScrollDestination;
 import com.vaadin.tests.widgetset.client.grid.GridBasicClientFeaturesWidget.Data;
 
 /**
@@ -767,6 +768,43 @@ public class GridBasicClientFeaturesWidget extends
                 grid.setWidth("1000px");
             }
         }, "Component", "State", "Width");
+
+        createScrollToRowMenu();
+    }
+
+    private void createScrollToRowMenu() {
+        String[] menupath = new String[] { "Component", "State",
+                "Scroll to...", null };
+
+        for (int i = 0; i < ROWS; i += 100) {
+            menupath[3] = "Row " + i + "...";
+            for (final ScrollDestination scrollDestination : ScrollDestination
+                    .values()) {
+                final int row = i;
+                addMenuCommand("Destination " + scrollDestination,
+                        new ScheduledCommand() {
+                            @Override
+                            public void execute() {
+                                grid.scrollToRow(row, scrollDestination);
+                            }
+                        }, menupath);
+            }
+        }
+
+        int i = ROWS - 1;
+        menupath[3] = "Row " + i + "...";
+        for (final ScrollDestination scrollDestination : ScrollDestination
+                .values()) {
+            final int row = i;
+            addMenuCommand("Destination " + scrollDestination,
+                    new ScheduledCommand() {
+                        @Override
+                        public void execute() {
+                            grid.scrollToRow(row, scrollDestination);
+                        }
+                    }, menupath);
+        }
+
     }
 
     private void createColumnsMenu() {
@@ -1423,24 +1461,21 @@ public class GridBasicClientFeaturesWidget extends
             }
         }, menupath);
 
-        addMenuCommand("Toggle details for row 1", new ScheduledCommand() {
-            boolean visible = false;
+        String[] togglemenupath = new String[] { menupath[0], menupath[1],
+                "Toggle details for..." };
+        for (int i : new int[] { 0, 1, 100, 200, 300, 400, 500, 600, 700, 800,
+                900, 999 }) {
+            final int rowIndex = i;
+            addMenuCommand("Row " + rowIndex, new ScheduledCommand() {
+                boolean visible = false;
 
-            @Override
-            public void execute() {
-                visible = !visible;
-                grid.setDetailsVisible(1, visible);
-            }
-        }, menupath);
-
-        addMenuCommand("Toggle details for row 100", new ScheduledCommand() {
-            boolean visible = false;
+                @Override
+                public void execute() {
+                    visible = !visible;
+                    grid.setDetailsVisible(rowIndex, visible);
+                }
+            }, togglemenupath);
+        }
 
-            @Override
-            public void execute() {
-                visible = !visible;
-                grid.setDetailsVisible(100, visible);
-            }
-        }, menupath);
     }
 }