Bug 1396053 - Page action urlbar button ordering can get messed up for new windows. r?Gijs draft
authorDrew Willcoxon <adw@mozilla.com>
Mon, 04 Sep 2017 23:58:27 -0700
changeset 658958 a9742a4d659e51ee32cc1035d0043ccc5fc70acc
parent 658813 1401e3eec44df87963d3af329ef8a4183ab0483f
child 729819 ee46466271e076807d41ac817918ca8af63a4ea7
push id77948
push userdwillcoxon@mozilla.com
push dateTue, 05 Sep 2017 06:59:15 +0000
reviewersGijs
bugs1396053
milestone57.0a1
Bug 1396053 - Page action urlbar button ordering can get messed up for new windows. r?Gijs MozReview-Commit-ID: 1KwrS6tsnhO
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
@@ -42,51 +42,64 @@ var BrowserPageActions = {
     delete this.mainViewBodyNode;
     return this.mainViewBodyNode = this.mainViewNode.querySelector(".panel-subview-body");
   },
 
   /**
    * Inits.  Call to init.
    */
   init() {
+    this.placeAllActions();
+  },
+
+  /**
+   * Places all registered actions.
+   */
+  placeAllActions() {
+    // Place actions in the panel.  Notify of onBeforePlacedInWindow too.
     for (let action of PageActions.actions) {
-      this.placeAction(action,
-                       PageActions.insertBeforeActionIDInPanel(action),
-                       PageActions.insertBeforeActionIDInUrlbar(action));
+      action.onBeforePlacedInWindow(window);
+      this.placeActionInPanel(action);
+    }
+
+    // Place actions in the urlbar.  Do this in reverse order.  The reason is
+    // subtle.  If there were no urlbar nodes already in markup (like the
+    // bookmark star button), then doing this in forward order would be fine.
+    // Forward order means that the insert-before relationship is always broken:
+    // there's never a next-sibling node before which to insert a new node, so
+    // node.insertBefore() is always passed null, and nodes are always appended.
+    // That will break the position of nodes that should be inserted before
+    // nodes that are in markup, which in turn can break other nodes.
+    let actionsInUrlbar = PageActions.actionsInUrlbar;
+    for (let i = actionsInUrlbar.length - 1; i >= 0; i--) {
+      let action = actionsInUrlbar[i];
+      this.placeActionInUrlbar(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) {
+  placeAction(action) {
     action.onBeforePlacedInWindow(window);
-    this.placeActionInPanel(action, panelInsertBeforeID);
-    this.placeActionInUrlbar(action, urlbarInsertBeforeID);
+    this.placeActionInPanel(action);
+    this.placeActionInUrlbar(action);
   },
 
   /**
    * 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.  Pass null to append.
    */
-  placeActionInPanel(action, insertBeforeID) {
+  placeActionInPanel(action) {
+    let insertBeforeID = PageActions.nextActionID(action, PageActions.actions);
     let id = this._panelButtonNodeIDForActionID(action.id);
     let node = document.getElementById(id);
     if (!node) {
       let panelViewNode;
       [node, panelViewNode] = this._makePanelButtonNodeForAction(action);
       node.id = id;
       let insertBeforeNode = null;
       if (insertBeforeID) {
@@ -280,21 +293,20 @@ var BrowserPageActions = {
     return "pageActionActivatedActionPanel";
   },
 
   /**
    * Adds or removes as necessary a DOM node for the given action in the urlbar.
    *
    * @param  action (PageActions.Action, required)
    *         The action to place.
-   * @param  insertBeforeID (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.
    */
-  placeActionInUrlbar(action, insertBeforeID) {
+  placeActionInUrlbar(action) {
+    let insertBeforeID =
+      PageActions.nextActionID(action, PageActions.actionsInUrlbar);
     let id = this._urlbarButtonNodeIDForActionID(action.id);
     let node = document.getElementById(id);
 
     if (!action.shownInUrlbar) {
       if (node) {
         if (action.__urlbarNodeInMarkup) {
           node.hidden = true;
         } else {
--- a/browser/modules/PageActions.jsm
+++ b/browser/modules/PageActions.jsm
@@ -35,23 +35,32 @@ this.PageActions = {
    * Inits.  Call to init.
    */
   init() {
     let callbacks = this._deferredAddActionCalls;
     delete this._deferredAddActionCalls;
 
     this._loadPersistedActions();
 
-    // Add the built-in actions, which are defined below in this file.
+    // Register the built-in actions, which are defined below in this file.
     for (let options of gBuiltInActions) {
       if (!this.actionForID(options.id)) {
-        this.addAction(new Action(options));
+        this._registerAction(new Action(options));
       }
     }
 
+    // Now place them all in each window.  Instead of splitting the register and
+    // place steps, we could simply call addAction, which does both, but doing
+    // it this way means that all windows initially place their actions the same
+    // way -- placeAllActions -- regardless of whether they're open when this
+    // method is called or opened later.
+    for (let bpa of allBrowserPageActions()) {
+      bpa.placeAllActions();
+    }
+
     // These callbacks are deferred until init happens and all built-in actions
     // are added.
     while (callbacks && callbacks.length) {
       callbacks.shift()();
     }
   },
 
   _deferredAddActionCalls: [],
@@ -90,16 +99,37 @@ this.PageActions = {
   /**
    * The list of non-built-in actions.  Not live.  (array of Action objects)
    */
   get nonBuiltInActions() {
     return this._nonBuiltInActions.slice();
   },
 
   /**
+   * The list of actions in the urlbar, sorted in the order in which they should
+   * appear there.  Not live.  (array of Action objects)
+   */
+  get actionsInUrlbar() {
+    if (!this._persistedActions) {
+      // This is the case before init is called.  No one should be calling us
+      // then, but return something sensible.
+      return [];
+    }
+    // Remember that IDs in idsInUrlbar may belong to actions that aren't
+    // currently registered.
+    return this._persistedActions.idsInUrlbar.reduce((actions, id) => {
+      let action = this.actionForID(id);
+      if (action) {
+        actions.push(action);
+      }
+      return actions;
+    }, []);
+  },
+
+  /**
    * Gets an action.
    *
    * @param  id (string, required)
    *         The ID of the action to get.
    * @return The Action object, or null if none.
    */
   actionForID(id) {
     return this._actionsByID.get(id);
@@ -123,69 +153,72 @@ 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;
     }
 
+    let hadSep = this.actions.some(a => a.id == ACTION_ID_BUILT_IN_SEPARATOR);
+
+    this._registerAction(action);
+
+    let sep = null;
+    if (!hadSep) {
+      sep = this.actions.find(a => a.id == ACTION_ID_BUILT_IN_SEPARATOR);
+    }
+
+    for (let bpa of allBrowserPageActions()) {
+      if (sep) {
+        // There are now both built-in and non-built-in actions, so place the
+        // separator between the two groups.
+        bpa.placeAction(sep);
+      }
+      bpa.placeAction(action);
+    }
+
+    return action;
+  },
+
+  _registerAction(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 oldBuiltInCount = this._builtInActions.length;
-    let oldNonBuiltInCount = this._nonBuiltInActions.length;
-
     // Insert the action into the appropriate list, either _builtInActions or
-    // _nonBuiltInActions, and find panelInsertBeforeID.
+    // _nonBuiltInActions.
 
     // 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);
-      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.
@@ -194,106 +227,51 @@ this.PageActions = {
     } 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 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,
-        });
-        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.
+   * The DOM nodes of actions should be ordered properly, both in the panel and
+   * the urlbar.  This method returns the ID of the action that comes after the
+   * given action in the given array.  You can use the returned ID to get a DOM
+   * node ID to pass to node.insertBefore().
    *
+   * Pass PageActions.actions to get the ID of the next action in the panel.
+   * Pass PageActions.actionsInUrlbar to get the ID of the next action in the
+   * urlbar.
+   *
+   * @param  action
+   *         The action whose node you want to insert into your DOM.
+   * @param  actionArray
+   *         The relevant array of actions, either PageActions.actions or
+   *         actionsInUrlbar.
    * @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.
-   */
-  insertBeforeActionIDInUrlbar(action) {
-    // First, find the index of the given action.
-    let idsInUrlbar = this._persistedActions.idsInUrlbar;
-    let index = idsInUrlbar.indexOf(action.id);
+  nextActionID(action, actionArray) {
+    let index = actionArray.findIndex(a => a.id == action.id);
     if (index < 0) {
       return null;
     }
-    // Now start at the next index and find the ID of the first action that's
-    // currently registered.  Remember that IDs in idsInUrlbar may belong to
-    // actions that aren't currently registered.
-    for (let i = index + 1; i < idsInUrlbar.length; i++) {
-      let id = idsInUrlbar[i];
-      if (this.actionForID(id)) {
-        return id;
-      }
+    let nextAction = actionArray[index + 1];
+    if (!nextAction) {
+      return null;
     }
-    return null;
+    return nextAction.id;
   },
 
   /**
    * Call this when an action is removed.
    *
    * @param  action (Action object, required)
    *         The action that was removed.
    */
@@ -315,50 +293,50 @@ this.PageActions = {
     // Remove the action from persisted storage.
     delete this._persistedActions.ids[action.id];
     let index = this._persistedActions.idsInUrlbar.indexOf(action.id);
     if (index >= 0) {
       this._persistedActions.idsInUrlbar.splice(index, 1);
     }
     this._storePersistedActions();
 
-    for (let win of browserWindows()) {
-      browserPageActions(win).removeAction(action);
+    for (let bpa of allBrowserPageActions()) {
+      bpa.removeAction(action);
     }
   },
 
   /**
    * Call this when an action's iconURL changes.
    *
    * @param  action (Action object, required)
    *         The action whose iconURL property changed.
    */
   onActionSetIconURL(action) {
     if (!this.actionForID(action.id)) {
       // This may be called before the action has been added.
       return;
     }
-    for (let win of browserWindows()) {
-      browserPageActions(win).updateActionIconURL(action);
+    for (let bpa of allBrowserPageActions()) {
+      bpa.updateActionIconURL(action);
     }
   },
 
   /**
    * Call this when an action's title changes.
    *
    * @param  action (Action object, required)
    *         The action whose title property changed.
    */
   onActionSetTitle(action) {
     if (!this.actionForID(action.id)) {
       // This may be called before the action has been added.
       return;
     }
-    for (let win of browserWindows()) {
-      browserPageActions(win).updateActionTitle(action);
+    for (let bpa of allBrowserPageActions()) {
+      bpa.updateActionTitle(action);
     }
   },
 
   /**
    * Call this when an action's shownInUrlbar property changes.
    *
    * @param  action (Action object, required)
    *         The action whose shownInUrlbar property changed.
@@ -375,19 +353,18 @@ this.PageActions = {
       if (index < 0) {
         this._persistedActions.idsInUrlbar.push(action.id);
       }
     } else if (index >= 0) {
       this._persistedActions.idsInUrlbar.splice(index, 1);
     }
     this._storePersistedActions();
 
-    let insertBeforeID = this.insertBeforeActionIDInUrlbar(action);
-    for (let win of browserWindows()) {
-      browserPageActions(win).placeActionInUrlbar(action, insertBeforeID);
+    for (let bpa of allBrowserPageActions()) {
+      bpa.placeActionInUrlbar(action);
     }
   },
 
   _storePersistedActions() {
     let json = JSON.stringify(this._persistedActions);
     Services.prefs.setStringPref(PREF_PERSISTED_ACTIONS, json);
   },
 
@@ -1005,23 +982,29 @@ function browserPageActions(obj) {
     return obj.BrowserPageActions;
   }
   return obj.ownerGlobal.BrowserPageActions;
 }
 
 /**
  * A generator function for all open browser windows.
  */
-function* browserWindows() {
+function* allBrowserWindows() {
   let windows = Services.wm.getEnumerator("navigator:browser");
   while (windows.hasMoreElements()) {
     yield windows.getNext();
   }
 }
 
+function* allBrowserPageActions() {
+  for (let win of allBrowserWindows()) {
+    yield browserPageActions(win);
+  }
+}
+
 /**
  * A simple function that sets properties on a given object while doing basic
  * required-properties checking.  If a required property isn't specified in the
  * given options object, or if the options object has properties that aren't in
  * the given schema, then an error is thrown.
  *
  * @param  obj
  *         The object to set properties on.
--- a/browser/modules/test/browser/browser_PageActions.js
+++ b/browser/modules/test/browser/browser_PageActions.js
@@ -757,16 +757,93 @@ add_task(async function nonBuiltFirst() 
   Assert.deepEqual(
     Array.map(BrowserPageActions.mainViewBodyNode.childNodes, n => n.id),
     initialActions.map(a => BrowserPageActions._panelButtonNodeIDForActionID(a.id)),
     "Action should no longer be in panel"
   );
 });
 
 
+// Makes sure that urlbar nodes appear in the correct order in a new window.
+add_task(async function urlbarOrderNewWindow() {
+  // Make some new actions.
+  let actions = [0, 1, 2].map(i => {
+    return PageActions.addAction(new PageActions.Action({
+      id: `test-urlbarOrderNewWindow-${i}`,
+      title: `Test urlbarOrderNewWindow ${i}`,
+      shownInUrlbar: true,
+    }));
+  });
+
+  // Make sure PageActions knows they're appended to the urlbar actions.
+  Assert.deepEqual(
+    PageActions._persistedActions.idsInUrlbar.slice(
+      PageActions._persistedActions.idsInUrlbar.length - actions.length
+    ),
+    actions.map(a => a.id),
+    "PageActions._persistedActions.idsInUrlbar has new actions appended"
+  );
+  Assert.deepEqual(
+    PageActions.actionsInUrlbar.slice(
+      PageActions.actionsInUrlbar.length - actions.length
+    ).map(a => a.id),
+    actions.map(a => a.id),
+    "PageActions.actionsInUrlbar has new actions appended"
+  );
+
+  // Reach into _persistedActions to move the new actions to the front of the
+  // urlbar, same as if the user moved them.  That way we can test that insert-
+  // before IDs are correctly non-null when the urlbar nodes are inserted in the
+  // new window below.
+  PageActions._persistedActions.idsInUrlbar.splice(
+    PageActions._persistedActions.idsInUrlbar.length - actions.length,
+    actions.length
+  );
+  for (let i = 0; i < actions.length; i++) {
+    PageActions._persistedActions.idsInUrlbar.splice(i, 0, actions[i].id);
+  }
+
+  // Save the right-ordered IDs to use below, just in case they somehow get
+  // changed when the new window opens, which shouldn't happen, but maybe
+  // there's bugs.
+  let ids = PageActions._persistedActions.idsInUrlbar.slice();
+
+  // Make sure that worked.
+  Assert.deepEqual(
+    ids.slice(0, actions.length),
+    actions.map(a => a.id),
+    "PageActions._persistedActions.idsInUrlbar now has new actions at front"
+  );
+
+  // Open the new window.
+  let win = await BrowserTestUtils.openNewBrowserWindow();
+
+  // Collect its urlbar nodes.
+  let actualUrlbarNodeIDs = [];
+  for (let node = win.BrowserPageActions.mainButtonNode.nextSibling;
+       node;
+       node = node.nextSibling) {
+    actualUrlbarNodeIDs.push(node.id);
+  }
+
+  // Now check that they're in the right order.
+  Assert.deepEqual(
+    actualUrlbarNodeIDs,
+    ids.map(id => win.BrowserPageActions._urlbarButtonNodeIDForActionID(id)),
+    "Expected actions in new window's urlbar"
+  );
+
+  // Done, clean up.
+  await BrowserTestUtils.closeWindow(win);
+  for (let action of actions) {
+    action.remove();
+  }
+});
+
+
 function promisePageActionPanelOpen() {
   let button = document.getElementById("pageActionButton");
   // The main page action button is hidden for some URIs, so make sure it's
   // visible before trying to click it.
   let dwu = window.QueryInterface(Ci.nsIInterfaceRequestor)
                   .getInterface(Ci.nsIDOMWindowUtils);
   return BrowserTestUtils.waitForCondition(() => {
     info("Waiting for main page action button to have non-0 size");