Bug 1391082 - Page action panel ordering can get messed up. r?Gijs draft
authorDrew Willcoxon <adw@mozilla.com>
Thu, 17 Aug 2017 16:40:35 -0700
changeset 648564 85a23f0959d9b133ca2c37292e2408d5c7f99ac6
parent 648384 e365137fa61bfd729617ba1ebf9f1ed79facd1f2
child 649191 52942d66284eed77dc49c9788f90682c0c944636
push id74794
push userdwillcoxon@mozilla.com
push dateThu, 17 Aug 2017 23:40:49 +0000
reviewersGijs
bugs1391082
milestone57.0a1
Bug 1391082 - Page action panel ordering can get messed up. r?Gijs MozReview-Commit-ID: 3hQLTLF3RU8
browser/base/content/browser-pageActions.js
browser/modules/PageActions.jsm
browser/modules/test/browser/browser_PageActions.js
--- a/browser/base/content/browser-pageActions.js
+++ b/browser/base/content/browser-pageActions.js
@@ -43,50 +43,48 @@ var BrowserPageActions = {
     return this.mainViewBodyNode = this.mainViewNode.querySelector(".panel-subview-body");
   },
 
   /**
    * Inits.  Call to init.
    */
   init() {
     for (let action of PageActions.actions) {
-      this.placeAction(action, PageActions.insertBeforeActionIDInUrlbar(action));
+      this.placeAction(action,
+                       PageActions.insertBeforeActionIDInPanel(action),
+                       PageActions.insertBeforeActionIDInUrlbar(action));
     }
   },
 
   /**
    * Adds or removes as necessary DOM nodes for the given action.
    *
    * @param  action (PageActions.Action, required)
    *         The action to place.
    * @param  panelInsertBeforeID (string, required)
    *         The ID of the action in the panel before which the given action
    *         action should be inserted.
    * @param  urlbarInsertBeforeID (string, required)
    *         If the action is shown in the urlbar, then this is ID of the action
    *         in the urlbar before which the given action should be inserted.
    */
   placeAction(action, panelInsertBeforeID, urlbarInsertBeforeID) {
-    if (action.__isSeparator) {
-      this._appendPanelSeparator(action);
-      return;
-    }
     action.onBeforePlacedInWindow(window);
     this.placeActionInPanel(action, panelInsertBeforeID);
     this.placeActionInUrlbar(action, urlbarInsertBeforeID);
   },
 
   /**
    * Adds or removes as necessary DOM nodes for the action in the panel.
    *
    * @param  action (PageActions.Action, required)
    *         The action to place.
    * @param  insertBeforeID (string, required)
    *         The ID of the action in the panel before which the given action
-   *         action should be inserted.
+   *         action should be inserted.  Pass null to append.
    */
   placeActionInPanel(action, insertBeforeID) {
     let id = this._panelButtonNodeIDForActionID(action.id);
     let node = document.getElementById(id);
     if (!node) {
       let panelViewNode;
       [node, panelViewNode] = this._makePanelButtonNodeForAction(action);
       node.id = id;
@@ -101,16 +99,21 @@ var BrowserPageActions = {
       if (panelViewNode) {
         action.subview.onPlaced(panelViewNode);
       }
     }
     return node;
   },
 
   _makePanelButtonNodeForAction(action) {
+    if (action.__isSeparator) {
+      let node = document.createElement("toolbarseparator");
+      return [node, null];
+    }
+
     let buttonNode = document.createElement("toolbarbutton");
     buttonNode.classList.add(
       "subviewbutton",
       "subviewbutton-iconic",
       "pageAction-panel-button"
     );
     buttonNode.setAttribute("label", action.title);
     if (action.iconURL) {
@@ -358,22 +361,16 @@ var BrowserPageActions = {
         this._toggleActivatedActionPanelForAction(action);
         return;
       }
       action.onCommand(event, buttonNode);
     });
     return buttonNode;
   },
 
