Bug 1396053 - Page action urlbar button ordering can get messed up for new windows. r?Gijs
MozReview-Commit-ID: 1KwrS6tsnhO
--- 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");