From a3311d8135bcf153a7df709f93575b3654c0bff4 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Johannes=20Dahlstr=C3=B6m?= Date: Thu, 24 Apr 2014 17:17:42 +0300 Subject: [PATCH] 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 --- .../client/ui/AbstractComponentConnector.java | 29 ++++++- .../ui/gridlayout/GridLayoutConnector.java | 14 ++++ .../gridlayout/GridLayoutScrollPosition.java | 78 +++++++++++++++++++ .../GridLayoutScrollPositionTest.java | 48 ++++++++++++ 4 files changed, 167 insertions(+), 2 deletions(-) create mode 100644 uitest/src/com/vaadin/tests/components/gridlayout/GridLayoutScrollPosition.java create mode 100644 uitest/src/com/vaadin/tests/components/gridlayout/GridLayoutScrollPositionTest.java 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")); + } +} -- 2.39.5