Prevent scroll position reset on GridLayout hierarchy change (#13386)

State change handling by default clears measured size if the size is set
to undefined; this would cause GridLayout to shrink to zero size because
its child cells have position: absolute. The layout phase recomputes the
size, but in some cases the browser reflows first, affecting the scroll
position of the layout parent. This patch prevents GridLayout from
clearing once-computed sizes during state change.

Change-Id: Id6e066c3ea360083d16d3fcc5c6d7d4bb6cea8b7

4 files changed, 167 insertions, 2 deletions

diff --git a/client/src/com/vaadin/client/ui/AbstractComponentConnector.java b/client/src/com/vaadin/client/ui/AbstractComponentConnector.java
index f6c26cda05..47140348f9 100644
--- a/client/src/com/vaadin/client/ui/AbstractComponentConnector.java
+++ b/client/src/com/vaadin/client/ui/AbstractComponentConnector.java
@@ -217,11 +217,24 @@ public abstract class AbstractComponentConnector extends AbstractConnector
 
     }
 
+    /**
+     * Updates the component size based on the shared state, invoking the
+     * {@link LayoutManager layout manager} if necessary.
+     */
     protected void updateComponentSize() {
         updateComponentSize(getState().width == null ? "" : getState().width,
                 getState().height == null ? "" : getState().height);
     }
 
+    /**
+     * Updates the component size, invoking the {@link LayoutManager layout
+     * manager} if necessary.
+     * 
+     * @param newWidth
+     *            The new width as a CSS string. Cannot be null.
+     * @param newHeight
+     *            The new height as a CSS string. Cannot be null.
+     */
     protected void updateComponentSize(String newWidth, String newHeight) {
         Profiler.enter("AbstractComponentConnector.updateComponentSize");
 
@@ -255,13 +268,25 @@ public abstract class AbstractComponentConnector extends AbstractConnector
         Profiler.leave("AbstractComponentConnector.updateComponentSize update styleNames");
 
         Profiler.enter("AbstractComponentConnector.updateComponentSize update DOM");
-        widget.setHeight(newHeight);
-        widget.setWidth(newWidth);
+        updateWidgetSize(newWidth, newHeight);
         Profiler.leave("AbstractComponentConnector.updateComponentSize update DOM");
 
         Profiler.leave("AbstractComponentConnector.updateComponentSize");
     }
 
+    /**
+     * Updates the DOM size of this connector's {@link #getWidget() widget}.
+     * 
+     * @param newWidth
+     *            The new width as a CSS string. Cannot be null.
+     * @param newHeight
+     *            The new height as a CSS string. Cannot be null.
+     */
+    protected void updateWidgetSize(String newWidth, String newHeight) {
+        getWidget().setWidth(newWidth);
+        getWidget().setHeight(newHeight);
+    }
+
     @Override
     public boolean isRelativeHeight() {
         return ComponentStateUtil.isRelativeHeight(getState());
diff --git a/client/src/com/vaadin/client/ui/gridlayout/GridLayoutConnector.java b/client/src/com/vaadin/client/ui/gridlayout/GridLayoutConnector.java
index cc052fa6d5..c306282349 100644
--- a/client/src/com/vaadin/client/ui/gridlayout/GridLayoutConnector.java
+++ b/client/src/com/vaadin/client/ui/gridlayout/GridLayoutConnector.java
@@ -210,4 +210,18 @@ public class GridLayoutConnector extends AbstractComponentContainerConnector
     public void layoutHorizontally() {
         getWidget().updateWidth();
     }
+
+    @Override
+    protected void updateWidgetSize(String newWidth, String newHeight) {
+        // Prevent the element from momentarily shrinking to zero size
+        // when the size is set to undefined by a state change but before
+        // it is recomputed in the layout phase. This may affect scroll
+        // position in some cases; see #13386.
+        if (!isUndefinedHeight()) {
+            getWidget().setHeight(newHeight);
+        }
+        if (!isUndefinedWidth()) {
+            getWidget().setWidth(newWidth);
+        }
+    }
 }
