Bug 1391082 - Page action panel ordering can get messed up. r=Gijs
authorDrew Willcoxon <adw@mozilla.com>
Thu, 17 Aug 2017 16:40:35 -0700
changeset 375701 a08bb8b25b9b
parent 375700 089706dfb481
child 375702 d0892f7190ef
push id32361
push userphilringnalda@gmail.com
push dateSat, 19 Aug 2017 22:28:34 +0000
treeherdermozilla-central@c40774ced661 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1391082
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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;
 }