Maintain Grid scroll position on detach and reattach (#16220)

Change-Id: I6ac5c3304bcd22e23f298c4dbdd65358aa1c64f7

client/src/com/vaadin/client/widget/escalator/ScrollbarBundle.java

@@ -335,7 +335,7 @@ public abstract class ScrollbarBundle implements DeferredWorker {
 
     private boolean isLocked = false;
 
-    /** @deprecarted access via {@link #getHandlerManager()} instead. */
+    /** @deprecated access via {@link #getHandlerManager()} instead. */
     @Deprecated
     private HandlerManager handlerManager;
 
@@ -514,6 +514,15 @@ public abstract class ScrollbarBundle implements DeferredWorker {
         }
     }
 
+    /**
+     * Should be called whenever this bundle is attached to the DOM (typically,
+     * from the onLoad of the containing widget). Used to ensure the DOM scroll
+     * position is maintained when detaching and reattaching the bundle.
+     */
+    public void onLoad() {
+        internalSetScrollPos(toInt32(scrollPos));
+    }
+
     /**
      * Truncates a double such that no decimal places are retained.
      * <p>

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

@@ -47,6 +47,7 @@ import com.google.gwt.dom.client.TableRowElement;
 import com.google.gwt.dom.client.TableSectionElement;
 import com.google.gwt.event.shared.HandlerRegistration;
 import com.google.gwt.logging.client.LogConfiguration;
+import com.google.gwt.user.client.Command;
 import com.google.gwt.user.client.DOM;
 import com.google.gwt.user.client.Window;
 import com.google.gwt.user.client.ui.RequiresResize;
@@ -4479,7 +4480,28 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
 
         header.paintInsertRows(0, header.getRowCount());
         footer.paintInsertRows(0, footer.getRowCount());
-        recalculateElementSizes();
+
+        // recalculateElementSizes();
+
+        Scheduler.get().scheduleDeferred(new Command() {
+            @Override
+            public void execute() {
+                /*
+                 * Not a faintest idea why we have to defer this call, but
+                 * unless it is deferred, the size of the escalator will be 0x0
+                 * after it is first detached and then reattached to the DOM.
+                 * This only applies to a bare Escalator; inside a Grid
+                 * everything works fine either way.
+                 * 
+                 * The three autodetectRowHeightLater calls above seem obvious
+                 * suspects at first. However, they don't seem to have anything
+                 * to do with the issue, as they are no-ops in the
+                 * detach-reattach case.
+                 */
+                recalculateElementSizes();
+            }
+        });
+
         /*
          * Note: There's no need to explicitly insert rows into the body.
          * 
@@ -4506,6 +4528,9 @@ public class Escalator extends Widget implements RequiresResize, DeferredWorker
             footer.reapplyColumnWidths();
         }
 
+        verticalScrollbar.onLoad();
+        horizontalScrollbar.onLoad();
+
         scroller.attachScrollListener(verticalScrollbar.getElement());
         scroller.attachScrollListener(horizontalScrollbar.getElement());
         scroller.attachMousewheelListener(getElement());

uitest/src/com/vaadin/tests/components/grid/basicfeatures/EscalatorBasicClientFeaturesTest.java

@@ -52,6 +52,7 @@ public abstract class EscalatorBasicClientFeaturesTest extends MultiBrowserTest
 
     protected static final String GENERAL = "General";
     protected static final String DETACH_ESCALATOR = "Detach Escalator";
+    protected static final String ATTACH_ESCALATOR = "Attach Escalator";
     protected static final String POPULATE_COLUMN_ROW = "Populate Escalator (columns, then rows)";
     protected static final String POPULATE_ROW_COLUMN = "Populate Escalator (rows, then columns)";
     protected static final String CLEAR_COLUMN_ROW = "Clear (columns, then rows)";
@@ -234,10 +235,10 @@ public abstract class EscalatorBasicClientFeaturesTest extends MultiBrowserTest
     }
 
     protected void scrollVerticallyTo(int px) {
-        executeScript("arguments[0].scrollTop = " + px, getVeticalScrollbar());
+        executeScript("arguments[0].scrollTop = " + px, getVerticalScrollbar());
     }
 
-    private TestBenchElement getVeticalScrollbar() {
+    protected TestBenchElement getVerticalScrollbar() {
         return (TestBenchElement) getEscalator().findElement(
                 By.className("v-escalator-scroller-vertical"));
     }
@@ -247,7 +248,7 @@ public abstract class EscalatorBasicClientFeaturesTest extends MultiBrowserTest
                 getHorizontalScrollbar());
     }
 
-    private TestBenchElement getHorizontalScrollbar() {
+    protected TestBenchElement getHorizontalScrollbar() {
         return (TestBenchElement) getEscalator().findElement(
                 By.className("v-escalator-scroller-horizontal"));
     }

uitest/src/com/vaadin/tests/components/grid/basicfeatures/escalator/EscalatorBasicsTest.java

@@ -15,11 +15,13 @@
  */
 package com.vaadin.tests.components.grid.basicfeatures.escalator;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 
 import java.io.IOException;
 
+import org.junit.Before;
 import org.junit.Test;
 
 import com.vaadin.testbench.elements.NotificationElement;
@@ -27,20 +29,20 @@ import com.vaadin.tests.components.grid.basicfeatures.EscalatorBasicClientFeatur
 
 public class EscalatorBasicsTest extends EscalatorBasicClientFeaturesTest {
 
-    @Test
-    public void testDetachingAnEmptyEscalator() {
+    @Before
+    public void setUp() {
         setDebug(true);
         openTestURL();
+    }
 
+    @Test
+    public void testDetachingAnEmptyEscalator() {
         selectMenuPath(GENERAL, DETACH_ESCALATOR);
         assertEscalatorIsRemovedCorrectly();
     }
 
     @Test
     public void testDetachingASemiPopulatedEscalator() throws IOException {
-        setDebug(true);
-        openTestURL();
-
         selectMenuPath(COLUMNS_AND_ROWS, ADD_ONE_OF_EACH_ROW);
         selectMenuPath(COLUMNS_AND_ROWS, COLUMNS, ADD_ONE_COLUMN_TO_BEGINNING);
         selectMenuPath(GENERAL, DETACH_ESCALATOR);
@@ -49,14 +51,30 @@ public class EscalatorBasicsTest extends EscalatorBasicClientFeaturesTest {
 
     @Test
     public void testDetachingAPopulatedEscalator() {
-        setDebug(true);
-        openTestURL();
-
         selectMenuPath(GENERAL, POPULATE_COLUMN_ROW);
         selectMenuPath(GENERAL, DETACH_ESCALATOR);
         assertEscalatorIsRemovedCorrectly();
     }
 
+    @Test
+    public void testDetachingAndReattachingAnEscalator() {
+        selectMenuPath(GENERAL, POPULATE_COLUMN_ROW);
+
+        scrollVerticallyTo(50);
+        scrollHorizontallyTo(50);
+
+        selectMenuPath(GENERAL, DETACH_ESCALATOR);
+        selectMenuPath(GENERAL, ATTACH_ESCALATOR);
+
+        assertEquals("Vertical scroll position", "50", getVerticalScrollbar()
+                .getAttribute("scrollTop"));
+        assertEquals("Horizontal scroll position", "50",
+                getHorizontalScrollbar().getAttribute("scrollLeft"));
+
+        assertEquals("First cell of first visible row", "Row 2: 0,2",
+                getBodyCell(0, 0).getText());
+    }
+
     private void assertEscalatorIsRemovedCorrectly() {
         assertFalse($(NotificationElement.class).exists());
         assertNull(getEscalator());