Bug 1416231 - Add an asynchronous version of the description height workaround. r=Gijs
This prevents synchronous reflows when opening subviews. This also removes a superfluous invocation of the workaround while the panel is still hidden.
MozReview-Commit-ID: DohLjntVaPU
--- a/browser/base/content/test/performance/browser_appmenu_reflows.js
+++ b/browser/base/content/test/performance/browser_appmenu_reflows.js
@@ -39,41 +39,16 @@ const EXPECTED_APPMENU_OPEN_REFLOWS = [
"_calculateMaxHeight@resource:///modules/PanelMultiView.jsm",
"handleEvent@resource:///modules/PanelMultiView.jsm",
],
maxCount: 6, // This number should only ever go down - never up.
},
];
-const EXPECTED_APPMENU_SUBVIEW_REFLOWS = [
- /**
- * The synced tabs view has labels that are multiline. Because of bugs in
- * XUL layout relating to multiline text in scrollable containers, we need
- * to manually read their height in order to ensure container heights are
- * correct. Unfortunately this requires 2 sync reflows.
- *
- * If we add more views where this is necessary, we may need to duplicate
- * these expected reflows further. Bug 1392340 is on file to remove the
- * reflows completely when opening subviews.
- */
- {
- stack: [
- "descriptionHeightWorkaround@resource:///modules/PanelMultiView.jsm",
- "_transitionViews@resource:///modules/PanelMultiView.jsm",
- ],
-
- maxCount: 4, // This number should only ever go down - never up.
- },
-
- /**
- * Please don't add anything new!
- */
-];
-
add_task(async function() {
await ensureNoPreloadedBrowser();
// First, open the appmenu.
await withReflowObserver(async function() {
let popupShown =
BrowserTestUtils.waitForEvent(PanelUI.panel, "popupshown");
await PanelUI.show();
@@ -121,10 +96,10 @@ add_task(async function() {
}
}
await openSubViewsRecursively(PanelUI.mainView);
let hidden = BrowserTestUtils.waitForEvent(PanelUI.panel, "popuphidden");
PanelUI.hide();
await hidden;
- }, EXPECTED_APPMENU_SUBVIEW_REFLOWS);
+ }, []);
});
--- a/browser/components/customizableui/PanelMultiView.jsm
+++ b/browser/components/customizableui/PanelMultiView.jsm
@@ -873,29 +873,29 @@ var PanelMultiView = class extends this.
let width = prevPanelView.knownWidth;
let height = prevPanelView.knownHeight;
viewRect = Object.assign({height, width}, viewNode.customRectGetter());
let header = viewNode.firstChild;
if (header && header.classList.contains("panel-header")) {
viewRect.height += this._dwu.getBoundsWithoutFlushing(header).height;
}
nextPanelView.visible = true;
- nextPanelView.descriptionHeightWorkaround();
+ await nextPanelView.descriptionHeightWorkaround();
// Make sure that the view is located after all the other views that are
// already open in order for the transition to work as expected.
this._viewStack.appendChild(viewNode);
} else {
this._offscreenViewStack.style.minHeight = olderView.knownHeight + "px";
this._offscreenViewStack.appendChild(viewNode);
nextPanelView.visible = true;
// Now that the subview is visible, we can check the height of the
// description elements it contains.
- nextPanelView.descriptionHeightWorkaround();
+ await nextPanelView.descriptionHeightWorkaround();
viewRect = await window.promiseDocumentFlushed(() => {
return this._dwu.getBoundsWithoutFlushing(viewNode);
});
// Bail out if the panel was closed in the meantime.
if (!nextPanelView.isOpenIn(this)) {
return;
}
@@ -1101,19 +1101,21 @@ var PanelMultiView = class extends this.
this._viewStack.style.maxHeight = maxHeight + "px";
this._offscreenViewStack.style.maxHeight = maxHeight + "px";
}
break;
}
case "popupshown":
// The main view is always open and visible when the panel is first
// shown, so we can check the height of the description elements it
- // contains and notify consumers using the ViewShown event.
+ // contains and notify consumers using the ViewShown event. In order to
+ // minimize flicker we need to allow synchronous reflows, and we still
+ // make sure the ViewShown event is dispatched synchronously.
let mainPanelView = this.openViews[0];
- mainPanelView.descriptionHeightWorkaround();
+ mainPanelView.descriptionHeightWorkaround(true).catch(Cu.reportError);
this._activateView(mainPanelView);
break;
case "popuphidden": {
// WebExtensions consumers can hide the popup from viewshowing, or
// mid-transition, which disrupts our state:
this._transitioning = false;
this._viewContainer.removeAttribute("panelopen");
this._cleanupTransitionPhase();
@@ -1264,68 +1266,85 @@ var PanelView = class extends this.Assoc
/**
* If the main view or a subview contains wrapping elements, the attribute
* "descriptionheightworkaround" should be set on the view to force all the
* wrapping "description", "label" or "toolbarbutton" elements to a fixed
* height. If the attribute is set and the visibility, contents, or width
* of any of these elements changes, this function should be called to
* refresh the calculated heights.
*
- * This may trigger a synchronous layout.
+ * @param allowSyncReflows
+ * If set to true, the function takes a path that allows synchronous
+ * reflows, but minimizes flickering. This is used for the main view
+ * because we cannot use the workaround off-screen.
*/
- descriptionHeightWorkaround() {
+ async descriptionHeightWorkaround(allowSyncReflows = false) {
if (!this.node.hasAttribute("descriptionheightworkaround")) {
// This view does not require the workaround.
return;
}
// We batch DOM changes together in order to reduce synchronous layouts.
// First we reset any change we may have made previously. The first time
// this is called, and in the best case scenario, this has no effect.
let items = [];
- // Non-hidden <label> or <description> elements that also aren't empty
- // and also don't have a value attribute can be multiline (if their
- // text content is long enough).
- let isMultiline = ":not(:-moz-any([hidden],[value],:empty))";
- let selector = [
- "description" + isMultiline,
- "label" + isMultiline,
- "toolbarbutton[wrap]:not([hidden])",
- ].join(",");
- for (let element of this.node.querySelectorAll(selector)) {
- // Ignore items in hidden containers.
- if (element.closest("[hidden]")) {
- continue;
+ let collectItems = () => {
+ // Non-hidden <label> or <description> elements that also aren't empty
+ // and also don't have a value attribute can be multiline (if their
+ // text content is long enough).
+ let isMultiline = ":not(:-moz-any([hidden],[value],:empty))";
+ let selector = [
+ "description" + isMultiline,
+ "label" + isMultiline,
+ "toolbarbutton[wrap]:not([hidden])",
+ ].join(",");
+ for (let element of this.node.querySelectorAll(selector)) {
+ // Ignore items in hidden containers.
+ if (element.closest("[hidden]")) {
+ continue;
+ }
+ // Take the label for toolbarbuttons; it only exists on those elements.
+ element = element.labelElement || element;
+
+ let bounds = element.getBoundingClientRect();
+ let previous = gMultiLineElementsMap.get(element);
+ // We don't need to (re-)apply the workaround for invisible elements or
+ // on elements we've seen before and haven't changed in the meantime.
+ if (!bounds.width || !bounds.height ||
+ (previous && element.textContent == previous.textContent &&
+ bounds.width == previous.bounds.width)) {
+ continue;
+ }
+
+ items.push({ element });
}
- // Take the label for toolbarbuttons; it only exists on those elements.
- element = element.labelElement || element;
-
- let bounds = element.getBoundingClientRect();
- let previous = gMultiLineElementsMap.get(element);
- // We don't need to (re-)apply the workaround for invisible elements or
- // on elements we've seen before and haven't changed in the meantime.
- if (!bounds.width || !bounds.height ||
- (previous && element.textContent == previous.textContent &&
- bounds.width == previous.bounds.width)) {
- continue;
- }
-
- items.push({ element });
+ }
+ if (allowSyncReflows) {
+ collectItems();
+ } else {
+ await this.window.promiseDocumentFlushed(collectItems);
}
// Removing the 'height' property will only cause a layout flush in the next
// loop below if it was set.
for (let item of items) {
item.element.style.removeProperty("height");
}
// We now read the computed style to store the height of any element that
// may contain wrapping text.
- for (let item of items) {
- item.bounds = item.element.getBoundingClientRect();
+ let measureItems = () => {
+ for (let item of items) {
+ item.bounds = item.element.getBoundingClientRect();
+ }
+ }
+ if (allowSyncReflows) {
+ measureItems();
+ } else {
+ await this.window.promiseDocumentFlushed(measureItems);
}
// Now we can make all the necessary DOM changes at once.
for (let { element, bounds } of items) {
gMultiLineElementsMap.set(element, { bounds, textContent: element.textContent });
element.style.height = bounds.height + "px";
}
}