Bug 1397501 - Page actions added to the urlbar should always come before the bookmark star. r?Gijs
MozReview-Commit-ID: 6pVlr9d0crn
--- a/browser/base/content/browser-pageActions.js
+++ b/browser/base/content/browser-pageActions.js
@@ -89,17 +89,17 @@ var BrowserPageActions = {
/**
* Adds or removes as necessary DOM nodes for the action in the panel.
*
* @param action (PageActions.Action, required)
* The action to place.
*/
placeActionInPanel(action) {
- let insertBeforeID = PageActions.nextActionID(action, PageActions.actions);
+ let insertBeforeID = PageActions.nextActionIDInPanel(action);
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) {
@@ -108,17 +108,16 @@ var BrowserPageActions = {
insertBeforeNode = document.getElementById(insertBeforeNodeID);
}
this.mainViewBodyNode.insertBefore(node, insertBeforeNode);
action.onPlacedInPanel(node);
if (panelViewNode) {
action.subview.onPlaced(panelViewNode);
}
}
- return node;
},
_makePanelButtonNodeForAction(action) {
if (action.__isSeparator) {
let node = document.createElement("toolbarseparator");
return [node, null];
}
@@ -295,30 +294,29 @@ var BrowserPageActions = {
/**
* Adds or removes as necessary a DOM node for the given action in the urlbar.
*
* @param action (PageActions.Action, required)
* The action to place.
*/
placeActionInUrlbar(action) {
- let insertBeforeID =
- PageActions.nextActionID(action, PageActions.actionsInUrlbar);
+ let insertBeforeID = PageActions.nextActionIDInUrlbar(action);
let id = this._urlbarButtonNodeIDForActionID(action.id);
let node = document.getElementById(id);
if (!action.shownInUrlbar) {
if (node) {
if (action.__urlbarNodeInMarkup) {
node.hidden = true;
} else {
node.remove();
}
}
- return null;
+ return;
}
let newlyPlaced = false;
if (action.__urlbarNodeInMarkup) {
newlyPlaced = node && node.hidden;
node.hidden = false;
} else if (!node) {
newlyPlaced = true;
@@ -344,18 +342,16 @@ var BrowserPageActions = {
if (!node.hasAttribute("tooltiptext")) {
let panelNodeID = this._panelButtonNodeIDForActionID(action.id);
let panelNode = document.getElementById(panelNodeID);
if (panelNode) {
node.setAttribute("tooltiptext", panelNode.getAttribute("label"));
}
}
}
-
- return node;
},
_makeUrlbarButtonNode(action) {
let buttonNode = document.createElement("image");
buttonNode.classList.add("urlbar-icon", "urlbar-page-action");
buttonNode.setAttribute("role", "button");
if (action.tooltip) {
buttonNode.setAttribute("tooltiptext", action.tooltip);
--- a/browser/extensions/pocket/bootstrap.js
+++ b/browser/extensions/pocket/bootstrap.js
@@ -94,17 +94,16 @@ var PocketPageAction = {
this.pageAction = PageActions.addAction(new PageActions.Action({
id,
title: gPocketBundle.GetStringFromName("saveToPocketCmd.label"),
shownInUrlbar: true,
wantsIframe: true,
urlbarIDOverride: "pocket-button-box",
anchorIDOverride: "pocket-button",
_insertBeforeActionID: PageActions.ACTION_ID_BOOKMARK_SEPARATOR,
- _urlbarInsertBeforeActionID: PageActions.ACTION_ID_BOOKMARK,
_urlbarNodeInMarkup: true,
onBeforePlacedInWindow(window) {
let doc = window.document;
if (doc.getElementById("pocket-button-box")) {
return;
}
--- a/browser/modules/PageActions.jsm
+++ b/browser/modules/PageActions.jsm
@@ -25,16 +25,17 @@ XPCOMUtils.defineLazyModuleGetter(this,
"resource://gre/modules/BinarySearch.jsm");
const ACTION_ID_BOOKMARK = "bookmark";
const ACTION_ID_BOOKMARK_SEPARATOR = "bookmarkSeparator";
const ACTION_ID_BUILT_IN_SEPARATOR = "builtInSeparator";
const PREF_PERSISTED_ACTIONS = "browser.pageActions.persistedActions";
+const PERSISTED_ACTIONS_CURRENT_VERSION = 1
this.PageActions = {
/**
* Inits. Call to init.
*/
init() {
let callbacks = this._deferredAddActionCalls;
@@ -105,21 +106,16 @@ this.PageActions = {
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;
@@ -214,48 +210,84 @@ this.PageActions = {
// 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);
this._nonBuiltInActions.splice(index, 0, action);
}
- if (this._persistedActions.ids[action.id]) {
+ if (this._persistedActions.ids.includes(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) {
- // Also store it in idsInUrlbar.
- let index =
- !action.__urlbarInsertBeforeActionID ? -1 :
- this._persistedActions.idsInUrlbar.indexOf(
- action.__urlbarInsertBeforeActionID
- );
- if (index < 0) {
- // Append the action.
- index = this._persistedActions.idsInUrlbar.length;
+ this._updateIDsInUrlbarForAction(action);
+ }
+ },
+
+ _updateIDsInUrlbarForAction(action) {
+ let index = this._persistedActions.idsInUrlbar.indexOf(action.id);
+ if (action.shownInUrlbar) {
+ if (index < 0) {
+ let nextID = this.nextActionIDInUrlbar(action.id);
+ let nextIndex =
+ nextID ? this._persistedActions.idsInUrlbar.indexOf(nextID) : -1;
+ if (nextIndex < 0) {
+ nextIndex = this._persistedActions.idsInUrlbar.length;
}
- this._persistedActions.idsInUrlbar.splice(index, 0, action.id);
+ this._persistedActions.idsInUrlbar.splice(nextIndex, 0, action.id);
}
- this._storePersistedActions();
+ } else if (index >= 0) {
+ this._persistedActions.idsInUrlbar.splice(index, 1);
}
+ this._storePersistedActions();
},
_builtInActions: [],
_nonBuiltInActions: [],
_actionsByID: new Map(),
/**
+ * Returns the ID of the action before which the given action should be
+ * inserted in the urlbar.
+ *
+ * @param action (Action object, required)
+ * The action you're inserting.
+ * @return The ID of the reference action, or null if your action should be
+ * appended.
+ */
+ nextActionIDInUrlbar(action) {
+ // Actions in the urlbar are always inserted before the bookmark action,
+ // which always comes last if it's present.
+ if (action.id == ACTION_ID_BOOKMARK) {
+ return null;
+ }
+ let id = this._nextActionID(action, this.actionsInUrlbar);
+ return id || ACTION_ID_BOOKMARK;
+ },
+
+ /**
+ * Returns the ID of the action before which the given action should be
+ * inserted in the panel.
+ *
+ * @param action (Action object, required)
+ * The action you're inserting.
+ * @return The ID of the reference action, or null if your action should be
+ * appended.
+ */
+ nextActionIDInPanel(action) {
+ return this._nextActionID(action, this.actions);
+ },
+
+ /**
* 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.
@@ -264,17 +296,17 @@ this.PageActions = {
* 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.
*/
- nextActionID(action, actionArray) {
+ _nextActionID(action, actionArray) {
let index = actionArray.findIndex(a => a.id == action.id);
if (index < 0) {
return null;
}
let nextAction = actionArray[index + 1];
if (!nextAction) {
return null;
}
@@ -298,20 +330,22 @@ this.PageActions = {
let index = list.findIndex(a => a.id == action.id);
if (index >= 0) {
list.splice(index, 1);
break;
}
}
// 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);
+ for (let name of ["ids", "idsInUrlbar"]) {
+ let array = this._persistedActions[name];
+ let index = array.indexOf(action.id);
+ if (index >= 0) {
+ array.splice(index, 1);
+ }
}
this._storePersistedActions();
for (let bpa of allBrowserPageActions()) {
bpa.removeAction(action);
}
},
@@ -353,48 +387,71 @@ this.PageActions = {
* @param action (Action object, required)
* The action whose shownInUrlbar property changed.
*/
onActionToggledShownInUrlbar(action) {
if (!this.actionForID(action.id)) {
// This may be called before the action has been added.
return;
}
-
- // Update persisted storage.
- let index = this._persistedActions.idsInUrlbar.indexOf(action.id);
- if (action.shownInUrlbar) {
- if (index < 0) {
- this._persistedActions.idsInUrlbar.push(action.id);
- }
- } else if (index >= 0) {
- this._persistedActions.idsInUrlbar.splice(index, 1);
- }
- this._storePersistedActions();
-
+ this._updateIDsInUrlbarForAction(action);
for (let bpa of allBrowserPageActions()) {
bpa.placeActionInUrlbar(action);
}
},
_storePersistedActions() {
let json = JSON.stringify(this._persistedActions);
Services.prefs.setStringPref(PREF_PERSISTED_ACTIONS, json);
},
_loadPersistedActions() {
try {
let json = Services.prefs.getStringPref(PREF_PERSISTED_ACTIONS);
- this._persistedActions = JSON.parse(json);
+ this._persistedActions = this._migratePersistedActions(JSON.parse(json));
} catch (ex) {}
},
+ _migratePersistedActions(actions) {
+ // Start with actions.version and migrate one version at a time, all the way
+ // up to the current version.
+ for (let version = actions.version || 0;
+ version < PERSISTED_ACTIONS_CURRENT_VERSION;
+ version++) {
+ let methodName = `_migratePersistedActionsTo${version + 1}`;
+ actions = this[methodName](actions);
+ actions.version = version + 1;
+ }
+ return actions;
+ },
+
+ _migratePersistedActionsTo1(actions) {
+ // The `ids` object is a mapping: action ID => true. Convert it to an array
+ // to save space in the prefs.
+ let ids = [];
+ for (let id in actions.ids) {
+ ids.push(id);
+ }
+ // Move the bookmark ID to the end of idsInUrlbar. The bookmark action
+ // should always remain at the end of the urlbar, if present.
+ let bookmarkIndex = actions.idsInUrlbar.indexOf(ACTION_ID_BOOKMARK);
+ if (bookmarkIndex >= 0) {
+ actions.idsInUrlbar.splice(bookmarkIndex, 1);
+ actions.idsInUrlbar.push(ACTION_ID_BOOKMARK);
+ }
+ return {
+ ids,
+ idsInUrlbar: actions.idsInUrlbar,
+ };
+ },
+
_persistedActions: {
- // action ID => true, for actions that have ever been seen and not removed
- ids: {},
+ version: PERSISTED_ACTIONS_CURRENT_VERSION,
+ // action IDs that have ever been seen and not removed, order not important
+ ids: [],
// action IDs ordered by position in urlbar
idsInUrlbar: [],
},
};
/**
* A single page action.
*
@@ -497,21 +554,16 @@ function Action(options) {
// private
// (string, optional)
// The ID of another action before which to insert this new action in the
// panel.
_insertBeforeActionID: false,
- // (string, optional)
- // The ID of another action before which to insert this new action in the
- // urlbar.
- _urlbarInsertBeforeActionID: false,
-
// (bool, optional)
// True if this isn't really an action but a separator to be shown in the
// page action panel.
_isSeparator: false,
// (bool, optional)
// True if the action's urlbar button is defined in markup. In that case, a
// node with the action's urlbar node ID should already exist in the DOM
@@ -892,16 +944,17 @@ Button.prototype = {
};
this.PageActions.Button = Button;
// These are only necessary so that Pocket and the test can use them.
this.PageActions.ACTION_ID_BOOKMARK = ACTION_ID_BOOKMARK;
this.PageActions.ACTION_ID_BOOKMARK_SEPARATOR = ACTION_ID_BOOKMARK_SEPARATOR;
+this.PageActions.PREF_PERSISTED_ACTIONS = PREF_PERSISTED_ACTIONS;
// This is only necessary so that the test can access it.
this.PageActions.ACTION_ID_BUILT_IN_SEPARATOR = ACTION_ID_BUILT_IN_SEPARATOR;
// Sorted in the order in which they should appear in the page action panel.
// Does not include the page actions of extensions bundled with the browser.
// They're added by the relevant extension code.
--- a/browser/modules/test/browser/browser_PageActions.js
+++ b/browser/modules/test/browser/browser_PageActions.js
@@ -154,16 +154,26 @@ add_task(async function simple() {
Assert.notEqual(urlbarButtonNode, null, "urlbarButtonNode");
for (let name in action.nodeAttributes) {
Assert.ok(urlbarButtonNode.hasAttribute(name), name,
"Has attribute: " + name);
Assert.equal(urlbarButtonNode.getAttribute(name),
action.nodeAttributes[name],
"Equal attribute: " + name);
}
+
+ // The button should have been inserted before the bookmark star.
+ Assert.notEqual(urlbarButtonNode.nextSibling, null, "Should be a next node");
+ Assert.equal(
+ urlbarButtonNode.nextSibling.id,
+ PageActions.actionForID(PageActions.ACTION_ID_BOOKMARK).urlbarIDOverride,
+ "Next node should be the bookmark star"
+ );
+
+ // Click the urlbar button.
onCommandExpectedButtonID = urlbarButtonID;
EventUtils.synthesizeMouseAtCenter(urlbarButtonNode, {});
Assert.equal(onCommandCallCount, 2, "onCommandCallCount should be inc'ed");
// Set a new title.
let newTitle = title + " new title";
action.title = newTitle;
Assert.equal(action.title, newTitle, "New title");
@@ -312,16 +322,24 @@ add_task(async function withSubview() {
let panelViewButtonNodePanel =
document.getElementById(panelViewButtonIDPanel);
Assert.notEqual(panelViewButtonNodePanel, null, "panelViewButtonNodePanel");
// The action's urlbar button should have been created.
let urlbarButtonNode = document.getElementById(urlbarButtonID);
Assert.notEqual(urlbarButtonNode, null, "urlbarButtonNode");
+ // The button should have been inserted before the bookmark star.
+ Assert.notEqual(urlbarButtonNode.nextSibling, null, "Should be a next node");
+ Assert.equal(
+ urlbarButtonNode.nextSibling.id,
+ PageActions.actionForID(PageActions.ACTION_ID_BOOKMARK).urlbarIDOverride,
+ "Next node should be the bookmark star"
+ );
+
// Open the panel, click the action's button, click the subview's first
// button.
await promisePageActionPanelOpen();
Assert.equal(onSubviewShowingCount, 0,
"onSubviewShowingCount should remain 0");
let subviewShownPromise = promisePageActionViewShown();
onSubviewShowingExpectedPanelViewID = panelViewIDPanel;
@@ -433,16 +451,24 @@ add_task(async function withIframe() {
// The action's panel button should have been created.
let panelButtonNode = document.getElementById(panelButtonID);
Assert.notEqual(panelButtonNode, null, "panelButtonNode");
// The action's urlbar button should have been created.
let urlbarButtonNode = document.getElementById(urlbarButtonID);
Assert.notEqual(urlbarButtonNode, null, "urlbarButtonNode");
+ // The button should have been inserted before the bookmark star.
+ Assert.notEqual(urlbarButtonNode.nextSibling, null, "Should be a next node");
+ Assert.equal(
+ urlbarButtonNode.nextSibling.id,
+ PageActions.actionForID(PageActions.ACTION_ID_BOOKMARK).urlbarIDOverride,
+ "Next node should be the bookmark star"
+ );
+
// Open the panel, click the action's button.
await promisePageActionPanelOpen();
Assert.equal(onIframeShownCount, 0, "onIframeShownCount should remain 0");
EventUtils.synthesizeMouseAtCenter(panelButtonNode, {});
await promisePanelShown(BrowserPageActions._activatedActionPanelID);
Assert.equal(onCommandCallCount, 0, "onCommandCallCount should remain 0");
Assert.equal(onIframeShownCount, 1, "onIframeShownCount should be inc'ed");
@@ -649,16 +675,17 @@ 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) {
@@ -768,38 +795,39 @@ add_task(async function urlbarOrderNewWi
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.
+ // Make sure PageActions knows they're inserted before the bookmark action in
+ // the urlbar.
Assert.deepEqual(
PageActions._persistedActions.idsInUrlbar.slice(
- PageActions._persistedActions.idsInUrlbar.length - actions.length
+ PageActions._persistedActions.idsInUrlbar.length - (actions.length + 1)
),
- actions.map(a => a.id),
- "PageActions._persistedActions.idsInUrlbar has new actions appended"
+ actions.map(a => a.id).concat([PageActions.ACTION_ID_BOOKMARK]),
+ "PageActions._persistedActions.idsInUrlbar has new actions inserted"
);
Assert.deepEqual(
PageActions.actionsInUrlbar.slice(
- PageActions.actionsInUrlbar.length - actions.length
+ PageActions.actionsInUrlbar.length - (actions.length + 1)
).map(a => a.id),
- actions.map(a => a.id),
- "PageActions.actionsInUrlbar has new actions appended"
+ actions.map(a => a.id).concat([PageActions.ACTION_ID_BOOKMARK]),
+ "PageActions.actionsInUrlbar has new actions inserted"
);
// 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,
+ PageActions._persistedActions.idsInUrlbar.length - (actions.length + 1),
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
@@ -834,16 +862,95 @@ add_task(async function urlbarOrderNewWi
// Done, clean up.
await BrowserTestUtils.closeWindow(win);
for (let action of actions) {
action.remove();
}
});
+// Stores version-0 (unversioned actually) persisted actions and makes sure that
+// migrating to version 1 works.
+add_task(async function migrate1() {
+ // Add the bookmark action first to make sure it ends up last after migration.
+ // Also include a non-default action (copyURL) to make sure we're not
+ // accidentally testing default behavior.
+ let ids = [
+ PageActions.ACTION_ID_BOOKMARK,
+ "pocket",
+ "copyURL",
+ ];
+ let persisted = ids.reduce((memo, id) => {
+ memo.ids[id] = true;
+ memo.idsInUrlbar.push(id);
+ return memo;
+ }, { ids: {}, idsInUrlbar: [] });
+
+ Services.prefs.setStringPref(
+ PageActions.PREF_PERSISTED_ACTIONS,
+ JSON.stringify(persisted)
+ );
+
+ // Migrate.
+ PageActions._loadPersistedActions();
+
+ Assert.equal(PageActions._persistedActions.version, 1, "Correct version");
+
+ // Need to set copyURL's _shownInUrlbar. It won't be set since it's false by
+ // default and we reached directly into persisted storage above.
+ PageActions.actionForID("copyURL")._shownInUrlbar = true;
+
+ // expected order
+ let orderedIDs = [
+ "pocket",
+ "copyURL",
+ PageActions.ACTION_ID_BOOKMARK,
+ ];
+
+ // Check the ordering.
+ Assert.deepEqual(
+ PageActions._persistedActions.idsInUrlbar,
+ orderedIDs,
+ "PageActions._persistedActions.idsInUrlbar has right order"
+ );
+ Assert.deepEqual(
+ PageActions.actionsInUrlbar.map(a => a.id),
+ orderedIDs,
+ "PageActions.actionsInUrlbar has right order"
+ );
+
+ // Open a new window.
+ let win = await BrowserTestUtils.openNewBrowserWindow();
+ await BrowserTestUtils.openNewForegroundTab({
+ gBrowser: win.gBrowser,
+ url: "http://example.com/",
+ });
+
+ // 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,
+ orderedIDs.map(id => win.BrowserPageActions._urlbarButtonNodeIDForActionID(id)),
+ "Expected actions in new window's urlbar"
+ );
+
+ // Done, clean up.
+ await BrowserTestUtils.closeWindow(win);
+ Services.prefs.clearUserPref(PageActions.PREF_PERSISTED_ACTIONS);
+ PageActions.actionForID("copyURL")._shownInUrlbar = false;
+});
+
+
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");