Skip to content

Commit

Permalink
Fix to PopupView and LayoutManager size calculations during transform.
Browse files Browse the repository at this point in the history
- 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 0 for all the popup content sizes while the opening animation is
active. ComputedStyle ignores the transform and returns the expected
value.
- On occasion PopupView's popup contents are measured too early and any
calculations that rely on those measurements get erroneous results.
Check at the end of the layout whether the measuring succeeded, and
re-calculate if it did not.
- If layout is still running but actual layout phases are over, leave
elements that need measuring for the next round.
- If there are pending layouts, trigger them immediately after previous
layout round finishes, rather than hoping that the timer will trigger
eventually. Allow for one millisecond delay in case there are other
pending changes waiting for an opportunity to get processed first.
- If the timer is run while layout is still running, do nothing and wait
for the timer to get triggered again when the layout round is finished.
- Manual test, problem isn't reproducible by TestBench.

Fixes: vaadin#11187
  • Loading branch information
Ansku committed Dec 10, 2020
1 parent 49f7039 commit 18e138d
Show file tree
Hide file tree
Showing 8 changed files with 145 additions and 8 deletions.
32 changes: 29 additions & 3 deletions client/src/main/java/com/vaadin/client/LayoutManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,15 @@ public class LayoutManager {
private Timer layoutTimer = new Timer() {
@Override
public void run() {
layoutNow();
// only run if there is no active layouting going on,
// otherwise wait to get triggered again
if (!isLayoutRunning()) {
layoutNow();
}
}
};
private boolean everythingNeedsMeasure = false;
private boolean layoutOngoing = false;

/**
* Sets the application connection this instance is connected to. Called
Expand Down Expand Up @@ -239,13 +244,27 @@ private void countLayout(FastStringMap<Integer> layoutCounts,
}
}

/**
* Trigger a layout phase at the first opportunity. There is at minimum a 1
* millisecond delay, or the delay might last the full remaining duration of
* the already running layout phase.
*/
public void layoutLater() {
if (!layoutPending) {
layoutPending = true;
layoutTimer.schedule(100);
// scheduled times are only suggestions, any delay
// allows other pending actions to complete first
layoutTimer.schedule(1);
}
}

/**
* Trigger a layout phase immediately and cancel any delayed layout
* requests. If there already is a layout running, calling this method will
* result in an {@link IllegalStateException}. If a new delayed layout
* request is received while this method is running, that new layout phase
* will be triggered after a minimal delay.
*/
public void layoutNow() {
if (isLayoutRunning()) {
throw new IllegalStateException(
Expand All @@ -270,6 +289,11 @@ public void layoutNow() {
doLayout();
} finally {
currentDependencyTree = null;
if (layoutPending) {
// trigger the timer in order to allow for
// other pending actions to complete first
layoutTimer.schedule(1);
}
}
}

Expand Down Expand Up @@ -324,6 +348,7 @@ private void doLayout() {

Profiler.leave("LayoutManager phase init");

layoutOngoing = true;
while (true) {
Profiler.enter("Layout pass");
passes++;
Expand Down Expand Up @@ -571,6 +596,7 @@ private void doLayout() {
break;
}
}
layoutOngoing = false;

Profiler.enter("layout PostLayoutListener");
JsArrayObject<ComponentConnector> componentConnectors = connectorMap
Expand Down Expand Up @@ -1783,7 +1809,7 @@ private void stopMeasuringIfUnecessary(Element element) {
* the component whose size might have changed.
*/
public void setNeedsMeasure(ComponentConnector component) {
if (isLayoutRunning()) {
if (isLayoutRunning() && layoutOngoing) {
currentDependencyTree.setNeedsMeasure(component, true);
} else {
needsMeasure.add(component.getConnectorId());
Expand Down
12 changes: 12 additions & 0 deletions client/src/main/java/com/vaadin/client/MeasuredSize.java
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,12 @@ public MeasureResult measure(Element element) {

Profiler.enter("Measure height");
double requiredHeight = WidgetUtil.getRequiredHeightDouble(element);
if (requiredHeight == 0 && element.getOffsetHeight() > 0) {
requiredHeight = computedStyle.getHeightIncludingBorderPadding();
if (Double.isNaN(requiredHeight)) {
requiredHeight = 0;
}
}
double outerHeight = requiredHeight + sumHeights(margins);
double oldHeight = height;
if (setOuterHeight(outerHeight)) {
Expand All @@ -250,6 +256,12 @@ public MeasureResult measure(Element element) {

Profiler.enter("Measure width");
double requiredWidth = WidgetUtil.getRequiredWidthDouble(element);
if (requiredWidth == 0 && element.getOffsetWidth() > 0) {
requiredWidth = computedStyle.getWidthIncludingBorderPadding();
if (Double.isNaN(requiredWidth)) {
requiredWidth = 0;
}
}
double outerWidth = requiredWidth + sumWidths(margins);
double oldWidth = width;
if (setOuterWidth(outerWidth)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,15 @@
import java.util.ArrayList;
import java.util.List;

import com.google.gwt.dom.client.Element;
import com.google.gwt.event.shared.HandlerRegistration;
import com.vaadin.client.ComponentConnector;
import com.vaadin.client.ConnectorHierarchyChangeEvent;
import com.vaadin.client.LayoutManager;
import com.vaadin.client.MeasuredSize;
import com.vaadin.client.VCaption;
import com.vaadin.client.VCaptionWrapper;
import com.vaadin.client.WidgetUtil;
import com.vaadin.client.communication.StateChangeEvent;
import com.vaadin.client.ui.AbstractHasComponentsConnector;
import com.vaadin.client.ui.PostLayoutListener;
Expand All @@ -35,6 +39,7 @@
import com.vaadin.shared.ui.popupview.PopupViewState;
import com.vaadin.ui.PopupView;

@SuppressWarnings("deprecation")
@Connect(PopupView.class)
public class PopupViewConnector extends AbstractHasComponentsConnector
implements PostLayoutListener, VisibilityChangeHandler {
Expand Down Expand Up @@ -93,8 +98,36 @@ public void postLayout() {
centerAfterLayout = false;
getWidget().center();
}
if (!getChildComponents().isEmpty()) {
checkPopupLayout();
}
}

/**
* In some situations the default LayoutManager handling happens too early,
* and the popup contents end up being measured with incorrect size. Check
* if this is the case, and trigger re-measuring if needed. This check needs
* to happen immediately after the previous layout phase has finished in
* order to minimise the contents jumping as a result, waiting for
* AnimationEndListener to trigger would be too late.
*/
private void checkPopupLayout() {
ComponentConnector content = getChildComponents().get(0);
Element contentElement = content.getWidget().getElement();
int offsetHeight = contentElement.getOffsetHeight();
double measuredHeight = getMeasuredSize(getLayoutManager(),
contentElement).getOuterHeight();
if (!WidgetUtil.pixelValuesEqual(offsetHeight, measuredHeight)) {
getLayoutManager().setNeedsMeasureRecursively(content);
}
}

private static final native MeasuredSize getMeasuredSize(
LayoutManager layoutManager, Element element)
/*-{
return layoutManager.@com.vaadin.client.LayoutManager::getMeasuredSize(Lcom/google/gwt/dom/client/Element;)(element);
}-*/;

@Override
public void onConnectorHierarchyChange(
ConnectorHierarchyChangeEvent connectorHierarchyChangeEvent) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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.";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ public class FrontendInitialResourceUI extends AbstractTestUIWithLog {

@Override
protected void setup(VaadinRequest request) {
getPage().getJavaScript()
.execute("document.body.innerHTML=window.jsFile;\n");
getLayout().setId("layout");
getPage().getJavaScript().execute("layout.innerHTML=window.jsFile;\n");

}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@ public static class MyButton extends Button {

@Override
protected void setup(VaadinRequest request) {
getLayout().setId("layout");
Button b = new MyButton();
b.addClickListener(event -> getPage().getJavaScript()
.execute("document.body.innerHTML=window.jsFile;\n"));
.execute("layout.innerHTML=window.jsFile;\n"));
addComponent(b);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public void correctEs5Es6FileImportedThroughFrontend() {
}

assertEquals("/VAADIN/frontend/" + es + "/logFilename.js",
findElement(By.tagName("body")).getText());
findElement(By.id("layout")).getText());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public void correctEs5Es6FileImportedThroughFrontend() {
}

assertEquals("/VAADIN/frontend/" + es + "/logFilename.js",
findElement(By.tagName("body")).getText());
findElement(By.id("layout")).getText());
}

}

0 comments on commit 18e138d

Please sign in to comment.