Bug 1800967 - Update internal OverflowableToolbar state when unpinning an extension button from the toolbar when overflowed. r=willdurand,Gijs

We have some internal bookkeeping within OverflowableToolbar to remember the state of things that
have overflowed, like how wide the window needs to be before they can be moved back, etc. When an
item is removed from an overflowable toolbar while overflowed, we update that internal bookkeeping
so that OverflowableToolbar doesn't accidentally try to move those items back into the toolbar
when the window becomes wide enough again.

We've added a new overflow list for extension buttons, but we weren't updating our internal accounting
when items had been overflowed into that list. This patch fixes that.

Differential Revision: https://phabricator.services.mozilla.com/D162434

browser/components/customizableui/CustomizableUI.jsm

@@ -5614,7 +5614,8 @@ class OverflowableToolbar {
 
   /**
    * Allows callers to query for the current parent of a toolbar item that may
-   * or may not be overflowed. That parent will either be #defaultList or #target.
+   * or may not be overflowed. That parent will either be #defaultList,
+   * #webExtList (if it's an extension button) or #target.
    *
    * Note: It is assumed that the caller has verified that aNode is placed
    * within the toolbar customizable area according to CustomizableUI.
@@ -5625,7 +5626,9 @@ class OverflowableToolbar {
    */
   getContainerFor(aNode) {
     if (aNode.getAttribute("overflowedItem") == "true") {
-      return this.#defaultList;
+      return CustomizableUI.isWebExtensionWidget(aNode.id)
+        ? this.#webExtList
+        : this.#defaultList;
     }
     return this.#target;
   }
@@ -6003,6 +6006,17 @@ class OverflowableToolbar {
     return this.#webExtList;
   }
 
+  /**
+   * Returns true if aNode is not null and is one of either this.#webExtList or
+   * this.#defaultList.
+   *
+   * @param {DOMElement} aNode The node to test.
+   * @returns {boolean}
+   */
+  #isOverflowList(aNode) {
+    return aNode && (aNode == this.#defaultList || aNode == this.#webExtList);
+  }
+
   /**
    * Private event handlers start here.
    */
@@ -6075,30 +6089,25 @@ class OverflowableToolbar {
     // moved or removed from an area via the CustomizableUI API while
     // overflowed. It reorganizes the internal state of this OverflowableToolbar
     // to handle that change.
-    if (
-      !this.#enabled ||
-      (aContainer != this.#target && aContainer != this.#defaultList)
-    ) {
+    if (!this.#enabled || !this.#isOverflowList(aContainer)) {
       return;
     }
     // When we (re)move an item, update all the items that come after it in the list
     // with the minsize *of the item before the to-be-removed node*. This way, we
     // ensure that we try to move items back as soon as that's possible.
-    if (aNode.parentNode == this.#defaultList) {
-      let updatedMinSize;
-      if (aNode.previousElementSibling) {
-        updatedMinSize = this.#overflowedInfo.get(
-          aNode.previousElementSibling.id
-        );
-      } else {
-        // Force (these) items to try to flow back into the bar:
-        updatedMinSize = 1;
-      }
-      let nextItem = aNode.nextElementSibling;
-      while (nextItem) {
-        this.#overflowedInfo.set(nextItem.id, updatedMinSize);
-        nextItem = nextItem.nextElementSibling;
-      }
+    let updatedMinSize;
+    if (aNode.previousElementSibling) {
+      updatedMinSize = this.#overflowedInfo.get(
+        aNode.previousElementSibling.id
+      );
+    } else {
+      // Force (these) items to try to flow back into the bar:
+      updatedMinSize = 1;
+    }
+    let nextItem = aNode.nextElementSibling;
+    while (nextItem) {
+      this.#overflowedInfo.set(nextItem.id, updatedMinSize);
+      nextItem = nextItem.nextElementSibling;
     }
   }
 
@@ -6109,12 +6118,12 @@ class OverflowableToolbar {
     // causes overflow or underflow of the toolbar.
     if (
       !this.#enabled ||
-      (aContainer != this.#target && aContainer != this.#defaultList)
+      (aContainer != this.#target && !this.#isOverflowList(aContainer))
     ) {
       return;
     }
 
-    let nowOverflowed = aNode.parentNode == this.#defaultList;
+    let nowOverflowed = this.#isOverflowList(aNode.parentNode);
     let wasOverflowed = this.#overflowedInfo.has(aNode.id);
 
     // If this wasn't overflowed before...

browser/components/extensions/test/browser/browser_unified_extensions_overflowable_toolbar.js

@@ -10,6 +10,10 @@
 
 loadTestSubscript("head_unified_extensions.js");
 
+const { ExtensionCommon } = ChromeUtils.import(
+  "resource://gre/modules/ExtensionCommon.jsm"
+);
+
 const NUM_EXTENSIONS = 5;
 const OVERFLOW_WINDOW_WIDTH_PX = 450;
 const DEFAULT_WIDGET_IDS = [
@@ -85,11 +89,13 @@ function getVisibleMenuItems(popup) {
  *     {Element} unifiedExtensionList: The DOM element that holds overflowed
  *       WebExtension browser_actions when Unified Extensions is enabled.
  *     {string[]} extensionIDs: The IDs of the test WebExtensions.
+ * @param {Function} afterUnderflowFn An optional async function that will be run
+ *   once the toolbar underflows, before the extensions are removed.
  *
  *   The function is expected to return a Promise that does not resolve
  *   with anything.
  */
-async function withWindowOverflowed(win, taskFn) {
+async function withWindowOverflowed(win, taskFn, afterUnderflowFn) {
   const doc = win.document;
   doc.documentElement.removeAttribute("persist");
   const navbar = doc.getElementById(CustomizableUI.AREA_NAVBAR);
@@ -277,7 +283,13 @@ async function withWindowOverflowed(win, taskFn) {
         .hasAttribute("overflowedItem");
     });
 
-    await Promise.all(extensions.map(extension => extension.unload()));
+    try {
+      if (afterUnderflowFn) {
+        await afterUnderflowFn();
+      }
+    } finally {
+      await Promise.all(extensions.map(extension => extension.unload()));
+    }
   }
 }
 
@@ -394,6 +406,7 @@ async function verifyExtensionWidget(win, widget, unifiedExtensionsEnabled) {
  */
 add_task(async function test_overflowable_toolbar() {
   let win = await promiseEnableUnifiedExtensions();
+  let movedNode;
 
   await withWindowOverflowed(
     win,
@@ -423,6 +436,33 @@ add_task(async function test_overflowable_toolbar() {
         );
         await verifyExtensionWidget(win, child, true);
       }
+
+      let extensionWidgetID = `${ExtensionCommon.makeWidgetId(
+        extensionIDs.at(-1)
+      )}-browser-action`;
+      movedNode = CustomizableUI.getWidget(extensionWidgetID).forWindow(win)
+        .node;
+      Assert.equal(movedNode.getAttribute("cui-areatype"), "toolbar");
+
+      CustomizableUI.addWidgetToArea(
+        extensionWidgetID,
+        CustomizableUI.AREA_ADDONS
+      );
+
+      Assert.equal(
+        movedNode.getAttribute("cui-areatype"),
+        "panel",
+        "The moved browser action button should have the right cui-areatype set."
+      );
+    },
+    async () => {
+      // Ensure that the moved node's parent is still the add-ons panel.
+      Assert.equal(
+        movedNode.parentElement.id,
+        CustomizableUI.AREA_ADDONS,
+        "The browser action should still be in the addons panel"
+      );
+      CustomizableUI.addWidgetToArea(movedNode.id, CustomizableUI.AREA_NAVBAR);
     }
   );