Bug 1447619 - avoid reflowing when returning items to the toolbar from the overflow menu, r=florian
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Fri, 20 Apr 2018 15:09:44 +0100
changeset 460823 1ce48405e58a609299a84192df04d340cd90de63
parent 460822 86b1e651aa0956e8fcbed6c63ff323b66545a3dd
child 460824 c2f10e92f864353ba95e5f48243f452b3d975bcc
push id165
push userfmarier@mozilla.com
push dateMon, 30 Apr 2018 23:50:51 +0000
reviewersflorian
bugs1447619
milestone61.0a1
Bug 1447619 - avoid reflowing when returning items to the toolbar from the overflow menu, r=florian MozReview-Commit-ID: BFGRssWb9F
browser/base/content/test/performance/browser_window_resize.js
browser/components/customizableui/CustomizableUI.jsm
browser/components/customizableui/test/browser_976792_insertNodeInWindow.js
browser/components/places/content/browserPlacesViews.js
--- a/browser/base/content/test/performance/browser_window_resize.js
+++ b/browser/base/content/test/performance/browser_window_resize.js
@@ -8,37 +8,19 @@
  * is a whitelist that should slowly go away as we improve the performance of
  * the front-end. Instead of adding more reflows to the whitelist, you should
  * be modifying your code to avoid the reflow.
  *
  * See https://developer.mozilla.org/en-US/Firefox/Performance_best_practices_for_Firefox_fe_engineers
  * for tips on how to do that.
  */
 const EXPECTED_REFLOWS = [
-  {
-    stack: [
-      "onOverflow@resource:///modules/CustomizableUI.jsm",
-    ],
-    maxCount: 48,
-  },
-
-  {
-    stack: [
-      "_moveItemsBackToTheirOrigin@resource:///modules/CustomizableUI.jsm",
-      "_onLazyResize@resource:///modules/CustomizableUI.jsm",
-    ],
-    maxCount: 5,
-  },
-
-  {
-    stack: [
-      "_onLazyResize@resource:///modules/CustomizableUI.jsm",
-    ],
-    maxCount: 4,
-  },
+   /**
+   * Nothing here! Please don't add anything new!
+   */
 ];
 
 const gToolbar = document.getElementById("PersonalToolbar");
 
 /**
  * Sets the visibility state on the Bookmarks Toolbar, and
  * waits for it to transition to fully visible.
  *
--- a/browser/components/customizableui/CustomizableUI.jsm
+++ b/browser/components/customizableui/CustomizableUI.jsm
@@ -4403,28 +4403,46 @@ OverflowableToolbar.prototype = {
     doc.defaultView.updateEditUIVisibility();
     let contextMenuId = this._panel.getAttribute("context");
     if (contextMenuId) {
       let contextMenu = doc.getElementById(contextMenuId);
       gELS.removeSystemEventListener(contextMenu, "command", this, true);
     }
   },
 
-  onOverflow(aEvent) {
-    // The rangeParent check is here because of bug 1111986 and ensuring that
-    // overflow events from the bookmarks toolbar items or similar things that
-    // manage their own overflow don't trigger an overflow on the entire toolbar
-    if (!this._enabled ||
-        (aEvent && aEvent.target != this._toolbar.customizationTarget) ||
-        (aEvent && aEvent.rangeParent))
+  /**
+   * Avoid re-entrancy in the overflow handling by keeping track of invocations:
+   */
+  _lastOverflowCounter: 0,
+
+  /**
+   * Handle overflow in the toolbar by moving items to the overflow menu.
+   * @param {Event} aEvent
+   *        The overflow event that triggered handling overflow. May be omitted
+   *        in some cases (e.g. when we run this method after overflow handling
+   *        is re-enabled from customize mode, to ensure correct handling of
+   *        initial overflow).
+   */
+  async onOverflow(aEvent) {
+    if (!this._enabled)
       return;
 
     let child = this._target.lastChild;
 
-    while (child && this._target.scrollLeftMin != this._target.scrollLeftMax) {
+    let thisOverflowResponse = ++this._lastOverflowCounter;
+
+    let win = this._target.ownerGlobal;
+    let [scrollLeftMin, scrollLeftMax] = await win.promiseDocumentFlushed(() => {
+      return [this._target.scrollLeftMin, this._target.scrollLeftMax];
+    });
+    if (win.closed || this._lastOverflowCounter != thisOverflowResponse) {
+      return;
+    }
+
+    while (child && scrollLeftMin != scrollLeftMax) {
       let prevChild = child.previousSibling;
 
       if (child.getAttribute("overflows") != "false") {
         this._collapsed.set(child.id, this._target.clientWidth);
         child.setAttribute("overflowedItem", true);
         child.setAttribute("cui-anchorid", this._chevron.id);
         CustomizableUIInternal.ensureButtonContextMenu(child, this._toolbar, true);
         CustomizableUIInternal.notifyListeners("onWidgetOverflow", child, this._target);
@@ -4433,40 +4451,70 @@ OverflowableToolbar.prototype = {
         if (!this._addedListener) {
           CustomizableUI.addListener(this);
         }
         if (!CustomizableUI.isSpecialWidget(child.id)) {
           this._toolbar.setAttribute("overflowing", "true");
         }
       }
       child = prevChild;
-    }
-
-    let win = this._target.ownerGlobal;
+      [scrollLeftMin, scrollLeftMax] = await win.promiseDocumentFlushed(() => {
+        return [this._target.scrollLeftMin, this._target.scrollLeftMax];
+      });
+      // If the window has closed or if we re-enter because we were waiting
+      // for layout, stop.
+      if (win.closed || this._lastOverflowCounter != thisOverflowResponse) {
+        return;
+      }
+    }
+
     win.UpdateUrlbarSearchSplitterState();
+    // Reset the counter because we finished handling overflow.
+    this._lastOverflowCounter = 0;
   },
 
   _onResize(aEvent) {
+    // Ignore bubbled-up resize events.
+    if (aEvent.target != aEvent.target.ownerGlobal.top) {
+      return;
+    }
     if (!this._lazyResizeHandler) {
       this._lazyResizeHandler = new DeferredTask(this._onLazyResize.bind(this),
                                                  LAZY_RESIZE_INTERVAL_MS, 0);
     }
     this._lazyResizeHandler.arm();
   },
 
-  _moveItemsBackToTheirOrigin(shouldMoveAllItems) {
+  /**
+   * Try to move toolbar items back to the toolbar from the overflow menu.
+   * @param {boolean} shouldMoveAllItems
+   *        Whether we should move everything (e.g. because we're being disabled)
+   * @param {number} targetWidth
+   *        Optional; the width of the toolbar in which we can put things.
+   *        Some consumers pass this to avoid reflows.
+   *        While there are items in the list, this width won't change, and so
+   *        we can avoid flushing layout by providing it and/or caching it.
+   *        Note that if `shouldMoveAllItems` is true, we never need the width
+   *        anyway.
+   */
+  _moveItemsBackToTheirOrigin(shouldMoveAllItems, targetWidth) {
     let placements = gPlacements.get(this._toolbar.id);
+    let win = this._target.ownerGlobal;
     while (this._list.firstChild) {
       let child = this._list.firstChild;
       let minSize = this._collapsed.get(child.id);
 
-      if (!shouldMoveAllItems &&
-          minSize &&
-          this._target.clientWidth <= minSize) {
-        break;
+      if (!shouldMoveAllItems && minSize) {
+        if (!targetWidth) {
+          let dwu = win.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
+          targetWidth = Math.floor(dwu.getBoundsWithoutFlushing(this._target).width);
+        }
+        if (targetWidth <= minSize) {
+          break;
+        }
       }
 
       this._collapsed.delete(child.id);
       let beforeNodeIndex = placements.indexOf(child.id) + 1;
       // If this is a skipintoolbarset item, meaning it doesn't occur in the placements list,
       // we're inserting it at the end. This will mean first-in, first-out (more or less)
       // leading to as little change in order as possible.
       if (beforeNodeIndex == 0) {
@@ -4488,37 +4536,43 @@ OverflowableToolbar.prototype = {
         this._target.appendChild(child);
       }
       child.removeAttribute("cui-anchorid");
       child.removeAttribute("overflowedItem");
       CustomizableUIInternal.ensureButtonContextMenu(child, this._target);
       CustomizableUIInternal.notifyListeners("onWidgetUnderflow", child, this._target);
     }
 
-    let win = this._target.ownerGlobal;
     win.UpdateUrlbarSearchSplitterState();
 
     let collapsedWidgetIds = Array.from(this._collapsed.keys());
     if (collapsedWidgetIds.every(w => CustomizableUI.isSpecialWidget(w))) {
       this._toolbar.removeAttribute("overflowing");
     }
     if (this._addedListener && !this._collapsed.size) {
       CustomizableUI.removeListener(this);
       this._addedListener = false;
     }
   },
 
-  _onLazyResize() {
+  async _onLazyResize() {
     if (!this._enabled)
       return;
 
-    if (this._target.scrollLeftMin != this._target.scrollLeftMax) {
+    let win = this._target.ownerGlobal;
+    let [min, max, targetWidth] = await win.promiseDocumentFlushed(() => {
+      return [this._target.scrollLeftMin, this._target.scrollLeftMax, this._target.clientWidth];
+    });
+    if (win.closed) {
+      return;
+    }
+    if (min != max) {
       this.onOverflow();
     } else {
-      this._moveItemsBackToTheirOrigin();
+      this._moveItemsBackToTheirOrigin(false, targetWidth);
     }
   },
 
   _disable() {
     this._enabled = false;
     this._moveItemsBackToTheirOrigin(true);
     if (this._lazyResizeHandler) {
       this._lazyResizeHandler.disarm();
@@ -4603,17 +4657,17 @@ OverflowableToolbar.prototype = {
     } else if (aNode.previousSibling) {
       // but if it still is, it must have changed places. Bookkeep:
       let prevId = aNode.previousSibling.id;
       let minSize = this._collapsed.get(prevId);
       this._collapsed.set(aNode.id, minSize);
     } else {
       // If it's now the first item in the overflow list,
       // maybe we can return it:
-      this._moveItemsBackToTheirOrigin();
+      this._moveItemsBackToTheirOrigin(false);
     }
   },
 
   findOverflowedInsertionPoints(aNode) {
     let newNodeCanOverflow = aNode.getAttribute("overflows") != "false";
     let areaId = this._toolbar.id;
     let placements = gPlacements.get(areaId);
     let nodeIndex = placements.indexOf(aNode.id);
--- a/browser/components/customizableui/test/browser_976792_insertNodeInWindow.js
+++ b/browser/components/customizableui/test/browser_976792_insertNodeInWindow.js
@@ -221,17 +221,24 @@ add_task(async function() {
   }
 
   for (let id of widgetIds) {
     document.getElementById(id).style.minWidth = "200px";
   }
 
   let originalWindowWidth = window.outerWidth;
   window.resizeTo(kForceOverflowWidthPx, window.outerHeight);
-  await waitForCondition(() => navbar.hasAttribute("overflowing"));
+  // Wait for all the widgets to overflow. We can't just wait for the
+  // `overflowing` attribute because we leave time for layout flushes
+  // inbetween, so it's possible for the timeout to run before the
+  // navbar has "settled"
+  await waitForCondition(() => {
+    return navbar.hasAttribute("overflowing") &&
+      navbar.customizationTarget.lastChild.getAttribute("overflows") == "false";
+  });
 
   // Find last widget that doesn't allow overflowing
   let nonOverflowing = navbar.customizationTarget.lastChild;
   is(nonOverflowing.getAttribute("overflows"), "false", "Last child is expected to not allow overflowing");
   isnot(nonOverflowing.getAttribute("skipintoolbarset"), "true", "Last child is expected to not be skipintoolbarset");
 
   let testWidgetId = kTestWidgetPrefix + 10;
   CustomizableUI.destroyWidget(testWidgetId);
--- a/browser/components/places/content/browserPlacesViews.js
+++ b/browser/components/places/content/browserPlacesViews.js
@@ -1167,21 +1167,25 @@ PlacesToolbar.prototype = {
         // the overflow status of the toolbar.
         if (aEvent.target == aEvent.currentTarget) {
           this.updateNodesVisibility();
         }
         break;
       case "overflow":
         if (!this._isOverflowStateEventRelevant(aEvent))
           return;
+        // Avoid triggering overflow in containers if possible
+        aEvent.stopPropagation();
         this._onOverflow();
         break;
       case "underflow":
         if (!this._isOverflowStateEventRelevant(aEvent))
           return;
+        // Avoid triggering underflow in containers if possible
+        aEvent.stopPropagation();
         this._onUnderflow();
         break;
       case "TabOpen":
       case "TabClose":
         this.updateNodesVisibility();
         break;
       case "dragstart":
         this._onDragStart(aEvent);