Bug 1391082 - Page action panel ordering can get messed up. r?Gijs
MozReview-Commit-ID: 3hQLTLF3RU8
--- 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;
}