Bug 1447619 - avoid reflowing when returning items to the toolbar from the overflow menu, r=florian draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Fri, 20 Apr 2018 15:09:44 +0100
changeset 789862 cf6011f2ecfe78df11a6741dfa074c7d985ec294
parent 789560 c552490c8659bf2e7d279ed28d46fb5d5a245a96
push id108344
push userbmo:gijskruitbosch+bugs@gmail.com
push dateMon, 30 Apr 2018 16:15:33 +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_901207_searchbar_in_panel.js
browser/components/customizableui/test/browser_976792_insertNodeInWindow.js
browser/components/customizableui/test/browser_editcontrols_update.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_901207_searchbar_in_panel.js
+++ b/browser/components/customizableui/test/browser_901207_searchbar_in_panel.js
@@ -44,26 +44,29 @@ add_task(async function() {
 
   let hiddenPanelPromise = promiseOverflowHidden(window);
   EventUtils.synthesizeKey("KEY_Escape");
   await hiddenPanelPromise;
   CustomizableUI.reset();
 });
 
 // Ctrl+K should open the overflow panel and focus the search bar if the search bar is overflowed.
-add_task(async function() {
+add_task(async function check_shortcut_when_in_overflow() {
   this.originalWindowWidth = window.outerWidth;
   let navbar = document.getElementById(CustomizableUI.AREA_NAVBAR);
   ok(!navbar.hasAttribute("overflowing"), "Should start with a non-overflowing toolbar.");
   ok(CustomizableUI.inDefaultState, "Should start in default state.");
 
   Services.prefs.setBoolPref("browser.search.widget.inNavBar", true);
 
   window.resizeTo(kForceOverflowWidthPx, window.outerHeight);
-  await waitForCondition(() => navbar.getAttribute("overflowing") == "true");
+  await waitForCondition(() => {
+    return navbar.getAttribute("overflowing") == "true" &&
+      !navbar.querySelector("#search-container");
+  });
   ok(!navbar.querySelector("#search-container"), "Search container should be overflowing");
 
   let shownPanelPromise = promiseOverflowShown(window);
   sendWebSearchKeyCommand();
   await shownPanelPromise;
 
   let chevron = document.getElementById("nav-bar-overflow-button");
   await waitForCondition(() => chevron.open);
--- a/browser/components/customizableui/test/browser_976792_insertNodeInWindow.js
+++ b/browser/components/customizableui/test/browser_976792_insertNodeInWindow.js
@@ -63,17 +63,17 @@ 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"));
+  await waitForCondition(() => navbar.hasAttribute("overflowing") && !navbar.querySelector("#" + widgetIds[0]));
 
   let testWidgetId = kTestWidgetPrefix + 3;
 
   CustomizableUI.destroyWidget(testWidgetId);
 
   let btn = createDummyXULButton(testWidgetId, "test");
   CustomizableUI.ensureWidgetPlacedInWindow(testWidgetId, window);
 
@@ -112,17 +112,17 @@ 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"));
+  await waitForCondition(() => navbar.hasAttribute("overflowing") && !navbar.querySelector("#" + widgetIds[0]));
 
   let testWidgetId = kTestWidgetPrefix + 3;
 
   CustomizableUI.destroyWidget(kTestWidgetPrefix + 2);
   CustomizableUI.destroyWidget(testWidgetId);
 
   let btn = createDummyXULButton(testWidgetId, "test");
   CustomizableUI.ensureWidgetPlacedInWindow(testWidgetId, window);
@@ -162,17 +162,17 @@ 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"));
+  await waitForCondition(() => navbar.hasAttribute("overflowing") && !navbar.querySelector("#" + widgetIds[0]));
 
   let testWidgetId = kTestWidgetPrefix + 3;
 
   CustomizableUI.destroyWidget(kTestWidgetPrefix + 2);
   CustomizableUI.destroyWidget(testWidgetId);
   CustomizableUI.destroyWidget(kTestWidgetPrefix + 4);
 
   let btn = createDummyXULButton(testWidgetId, "test");
@@ -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);
@@ -282,17 +289,17 @@ add_task(async function() {
   }
 
   let toolbarNode = createOverflowableToolbarWithPlacements(kToolbarName, widgetIds);
   assertAreaPlacements(kToolbarName, widgetIds);
   ok(!toolbarNode.hasAttribute("overflowing"), "Toolbar shouldn't overflow to start with.");
 
   let originalWindowWidth = window.outerWidth;
   window.resizeTo(kForceOverflowWidthPx, window.outerHeight);
-  await waitForCondition(() => toolbarNode.hasAttribute("overflowing"));
+  await waitForCondition(() => toolbarNode.hasAttribute("overflowing") && !toolbarNode.querySelector("#" + widgetIds[1]));
   ok(toolbarNode.hasAttribute("overflowing"), "Should have an overflowing toolbar.");
 
   let btnId = kTestWidgetPrefix + missingId;
   let btn = createDummyXULButton(btnId, "test");
   CustomizableUI.ensureWidgetPlacedInWindow(btnId, window);
 
   is(btn.parentNode.id, kToolbarName + "-overflow-list", "New XUL widget should be placed inside new toolbar's overflow");
   is(btn.previousSibling.id, kTestWidgetPrefix + 1,
--- a/browser/components/customizableui/test/browser_editcontrols_update.js
+++ b/browser/components/customizableui/test/browser_editcontrols_update.js
@@ -86,17 +86,17 @@ add_task(async function test_panelui_ope
   await overridePromise;
   checkState(true, "Update when edit-controls is on panel, hidden and selection changed");
 });
 
 // Test updating when the edit-controls are moved to the toolbar.
 add_task(async function test_panelui_customize_to_toolbar() {
   await startCustomizing();
   let navbar = document.getElementById("nav-bar");
-  simulateItemDrag(document.getElementById("edit-controls"), navbar.customizationTarget);
+  simulateItemDrag(document.getElementById("edit-controls"), navbar.customizationTarget, "end");
   await endCustomizing();
 
   // updateEditUIVisibility should be called when customization ends but isn't. See bug 1359790.
   updateEditUIVisibility();
 
   // The URL bar may have been focused to begin with, which means
   // that subsequent calls to focus it won't result in command
   // updates, so we'll make sure to blur it.
@@ -120,17 +120,18 @@ add_task(async function test_panelui_cus
   registerCleanupFunction(async function() {
     kOverflowPanel.removeAttribute("animate");
     window.resizeTo(originalWidth, window.outerHeight);
     await waitForCondition(() => !navbar.hasAttribute("overflowing"));
     CustomizableUI.reset();
   });
 
   window.resizeTo(kForceOverflowWidthPx, window.outerHeight);
-  await waitForCondition(() => navbar.hasAttribute("overflowing"));
+  await waitForCondition(() =>
+    navbar.hasAttribute("overflowing") && !navbar.querySelector("edit-controls"));
 
   // Mac will update the enabled state even when the buttons are overflowing,
   // so main menubar shortcuts will work properly.
   overridePromise = expectCommandUpdate(isMac ? 1 : 0);
   gURLBar.select();
   await overridePromise;
   checkState(true, "Update when edit-controls is on overflow panel, hidden and selection changed");
 
--- a/browser/components/places/content/browserPlacesViews.js
+++ b/browser/components/places/content/browserPlacesViews.js
@@ -1165,21 +1165,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);