From fa8174a94616d3d0f76ec41dbac389269e1fcb59 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Johannes=20Dahlstr=C3=B6m?= Date: Wed, 28 Jan 2015 15:50:54 +0200 Subject: [PATCH] Maintain Grid scroll position on detach and reattach (#16220) Change-Id: I6ac5c3304bcd22e23f298c4dbdd65358aa1c64f7 --- .../widget/escalator/ScrollbarBundle.java | 11 +++++- .../com/vaadin/client/widgets/Escalator.java | 27 ++++++++++++++- .../EscalatorBasicClientFeaturesTest.java | 7 ++-- .../escalator/EscalatorBasicsTest.java | 34 ++++++++++++++----- 4 files changed, 66 insertions(+), 13 deletions(-) diff --git a/client/src/com/vaadin/client/widget/escalator/ScrollbarBundle.java b/client/src/com/vaadin/client/widget/escalator/ScrollbarBundle.java index 21f56c6c4d..2345641408 100644 --- a/client/src/com/vaadin/client/widget/escalator/ScrollbarBundle.java +++ b/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. *

diff --git a/client/src/com/vaadin/client/widgets/Escalator.java b/client/src/com/vaadin/client/widgets/Escalator.java index a81d0f3e18..129100e073 100644 --- a/client/src/com/vaadin/client/widgets/Escalator.java +++ b/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()); 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 92c7f3e6a6..a4fa55441a 100644 --- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/EscalatorBasicClientFeaturesTest.java +++ b/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")); } diff --git a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/escalator/EscalatorBasicsTest.java b/uitest/src/com/vaadin/tests/components/grid/basicfeatures/escalator/EscalatorBasicsTest.java index 95ed6ab3ff..4742236ac6 100644 --- a/uitest/src/com/vaadin/tests/components/grid/basicfeatures/escalator/EscalatorBasicsTest.java +++ b/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()); -- 2.39.5