Bug 1416231 - Add an asynchronous version of the description height workaround. r=Gijs draft
authorPaolo Amadini <paolo.mozmail@amadzone.org>
Tue, 27 Feb 2018 22:02:55 +0000
changeset 760712 7c50e7a33c8bbefc118af24ab385bfa5ea6756fb
parent 760711 616097e7508d48ebdea6083d35a638b8d861bebf
push id100730
push userpaolo.mozmail@amadzone.org
push dateWed, 28 Feb 2018 00:27:14 +0000
reviewersGijs
bugs1416231
milestone60.0a1
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
browser/base/content/test/performance/browser_appmenu_reflows.js
browser/components/customizableui/PanelMultiView.jsm
--- 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";
     }
   }