From c8e04dc5b30f8967c58df928872ca2e5e2be5c5b Mon Sep 17 00:00:00 2001 From: Anna Koskinen Date: Fri, 18 Dec 2020 13:16:26 +0200 Subject: [PATCH] Fix to LayoutManager size calculations during transform. (#12138) * Fix to LayoutManager size calculations during transform. - ComputedStyle is slower but more reliable than using getBoundingClientRect, which does not work as expected if a transform has been applied to the element or one of its parents. This is a problem e.g. with PopupView, where getBoundingClientRect will return too small size (or even zero size) for all the popup contents while the opening animation is active. ComputedStyle ignores the transform and returns the expected value. - The presence of the element in DOM must be checked before the size is requested from ComputedStyle, if the element has disappeared from DOM without a warning and calculation is attempted anyway, the browser gets stuck. - Possibility to configure LayoutManager to use the less reliable calculations for applications where the slight performance difference is more important than layout issues within elements that have transform animations. - Manual test, problem isn't reproducible by TestBench. Fixes: #11187 --- .../java/com/vaadin/client/LayoutManager.java | 22 ++++++- .../java/com/vaadin/client/MeasuredSize.java | 58 ++++++++++++++++- .../com/vaadin/client/ui/ui/UIConnector.java | 26 +++++--- server/src/main/java/com/vaadin/ui/UI.java | 34 ++++++++++ .../java/com/vaadin/shared/ui/ui/UIState.java | 11 +++- .../PopupViewContentWithExpandRatio.java | 65 +++++++++++++++++++ 6 files changed, 202 insertions(+), 14 deletions(-) create mode 100644 uitest/src/main/java/com/vaadin/tests/components/popupview/PopupViewContentWithExpandRatio.java diff --git a/client/src/main/java/com/vaadin/client/LayoutManager.java b/client/src/main/java/com/vaadin/client/LayoutManager.java index 1639004d98..3227555b54 100644 --- a/client/src/main/java/com/vaadin/client/LayoutManager.java +++ b/client/src/main/java/com/vaadin/client/LayoutManager.java @@ -69,6 +69,7 @@ public class LayoutManager { } }; private boolean everythingNeedsMeasure = false; + private boolean thoroughSizeCheck = true; /** * Sets the application connection this instance is connected to. Called @@ -85,6 +86,24 @@ public class LayoutManager { this.connection = connection; } + /** + * Set whether the measuring should use a thorough size check that evaluates + * the presence of the element and uses calculated size, or default to a + * slightly faster check that can result in incorrect size information if + * the check is triggered while a transform animation is ongoing. This can + * happen e.g. when a PopupView is opened. + *

+ * By default, the thorough size check is enabled. + * + * @param thoroughSizeCheck + * {@code true} if thorough size check enabled, {@code false} if + * not + * @since + */ + public void setThoroughSizeChck(boolean thoroughSizeCheck) { + this.thoroughSizeCheck = thoroughSizeCheck; + } + /** * Returns the application connection for this layout manager. * @@ -822,7 +841,8 @@ public class LayoutManager { private MeasureResult measuredAndUpdate(Element element, MeasuredSize measuredSize) { - MeasureResult measureResult = measuredSize.measure(element); + MeasureResult measureResult = measuredSize.measure(element, + thoroughSizeCheck); if (measureResult.isChanged()) { notifyListenersAndDepdendents(element, measureResult.isWidthChanged(), diff --git a/client/src/main/java/com/vaadin/client/MeasuredSize.java b/client/src/main/java/com/vaadin/client/MeasuredSize.java index 89732dea6d..6814168bcc 100644 --- a/client/src/main/java/com/vaadin/client/MeasuredSize.java +++ b/client/src/main/java/com/vaadin/client/MeasuredSize.java @@ -18,6 +18,7 @@ package com.vaadin.client; import java.util.logging.Logger; import com.google.gwt.core.client.JsArrayString; +import com.google.gwt.dom.client.Document; import com.google.gwt.dom.client.Element; public class MeasuredSize { @@ -186,7 +187,44 @@ public class MeasuredSize { return paddings[3]; } + /** + * Measures paddings, margins, border, height, and weight of the given + * element and stores the results within this {@link MeasuredSize} object. + * The measurements are unreliable if the element or any of its parents are + * in the middle of a transform animation. + * + * @param element + * element to be measured + * @return data object for whether the width or height of the given element + * has changed since previous measure + * @see {@link #measure(Element, boolean)} + */ public MeasureResult measure(Element element) { + return measure(element, false); + } + + /** + * Measures paddings, margins, border, height, and weight of the given + * element and stores the results within this {@link MeasuredSize} object. + * + * @param element + * element to be measured + * @param thoroughSizeCheck + * {@code true} if the measuring should use the more reliable + * size check that requires ensuring that the element is still + * present in the DOM tree, {@code false} for the slightly faster + * check that will give incorrect size information if this method + * is called while the element or any of its parents are in the + * middle of a transform animation. + * @return data object for whether the width or height of the given element + * has changed since previous measure + */ + public MeasureResult measure(Element element, boolean thoroughSizeCheck) { + if (thoroughSizeCheck + && !Document.get().getBody().isOrHasChild(element)) { + return new MeasureResult(false, false); + } + Profiler.enter("MeasuredSize.measure"); boolean heightChanged = false; boolean widthChanged = false; @@ -239,7 +277,15 @@ public class MeasuredSize { Profiler.leave("Measure borders"); Profiler.enter("Measure height"); - double requiredHeight = WidgetUtil.getRequiredHeightDouble(element); + double requiredHeight; + if (thoroughSizeCheck) { + requiredHeight = computedStyle.getHeightIncludingBorderPadding(); + if (Double.isNaN(requiredHeight)) { + requiredHeight = 0; + } + } else { + requiredHeight = WidgetUtil.getRequiredHeightDouble(element); + } double outerHeight = requiredHeight + sumHeights(margins); double oldHeight = height; if (setOuterHeight(outerHeight)) { @@ -249,7 +295,15 @@ public class MeasuredSize { Profiler.leave("Measure height"); Profiler.enter("Measure width"); - double requiredWidth = WidgetUtil.getRequiredWidthDouble(element); + double requiredWidth; + if (thoroughSizeCheck) { + requiredWidth = computedStyle.getWidthIncludingBorderPadding(); + if (Double.isNaN(requiredWidth)) { + requiredWidth = 0; + } + } else { + requiredWidth = WidgetUtil.getRequiredWidthDouble(element); + } double outerWidth = requiredWidth + sumWidths(margins); double oldWidth = width; if (setOuterWidth(outerWidth)) { diff --git a/client/src/main/java/com/vaadin/client/ui/ui/UIConnector.java b/client/src/main/java/com/vaadin/client/ui/ui/UIConnector.java index 5630cfab3c..afe185e769 100644 --- a/client/src/main/java/com/vaadin/client/ui/ui/UIConnector.java +++ b/client/src/main/java/com/vaadin/client/ui/ui/UIConnector.java @@ -15,6 +15,16 @@ */ package com.vaadin.client.ui.ui; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.Comparator; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.logging.Logger; + import com.google.gwt.core.client.Scheduler; import com.google.gwt.dom.client.Document; import com.google.gwt.dom.client.Element; @@ -91,17 +101,8 @@ import com.vaadin.shared.ui.ui.UIServerRpc; import com.vaadin.shared.ui.ui.UIState; import com.vaadin.shared.util.SharedUtil; import com.vaadin.ui.UI; -import elemental.client.Browser; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collections; -import java.util.Comparator; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.logging.Logger; +import elemental.client.Browser; @Connect(value = UI.class, loadStyle = LoadStyle.EAGER) public class UIConnector extends AbstractSingleComponentContainerConnector @@ -924,6 +925,11 @@ public class UIConnector extends AbstractSingleComponentContainerConnector getRpcProxy(DebugWindowServerRpc.class).showServerDesign(connector); } + @OnStateChange("thoroughSizeCheck") + void onThoroughSizeChckChange() { + getLayoutManager().setThoroughSizeChck(getState().thoroughSizeCheck); + } + @OnStateChange("theme") void onThemeChange() { final String oldTheme = activeTheme; diff --git a/server/src/main/java/com/vaadin/ui/UI.java b/server/src/main/java/com/vaadin/ui/UI.java index 7430c04bb9..e291952f85 100644 --- a/server/src/main/java/com/vaadin/ui/UI.java +++ b/server/src/main/java/com/vaadin/ui/UI.java @@ -2021,6 +2021,40 @@ public abstract class UI extends AbstractSingleComponentContainer getRpcProxy(PageClientRpc.class).initializeMobileHtml5DndPolyfill(); } + /** + * Returns whether LayoutManager uses thorough size check that evaluates the + * presence of the element and uses calculated size, or defaults to a + * slightly faster check that can result in incorrect size information if + * the check is triggered while a transform animation is ongoing. This can + * happen e.g. when a PopupView is opened. + *

+ * By default, the thorough size check is enabled. + * + * @return {@code true} if thorough size check enabled, {@code false} if not + * @since + */ + public boolean isUsingThoroughSizeCheck() { + return getState(false).thoroughSizeCheck; + } + + /** + * Set whether LayoutManager should use thorough size check that evaluates + * the presence of the element and uses calculated size, or default to a + * slightly faster check that can result in incorrect size information if + * the check is triggered while a transform animation is ongoing. This can + * happen e.g. when a PopupView is opened. + *

+ * By default, the thorough size check is enabled. + * + * @param thoroughSizeCheck + * {@code true} if thorough size check enabled, {@code false} if + * not + * @since + */ + public void setUsingThoroughSizeCheck(boolean thoroughSizeCheck) { + getState().thoroughSizeCheck = thoroughSizeCheck; + } + /** * Event which is fired when the ordering of the windows is updated. *

diff --git a/shared/src/main/java/com/vaadin/shared/ui/ui/UIState.java b/shared/src/main/java/com/vaadin/shared/ui/ui/UIState.java index 90269a8b09..3b8f1d0a46 100644 --- a/shared/src/main/java/com/vaadin/shared/ui/ui/UIState.java +++ b/shared/src/main/java/com/vaadin/shared/ui/ui/UIState.java @@ -85,13 +85,22 @@ public class UIState extends AbstractSingleComponentContainerState { // Default is 1 for legacy reasons tabIndex = 1; } - /** * Enable Mobile HTML5 DnD support. * * @since 8.1 */ public boolean enableMobileHTML5DnD = false; + /** + * Should the more thorough size check be in use in LayoutManager + * calculations. If this value is changed to {@code false}, the size + * calculations can result in incorrect values if they are triggered while a + * transform animation is ongoing. This can happen e.g. when a PopupView is + * opened. + * + * @since + */ + public boolean thoroughSizeCheck = true; public static class LoadingIndicatorConfigurationState implements Serializable { diff --git a/uitest/src/main/java/com/vaadin/tests/components/popupview/PopupViewContentWithExpandRatio.java b/uitest/src/main/java/com/vaadin/tests/components/popupview/PopupViewContentWithExpandRatio.java new file mode 100644 index 0000000000..fd34d34430 --- /dev/null +++ b/uitest/src/main/java/com/vaadin/tests/components/popupview/PopupViewContentWithExpandRatio.java @@ -0,0 +1,65 @@ +package com.vaadin.tests.components.popupview; + +import com.vaadin.server.VaadinRequest; +import com.vaadin.tests.components.AbstractTestUI; +import com.vaadin.tests.util.LoremIpsum; +import com.vaadin.ui.Button; +import com.vaadin.ui.JavaScript; +import com.vaadin.ui.Label; +import com.vaadin.ui.PopupView; +import com.vaadin.ui.VerticalLayout; + +public class PopupViewContentWithExpandRatio extends AbstractTestUI { + private PopupView popup; + + @Override + protected void setup(VaadinRequest request) { + popup = new PopupView("Open popup", createPopupContent()); + popup.setHideOnMouseOut(false); + popup.setPopupVisible(false); + addComponent(popup); + } + + private VerticalLayout createPopupContent() { + Label label = new Label( + "Placeholder content that should take up most of the available space"); + label.setValue(LoremIpsum.get(56)); + label.setSizeFull(); + label.setId("label"); + + Button refreshBtn = new Button("Force layout", e -> { + JavaScript.eval("vaadin.forceLayout()"); + }); + refreshBtn.setId("refresh"); + + Button submitBtn = new Button("Close popup"); + submitBtn.addClickListener(clickEvent -> { + popup.setPopupVisible(false); + }); + submitBtn.setId("close"); + + VerticalLayout content = new VerticalLayout(); + content.setHeight("300px"); + content.setSpacing(true); + content.setMargin(true); + + content.addComponent(label); + content.addComponent(refreshBtn); + content.addComponent(submitBtn); + content.setExpandRatio(label, 2.0f); + return content; + } + + @Override + protected Integer getTicketNumber() { + return 11187; + } + + @Override + protected String getTestDescription() { + return "Expand ratio shouldn't cause contents to overflow " + + "from popup view. The popup should be opened at least " + + "20 times without SuperDevMode or TestBench or other " + + "configurations that might slow down the processing."; + } +} -- 2.39.5