-  _appendPanelSeparator(action) {
-    let node = document.createElement("toolbarseparator");
-    node.id = this._panelButtonNodeIDForActionID(action.id);
-    this.mainViewBodyNode.appendChild(node);
-  },
-
   /**
    * Removes all the DOM nodes of the given action.
    *
    * @param  action (PageActions.Action, required)
    *         The action to remove.
    */
   removeAction(action) {
     this._removeActionFromPanel(action);
--- a/browser/modules/PageActions.jsm
+++ b/browser/modules/PageActions.jsm
@@ -37,17 +37,17 @@ this.PageActions = {
   init() {
     let callbacks = this._deferredAddActionCalls;
     delete this._deferredAddActionCalls;
 
     this._loadPersistedActions();
 
     // Add the built-in actions, which are defined below in this file.
     for (let options of gBuiltInActions) {
-      if (options._isSeparator || !this.actionForID(options.id)) {
+      if (!this.actionForID(options.id)) {
         this.addAction(new Action(options));
       }
     }
 
     // These callbacks are deferred until init happens and all built-in actions
     // are added.
     while (callbacks && callbacks.length) {
       callbacks.shift()();
@@ -60,23 +60,26 @@ this.PageActions = {
    * The list of Action objects, sorted in the order in which they should be
    * placed in the page action panel.  If there are both built-in and non-built-
    * in actions, then the list will include the separator between the two.  The
    * list is not live.  (array of Action objects)
    */
   get actions() {
     let actions = this.builtInActions;
     if (this.nonBuiltInActions.length) {
-      // There are non-built-in actions, so include them too.  Add a separator
-      // between the built-ins and non-built-ins so that the returned array
-      // looks like: [...built-ins, separator, ...non-built-ins]
-      actions.push(new Action({
-        id: ACTION_ID_BUILT_IN_SEPARATOR,
-        _isSeparator: true,
-      }));
+      // There are non-built-in actions, so include them too.
+      if (actions.length) {
+        // There are both built-in and non-built-in actions.  Add a separator
+        // between the two groups so that the returned array looks like:
+        // [...built-ins, separator, ...non-built-ins]
+        actions.push(new Action({
+          id: ACTION_ID_BUILT_IN_SEPARATOR,
+          _isSeparator: true,
+        }));
+      }
       actions.push(...this.nonBuiltInActions);
     }
     return actions;
   },
 
   /**
    * The list of built-in actions.  Not live.  (array of Action objects)
    */
@@ -120,120 +123,152 @@ this.PageActions = {
   addAction(action) {
     if (this._deferredAddActionCalls) {
       // init() hasn't been called yet.  Defer all additions until it's called,
       // at which time _deferredAddActionCalls will be deleted.
       this._deferredAddActionCalls.push(() => this.addAction(action));
       return action;
     }
 
+    if (this.actionForID(action.id)) {
+      throw new Error(`Action with ID '${action.id}' already added`);
+    }
+    this._actionsByID.set(action.id, action);
+
     // The IDs of the actions in the panel and urlbar before which the new
     // action shoud be inserted.  null means at the end, or it's irrelevant.
     let panelInsertBeforeID = null;
     let urlbarInsertBeforeID = null;
 
-    let placeBuiltInSeparator = false;
+    let oldBuiltInCount = this._builtInActions.length;
+    let oldNonBuiltInCount = this._nonBuiltInActions.length;
 
-    if (action.__isSeparator) {
-      this._builtInActions.push(action);
-    } else {
-      if (this.actionForID(action.id)) {
-        throw new Error(`An Action with ID '${action.id}' has already been added.`);
-      }
-      this._actionsByID.set(action.id, action);
-
-      // Insert the action into the appropriate list, either _builtInActions or
-      // _nonBuiltInActions, and find panelInsertBeforeID.
+    // Insert the action into the appropriate list, either _builtInActions or
+    // _nonBuiltInActions, and find panelInsertBeforeID.
 
-      // Keep in mind that _insertBeforeActionID may be present but null, which
-      // means the action should be appended to the built-ins.
-      if ("__insertBeforeActionID" in action) {
-        // A "semi-built-in" action, probably an action from an extension
-        // bundled with the browser.  Right now we simply assume that no other
-        // consumers will use _insertBeforeActionID.
-        let index =
-          !action.__insertBeforeActionID ? -1 :
-          this._builtInActions.findIndex(a => {
-            return a.id == action.__insertBeforeActionID;
-          });
-        if (index < 0) {
-          // Append the action.
-          index = this._builtInActions.length;
-          if (this._nonBuiltInActions.length) {
-            panelInsertBeforeID = ACTION_ID_BUILT_IN_SEPARATOR;
-          }
-        } else {
-          panelInsertBeforeID = this._builtInActions[index].id;
-        }
-        this._builtInActions.splice(index, 0, action);
-      } else if (gBuiltInActions.find(a => a.id == action.id)) {
-        // A built-in action.  These are always added on init before all other
-        // actions, one after the other, so just push onto the array.
-        this._builtInActions.push(action);
+    // Keep in mind that _insertBeforeActionID may be present but null, which
+    // means the action should be appended to the built-ins.
+    if ("__insertBeforeActionID" in action) {
+      // A "semi-built-in" action, probably an action from an extension
+      // bundled with the browser.  Right now we simply assume that no other
+      // consumers will use _insertBeforeActionID.
+      let index =
+        !action.__insertBeforeActionID ? -1 :
+        this._builtInActions.findIndex(a => {
+          return a.id == action.__insertBeforeActionID;
+        });
+      if (index < 0) {
+        // Append the action.
+        index = this._builtInActions.length;
         if (this._nonBuiltInActions.length) {
           panelInsertBeforeID = ACTION_ID_BUILT_IN_SEPARATOR;
         }
       } else {
-        // A non-built-in action, like a non-bundled extension potentially.
-        // Keep this list sorted by title.
-        let index = BinarySearch.insertionIndexOf((a1, a2) => {
-          return a1.title.localeCompare(a2.title);
-        }, this._nonBuiltInActions, action);
-        if (index < this._nonBuiltInActions.length) {
-          panelInsertBeforeID = this._nonBuiltInActions[index].id;
-        }
-        // If this is the first non-built-in, then the built-in separator must
-        // be placed between the built-ins and non-built-ins.
-        if (!this._nonBuiltInActions.length) {
-          placeBuiltInSeparator = true;
-        }
-        this._nonBuiltInActions.splice(index, 0, action);
+        panelInsertBeforeID = this._builtInActions[index].id;
+      }
+      this._builtInActions.splice(index, 0, action);
+    } else if (gBuiltInActions.find(a => a.id == action.id)) {
+      // A built-in action.  These are always added on init before all other
+      // actions, one after the other, so just push onto the array.
+      this._builtInActions.push(action);
+      if (this._nonBuiltInActions.length) {
+        panelInsertBeforeID = ACTION_ID_BUILT_IN_SEPARATOR;
       }
+    } else {
+      // A non-built-in action, like a non-bundled extension potentially.
+      // Keep this list sorted by title.
+      let index = BinarySearch.insertionIndexOf((a1, a2) => {
+        return a1.title.localeCompare(a2.title);
+      }, this._nonBuiltInActions, action);
+      if (index < this._nonBuiltInActions.length) {
+        panelInsertBeforeID = this._nonBuiltInActions[index].id;
+      }
+      this._nonBuiltInActions.splice(index, 0, action);
+    }
 
-      if (this._persistedActions.ids[action.id]) {
-        // The action has been seen before.  Override its shownInUrlbar value
-        // with the persisted value.  Set the private version of that property
-        // so that onActionToggledShownInUrlbar isn't called, which happens when
-        // the public version is set.
-        action._shownInUrlbar =
-          this._persistedActions.idsInUrlbar.includes(action.id);
-      } else {
-        // The action is new.  Store it in the persisted actions.
-        this._persistedActions.ids[action.id] = true;
-        if (action.shownInUrlbar) {
-          this._persistedActions.idsInUrlbar.push(action.id);
-        }
-        this._storePersistedActions();
+    if (this._persistedActions.ids[action.id]) {
+      // The action has been seen before.  Override its shownInUrlbar value
+      // with the persisted value.  Set the private version of that property
+      // so that onActionToggledShownInUrlbar isn't called, which happens when
+      // the public version is set.
+      action._shownInUrlbar =
+        this._persistedActions.idsInUrlbar.includes(action.id);
+    } else {
+      // The action is new.  Store it in the persisted actions.
+      this._persistedActions.ids[action.id] = true;
+      if (action.shownInUrlbar) {
+        this._persistedActions.idsInUrlbar.push(action.id);
       }
+      this._storePersistedActions();
+    }
 
-      if (action.shownInUrlbar) {
-        urlbarInsertBeforeID = this.insertBeforeActionIDInUrlbar(action);
-      }
+    if (action.shownInUrlbar) {
+      urlbarInsertBeforeID = this.insertBeforeActionIDInUrlbar(action);
     }
 
+    // If there are now both built-in and non-built-in actions, add a separator
+    // in the panel between the two groups.
+    let placeBuiltInSeparator =
+      (oldNonBuiltInCount == 0 &&
+       this._nonBuiltInActions.length &&
+       this._builtInActions.length) ||
+      (oldBuiltInCount == 0 &&
+       this._builtInActions.length &&
+       this._nonBuiltInActions.length);
+
     for (let win of browserWindows()) {
       if (placeBuiltInSeparator) {
         let sep = new Action({
           id: ACTION_ID_BUILT_IN_SEPARATOR,
           _isSeparator: true,
         });
-        browserPageActions(win).placeAction(sep, null, null);
+        let sepPanelInsertBeforeID =
+          this._nonBuiltInActions.length ? this._nonBuiltInActions[0].id : null;
+        browserPageActions(win).placeAction(sep, sepPanelInsertBeforeID, null);
       }
       browserPageActions(win).placeAction(action, panelInsertBeforeID,
                                           urlbarInsertBeforeID);
     }
 
     return action;
   },
 
   _builtInActions: [],
   _nonBuiltInActions: [],
   _actionsByID: new Map(),
 
   /**
+   * Returns the ID of the action among the actions in the panel before which
+   * the given action should be inserted.
+   *
+   * @return The ID of the action before which the given action should be
+   *         inserted.  If the given action should be inserted last, returns
+   *         null.
+   */
+  insertBeforeActionIDInPanel(action) {
+    let index = this._builtInActions.findIndex(a => {
+      return a.id == action.id;
+    });
+    if (index >= 0) {
+      return this._builtInActions[index + 1] ||
+             this._nonBuiltInActions.length ? ACTION_ID_BUILT_IN_SEPARATOR
+                                            : null;
+    }
+
+    index = this._nonBuiltInActions.findIndex(a => {
+      return a.id == action.id;
+    });
+    if (index >= 0) {
+      return this._nonBuiltInActions[index + 1] || null;
+    }
+
+    return null;
+  },
+
+  /**
    * Returns the ID of the action among the current registered actions in the
    * urlbar before which the given action should be inserted, ignoring whether
    * the given action's shownInUrlbar is true or false.
    *
    * @return The ID of the action before which the given action should be
    *         inserted.  If the given action should be inserted last or it should
    *         not be inserted at all, returns null.
    */
--- a/browser/modules/test/browser/browser_PageActions.js
+++ b/browser/modules/test/browser/browser_PageActions.js
@@ -649,16 +649,123 @@ add_task(async function multipleNonBuilt
         PageActions.ACTION_ID_BUILT_IN_SEPARATOR
       )
     ),
     null,
     "Separator should be gone"
   );
 });
 
+// Makes sure the panel is correctly updated when a non-built-in action is
+// added before the built-in actions; and when all built-in actions are removed
+// and added back.
+add_task(async function nonBuiltFirst() {
+  let initialActions = PageActions.actions;
+
+  // Remove all actions.
+  for (let action of initialActions) {
+    action.remove();
+  }
+
+  // Check the actions.
+  Assert.deepEqual(PageActions.actions.map(a => a.id), [],
+                   "PageActions.actions should be empty");
+  Assert.deepEqual(PageActions.builtInActions.map(a => a.id), [],
+                   "PageActions.builtInActions should be empty");
+  Assert.deepEqual(PageActions.nonBuiltInActions.map(a => a.id), [],
+                   "PageActions.nonBuiltInActions should be empty");
+
+  // Check the panel.
+  Assert.equal(BrowserPageActions.mainViewBodyNode.childNodes.length, 0,
+               "All nodes should be gone");
+
+  // Add a non-built-in action.
+  let action = PageActions.addAction(new PageActions.Action({
+    id: "test-nonBuiltFirst",
+    title: "Test nonBuiltFirst",
+  }));
+
+  // Check the actions.
+  Assert.deepEqual(PageActions.actions.map(a => a.id), [action.id],
+                   "Action should be in PageActions.actions");
+  Assert.deepEqual(PageActions.builtInActions.map(a => a.id), [],
+                   "PageActions.builtInActions should be empty");
+  Assert.deepEqual(PageActions.nonBuiltInActions.map(a => a.id), [action.id],
+                   "Action should be in PageActions.nonBuiltInActions");
+
+  // Check the panel.
+  Assert.deepEqual(
+    Array.map(BrowserPageActions.mainViewBodyNode.childNodes, n => n.id),
+    [BrowserPageActions._panelButtonNodeIDForActionID(action.id)],
+    "Action should be in panel"
+  );
+
+  // Now add back all the actions.
+  for (let a of initialActions) {
+    PageActions.addAction(a);
+  }
+
+  // Check the actions.
+  Assert.deepEqual(
+    PageActions.actions.map(a => a.id),
+    initialActions.map(a => a.id).concat(
+      [PageActions.ACTION_ID_BUILT_IN_SEPARATOR],
+      [action.id]
+    ),
+    "All actions should be in PageActions.actions"
+  );
+  Assert.deepEqual(
+    PageActions.builtInActions.map(a => a.id),
+    initialActions.map(a => a.id),
+    "PageActions.builtInActions should be initial actions"
+  );
+  Assert.deepEqual(
+    PageActions.nonBuiltInActions.map(a => a.id),
+    [action.id],
+    "PageActions.nonBuiltInActions should contain action"
+  );
+
+  // Check the panel.
+  Assert.deepEqual(
+    Array.map(BrowserPageActions.mainViewBodyNode.childNodes, n => n.id),
+    initialActions.map(a => a.id).concat(
+      [PageActions.ACTION_ID_BUILT_IN_SEPARATOR],
+      [action.id]
+    ).map(id => BrowserPageActions._panelButtonNodeIDForActionID(id)),
+    "Panel should contain all actions"
+  );
+
+  // Remove the test action.
+  action.remove();
+
+  // Check the actions.
+  Assert.deepEqual(
+    PageActions.actions.map(a => a.id),
+    initialActions.map(a => a.id),
+    "Action should no longer be in PageActions.actions"
+  );
+  Assert.deepEqual(
+    PageActions.builtInActions.map(a => a.id),
+    initialActions.map(a => a.id),
+    "PageActions.builtInActions should be initial actions"
+  );
+  Assert.deepEqual(
+    PageActions.nonBuiltInActions.map(a => a.id),
+    [],
+    "Action should no longer be in PageActions.nonBuiltInActions"
+  );
+
+  // Check the panel.
+  Assert.deepEqual(
+    Array.map(BrowserPageActions.mainViewBodyNode.childNodes, n => n.id),
+    initialActions.map(a => BrowserPageActions._panelButtonNodeIDForActionID(a.id)),
+    "Action should no longer be in panel"
+  );
+});
+
 
 function promisePageActionPanelOpen() {
   let button = document.getElementById("pageActionButton");
   let shownPromise = promisePageActionPanelShown();
   EventUtils.synthesizeMouseAtCenter(button, {});
   return shownPromise;
 }