diff --git a/uitest/src/com/vaadin/tests/components/gridlayout/GridLayoutScrollPosition.java b/uitest/src/com/vaadin/tests/components/gridlayout/GridLayoutScrollPosition.java
new file mode 100644
index 0000000000..9bc29b5c82
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/components/gridlayout/GridLayoutScrollPosition.java
@@ -0,0 +1,78 @@
+/*
+ * Copyright 2000-2014 Vaadin Ltd.
+ * 
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.vaadin.tests.components.gridlayout;
+
+import com.vaadin.data.Property.ValueChangeEvent;
+import com.vaadin.data.Property.ValueChangeListener;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.CheckBox;
+import com.vaadin.ui.GridLayout;
+import com.vaadin.ui.Label;
+import com.vaadin.ui.Panel;
+
+public class GridLayoutScrollPosition extends AbstractTestUI {
+
+    @Override
+    protected void setup(VaadinRequest request) {
+
+        Panel panel = new Panel();
+        setContent(panel);
+
+        GridLayout gridLayout = new GridLayout();
+        gridLayout.setWidth("500px");
+        panel.setContent(gridLayout);
+        gridLayout.setColumns(1);
+        gridLayout.setRows(1);
+
+        Label dummyLabel = new Label("Dummy");
+        dummyLabel.setHeight("500px");
+        gridLayout.addComponent(dummyLabel);
+
+        final CheckBox visibilityToggleCheckBox = new CheckBox(
+                "Hide / Show toggleable components");
+        visibilityToggleCheckBox.setId("visibility-toggle");
+        visibilityToggleCheckBox.setHeight("2000px");
+        visibilityToggleCheckBox.setImmediate(true);
+        visibilityToggleCheckBox.setValue(false); // Initially unchecked
+        gridLayout.addComponent(visibilityToggleCheckBox);
+
+        final Label toggleableLabel = new Label("Toggleable Label");
+        toggleableLabel.setHeight("2000px");
+        toggleableLabel.setVisible(false); // Initially hidden
+        gridLayout.addComponent(toggleableLabel);
+
+        visibilityToggleCheckBox
+                .addValueChangeListener(new ValueChangeListener() {
+                    @Override
+                    public void valueChange(ValueChangeEvent event) {
+                        toggleableLabel.setVisible(visibilityToggleCheckBox
+                                .getValue());
+                    }
+                });
+
+    }
+
+    @Override
+    protected String getTestDescription() {
+        return "The UI scroll position should not be reset when visibility of GridLayout children is toggled";
+    }
+
+    @Override
+    protected Integer getTicketNumber() {
+        return 13386;
+    }
+}
\ No newline at end of file
diff --git a/uitest/src/com/vaadin/tests/components/gridlayout/GridLayoutScrollPositionTest.java b/uitest/src/com/vaadin/tests/components/gridlayout/GridLayoutScrollPositionTest.java
new file mode 100644
index 0000000000..961b08002b
--- /dev/null
+++ b/uitest/src/com/vaadin/tests/components/gridlayout/GridLayoutScrollPositionTest.java
@@ -0,0 +1,48 @@
+/*
+ * Copyright 2000-2014 Vaadin Ltd.
+ * 
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License. You may obtain a copy of
+ * the License at
+ * 
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package com.vaadin.tests.components.gridlayout;
+
+import static org.junit.Assert.assertEquals;
+
+import org.junit.Test;
+import org.openqa.selenium.By;
+import org.openqa.selenium.WebDriver;
+import org.openqa.selenium.WebElement;
+
+import com.vaadin.tests.tb3.MultiBrowserTest;
+
+public class GridLayoutScrollPositionTest extends MultiBrowserTest {
+
+    @Test
+    public void testToggleChildComponents() throws Exception {
+
+        final int SCROLLTOP = 100;
+
+        openTestURL();
+
+        WebDriver driver = getDriver();
+
+        WebElement ui = driver.findElement(By.className("v-ui"));
+
+        testBenchElement(ui).scroll(SCROLLTOP);
+
+        driver.findElement(By.id("visibility-toggle"))
+                .findElement(By.tagName("input")).click();
+
+        assertEquals("UI scroll position", String.valueOf(SCROLLTOP),
+                ui.getAttribute("scrollTop"));
+    }
+}