From: Tomi Virtanen Date: Fri, 16 May 2014 08:25:01 +0000 (+0300) Subject: Fix for 'Aborting layout after 100 passess' (#13359) X-Git-Tag: 7.3.0.beta1~2^2~66 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=9e494f526cad2e6302cddd31200800450d3c857e;p=vaadin-framework.git Fix for 'Aborting layout after 100 passess' (#13359) 'Aborting layout after 100 passes.' is caused by LayoutManager falling into a loop on rounding fractional layout slot sizes up and down while trying to fit layout's content in the space available. LayoutManager round always up, that causes this issue with IE9+ and Chrome. This change helps LayoutManager to round fractional sizes down for browsers that causes problems if rounded up. Browsers may fall into the loop especially with a zoom level other than 100%. Not with any zoom level though. Problematic zoom level varies by browser. OrderedLayoutExpandTest uses zoom levels other than 100%. Test for Chrome is the only one that really is able to reproduce error without the fix. IE9/10 would too, but the zoom level could not be set exactly to the required 95% for IE. Test works best as a regression test for other browsers. Change-Id: Ie840b074df5fed5ea3b15fba9a6fd372a5c0b76a --- diff --git a/client/src/com/vaadin/client/LayoutManager.java b/client/src/com/vaadin/client/LayoutManager.java index bf79009f4c..e17c0233b3 100644 --- a/client/src/com/vaadin/client/LayoutManager.java +++ b/client/src/com/vaadin/client/LayoutManager.java @@ -36,6 +36,7 @@ import com.vaadin.client.ui.VNotification; import com.vaadin.client.ui.layout.ElementResizeEvent; import com.vaadin.client.ui.layout.ElementResizeListener; import com.vaadin.client.ui.layout.LayoutDependencyTree; +import com.vaadin.client.ui.orderedlayout.AbstractOrderedLayoutConnector; public class LayoutManager { private static final String LOOP_ABORT_MESSAGE = "Aborting layout after 100 passes. This would probably be an infinite loop."; @@ -720,6 +721,16 @@ public class LayoutManager { Profiler.enter("LayoutManager.measureConnector"); Element element = connector.getWidget().getElement(); MeasuredSize measuredSize = getMeasuredSize(connector); + if (isBrowserOptimizedMeasuringNeeded() + && isWrappedInsideExpandBlock(connector)) { + // this fixes zoom/sub-pixel issues with ie9+ and Chrome + // (#13359) + // Update measuring logic to round up and/or down depending on + // browser. + measuredSize.setMeasurer(new MeasuredSize.RoundingMeasurer()); + } else { + measuredSize.setMeasurer(null);// resets to default measurer + } MeasureResult measureResult = measuredAndUpdate(element, measuredSize); if (measureResult.isChanged()) { @@ -729,6 +740,23 @@ public class LayoutManager { Profiler.leave("LayoutManager.measureConnector"); } + /** + * Return true if browser may need some optimized width and height measuring + * for MeasuredSize. + *

+ * Usually optimization is needed to avoid unnecessary scroll bars appearing + * in layouts caused by sub-pixel rounding. And to avoid LayoutManager + * doLayout() going into a loop trying to fit content with fractional size + * (percentages) inside a parent element and stopping only after safety + * iteration limit exceeds, also caused by sub-pixel rounding. + *

+ * For internal use only. May be removed or replaced in the future. + */ + private static boolean isBrowserOptimizedMeasuringNeeded() { + return BrowserInfo.get().isChrome() + || (BrowserInfo.get().isIE() && !BrowserInfo.get().isIE8()); + } + private void onConnectorChange(ComponentConnector connector, boolean widthChanged, boolean heightChanged) { Profiler.enter("LayoutManager.onConnectorChange"); @@ -810,6 +838,26 @@ public class LayoutManager { return connector instanceof ManagedLayout; } + /** + * Is the given connector wrapped inside a 'expanding' content. Detected by + * checking if the connector's parent is AbstractOrderedLayoutConnector that + * expands. 'Expand' means that some layout slots may expand its content + * width or height to some percentage fraction. + * + * @param connector + * The connector to check + * @return True if connector is wrapped inside a + * AbstractOrderedLayoutConnector that expands + */ + private static boolean isWrappedInsideExpandBlock( + ComponentConnector connector) { + ServerConnector parent = connector.getParent(); + if (parent instanceof AbstractOrderedLayoutConnector) { + return ((AbstractOrderedLayoutConnector) parent).needsExpand(); + } + return false; + } + public void forceLayout() { ConnectorMap connectorMap = connection.getConnectorMap(); JsArrayObject componentConnectors = connectorMap diff --git a/client/src/com/vaadin/client/MeasuredSize.java b/client/src/com/vaadin/client/MeasuredSize.java index 2531ff9389..4005a2651d 100644 --- a/client/src/com/vaadin/client/MeasuredSize.java +++ b/client/src/com/vaadin/client/MeasuredSize.java @@ -43,6 +43,49 @@ public class MeasuredSize { } } + public interface Measurer { + int measureHeight(Element element); + + int measureWidth(Element element); + } + + /** + * Default measurer rounds always up. + */ + public static class DefaultMeasurer implements Measurer { + + @Override + public int measureHeight(Element element) { + int requiredHeight = Util.getRequiredHeight(element); + return requiredHeight; + } + + @Override + public int measureWidth(Element element) { + int requiredWidth = Util.getRequiredWidth(element); + return requiredWidth; + } + } + + /** + * RoundingMeasurer measurer rounds up and down, optimized for different + * browsers. + */ + public static class RoundingMeasurer implements Measurer { + + @Override + public int measureHeight(Element element) { + double requiredHeight = Util.getPreciseRequiredHeight(element); + return Util.roundPreciseSize(requiredHeight); + } + + @Override + public int measureWidth(Element element) { + double requiredWidth = Util.getPreciseRequiredWidth(element); + return Util.roundPreciseSize(requiredWidth); + } + } + private int width = -1; private int height = -1; @@ -52,6 +95,15 @@ public class MeasuredSize { private FastStringSet dependents = FastStringSet.create(); + private Measurer measurer = new DefaultMeasurer(); + + public void setMeasurer(Measurer measurer) { + if (measurer == null) { + measurer = new DefaultMeasurer(); + } + this.measurer = measurer; + } + public int getOuterHeight() { return height; } @@ -236,7 +288,7 @@ public class MeasuredSize { Profiler.leave("Measure borders"); Profiler.enter("Measure height"); - int requiredHeight = Util.getRequiredHeight(element); + int requiredHeight = measurer.measureHeight(element); int marginHeight = sumHeights(margins); int oldHeight = height; int oldWidth = width; @@ -247,7 +299,7 @@ public class MeasuredSize { Profiler.leave("Measure height"); Profiler.enter("Measure width"); - int requiredWidth = Util.getRequiredWidth(element); + int requiredWidth = measurer.measureWidth(element); int marginWidth = sumWidths(margins); if (setOuterWidth(requiredWidth + marginWidth)) { debugSizeChange(element, "Width (outer)", oldWidth, width); diff --git a/client/src/com/vaadin/client/Util.java b/client/src/com/vaadin/client/Util.java index e031b37422..bec88032bb 100644 --- a/client/src/com/vaadin/client/Util.java +++ b/client/src/com/vaadin/client/Util.java @@ -627,16 +627,48 @@ public class Util { return reqHeight; } - public static native int getRequiredWidthBoundingClientRect( - com.google.gwt.dom.client.Element element) - /*-{ - if (element.getBoundingClientRect) { - var rect = element.getBoundingClientRect(); - return Math.ceil(rect.right - rect.left); + /** + * Gets the border-box width for the given element, i.e. element width + + * border + padding. + * + * @param element + * The element to check + * @return The border-box width for the element + */ + public static double getPreciseRequiredWidth( + com.google.gwt.dom.client.Element element) { + double reqWidth; + if (browserUsesPreciseSizeByComputedStyle()) { + reqWidth = getPreciseWidthByComputedStyle(element); } else { - return element.offsetWidth; + reqWidth = getPreciseBoundingClientRectWidth(element); } - }-*/; + return reqWidth; + } + + /** + * Gets the border-box height for the given element, i.e. element height + + * border + padding. + * + * @param element + * The element to check + * @return The border-box height for the element + */ + public static double getPreciseRequiredHeight( + com.google.gwt.dom.client.Element element) { + double reqHeight; + if (browserUsesPreciseSizeByComputedStyle()) { + reqHeight = getPreciseHeightByComputedStyle(element); + } else { + reqHeight = getPreciseBoundingClientRectHeight(element); + } + return reqHeight; + } + + public static int getRequiredWidthBoundingClientRect( + com.google.gwt.dom.client.Element element) { + return (int) Math.ceil(getPreciseBoundingClientRectWidth(element)); + } public static native int getRequiredHeightComputedStyle( com.google.gwt.dom.client.Element element) @@ -678,18 +710,10 @@ public class Util { return Math.ceil(width+border+padding); }-*/; - public static native int getRequiredHeightBoundingClientRect( - com.google.gwt.dom.client.Element element) - /*-{ - var height; - if (element.getBoundingClientRect != null) { - var rect = element.getBoundingClientRect(); - height = Math.ceil(rect.bottom - rect.top); - } else { - height = element.offsetHeight; - } - return height; - }-*/; + public static int getRequiredHeightBoundingClientRect( + com.google.gwt.dom.client.Element element) { + return (int) Math.ceil(getPreciseBoundingClientRectHeight(element)); + } public static int getRequiredWidth(Widget widget) { return getRequiredWidth(widget.getElement()); @@ -699,6 +723,128 @@ public class Util { return getRequiredHeight(widget.getElement()); } + /** + * Rounds pixel size up or down depending on the browser. + *

+ *

  • + * IE9, IE10 - rounds always down to closest integer + *
  • + * Others - rounds half up to closest integer (1.49 -> 1.0, 1.5 -> 2.0) + * + * @param preciseSize + * Decimal number to round. + * @return Rounded integer + */ + public static int roundPreciseSize(double preciseSize) { + boolean roundAlwaysDown = BrowserInfo.get().isIE9() + || BrowserInfo.get().isIE10(); + if (roundAlwaysDown) { + return (int) preciseSize; + } else { + // round to closest integer + return (int) Math.round(preciseSize); + } + } + + /** + * Returns getBoundingClientRect.width, if supported. Otherwise return + * offsetWidth. Includes the padding, scrollbar, and the border. Excludes + * the margin. + * + * @param elem + * Target element to measure + */ + private static native double getPreciseBoundingClientRectWidth(Element elem) + /*-{ + try { + if (elem.getBoundingClientRect) { + return elem.getBoundingClientRect().width; + } else { + return (elem.offsetWidth) || 0; + } + } catch (e) { + // JS exception is thrown if the elem is not attached to the document. + return 0; + } + }-*/; + + /** + * Returns getBoundingClientRect.height, if supported. Otherwise return + * offsetHeight. Includes the padding, scrollbar, and the border. Excludes + * the margin. + * + * @param elem + * Target element to measure + */ + private static native double getPreciseBoundingClientRectHeight(Element elem) + /*-{ + try { + if (elem.getBoundingClientRect) { + return elem.getBoundingClientRect().height; + } else { + return (elem.offsetHeight) || 0; + } + } catch (e) { + // JS exception is thrown if the elem is not attached to the document. + return 0; + } + }-*/; + + /** + * Parse computed styles to get precise width for the given element. + * Excludes the margin. Fallback to offsetWidth if computed styles is not + * defined, or if width can't be read or it's not fractional. + * + * @param elem + * Target element to measure + */ + private static native double getPreciseWidthByComputedStyle( + com.google.gwt.dom.client.Element elem) + /*-{ + var cs = elem.ownerDocument.defaultView.getComputedStyle(elem); + var size = cs && cs.getPropertyValue('width') || ''; + if (size && size.indexOf('.') > -1) { + size = parseFloat(size) + + parseInt(cs.getPropertyValue('padding-left')) + + parseInt(cs.getPropertyValue('padding-right')) + + parseInt(cs.getPropertyValue('border-left-width')) + + parseInt(cs.getPropertyValue('border-right-width')); + } else { + size = elem.offsetWidth; + } + return size; + }-*/; + + /** + * Parse computed styles to get precise height for the given element. + * Excludes the margin. Fallback to offsetHeight if computed styles is not + * defined, or if height can't be read or it's not fractional. + * + * @param elem + * Target element to measure + */ + private static native double getPreciseHeightByComputedStyle( + com.google.gwt.dom.client.Element elem) + /*-{ + var cs = elem.ownerDocument.defaultView.getComputedStyle(elem); + var size = cs && cs.getPropertyValue('height') || ''; + if (size && size.indexOf('.') > -1) { + size = parseFloat(size) + + parseInt(cs.getPropertyValue('padding-top')) + + parseInt(cs.getPropertyValue('padding-bottom')) + + parseInt(cs.getPropertyValue('border-top-width')) + + parseInt(cs.getPropertyValue('border-bottom-width')); + } else { + size = elem.offsetHeight; + } + return size; + }-*/; + + /** For internal use only. May be removed or replaced in the future. */ + private static boolean browserUsesPreciseSizeByComputedStyle() { + return BrowserInfo.get().isIE9(); + } + /** * Detects what is currently the overflow style attribute in given element. * diff --git a/client/src/com/vaadin/client/ui/orderedlayout/AbstractOrderedLayoutConnector.java b/client/src/com/vaadin/client/ui/orderedlayout/AbstractOrderedLayoutConnector.java index 0c09ae49c6..c20eda71dc 100644 --- a/client/src/com/vaadin/client/ui/orderedlayout/AbstractOrderedLayoutConnector.java +++ b/client/src/com/vaadin/client/ui/orderedlayout/AbstractOrderedLayoutConnector.java @@ -551,8 +551,10 @@ public abstract class AbstractOrderedLayoutConnector extends /** * Does the layout need to expand? + *

    + * For internal use only. May be removed or replaced in the future. */ - private boolean needsExpand() { + public boolean needsExpand() { return needsExpand; } diff --git a/uitest/src/com/vaadin/tests/components/orderedlayout/OrderedLayoutExpand.java b/uitest/src/com/vaadin/tests/components/orderedlayout/OrderedLayoutExpand.java new file mode 100644 index 0000000000..e51e9a49a5 --- /dev/null +++ b/uitest/src/com/vaadin/tests/components/orderedlayout/OrderedLayoutExpand.java @@ -0,0 +1,109 @@ +/* + * 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.orderedlayout; + +import com.vaadin.server.VaadinRequest; +import com.vaadin.tests.components.AbstractTestUI; +import com.vaadin.ui.Alignment; +import com.vaadin.ui.Component; +import com.vaadin.ui.HorizontalLayout; +import com.vaadin.ui.Label; +import com.vaadin.ui.UI; + +@SuppressWarnings("serial") +public class OrderedLayoutExpand extends AbstractTestUI { + + @Override + protected void setup(VaadinRequest request) { + + ThreeColumnLayout layout = new ThreeColumnLayout( + createLabelWithUndefinedSize("first component"), + createLabelWithUndefinedSize("second component"), + createLabelWithUndefinedSize("third component")); + + addComponent(layout); + setWidth("600px"); + + if (UI.getCurrent().getPage().getWebBrowser().isChrome()) { + getPage().getStyles().add("body { zoom: 1.10; }"); + } + } + + @Override + protected String getTestDescription() { + StringBuilder sb = new StringBuilder( + "You should not see 'Aborting layout after 100 passes.' error with debug mode and "); + if (UI.getCurrent().getPage().getWebBrowser().isChrome()) { + sb.append(" Zoom is "); + sb.append("1.10"); + } else { + sb.append(" zoom level 95%."); + } + return sb.toString(); + } + + @Override + protected Integer getTicketNumber() { + return 13359; + } + + private Label createLabelWithUndefinedSize(String caption) { + Label label = new Label(caption); + label.setSizeUndefined(); + return label; + } + + private static class ThreeColumnLayout extends HorizontalLayout { + public ThreeColumnLayout(Component leftComponent, + Component centerComponent, Component rightComponent) { + setWidth("100%"); + setMargin(true); + Component left = createLeftHolder(leftComponent); + this.addComponent(left); + setComponentAlignment(left, Alignment.MIDDLE_LEFT); + setExpandRatio(left, 1f); + Component center = createCenterHolder(centerComponent); + this.addComponent(center); + setComponentAlignment(center, Alignment.MIDDLE_CENTER); + setExpandRatio(center, 0f); + Component right = createRightHolder(rightComponent); + this.addComponent(right); + setComponentAlignment(right, Alignment.MIDDLE_RIGHT); + setExpandRatio(right, 1f); + } + + private Component createLeftHolder(Component c) { + HorizontalLayout hl = new HorizontalLayout(); + hl.addComponent(c); + hl.setWidth(100, Unit.PERCENTAGE); + return hl; + } + + private Component createCenterHolder(Component c) { + HorizontalLayout hl = new HorizontalLayout(); + hl.addComponent(c); + hl.setComponentAlignment(c, Alignment.MIDDLE_CENTER); + return hl; + } + + private Component createRightHolder(Component c) { + HorizontalLayout hl = new HorizontalLayout(); + hl.addComponent(c); + hl.setWidth(100, Unit.PERCENTAGE); + return hl; + } + } +} diff --git a/uitest/src/com/vaadin/tests/components/orderedlayout/OrderedLayoutExpandTest.java b/uitest/src/com/vaadin/tests/components/orderedlayout/OrderedLayoutExpandTest.java new file mode 100644 index 0000000000..46b5f6b8b0 --- /dev/null +++ b/uitest/src/com/vaadin/tests/components/orderedlayout/OrderedLayoutExpandTest.java @@ -0,0 +1,132 @@ +/* + * 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.orderedlayout; + +import java.util.List; + +import org.junit.Assert; +import org.junit.Test; +import org.openqa.selenium.By; +import org.openqa.selenium.Keys; +import org.openqa.selenium.NoSuchElementException; +import org.openqa.selenium.WebElement; +import org.openqa.selenium.remote.DesiredCapabilities; + +import com.vaadin.tests.tb3.MultiBrowserTest; + +public class OrderedLayoutExpandTest extends MultiBrowserTest { + + /* + * (non-Javadoc) + * + * @see com.vaadin.tests.tb3.MultiBrowserTest#getBrowsersToTest() + */ + @Override + public List getBrowsersToTest() { + List browsersToTest = super.getBrowsersToTest(); + + // Not sure why, but IE11 might give the following error message: + // "org.openqa.selenium.WebDriverException: Unexpected error launching + // Internet Explorer. Browser zoom level was set to 75%. It should be + // set to 100%". + + // setting "ignoreZoomSetting" capability to true seems to help, but if + // it keeps bugging, just skip the IE11 completely bu uncommenting the + // line below. + // browsersToTest.remove(Browser.IE11.getDesiredCapabilities()); + Browser.IE11.getDesiredCapabilities().setCapability( + "ignoreZoomSetting", true); + + // Can't test with IE8 with a zoom level other than 100%. IE8 could be + // removed to speed up the test run. + // browsersToTest.remove(Browser.IE8.getDesiredCapabilities()); + + return browsersToTest; + } + + @Test + public void testNoAbortingLayoutAfter100PassesError() throws Exception { + setDebug(true); + openTestURL(); + + if (!getDesiredCapabilities().equals( + Browser.CHROME.getDesiredCapabilities())) { + // Chrome uses css to to set zoom level to 110% that is known to + // cause issues with the test app. + // Other browsers tries to set browser's zoom level directly. + WebElement html = driver.findElement(By.tagName("html")); + // reset to 100% just in case + html.sendKeys(Keys.chord(Keys.CONTROL, "0")); + // zoom browser to 75% (ie) or 90% (FF). It depends on browser + // how much "Ctrl + '-'" zooms out. + html.sendKeys(Keys.chord(Keys.CONTROL, Keys.SUBTRACT)); + } + + // open debug window's Examine component hierarchy tab + openDebugExamineComponentHierarchyTab(); + + // click "Check layouts for potential problems" button + clickDebugCheckLayoutsForPotentialProblems(); + + // find div containing a successful layout analyze result + WebElement pass = findLayoutAnalyzePassElement(); + // find div containing a error message with + // "Aborting layout after 100 passess" message. + WebElement error = findLayoutAnalyzeAbortedElement(); + + Assert.assertNull(error); + Assert.assertNotNull(pass); + + if (!getDesiredCapabilities().equals( + Browser.CHROME.getDesiredCapabilities())) { + WebElement html = driver.findElement(By.tagName("html")); + // reset zoom level back to 100% + html.sendKeys(Keys.chord(Keys.CONTROL, "0")); + } + } + + private void openDebugExamineComponentHierarchyTab() { + WebElement button = findElement(By + .xpath("//button[@title='Examine component hierarchy']")); + // can't use 'click()' with zoom levels other than 100% + button.sendKeys(Keys.chord(Keys.SPACE)); + } + + private void clickDebugCheckLayoutsForPotentialProblems() { + WebElement button = findElement(By + .xpath("//button[@title='Check layouts for potential problems']")); + + button.sendKeys(Keys.chord(Keys.SPACE)); + } + + private WebElement findLayoutAnalyzePassElement() { + try { + return findElement(By + .xpath("//div[text()='Layouts analyzed, no top level problems']")); + } catch (NoSuchElementException e) { + return null; + } + } + + private WebElement findLayoutAnalyzeAbortedElement() { + try { + return findElement(By + .xpath("//div[text()='Aborting layout after 100 passess']")); + } catch (NoSuchElementException e) { + return null; + } + } +}