The functionality of both the screenshots and Pocket menu items live in extensions. The screenshots extension at least would like to call into browser code to add its item, as opposed to the browser adding the item and calling into the extension, and ultimately that's the design we want for extensions anyway. Also, the Photon spec calls for the ability to move items between the panel and the urlbar. All of these things mean that we need to implement an API for adding and removing page actions and moving them between the panel and urlbar. AFAICT there are two use cases: actions that are simply a typical menu item like screenshots (or more generally either a menu item or a menu with submenus/subviews and subitems, like the Send to Device menu?), and actions that are a menu that expands into an iframe shown in a subview, like Pocket. So the API needs to support both cases. The API I'm talking about is just a typical browser JSM. I'm thinking of CustomizableUI.jsm. I'm not talking about a WebExtension API. I'm planning on updating Pocket to call into it just like it calls into CustomizableUI. I'm less clear on screenshots right now.

Work-in-progress patch. The JS objects and prototypes are the following. Basically it's just creating a JS interface for adding DOM elements and managing them across browser windows, as you might expect. PageActions singleton: The entry point. PageAction prototype: A single page action. It can have a placement, the panel or urlbar. PageActionButton prototype: The top-level UI element representing the page action. Not the DOM element itself. Has an onCommand function that's called when it's clicked. You can also create your own buttons, which is necessary for building a menu hierarchy. In that case, instead of an onCommand function, you set the subview property (described next), and in that case the button is a menu that expand into the subview. PageActionSubview prototype: You can create a subview and assign it to button.subview. In that case, the button will be a menu that expands into the subview when clicked. Subviews can themselves have a list of buttons -- a standard submenu. Or they can have an iframe that your code runs in.

I misunderstood the design: items should always be in the panel, and optionally they can be in the urlbar, too. An important difference but not really harder to implement (knock on wood).

Another messy work in progress that starts to add the context menu for adding/removing buttons to and from the urlbar. Ultimately I'll factor out that part since this there's a separate bug for it, but it's useful for me to see how it needs to hook up to the API and browser.js.

Another work in progress. Struggling to show the Pocket iframe directly inside the action panel.

(In reply to Drew Willcoxon :adw from comment #7) > Another work in progress. Struggling to show the Pocket iframe directly > inside the action panel. FWIW, I think it would be fine to move doing this to a followup and just show the pocket panel anchored on the page action icon (if the pocket icon is not present) for now? Alternatively, have you looked at what the toolbar button does right now if it's moved into the old hamburger panel / new overflow panel? Maybe that can help. Also happy to chat about this on Monday if I can do something to help.

(In reply to :Gijs from comment #8) > Alternatively, have you looked at what the toolbar button does right now if > it's moved into the old hamburger panel / new overflow panel? Yes -- sorry my previous comment was so terse, but the (main) problem is that the page action panel is pretty small, both narrow and short. So to show the iframe directly in the panel, which is what Bryan or Aaron (I don't remember who exactly) suggested we do, and I agree would be best to do, the panel would need to grow. I was having a hard time getting that to happen, especially considering animating the size change. (I also wonder if that might end up looking strange.) It's not a problem in the current hamburger panel because the hamburger panel is pretty big, both wide and tall. It doesn't actually grow at all when showing the iframe. It's a little tight on space, so maybe it should grow, but it doesn't, and it doesn't look bad. > (In reply to Drew Willcoxon :adw from comment #7) > FWIW, I think it would be fine to move doing this to a followup and just > show the pocket panel anchored on the page action icon (if the pocket icon > is not present) for now? Good idea -- easier to implement, I'm all for it. This latest work in progress does just that. Another benefit of doing it that way is that there's then a single code path for showing iframe-based buttons like Pocket, regardless of whether the button is in the panel or toolbar. Still more to do: finish nailing down API, hook up clicks on the screenshots button, clean up, factor out code into separate patches for the several bugs that depend on the API, and probably tests.

Mike, is it possible to create a photonmultiview without a main view and then use showSubview to show a given view as the main view? What I'm trying to do is to have one panel that's reused when you click on any action in the urlbar that shows a panel with a (sub)view. The Send to Device subview for example. So I make panel and add a photonmultiview to it and then keep that around. Then when an action in the urlbar is clicked, I want to show the action's subview as the main view in the panel. Also, I'm trying to call showSubview on the photonmultiview, but I get "TypeError: multiView.showSubview is not a function." Same error if I call it on multiView.instance instead. Know what that might be about?

Another WIP, focusing on cleanup, tightening the API, opening panels for actions in the urlbar.

More WIP. I think trying to allow actions to be specified in markup is a bad idea. At least it's too complicated or I'm too dumb to implement it. This focuses on moving built-in actions to JS and building subviews via JS.

(In reply to Drew Willcoxon :adw from comment #11) Think I've been able to work around this.

(In reply to Drew Willcoxon :adw from comment #11) > Also, I'm trying to call showSubview on the photonmultiview, but I get > "TypeError: multiView.showSubview is not a function." Same error if I call > it on multiView.instance instead. Know what that might be about? Aaaaaand I just noticed that it's showSubView, not showSubview. Same as the method name on PanelUI.

Making more progress on defining all actions in JS, removing actions when their extensions are unloaded.

This still needs a test, but it's ready to start reviewing I think. It's fairly big. I removed the Pocket and Screenshots changes. I'll add them to their respective bugs. Summary: * PageActions.jsm keeps track of your actions globally. It also defines the set of built-in actions (in JS of course), not including Pocket and Screenshots, which will be created by the relevant extension code. * PageActions.jsm also defines a few prototypes: Action, Subview, and Button. Action is a page action. Subview represents a panelview subview. Button represents a toolbarbutton. An Action can have a Subview or an iframe. A Subview can have Buttons. * browser-pageActions.js creates a new BrowserPageActions object per browser window that updates its window with your actions. Most of the work is done here: creating DOM nodes, setting up the built-in actions, etc. PageActions.jsm is simple in comparison. * browser-pageActions.js sets up the built-in actions, but I moved that setup to a new object per action. That keeps the main BrowserPageActions object uncluttered. * None of the built-in page actions are defined in markup anymore. It was a pain trying to convert DOM nodes to action objects, and I ended up spending too much time on it. We can always go back and add that now on top if we want, but I don't think it's worth anything. * That includes the bookmark star button. I removed it from markup, and now it's a built-in page action that's shown in the urlbar by default. * The IDs of DOM nodes related to existing page actions have all changed. DOM IDs are generated automatically now based on its action's ID and where the DOM node lives: the panel or the urlbar. Many CSS selectors had to change as a result. * I renamed some other existing DOM IDs too, like the main panel itself, to use a convention, more or less, to keep myself from going crazy. * I added a new hbox in the urlbar for all the page action icons. I'm also now using <image>s instead of <toolbarbutton>s since that seems to be the convention for icons in the urlbar. * I modified existing SVGs where necessary to add viewBox and fill-opacity attributes so that icons are drawn with the right size and opacity in the urlbar. Bryan mentioned he wanted viewBox in the SVGs anyway. * This includes context menu code for the panel. That by itself is pretty small, so I think it didn't make sense after all to factor that out into bug 1363188. The API for adding/removing is the important part, and that's all included here in this bug. But, the context menu for the urlbar still needs to be implemented.

Mike, here's some info on the important code paths that might be a good place to start understanding the patch. How built-in actions are added: (1) PageActions.jsm has a gBuiltInActions array. Each object in the array is an options object suitable for passing to the Action constructor (in PageActions.jsm). (2) nsBrowserGlue calls PageActions.init. (3) PageActions.init loops through gBuiltInActions, makes an Action object for each options object, and passes it to PageActions.addAction. (4) PageActions.addAction iterates over all browser windows and calls BrowserPageActions.placeAction on each. After that, each window's BrowserPageActions pretty much takes over. (5) BrowserPageActions.placeAction calls placeActionInPanel and placeActionInUrlbar. Each of those functions adds DOM nodes for the action to the panel and urlbar unless they've already been added, and in the case of the urlbar, only if action.shownInUrlbar is true. If action.shownInUrlbar is false and the action's DOM node is in the urlbar, then placeActionInUrlbar removes that node. (That's why these methods use "place" instead of "add.") * * * How non-built-in actions are added (e.g., by an extension): (1) The consumer makes a new Action by calling the PageActions.Action constructor. (2) It adds the new action with PageActions.addAction. (3) From there, steps 4 and 5 above happen. * * * How a new browser window places actions after PageActions.init has already been called (i.e., windows that are opened after startup): (1) _delayedStartup in browser.js calls BrowserPageActions.init. (2) BrowserPageActions.init loops through PageActions.actions, a list of all the added actions, and calls placeAction for each. (3) From here it's step 5 above. * * * How actions are removed (e.g., by an extension when it's unloaded): (1) The consumer must keep a reference to the Action object it created. Then it just calls action.remove on it. (2) Action.remove calls PageActions._actionRemoved. (3) PageActions._actionRemoved loops through all the browser windows and calls BrowserPageActions.removeAction. (4) BrowserPageActions.removeAction calls _removeActionFromPanel and _removeActionFromUrlbar, which remove the DOM nodes for the action. * * * How actions are added to and removed from the urlbar: (1) The context menu in the page action panel has a single menu item that calls BrowserPageActions.toggleShownInUrlbarForContextAction on command. (2) toggleShownInUrlbarForContextAction toggles the action.shownInUrlbar bool property for the action that was context-clicked. (3) The Action.shownInUrlbar setter calls PageActions._actionToggledShownInUrlbar. (4) PageActions._actionToggledShownInUrlbar calls BrowserPageActions.placeActionInUrlbar for each browser window. (I'm eliding some complexity here that determines which position among the actions in the urlbar the action should occupy, if it's being added to the urlbar.) (5) From there, it's step 5 above except only for placeActionInUrlbar. No need to call placeActionInPanel in this case.

Rebased on the current tip.

Fixes for current tests (updating changed identifiers, etc.), and replaces a couple of leftover hardcoded English strings with DTD entities.

Some failures on try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae94048c407e

(In reply to Drew Willcoxon :adw from comment #15) > [...] or I'm too dumb to implement it. [...] That's unfathomable, there's no way on this planet that you are.

Great to see the progress here and thanks for letting me take a look! I think this is almost going in the right direction, but before we continue I'd like to talk about a few things... I have three general questions about this work: 1. At a certain point you must have noticed how familiar this logic is to CustomizableUI.jsm... did you consider following that route? (For determining and persisting widget placement) 2. The division of logic between the JSM and browser script is a bit unclear to me. Where and how did you decide when a piece of code needed to live in the JSM and when in the browser script? I really like the concept of passing around Action objects; can you put the build-in ones in their own JSM? (Question 2.1 ;-) ) 3. For the next round of review, can you please cut the set of patches up into a few parts?

(In reply to Mike de Boer [:mikedeboer] from comment #29) > 1. At a certain point you must have noticed how familiar this logic is to > CustomizableUI.jsm... did you consider following that route? (For > determining and persisting widget placement) I'm not very familiar with CustomizableUI.jsm. I've certainly looked at it and that's where I got the idea of "placement" from. (Although once it was clear that page actions could only be copied to the urlbar instead of being *moved* there, I think the parallel to the concept of placement starts to become less clear.) I think both you and Gijs have worked on it? So in hindsight it was a mistake for me to work on the page action panel. Does CustomizableUI even still make sense anymore in Photon? Isn't it mostly about the hamburger panel? > 2. The division of logic between the JSM and browser script is a bit unclear > to me. Where and how did you decide when a piece of code needed to live in > the JSM and when in the browser script? It's pretty simple I think (necessarily for me to be able to hold it in my head): if something touches DOM, then it lives in BrowserPageActions in browser-pageActions.js. I didn't want to have to pass around browser windows and whatnot all over the place in the JSM. browser-pageActions.js manages DOM state for a browser window, PageActions.jsm manages global actions state (including coordination between the global instances of Action, Subview, and Button). > can you put the build-in ones in their own JSM? OK. > 3. For the next round of review, can you please cut the set of patches up > into a few parts? OK. I might end up posting them as old-fashioned patches for Splinter review because I don't think it's possible to post multiple mozreviews to a bug?

(In reply to Drew Willcoxon :adw from comment #30) > I'm not very familiar with CustomizableUI.jsm. I've certainly looked at it > and that's where I got the idea of "placement" from. (Although once it was > clear that page actions could only be copied to the urlbar instead of being > *moved* there, I think the parallel to the concept of placement starts to > become less clear.) I think both you and Gijs have worked on it? So in > hindsight it was a mistake for me to work on the page action panel. Does > CustomizableUI even still make sense anymore in Photon? Isn't it mostly > about the hamburger panel? True, the concept of copying things is not in there. You're absolutely right ;) So I think that adjusting CustomizableUI.jsm to retrofit PageActions in there isn't wise. Like you said, looking at it for ideas would be its use. I don't think it's a mistake for you to work on a big piece of functionality, just because Gijs or I have worked on similar stuff before. That wouldn't be fair and withhold many fun new projects from us too! > It's pretty simple I think (necessarily for me to be able to hold it in my > head): if something touches DOM, then it lives in BrowserPageActions in > browser-pageActions.js. I didn't want to have to pass around browser > windows and whatnot all over the place in the JSM. Well, I'm actually quite comfortable with passing windows around in the JSM: I usually care mostly about logic being concentrated in a single place, so that concepts are easily separated and documented. > browser-pageActions.js manages DOM state for a browser window, > PageActions.jsm manages global actions state (including coordination between > the global instances of Action, Subview, and Button). Preferably I'd like browser-pageActions.js to only do the init() part and the rest done by a logically compartmentalized set of JSMs that are lazy-loaded as well. Because we also need to be careful not to regress perf, which might already be negatively affected by moving the built-in actions from markup to JS. If we have a couple of items in the urlbar, will they be shown directly or with a slow delay at window open/ browser startup? > OK. I might end up posting them as old-fashioned patches for Splinter > review because I don't think it's possible to post multiple mozreviews to a > bug? You can! When you create multiple commits or have multiple queues atop each other, you can push them all on one go, as long as you don't have any other patches applied.

Comment on attachment 8879364 [details] Bug 1374477 - Remove page actions from browser.xul. I'm expecting a new set of patches, so I'll be happy to review that.

OK, six commits pushed to mozreview. Looks like they all made it. The previous commits I pushed got morphed into the first new one, "Add PageActions.jsm for Photon page actions." Thanks Mike.

Comment on attachment 8879364 [details] Bug 1374477 - Remove page actions from browser.xul. https://reviewboard.mozilla.org/r/150666/#review164166 This is coming together quite nicely, but there are quite a number of things that warrant another iteration. ::: browser/modules/PageActions.jsm:16 (Diff revision 11) > + // PageActions.Subview > + // PageActions.BOOKMARK_ACTION_ID > + // PageActions.SEND_TO_DEVICE_ACTION_ID > +]; > + > +const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components; I believe you're only using `Cu` in this module. Can you make it do less? ::: browser/modules/PageActions.jsm:20 (Diff revision 11) > + > +const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components; > + > +Cu.import("resource://gre/modules/Services.jsm"); > + > +const ACTION_IDS_IN_URLBAR_PREF = "browser.pageActions.actionIDsInUrlbar"; nit: can you add `PREF` as a prefix instead? That way it'll be easier to group when other pref-name constants are added later on. ::: browser/modules/PageActions.jsm:22 (Diff revision 11) > + > +Cu.import("resource://gre/modules/Services.jsm"); > + > +const ACTION_IDS_IN_URLBAR_PREF = "browser.pageActions.actionIDsInUrlbar"; > + > + nit: superfluous newline. ::: browser/modules/PageActions.jsm:24 (Diff revision 11) > + > +const ACTION_IDS_IN_URLBAR_PREF = "browser.pageActions.actionIDsInUrlbar"; > + > + > +this.PageActions = { > + nit: superfluous newline. I think you should add docblock comments for each global (API-)object and member variable/ method. Even though they may be one-liners and the code may be self-explanatory, I think it's good to do this whilst the code is still fresh in your head. Like now :) ::: browser/modules/PageActions.jsm:27 (Diff revision 11) > + > +this.PageActions = { > + > + init() { > + // These callbacks are deferred until init happens. > + let callbacks = this._onInitCallbacks; Perhaps it's more clear to call these `deferredAddActionCalls` or something? ::: browser/modules/PageActions.jsm:32 (Diff revision 11) > + let callbacks = this._onInitCallbacks; > + delete this._onInitCallbacks; > + > + this._actionIDsInUrlbar = this._loadActionIDsInUrlbar(); > + > + for (let options of gBuiltInActions) { nit: please add a comment like 'Add the built-in browser actions, which you can find declared below in this file.' or something. ::: browser/modules/PageActions.jsm:100 (Diff revision 11) > + > + if (action.isSeparator) { > + this._actions.push(action); > + } else { > + if (this.actionForID(action.id)) { > + throw new Error( nit: I think this can fit on a single line. Using a template string in this case might be nice too. In general I'd recommend taking the 80ch limit as a guideline and allow for a hard limit at 120ch when and if it improves code readability. That's the practice I've been seeing in recent JS code. ::: browser/modules/PageActions.jsm:105 (Diff revision 11) > + throw new Error( > + "A PageAction with ID '" + action.id + "' has already been added." > + ); > + } > + > + let actionsIndex = nit: can you keep the first part of the ternary on the same line here? Also, please insert a short comment what the index means in practice. ::: browser/modules/PageActions.jsm:133 (Diff revision 11) > + }, > + > + _actions: [], > + _actionsByID: new Map(), > + > + _actionRemoved(action) { nit: perhaps it'd be nice and clear if you named event-like methods like 'onActionRemoved' and not underscoring them. ::: browser/modules/PageActions.jsm:146 (Diff revision 11) > + this._actions.splice(index, 1); > + } > + this._actionsByID.delete(action.id); > + for (let win of browserWindows()) { > + browserPageActions(win).removeAction(action); > + } For posterity, shouldn't this also call `this._storeActionIDsInUrlbar()`? ::: browser/modules/PageActions.jsm:187 (Diff revision 11) > + } > + } catch (ex) {} > + return null; > + }, > + > + _actionIDsInUrlbar: null, I don't think the trailing-comma rule is enforced and I (personally) hope it will never be. Can you remove it here and in other places below too? ::: browser/modules/PageActions.jsm:509 (Diff revision 11) > + > + > +// 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. > +let gBuiltInActions = [ `var` in the module scope is not allowed.

Comment on attachment 8887732 [details] Bug 1374477 - Add browser-pageActions.js for Photon page actions. https://reviewboard.mozilla.org/r/158640/#review164200 My main concern here is that introducing another browser script load like this would regress Talos startup tests, whilst other teams are working hard to reduce the time spent there. Can you push this to try, running the talos suite with a number of retries so that we can start looking at some preliminary numbers? ::: browser/base/content/browser-pageActions.js:5 (Diff revision 1) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +var BrowserPageActions = { I'm also missing the docblock comments for the global and its member variables and methods. ::: browser/base/content/browser-pageActions.js:65 (Diff revision 1) > + let panelViewNode; > + [node, panelViewNode] = this._makePanelButtonNodeForAction(action); > + node.id = id; > + let insertAfterNode = null; > + if (action.insertAfter) { > + let insertAfterNodeID = nit: I think you can keep this on a single line. ::: browser/base/content/browser-pageActions.js:188 (Diff revision 1) > + panelNode.addEventListener("popuphidden", () => { > + panelNode.remove(); > + }, { once: true }); > + > + if (panelViewNode) { > + action.subview.placed(panelViewNode); I think you'll want to be consistent with the naming of your hooks here... like `onPlaced`, `onShowing` or `onIframeShown`. ::: browser/base/content/browser-pageActions.js:197 (Diff revision 1) > + this.panelNode.hidePopup(); > + > + let urlbarNodeID = this._urlbarButtonNodeIDForActionID(action.id); > + let anchorNode = > + document.getElementById(urlbarNodeID) || this.mainButtonNode; > + panelNode.openPopup(anchorNode, "bottomcenter topright"); I think you'll also want to pass in the event soon for Touch Mode - Johannh can tell you more about it. ::: browser/base/content/browser-pageActions.js:343 (Diff revision 1) > + > + _actionIDForNodeID(nodeID) { > + if (!nodeID) { > + return null; > + } > + let match = nodeID.match(/^pageAction-(panel|urlbar)-(.+)$/); nit: doing `(?:panel|urlbar)` would make it `matches[1]`. ::: browser/base/content/browser.js:1417 (Diff revision 1) > > gBrowser.addEventListener("PermissionStateChange", function() { > gIdentityHandler.refreshIdentityBlock(); > }); > > + BrowserPageActions.init(); Isn't `_delayedStartup` too late to show the action buttons? As in, won't they 'plop' into view after all the other UI elements?

Comment on attachment 8887733 [details] Bug 1374477 - Remove page actions from browser.xul. https://reviewboard.mozilla.org/r/158642/#review164224 ::: browser/base/content/browser.xul:462 (Diff revision 1) > position="bottomcenter topright" > - noautofocus="true"> > - <photonpanelmultiview id="page-action-multiView" > - mainViewId="page-action-mainView"> > - <panelview id="page-action-mainView" > + context="pageActionPanelContextMenu" > + oncontextmenu="BrowserPageActions.onContextMenu(event);" > + noautofocus="true" > + copyURL-title="&copyURLCmd.label;" > + emailLink-title="&emailPageCmd.label;" I think putting the labels here makes sense, kind of, but can you keep the attribute names more-or-less mirrored with the entity names? ::: browser/base/content/browser.xul (Diff revision 1) > + mainViewId="pageActionPanelMainView" > + viewCacheId="appMenu-viewCache"> > + <panelview id="pageActionPanelMainView" > class="PanelUI-subView"> > - <vbox class="panel-subview-body"> > + <vbox class="panel-subview-body"/> > - <toolbarbutton id="page-action-bookmark-button" I've been thinking a little bit about not being able to have static markup as well... couldn't we just check _before_ we create a toolbarbutton or panelview node for an action, that an element with that ID doesn't already exist? You'll have to change the IDs here a bit to make them work with the ones you generate now, but I think a flow like that might work, right? ```js // In pseudo code, something like... _createToolbarbuttonForAction(action) { let id = _getButtonIDForAction(action); let button = document.getElementById(id); if (button) { // The button already exists, yay! Now the event // handlers and placement needs to be done someplace // else still. return button; } return document.createElement("button"); }, _createPanelviewForAction(action) { let id = _getPanelIDForAction(action); let panel = document.getElementById(id); if (panel) { // The panel already exists, yay! Now the event // handlers and placement needs to be done someplace // else still. return panel; } return document.createElement("panelview"); } ``` What do you think?

Comment on attachment 8887734 [details] Bug 1374477 - Update Photon page action panel CSS. https://reviewboard.mozilla.org/r/158644/#review164228 LGTM! The move of the zoom-control stuff makes the diff go bonkers, but looks OK.

Comment on attachment 8887735 [details] Bug 1374477 - Update consumers of Photon page action panel for changed identifiers. https://reviewboard.mozilla.org/r/158646/#review164230 At a cursory glance this looks OK. I'd like to see a new test that adds coverage for moving and/ or removing items to/ from the urlbar. Can you add that as a separate patch? Did you already look at why Try is not happy with this set of changes?

Comment on attachment 8887736 [details] Bug 1374477 - Photon SVG changes for page action panel. https://reviewboard.mozilla.org/r/158648/#review164232 LGTM!

(In reply to Mike de Boer [:mikedeboer] from comment #42) > I've been thinking a little bit about not being able to have static markup > as well... couldn't we just check _before_ we create a toolbarbutton or > panelview node for an action, that an element with that ID doesn't already > exist? I spent a lot of time trying to allow actions to be defined in markup and then creating and registering Action objects based on the resulting DOM nodes. It wasn't easy and I gave up on that in favor of defining all actions in JS instead. You might ask, if an action is defined in markup, then why create and register an Action object for it at all? It's a fair question, but I think there are good reasons. It's setting up problems down the road if we have two tiers of actions, one tier that's registered with PageActions and that PageActions knows about, and one tier that's invisible to PageActions. Further -- and even if Action objects were created from markup -- that would mean that a large part of this new page actions implementation would not be exercised by Firefox itself, but only by extensions (Pocket and screenshots, at least for right now). (The part that builds DOM nodes from action objects.) So I'd like to keep this the way it is now: the built-in actions are defined in JS. That said, I'm running into some problems with tests related to the bookmark star button. I think the problem is that the star button is now being added to the DOM a little later than expected, which goes to your question about the actions popping in a little after the browser appears. I need to fix that, and it may involve special-casing and adding back the markup for at least the star button, I'm not sure yet. Adding the star button before delayedStartup might work too.

(In reply to Drew Willcoxon :adw from comment #46) > I spent a lot of time trying to allow actions to be defined in markup and > then creating and registering Action objects based on the resulting DOM > nodes. It wasn't easy and I gave up on that in favor of defining all > actions in JS instead. I understand, and I'm certainly not going to ask to revisit that design again. Please don't get me wrong. > You might ask, if an action is defined in markup, then why create and > register an Action object for it at all? Nope, I'm not asking that question (but thanks for the info tidbit ;-) ). I think the DOM node construction code is useful and can stay - in fact, that's how the addon-sdk widgets and even built-in ones interact with CustomizableUI. They are also allowed to be of 'custom' type and create their own markup. What I'm suggesting is: 1. Before you create a toolbarbutton or panelview element, check whether it already exists somewhere in the document. 2. If not, create the nodes just like currently happens. 3. If yes (which might be the right solution for the star-button), you can (re-)use the element(s), insert them at the right position and continue with those instead. You're free to reparent & clone stuff as you see fit, of course. This might be a bad idea, and/ or infeasible. I'm trying to think with you to get to the best solution currently possible.

Comments addressed, and: (In reply to Mike de Boer [:mikedeboer] from comment #41) > My main concern here is that introducing another browser script load like > this would regress Talos startup tests, whilst other teams are working hard > to reduce the time spent there. Can you push this to try, running the talos > suite with a number of retries so that we can start looking at some > preliminary numbers? I still need to do this. > ::: browser/base/content/browser-pageActions.js:197 > (Diff revision 1) > > + this.panelNode.hidePopup(); > > + > > + let urlbarNodeID = this._urlbarButtonNodeIDForActionID(action.id); > > + let anchorNode = > > + document.getElementById(urlbarNodeID) || this.mainButtonNode; > > + panelNode.openPopup(anchorNode, "bottomcenter topright"); > > I think you'll also want to pass in the event soon for Touch Mode - Johannh > can tell you more about it. I'll do that in a follow-up bug specifically for fixing the panel for touch as necessary. > ::: browser/base/content/browser.js:1417 > (Diff revision 1) > > > > gBrowser.addEventListener("PermissionStateChange", function() { > > gIdentityHandler.refreshIdentityBlock(); > > }); > > > > + BrowserPageActions.init(); > > Isn't `_delayedStartup` too late to show the action buttons? As in, won't > they 'plop' into view after all the other UI elements? Possibly -- that's something I'm still thinking about and working on. I notice that in current Nightlys, the bookmark star button appears to pop in already. It's not clear to me whether my patches make that worse, but definitely something I need to look at. I'd like to post another patch here in this bug after these initial patches are r+'ed that addresses this, if that's OK. (In reply to Mike de Boer [:mikedeboer] from comment #42) > ::: browser/base/content/browser.xul:462 > (Diff revision 1) > > position="bottomcenter topright" > > - noautofocus="true"> > > - <photonpanelmultiview id="page-action-multiView" > > - mainViewId="page-action-mainView"> > > - <panelview id="page-action-mainView" > > + context="pageActionPanelContextMenu" > > + oncontextmenu="BrowserPageActions.onContextMenu(event);" > > + noautofocus="true" > > + copyURL-title="&copyURLCmd.label;" > > + emailLink-title="&emailPageCmd.label;" > > I think putting the labels here makes sense, kind of, but can you keep the > attribute names more-or-less mirrored with the entity names? The more important thing IMO is that these attribute names mirror the object names in browser-pageActions. e.g., BrowserPageActions.sendToDevice, which is responsible for the send to device subview, has all its attribute names prefixed with "sendToDevice". Same for copyURL, emailLink, etc. And the "title" part of these attribute names correspond to the "title" property on actions. So I left this as is. > ::: browser/base/content/browser.xul > (Diff revision 1) > > + mainViewId="pageActionPanelMainView" > > + viewCacheId="appMenu-viewCache"> > > + <panelview id="pageActionPanelMainView" > > class="PanelUI-subView"> > > - <vbox class="panel-subview-body"> > > + <vbox class="panel-subview-body"/> > > - <toolbarbutton id="page-action-bookmark-button" > > I've been thinking a little bit about not being able to have static markup > as well... couldn't we just check _before_ we create a toolbarbutton or > panelview node for an action, that an element with that ID doesn't already > exist? This is related to the delayedStartup question, so I'd like to address this in the next patch if possible. (In reply to Mike de Boer [:mikedeboer] from comment #44) > At a cursory glance this looks OK. I'd like to see a new test that adds > coverage for moving and/ or removing items to/ from the urlbar. Can you add > that as a separate patch? Did you already look at why Try is not happy with > this set of changes? In addition to the next patch I just mentioned, I'd like to post yet another patch that adds tests and fixes current tests. I'm still working on what's going on with current tests. I might end up posting two more patches for that: one that fixes current tests, one that adds new tests.

The screenshots extension needs to dynamically set its page action's title, so these two commits add that ability to PageActions.jsm and browser-pageActions.js.

I screwed up the mozreview crap somehow, sorry.

You already r+'ed a few of these patches but I can't figure out how to tell mozreview that. And it lost the commit/diff history of other patches.

(In reply to Drew Willcoxon :adw from comment #60) > (In reply to Mike de Boer [:mikedeboer] from comment #41) > > My main concern here is that introducing another browser script load like > > this would regress Talos startup tests, whilst other teams are working hard > > to reduce the time spent there. Can you push this to try, running the talos > > suite with a number of retries so that we can start looking at some > > preliminary numbers? > > I still need to do this. Try sent me an email with a link to compare my talos push to a baseline. I think I remember hearing that comparisons between m-c and try aren't valid, so I chose as the baseline the parent of my try push, which seems like the right thing anyway. This link is the result: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=e0b0865639cebc1b5afa0268a4b073fcdde0e69c&newProject=try&newRevision=b80a3e92a09233bb38ad4c9f46195794d6c9a474&framework=1&showOnlyImportant=0 There are three big changes (according to the three bolded/color-coded cells in that table): * sessionrestore opt e10s, windows7-32 => +2.02% * sessionrestore_many_windows opt e10s, linux64 => +3.43% * sessionrestore_many_windows opt e10s, osx-10-10 => -4.46% So two big regressions and one even larger improvement. Don't know what to make of that. I asked try to run the tests 20 times.

There were no significant changes to "ts_paint opt e10s" BTW. ts is the startup test, right? Although I guess sessionrestore tests also measure that incidentally?

I pushed an empty commit to try (an empty mq patch with only a try commit message), again with 20 runs, comparing it to the same baseline try revision, and there were two big changes: * sessionrestore_many_windows opt e10s, linux64 => +3.10% * tabpaint opt e10s, windows7-32 => +6.86% So I'm not sure I trust the previous big changes in comment 73. "sessionrestore_many_windows opt e10s, linux64" in particular regressed about the same amount in both.

Some updates: * Some smallish fixes to problems I found. * Move BrowserPageActions.init() from delayedStartup to onLoad, i.e., earlier in the window init process. I think that may fix the two failing tests, but I can't reproduce the firefox-ui-functional failure locally, so I'll have to push to try to see. * Modify the browser_startup_images.js test (one of the two failing tests) by removing the bookmark star SVG from its whitelist. Now this test passes locally for me. * Action.iconURL needs to be settable because the screenshots extension needs to set it. * Still working on a new test.

(In reply to Drew Willcoxon :adw from comment #76) > I pushed an empty commit to try (an empty mq patch with only a try commit > message), again with 20 runs, comparing it to the same baseline try > revision, and there were two big changes: > > * sessionrestore_many_windows opt e10s, linux64 => +3.10% > * tabpaint opt e10s, windows7-32 => +6.86% > > So I'm not sure I trust the previous big changes in comment 73. > "sessionrestore_many_windows opt e10s, linux64" in particular regressed > about the same amount in both. This isn't working well because while you're putting the same rev in perfherder, it's just using the first push that contained that m-c rev, which if you click the link in perfherder seems like it's https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0b0865639cebc1b5afa0268a4b073fcdde0e69c , which is an activity stream trypush that has changes of its own. Generally to fix this kind of thing, you'll want to push your changes from a recent m-c, and then push a dummy commit (like what you did here) from the same m-c rev, and then compare those two revs. Mike is now out for 2 weeks, I think, (says so on the photon pto calendar), so I will try to take over the reviews.

(In reply to Drew Willcoxon :adw from comment #83) > * Move BrowserPageActions.init() from delayedStartup to onLoad, i.e., > earlier in the window init process. I think that may fix the two failing > tests, but I can't reproduce the firefox-ui-functional failure locally, so > I'll have to push to try to see. It looks like the failure on the last trypush, at least, is related to where new bookmarks created from the bookmark icon are getting filed. Perhaps your changes change the command that gets executed if you click the bookmark star? I think if you change this in the second patch, to match the XUL stuff you remove in the 3rd one (so call BookmarkingUI.onStarCommand(event)) then it should be fixed. See also bug 1120110 (but let's not try to fix that here, let's just make sure tests pass and this button continues to do what it used to do).

Comment on attachment 8888894 [details] Bug 1374477 - Photon SVG changes for page action panel. https://reviewboard.mozilla.org/r/159934/#review165776 ::: browser/themes/shared/icons/link.svg:5 (Diff revision 2) > <!-- This Source Code Form is subject to the terms of the Mozilla Public > - License, v. 2.0. If a copy of the MPL was not distributed with this > - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > <svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16"> > - <rect x="7" y="3.286" width="2" height="9.429" rx="1" ry="1" transform="rotate(-45 8 8)"/> > + <rect fill-opacity="context-fill-opacity" fill="context-fill" x="7" y="3.286" width="2" height="9.429" rx="1" ry="1" transform="rotate(-45 8 8)"/> Huh, can you file a followup to get rid of the rotate transform? We should just have the paths in there correctly to begin with.

Comment on attachment 8888893 [details] Bug 1374477 - Update consumers of Photon page action panel for changed identifiers. https://reviewboard.mozilla.org/r/159932/#review165778

Comment on attachment 8888892 [details] Bug 1374477 - Update Photon page action panel CSS. https://reviewboard.mozilla.org/r/159930/#review165780

Comment on attachment 8879364 [details] Bug 1374477 - Remove page actions from browser.xul. https://reviewboard.mozilla.org/r/150666/#review165806 If the onLoad stuff is guaranteed to deal with the builtin stuff correctly and there's no perf regression, r=me. Though I'm still curious whether we can avoid the DOM construction for builtin stuff... Given that there are some issues to sort out wrt the rebase and those earlier points, I've cleared review for now. ::: browser/base/content/browser.xul:466 (Diff revision 16) > + sendToDevice-fxaShortcut="&sendToDevice.fxaRequired.label;" > + sendToDevice-noDevicesTitle="&sendToDevice.noDevices.label;" Headsup that these strings got rebased away somehow.

Comment on attachment 8888890 [details] Bug 1374477 - Add PageActions.jsm for Photon page actions. https://reviewboard.mozilla.org/r/159926/#review165750 This effectively looks good to me, besides the issue of the ordering of multiple non-builtin items. If they both end up with the same `insertAfter`, which seems plausible, I would like us to guarantee ordering somehow. I'm not too fussed on whether we do this alphabetically or by storing something, though the former seems simpler (and won't require hijacking a pref for storage of something that isn't really a pref, plus the thorny issues of what happens with items stored there that no longer exist, etc.). Clearing review, but only because of that. More minor issues below. ::: browser/modules/PageActions.jsm:27 (Diff revision 2) > + // These callbacks are deferred until init happens. > + let callbacks = this._deferredAddActionCalls; > + delete this._deferredAddActionCalls; Move this to right before the callbacks loop, please? :-) ::: browser/modules/PageActions.jsm:92 (Diff revision 2) > + * @param action (Action, required) > + * The Action object to register. > + * @return The given Action. > + */ > + addAction(action) { > + if (this._deferredAddActionCalls) { Please either add an explicit `_initialized` bool property and use it in lieu of this check, or a comment clarifying that we need to defer adding this action if we haven't been initialized yet, and that `_deferredAddActionCalls` will be deleted once we've initialized. ::: browser/modules/PageActions.jsm:107 (Diff revision 2) > + // the index of the action in the _actions array > + let actionsIndex = !action.insertAfter ? this._actions.length : > + this._actions.findIndex(a => a.id == action.insertAfter) + 1; If the `insertAfter` doesn't exist, this will put this action first (index -1 for not found, + 1). I think ideally new non-builtin actions should always come after the builtin ones, especially if they specify invalid `insertAfter` values. ::: browser/modules/PageActions.jsm:696 (Diff revision 2) > + // use that for the button in the urlbar. > + urlbarIDOverride: "star-button", > + title: "", > + shownInUrlbar: true, > + nodeAttributes: { > + observes: "bookmarkThisPageBroadcaster", Does this work for both starred and tooltiptext state? I tried to test, but rebasing was... very hard, and then the resulting code didn't work, in that the panel opened but not fully, and the star button didn't get added to the url bar (fair warning for when you do the rebase, maybe sooner rather than later?).

Comment on attachment 8888891 [details] Bug 1374477 - Add browser-pageActions.js for Photon page actions. https://reviewboard.mozilla.org/r/159928/#review165772 ::: browser/base/content/browser-pageActions.js:87 (Diff revision 2) > + let insertAfterNode = null; > + if (action.insertAfter) { > + let insertAfterNodeID = this._panelButtonNodeIDForActionID(action.insertAfter); > + insertAfterNode = document.getElementById(insertAfterNodeID); > + } I don't know how this will ensure a consistent ordering for webextension-related nodes. If one is enabled and disabled, does that alter the ordering? Do they race with each other on startup for 'first' position? That seems undesirable. We could store an ordering (like for the urlbar), but I think the simplest might be to just sort items (other than the builtin browser ones) alphabetically. Would that work? ::: browser/base/content/browser-pageActions.js:241 (Diff revision 2) > + if (node) { > + node.remove(); > + } I think for at least the star button, we can't really do this. There are lazy getters in browser-places.js and other places that expect this node to simply be present. Instead, can we toggle the "hidden" attribute? ::: browser/base/content/browser-pageActions.js:252 (Diff revision 2) > + for (let childNode of parentNode.childNodes) { > + if (childNode == this.mainButtonNode) { > + continue; > + } > + if (index == urlbarIndex) { > + insertBeforeNode = childNode; > + break; > + } > + index++; > + } Could this just be parentNode.childNodes[index + 1] ? :-) I'm assuming we're forcing the page action button to always be the first item, per https://bugzilla.mozilla.org/show_bug.cgi?id=1378560 . ::: browser/base/content/browser-pageActions.js:270 (Diff revision 2) > + return node; > + }, > + > + _makeUrlbarButtonNode(action) { > + let buttonNode = document.createElement("image"); > + buttonNode.classList.add("urlbar-icon"); We should make sure https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.css#683 continues to apply here (ie the buttons get hidden when the pageproxystate is invalid). ::: browser/base/content/browser-pageActions.js:275 (Diff revision 2) > + buttonNode.classList.add("urlbar-icon"); > + if (action.tooltip) { > + buttonNode.setAttribute("tooltiptext", action.tooltip); > + } > + if (action.iconURL) { > + buttonNode.style.listStyleImage = `url('${action.iconURL}')`; We should make sure that larger icons still get resized to smaller ones (so specify a width/height in CSS), so that webextensions can specify hidpi icons. ::: browser/base/content/browser-pageActions.js:282 (Diff revision 2) > + if (action.nodeAttributes) { > + for (let name in action.nodeAttributes) { > + buttonNode.setAttribute(name, action.nodeAttributes[name]); > + } > + } > + buttonNode.addEventListener("click", event => { Do we need a command/keypress listener as well? ::: browser/base/content/browser-pageActions.js:346 (Diff revision 2) > + let url = action.iconURL ? `url('${action.iconURL}')` : null; > + let nodeIDs = [ > + this._panelButtonNodeIDForActionID(action.id), > + this._urlbarButtonNodeIDForActionID(action.id), > + ]; > + for (nodeID of nodeIDs) { Nit: missing let ::: browser/base/content/browser-pageActions.js:352 (Diff revision 2) > + let node = document.getElementById(nodeID); > + if (node) { > + if (url) { > + node.style.listStyleImage = url; > + } else { > + node.style.removeProperty("listStyleImage"); This needs to be list-style-image or it won't work. ::: browser/base/content/browser-pageActions.js:550 (Diff revision 2) > +}; > + > +// copy URL > +BrowserPageActions.copyURL = { > + /** > + * The onPlacedInPanel handler for the action. These copy-pasted docstring comments on the builtin action handler methods don't add much - I think Mike was talking mostly about the stuff directly on BrowserPageActions, so I would omit these for brevity, but it's up to you. Sorry for the back and forth! ::: browser/base/content/browser-pageActions.js:556 (Diff revision 2) > + * > + * @param buttonNode (DOM node, required) > + * The buttonNode arg passed to the action's onPlacedInPanel. > + */ > + onPlacedInPanel(buttonNode) { > + BrowserPageActions.takeNodeAttributeFromPanel(buttonNode, "title"); Can we do this somewhere more central for all the builtin actions, rather than copying this into the `onPlacedInPanel` version of each of these? Note that we will also need to set tooltiptexts on the buttons in the URL bar for the items in the URL bar. ::: browser/base/content/browser-pageActions.js:616 (Diff revision 2) > + * > + * @param panelViewNode (DOM node, required) > + * The panelViewNode arg passed to the action's onSubviewPlaced. > + */ > + onSubviewPlaced(panelViewNode) { > + BrowserPageActions.takeNodeAttributeFromPanel(panelViewNode, "title"); This shouldn't be necessary. The photonpanelmultiview should take this from the label of the anchor button anyway.

(In reply to :Gijs from comment #85) > Generally to fix this kind of thing, you'll want to push your changes from a > recent m-c, and then push a dummy commit (like what you did here) from the > same m-c rev, and then compare those two revs. Thanks. That shows no significant changes: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=8c0bc79904ccf4f46a73cc26a87cc1ca28812c65&newProject=try&newRevision=b80a3e92a09233bb38ad4c9f46195794d6c9a474&framework=1&showOnlyImportant=0

(In reply to :Gijs from comment #86) > It looks like the failure on the last trypush, at least, is related to where > new bookmarks created from the bookmark icon are getting filed. Perhaps your > changes change the command that gets executed if you click the bookmark > star? I think if you change this in the second patch, to match the XUL stuff > you remove in the 3rd one (so call BookmarkingUI.onStarCommand(event)) then > it should be fixed. See also bug 1120110 (but let's not try to fix that > here, let's just make sure tests pass and this button continues to do what > it used to do). Sounds plausible, yeah.

Thanks Gijs. (In reply to :Gijs from comment #91) > This effectively looks good to me, besides the issue of the ordering of > multiple non-builtin items. If they both end up with the same `insertAfter`, insertAfter is really just a generalized way of adding the semi-built-in Pocket and screenshots items in the right order in the panel. An alternative would be to acknowledge that they're semi-built-in and define their ordering in the jsm, even if they themselves aren't defined in the jsm. I don't really like their leaking into the jsm though. It would be better if they could be completely self contained. Sorting non-built-in actions alphabetically and putting them at the bottom of the panel sounds good. (It would be nice to add a separator between them and the built-ins too.) Given that, it seems clear that insertAfter is a bad idea for extensions in general. So I think what I'd like is for insertAfter to be "private" -- just not documented and maybe underscore prefixed or something. Only Pocket and screenshots would use it, but we wouldn't try preventing other extensions from using it. Do you like that idea? If not, then let's define the ordering in the jsm. (In reply to :Gijs from comment #92) > ::: browser/base/content/browser-pageActions.js:241 > (Diff revision 2) > > + if (node) { > > + node.remove(); > > + } > > I think for at least the star button, we can't really do this. There are > lazy getters in browser-places.js and other places that expect this node to > simply be present. > > Instead, can we toggle the "hidden" attribute? OK. Are you suggesting hidden only for the star button or for them all? I would think The Right Thing would be to remove when possible (i.e., non-star) to reclaim resources, etc., however negligible?

(In reply to Drew Willcoxon :adw from comment #93) > (In reply to :Gijs from comment #85) > > Generally to fix this kind of thing, you'll want to push your changes from a > > recent m-c, and then push a dummy commit (like what you did here) from the > > same m-c rev, and then compare those two revs. > > Thanks. That shows no significant changes: > https://treeherder.mozilla.org/perf.html#/ > compare?originalProject=try&originalRevision=8c0bc79904ccf4f46a73cc26a87cc1ca > 28812c65&newProject=try&newRevision=b80a3e92a09233bb38ad4c9f46195794d6c9a474& > framework=1&showOnlyImportant=0 \o/ Note that the trypush with changes shows a few minor eslint issues. You can configure eslint to complain on commit so you notice these immediately, see https://blog.mozilla.org/standard8/2017/05/04/running-eslint-on-commit-for-mozilla-central-git-mercurial/ . (In reply to Drew Willcoxon :adw from comment #96) > Thanks Gijs. > > (In reply to :Gijs from comment #91) > > This effectively looks good to me, besides the issue of the ordering of > > multiple non-builtin items. If they both end up with the same `insertAfter`, > > insertAfter is really just a generalized way of adding the semi-built-in > Pocket and screenshots items in the right order in the panel. An > alternative would be to acknowledge that they're semi-built-in and define > their ordering in the jsm, even if they themselves aren't defined in the > jsm. I don't really like their leaking into the jsm though. It would be > better if they could be completely self contained. > > Sorting non-built-in actions alphabetically and putting them at the bottom > of the panel sounds good. (It would be nice to add a separator between them > and the built-ins too.) Given that, it seems clear that insertAfter is a > bad idea for extensions in general. > > So I think what I'd like is for insertAfter to be "private" -- just not > documented and maybe underscore prefixed or something. Only Pocket and > screenshots would use it, but we wouldn't try preventing other extensions > from using it. Do you like that idea? If not, then let's define the > ordering in the jsm. Thanks for the detailed explanation. That (ie using it only for pocket/screenshots) sounds sensible to me, though I expect that for the webextensions API you *would* actually want to prevent non-screenshots add-ons being able to use it - but adding restrictions for that property is a hurdle we can jump when we add the webextensions stuff anyway. > (In reply to :Gijs from comment #92) > > ::: browser/base/content/browser-pageActions.js:241 > > (Diff revision 2) > > > + if (node) { > > > + node.remove(); > > > + } > > > > I think for at least the star button, we can't really do this. There are > > lazy getters in browser-places.js and other places that expect this node to > > simply be present. > > > > Instead, can we toggle the "hidden" attribute? > > OK. Are you suggesting hidden only for the star button or for them all? I > would think The Right Thing would be to remove when possible (i.e., > non-star) to reclaim resources, etc., however negligible? This is a good question. I hadn't really thought about it. I guess the star button alone would be OK for now. If you do it based on a property on the action or button options, rather than an ID check, it should be easy to use it for other buttons if we find that it is/becomes necessary for those due to other consumers. Note that the lazy getter stuff would mean you'd want to always insert the button on startup, too, even if the user has "removed" it from the URL bar, so as not to break the BookmarkingUI.star getter and its friends. Though, that being said, I wonder if we need to fix something with anchoring of the bookmarks popup there. I don't know what happens if the node is hidden / removed and the user hits cmd-d to bookmark. I guess we should anchor on the page action button in that case, but I expect we'll need to write code for that to happen correctly, maybe, as I imagine the current code doesn't check if the star itself is hidden, and would try to use it as an anchor anyway, which our popup code won't like. ... so maybe, instead of using the hidden attribute and always inserting the node, it would be better to just update the BookmarkingUI.star getter to not be self-deleting and just always document.getElementById... :-\

(In reply to :Gijs from comment #86) > It looks like the failure on the last trypush, at least, is related to where > new bookmarks created from the bookmark icon are getting filed. Perhaps your > changes change the command that gets executed if you click the bookmark > star? I think if you change this in the second patch, to match the XUL stuff > you remove in the 3rd one (so call BookmarkingUI.onStarCommand(event)) then > it should be fixed. That did fix the try failure.

Gijs, please don't review these three commits yet. I need to address some of your comments still. The stupid mozreview thing re-requested review even though I removed the r? from the commit messages. These are rebased on current m-c tip. They also address the new bookmark star button animation, fun. I added a new _urlbarNodeInMarkup property that addresses that as well as the discussion about how to hide the star button. If that property is true, then BrowserPageActions just shows/hides the urlbar node when the action is added/removed.

Ready for review, still working on new test though. (In reply to :Gijs from comment #91) > This effectively looks good to me, besides the issue of the ordering of > multiple non-builtin items. This now groups together non-built-in (and non-semi-built-in) actions at the end of the panel, sorted by localeCompare. > ::: browser/modules/PageActions.jsm:92 > (Diff revision 2) > > + * @param action (Action, required) > > + * The Action object to register. > > + * @return The given Action. > > + */ > > + addAction(action) { > > + if (this._deferredAddActionCalls) { > > Please either add an explicit `_initialized` bool property and use it in > lieu of this check, or a comment clarifying that we need to defer adding > this action if we haven't been initialized yet, and that > `_deferredAddActionCalls` will be deleted once we've initialized. Did the latter: added a comment. > ::: browser/modules/PageActions.jsm:107 > (Diff revision 2) > > + // the index of the action in the _actions array > > + let actionsIndex = !action.insertAfter ? this._actions.length : > > + this._actions.findIndex(a => a.id == action.insertAfter) + 1; > > If the `insertAfter` doesn't exist, this will put this action first (index > -1 for not found, + 1). I think ideally new non-builtin actions should > always come after the builtin ones, especially if they specify invalid > `insertAfter` values. Fixed because I changed this to be ID-based instead of index-based, plus I changed insert-after to insert-before, to be more congruent with the DOM's Node.insertBefore. So now (1) conceptually we go with the grain of Node.insertBefore instead of against it, and (2) it's easier to actually add nodes to the DOM since now I don't have to walk through the nodes and convert an insert-after ID to an insert-before ID. > ::: browser/modules/PageActions.jsm:696 > (Diff revision 2) > > + // use that for the button in the urlbar. > > + urlbarIDOverride: "star-button", > > + title: "", > > + shownInUrlbar: true, > > + nodeAttributes: { > > + observes: "bookmarkThisPageBroadcaster", > > Does this work for both starred and tooltiptext state? Yes.

(In reply to :Gijs from comment #92) > ::: browser/base/content/browser-pageActions.js:87 > (Diff revision 2) > > + let insertAfterNode = null; > > + if (action.insertAfter) { > > + let insertAfterNodeID = this._panelButtonNodeIDForActionID(action.insertAfter); > > + insertAfterNode = document.getElementById(insertAfterNodeID); > > + } > > I don't know how this will ensure a consistent ordering for > webextension-related nodes. If one is enabled and disabled, does that alter > the ordering? Do they race with each other on startup for 'first' position? > That seems undesirable. > > We could store an ordering (like for the urlbar), but I think the simplest > might be to just sort items (other than the builtin browser ones) > alphabetically. Alphabetically (localeCompare), as discussed. > ::: browser/base/content/browser-pageActions.js:241 > (Diff revision 2) > > + if (node) { > > + node.remove(); > > + } > > I think for at least the star button, we can't really do this. There are > lazy getters in browser-places.js and other places that expect this node to > simply be present. > > Instead, can we toggle the "hidden" attribute? I added a new _urlbarNodeInMarkup Action option, as I mentioned in an earlier comment. If that option is present, then the node is hidden/shown in the urlbar when the action is toggled. One other thing about the star button is that star-button isn't the root star button node anymore, star-button-box is (due to bug 1352063), and fortunately the ID change was all that was necessary. Things still seem to work OK. > ::: browser/base/content/browser-pageActions.js:252 > (Diff revision 2) > > + for (let childNode of parentNode.childNodes) { > > + if (childNode == this.mainButtonNode) { > > + continue; > > + } > > + if (index == urlbarIndex) { > > + insertBeforeNode = childNode; > > + break; > > + } > > + index++; > > + } > > Could this just be parentNode.childNodes[index + 1] ? :-) This was all simplified by using an insert-before ID instead of an insert-after-index. > I'm assuming we're forcing the page action button to always be the first > item, per https://bugzilla.mozilla.org/show_bug.cgi?id=1378560 . Yes, that now happens because of the insertAfterID passed to placeActionInUrlbar: it's either the ID of an action node already after the main button, or it's null, so Node.insertBefore will append the new action node to the parent (which also contains the main button). > ::: browser/base/content/browser-pageActions.js:270 > (Diff revision 2) > > + return node; > > + }, > > + > > + _makeUrlbarButtonNode(action) { > > + let buttonNode = document.createElement("image"); > > + buttonNode.classList.add("urlbar-icon"); > > We should make sure > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser. > css#683 continues to apply here (ie the buttons get hidden when the > pageproxystate is invalid). Previous commits added a new hbox#pageActionUrlbarIcons at the very end of the urlbar to contain these page action icons. After you mentioned this I looked and I think it should be OK to add them to the hbox#urlbar-icons. (The star button was there to begin with anyway.) That means that hbox#userContext-icons is now at the end of the urlbar, after the page actions... I guess that's OK? One other thing I noticed is that the star button no longer matches the selector that you linked to, due to the star-button-box. Not sure whether that was intentional. > ::: browser/base/content/browser-pageActions.js:275 > (Diff revision 2) > > + buttonNode.classList.add("urlbar-icon"); > > + if (action.tooltip) { > > + buttonNode.setAttribute("tooltiptext", action.tooltip); > > + } > > + if (action.iconURL) { > > + buttonNode.style.listStyleImage = `url('${action.iconURL}')`; > > We should make sure that larger icons still get resized to smaller ones (so > specify a width/height in CSS), so that webextensions can specify hidpi > icons. I added a width and height to .urlbar-icon. There are other .urlbar-icons besides page actions of course, so do you think that's OK? > ::: browser/base/content/browser-pageActions.js:282 > (Diff revision 2) > > + if (action.nodeAttributes) { > > + for (let name in action.nodeAttributes) { > > + buttonNode.setAttribute(name, action.nodeAttributes[name]); > > + } > > + } > > + buttonNode.addEventListener("click", event => { > > Do we need a command/keypress listener as well? I assume not because right now the star button only has a click handler, an onclick in the markup. I checked browser-places.js to make sure it didn't dynamically add another listener. Is it even possible to keyboard-focus these images? I can't seem to do it. > ::: browser/base/content/browser-pageActions.js:556 > (Diff revision 2) > > + * > > + * @param buttonNode (DOM node, required) > > + * The buttonNode arg passed to the action's onPlacedInPanel. > > + */ > > + onPlacedInPanel(buttonNode) { > > + BrowserPageActions.takeNodeAttributeFromPanel(buttonNode, "title"); > > Can we do this somewhere more central for all the builtin actions, rather > than copying this into the `onPlacedInPanel` version of each of these? Hmm, not sure I can think of a more succinct way. The obvious place to do that would be BrowserPageActions.placeActionInPanel, but the problem with that is that it doesn't know what action it's placing. It doesn't know whether action.title refers to an attribute on the panel or not. It would be easy to make a mapping or list of the built-in actions that would support that, but then that's basically what I've got here already. I could do something like: for (let actionSetup of [BrowserPageActions.copyURL, /* etc. */ ]) { actionSetup.onPlacedInPanel = buttonNode => { BrowserPageActions.takeNodeAttributeFromPanel(buttonNode, "title"); }; } But I think that might be harder to follow than what I have now, since it would split each of these action setup objects in two. > Note that we will also need to set tooltiptexts on the buttons in the URL > bar for the items in the URL bar. Action has a tooltip option/property, so I could do the same thing that I do for titles: grab the tooltip string off the panel and set it as the tooltip in onPlacedInUrlbar callbacks. Instead though, I added a backstop for urlbar nodes: if the node doesn't have a tooltip, then use the label of the panel button. > ::: browser/base/content/browser-pageActions.js:616 > (Diff revision 2) > > + * > > + * @param panelViewNode (DOM node, required) > > + * The panelViewNode arg passed to the action's onSubviewPlaced. > > + */ > > + onSubviewPlaced(panelViewNode) { > > + BrowserPageActions.takeNodeAttributeFromPanel(panelViewNode, "title"); > > This shouldn't be necessary. The photonpanelmultiview should take this from > the label of the anchor button anyway. OK. That means I can get rid of Subview.title (and not set the "title" of corresponding panelview nodes when they're created) and the current send-to-device subview title string.

(In reply to :Gijs from comment #97) > Note that the trypush with changes shows a few minor eslint issues. You can > configure eslint to complain on commit so you notice these immediately, see > https://blog.mozilla.org/standard8/2017/05/04/running-eslint-on-commit-for- > mozilla-central-git-mercurial/ . Should be fixed...

Whoops, I found a couple of problems around the separator between the built-ins and non-built-ins. One sec.

OK, should be fixed.

Note unrelated to code review, so sticking in bugzilla: on try, browser_startup_images is now complaining about page-action.svg: Loaded image chrome://browser/skin/page-action.svg should have been shown. also browser/base/content/test/urlbar/browser_page_action_menu.js is perma-orange, but I guess that's what comment #112 meant about tests?

(In reply to Drew Willcoxon :adw from comment #113) > (In reply to :Gijs from comment #92) > > ::: browser/base/content/browser-pageActions.js:270 > > (Diff revision 2) > > > + return node; > > > + }, > > > + > > > + _makeUrlbarButtonNode(action) { > > > + let buttonNode = document.createElement("image"); > > > + buttonNode.classList.add("urlbar-icon"); > > > > We should make sure > > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser. > > css#683 continues to apply here (ie the buttons get hidden when the > > pageproxystate is invalid). > > Previous commits added a new hbox#pageActionUrlbarIcons at the very end of > the urlbar to contain these page action icons. After you mentioned this I > looked and I think it should be OK to add them to the hbox#urlbar-icons. > (The star button was there to begin with anyway.) That means that > hbox#userContext-icons is now at the end of the urlbar, after the page > actions... I guess that's OK? > > One other thing I noticed is that the star button no longer matches the > selector that you linked to, due to the star-button-box. Not sure whether > that was intentional. Nice catch. Jared, I think this is a regression from the animation changes that I should have spotted. Can you fix this independently?

Comment on attachment 8879364 [details] Bug 1374477 - Remove page actions from browser.xul. https://reviewboard.mozilla.org/r/150666/#review167354 Yep, looks sane to me besides the comment below. As for ordering of the user context id and other url bar items, we can sort the rest out in bug https://bugzilla.mozilla.org/show_bug.cgi?id=1378560 . ::: browser/base/content/browser.xul:899 (Diff revisions 16 - 19) > <toolbarbutton id="urlbar-zoom-button" > onclick="FullZoom.reset();" > tooltip="dynamic-shortcut-tooltip" > hidden="true"/> > +#ifdef MOZ_PHOTON_THEME > + <image id="pageActionButton" Oh, interesting. This is certainly consistent, but I expect we should keep the page action button available even when the URL has been modified. I realize that's somewhat inconsistent, but to me it feels more like the old stop/go/reload item in that it should always "bookend" the location bar, plus the fact that to a normal person it's not very obvious how to revert back to a non-edited URL bar (just typing in the same URL or using the editor's "undo" functionality won't do, this is a known problem that also affects the identity block). Perhaps get UX feedback on whether they want this to disappear or not? I'll try to remember to bring this up in the meeting I have with the UX folks in an hour, that might be quickest. I don't want to scope-creep this bug into changing too much in the location bar otherwise, but now that you've moved the page action button itself I guess we should make sure that's OK.

(In reply to :Gijs from comment #125) > Comment on attachment 8879364 [details] > Bug 1374477 - Remove page actions from browser.xul. > > https://reviewboard.mozilla.org/r/150666/#review167354 > > Yep, looks sane to me besides the comment below. As for ordering of the user > context id and other url bar items, we can sort the rest out in bug > https://bugzilla.mozilla.org/show_bug.cgi?id=1378560 . > > ::: browser/base/content/browser.xul:899 > (Diff revisions 16 - 19) > > <toolbarbutton id="urlbar-zoom-button" > > onclick="FullZoom.reset();" > > tooltip="dynamic-shortcut-tooltip" > > hidden="true"/> > > +#ifdef MOZ_PHOTON_THEME > > + <image id="pageActionButton" > > Oh, interesting. This is certainly consistent, but I expect we should keep > the page action button available even when the URL has been modified. I > realize that's somewhat inconsistent, but to me it feels more like the old > stop/go/reload item in that it should always "bookend" the location bar, > plus the fact that to a normal person it's not very obvious how to revert > back to a non-edited URL bar (just typing in the same URL or using the > editor's "undo" functionality won't do, this is a known problem that also > affects the identity block). Perhaps get UX feedback on whether they want > this to disappear or not? I'll try to remember to bring this up in the > meeting I have with the UX folks in an hour, that might be quickest. I don't > want to scope-creep this bug into changing too much in the location bar > otherwise, but now that you've moved the page action button itself I guess > we should make sure that's OK. Talked to UX, you were right and I was wrong, they want it hidden. :-)

Comment on attachment 8888891 [details] Bug 1374477 - Add browser-pageActions.js for Photon page actions. https://reviewboard.mozilla.org/r/159928/#review167372 Very nice. Only one thing I noticed in the patch below. ::: browser/base/content/browser-pageActions.js:272 (Diff revisions 2 - 5) > - break; > - } > - index++; > } > + node.hidden = false; > parentNode.insertBefore(node, insertBeforeNode); Should we wrap this in an `if (!action.__urlbarNodeInMarkup)` check? Or do we still need to do this for the markup-included items to make sure the ordering is right, or something? ::: browser/base/content/browser-pageActions.js:636 (Diff revisions 2 - 5) > - return; > - } > - > // In the first ~10 sec after startup, Sync may not be loaded and the list > // of devices will be empty. > - if (!gSync.syncReady) { > + if (gSync.syncConfiguredAndLoading) { I'm assuming these changes are just copy-pasted from wherever they are now, so not reviewing closely. :-)

Comment on attachment 8888890 [details] Bug 1374477 - Add PageActions.jsm for Photon page actions. https://reviewboard.mozilla.org/r/159926/#review167378

All this work looks pretty fabulous and ready to land to me. Besides the tests, which I guess will be in a separate commit, and the updates to tests for my notes in comment #128, I noticed 2 things when testing these patches: 1. the end margin on the urlbar-icons container should be increased, and we can probably get rid of whatever CSS currently gives the page action button an extra end-margin. 2. I don't seem to get icons for devices in the synced devices view anymore, neither when opened from the main page action menu nor when putting the synced devices item in the url bar. Tested on OS X in case that matters.

(In reply to Drew Willcoxon :adw from comment #113) > (In reply to :Gijs from comment #92) > > ::: browser/base/content/browser-pageActions.js:275 > > (Diff revision 2) > > > + buttonNode.classList.add("urlbar-icon"); > > > + if (action.tooltip) { > > > + buttonNode.setAttribute("tooltiptext", action.tooltip); > > > + } > > > + if (action.iconURL) { > > > + buttonNode.style.listStyleImage = `url('${action.iconURL}')`; > > > > We should make sure that larger icons still get resized to smaller ones (so > > specify a width/height in CSS), so that webextensions can specify hidpi > > icons. > > I added a width and height to .urlbar-icon. There are other .urlbar-icons > besides page actions of course, so do you think that's OK? I think so, yeah. > > ::: browser/base/content/browser-pageActions.js:282 > > (Diff revision 2) > > > + if (action.nodeAttributes) { > > > + for (let name in action.nodeAttributes) { > > > + buttonNode.setAttribute(name, action.nodeAttributes[name]); > > > + } > > > + } > > > + buttonNode.addEventListener("click", event => { > > > > Do we need a command/keypress listener as well? > > I assume not because right now the star button only has a click handler, an > onclick in the markup. I checked browser-places.js to make sure it didn't > dynamically add another listener. Is it even possible to keyboard-focus > these images? I can't seem to do it. Fair points. > > ::: browser/base/content/browser-pageActions.js:556 > > (Diff revision 2) > > > + * > > > + * @param buttonNode (DOM node, required) > > > + * The buttonNode arg passed to the action's onPlacedInPanel. > > > + */ > > > + onPlacedInPanel(buttonNode) { > > > + BrowserPageActions.takeNodeAttributeFromPanel(buttonNode, "title"); > > > > Can we do this somewhere more central for all the builtin actions, rather > > than copying this into the `onPlacedInPanel` version of each of these? > > Hmm, not sure I can think of a more succinct way. The obvious place to do > that would be BrowserPageActions.placeActionInPanel, but the problem with > that is that it doesn't know what action it's placing. It doesn't know > whether action.title refers to an attribute on the panel or not. It would > be easy to make a mapping or list of the built-in actions that would support > that, but then that's basically what I've got here already. > > I could do something like: > > for (let actionSetup of [BrowserPageActions.copyURL, /* etc. */ ]) { > actionSetup.onPlacedInPanel = buttonNode => { > BrowserPageActions.takeNodeAttributeFromPanel(buttonNode, "title"); > }; > } > > But I think that might be harder to follow than what I have now, since it > would split each of these action setup objects in two. Yeah, don't worry about it for now.

(In reply to :Gijs from comment #123) > Note unrelated to code review, so sticking in bugzilla: on try, > browser_startup_images is now complaining about page-action.svg: > > Loaded image chrome://browser/skin/page-action.svg should have been shown. > > > also browser/base/content/test/urlbar/browser_page_action_menu.js is > perma-orange, but I guess that's what comment #112 meant about tests? Thanks for noticing, browser_page_action_menu.js is supposed to be passing with these changes, so I'll take a look.

(In reply to Drew Willcoxon :adw from comment #131) > Thanks for noticing, browser_page_action_menu.js is supposed to be passing > with these changes, so I'll take a look. The problem (or one problem at least) is that the main button is now hidden on about:blank, so the test can't open the panel when it expects to.

(In reply to :Gijs from comment #123) > Note unrelated to code review, so sticking in bugzilla: on try, > browser_startup_images is now complaining about page-action.svg: > > Loaded image chrome://browser/skin/page-action.svg should have been shown. That test has this comment: "Please don't add items to this list." But in this case the reason the svg isn't shown is because the test happens on about:blank, and now we're hiding the button on about:blank. It's similar to the urlbar's dropdown arrow, loaded but not shown. So I added the svg to the whitelist anyway. I guess the reason you're not supposed to add items to the whitelist is that you should make sure if an image isn't shown then it's not loaded, but I dunno in this case, seems legit. > also browser/base/content/test/urlbar/browser_page_action_menu.js is > perma-orange Should be fixed now, along with browser_page_action_menu_clipboard.js.

(In reply to :Gijs from comment #127) > ::: browser/base/content/browser-pageActions.js:272 > (Diff revisions 2 - 5) > > - break; > > - } > > - index++; > > } > > + node.hidden = false; > > parentNode.insertBefore(node, insertBeforeNode); > > Should we wrap this in an `if (!action.__urlbarNodeInMarkup)` check? Or do > we still need to do this for the markup-included items to make sure the > ordering is right, or something? Well, maybe (except it's the other way around: `if (action.__urlbarNodeInMarkup)`). The reason I didn't do that is because it should be a no-op for non-markup actions, since they are removed and re-added as necessary. So a conditional didn't seem necessary. But -- your comment made me take another look, and it makes more sense to move this line up to the `if (action.__urlbarNodeInMarkup)` that's already above. Much clearer that it only needs to happen in that case. > ::: browser/base/content/browser-pageActions.js:636 > (Diff revisions 2 - 5) > > - return; > > - } > > - > > // In the first ~10 sec after startup, Sync may not be loaded and the list > > // of devices will be empty. > > - if (!gSync.syncReady) { > > + if (gSync.syncConfiguredAndLoading) { > > I'm assuming these changes are just copy-pasted from wherever they are now, > so not reviewing closely. :-) Yes, there were recent changes to the send-to-devices subview that landed while I've been working on this.

(In reply to :Gijs from comment #129) > 1. the end margin on the urlbar-icons container should be increased, and we > can probably get rid of whatever CSS currently gives the page action button > an extra end-margin. I looked at the spec again, and it shows way more space between icons than my patches had. I measured, and it's 12 pts between the visible edges of icons, and 6 pts between the last icon and urlbar edge. So I modified margins to match that. One thing to note is that even though `width: 16px` is in the CSS on .urlbar-icon, the actual widths of these icons ends up being 22 pts. Assuming a visible image width of 16 pts, that's 6 pts of blank space left over in the <image>s' margins, 3 pts on each side. So that's how I arrived at `margin-inline-start: 6px`: 3 + 6 + 3 = 12 pts. > 2. I don't seem to get icons for devices in the synced devices view anymore, > neither when opened from the main page action menu nor when putting the > synced devices item in the url bar. Tested on OS X in case that matters. Fixed

Try looks OK so far... Still working on the new test, to be posted as a new mozreview thing.

(In reply to :Gijs from comment #124) > (In reply to Drew Willcoxon :adw from comment #113) > > (In reply to :Gijs from comment #92) > > > ::: browser/base/content/browser-pageActions.js:270 > > > (Diff revision 2) > > > > + return node; > > > > + }, > > > > + > > > > + _makeUrlbarButtonNode(action) { > > > > + let buttonNode = document.createElement("image"); > > > > + buttonNode.classList.add("urlbar-icon"); > > > > > > We should make sure > > > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser. > > > css#683 continues to apply here (ie the buttons get hidden when the > > > pageproxystate is invalid). > > > > Previous commits added a new hbox#pageActionUrlbarIcons at the very end of > > the urlbar to contain these page action icons. After you mentioned this I > > looked and I think it should be OK to add them to the hbox#urlbar-icons. > > (The star button was there to begin with anyway.) That means that > > hbox#userContext-icons is now at the end of the urlbar, after the page > > actions... I guess that's OK? > > > > One other thing I noticed is that the star button no longer matches the > > selector that you linked to, due to the star-button-box. Not sure whether > > that was intentional. > > Nice catch. Jared, I think this is a regression from the animation changes > that I should have spotted. Can you fix this independently? Thanks, I filed bug 1385407 for this.

New test, ready for review. Also in the new test commit are some code changes to problems that the test revealed. Gijs, there's one thing I want to ask about. The way I wrote the test, I happen to add an action with a subview after the panel has already been opened. (The first non-init test task opens the panel, and then the second test task adds an action with a subview.) That created a problem where an exception is raised when the test shows the subview: ("SlidingPanelViews :: index -1 out of bounds"). I saw that problem awhile ago and asked Mike about it at the all-hands. IIRC he suggested adding new panelviews to the view cache instead of directly to the multiview, and that did work. Since then, I made a bunch of changes and didn't hit that problem again, until now. I tried again using the view cache but got the same exception. So what this does is to null out the multiview's _panelViews so that it's recreated, since AFAICT the problem is that _panelViews doesn't pick up the newly added panelview. That basically works, but even then sometimes the panel gets in a weird state, along with that exception, and I found that waiting a second when the 2nd test task starts seems to fix it. I don't know whether a second is more than enough time or what. Any ideas? Other code changes made for the test: * Add BrowserPageAction methods for generating panelview IDs and the IDs of buttons in panelviews, so that the test can generate them. * Add BrowserPageAction._tempPanelID so the test cna use it. * Set "tabspecific" attr on BrowserPageAction's temp panel. * Remove the separator in the panel between built-in and non-built-in actions when the last non-built-in action is removed. * Add PageActions.nonBuiltInActions basically for that purpose. For completeness, add PageActions.builtInActions too. * When the main page action button is clicked, hide the temp panel if it's open.

Re: the problem in comment 147, it's worth noting that we won't actually hit that in practice since there are no non-built-in actions -- no actions that are added after the user could possibly open the panel -- since PageActions isn't hooked up to WebExtensions (yet, possibly). So it's not worth spending a lot of time on right now IMO.

Failures on try after the subview is shown, sigh... Let's see if it works to use the five-second timeout that other subview tests use.

Fixed the eslint errors locally.

Comment on attachment 8891613 [details] Bug 1374477 - Add a new test for Photon page actions, along with some related code changes. https://reviewboard.mozilla.org/r/162722/#review168082 r=me with the timeout removed per the comments below. ::: browser/base/content/browser-pageActions.js:127 (Diff revision 1) > } > let panelViewNode = null; > if (action.subview) { > buttonNode.classList.add("subviewbutton-nav"); > panelViewNode = this._makePanelViewNodeForAction(action, false); > + this.multiViewNode._panelViews = null; Can you file a followup issue for needing to do this? We should ideally update PanelMultiView to not require this workaround. ::: browser/modules/test/browser/browser_PageActions.js:53 (Diff revision 1) > + Assert.ok(!!event, event); > + Assert.ok(!!buttonNode, buttonNode); You use 2-param versions of Assert.ok, but the second param is usually not a string but a node or event object, which doesn't serialize usefully. Can you update these to just say "should have event", or log the id of the target or something? Note that for Assert.ok(), I don't think you need to coerce to boolean yourself. :-) ::: browser/modules/test/browser/browser_PageActions.js:74 (Diff revision 1) > + Assert.ok(!!buttonNode, buttonNode); > + Assert.equal(buttonNode.id, panelButtonID); > + }, > + })); > + > + Assert.equal(action.iconURL, iconURL); While I don't like making work for you, can we add short 'reason' bits to the assertions in this test ("action iconURL should match" would already help) ? Right now, when stuff fails (I played around with the patch because of the setTimeout stuff), it's Really Hard to figure out where "1 == 0" failures and such actually come from, or where in the code things broke. ::: browser/modules/test/browser/browser_PageActions.js:134 (Diff revision 1) > + // Open the panel, click the action's button. > + await promisePageActionPanelOpen(); > + Assert.equal(onShowingInPanelCallCount, 1); > + onCommandExpectedButtonID = panelButtonID; > + EventUtils.synthesizeMouseAtCenter(panelButtonNode, {}); > + await promisePageActionPanelHidden; I think you're missing '()' here. ::: browser/modules/test/browser/browser_PageActions.js:183 (Diff revision 1) > + // Without this, sometimes the subview isn't correctly shown. Something to > + // do with how the panelview is added after the panel has already been shown. > + // ("SlidingPanelViews :: index -1 out of bounds") > + await new Promise(r => setTimeout(r, 1000)); I can get rid of this by replacing some synthesizeMouseAtCenters in the test with just node.click(). I think the issue is that somehow synthesizeMouse does not work well with the stacked panelviews (inside the panelmultiview) at all times, because it tries to get a node based on coordinates, and the stack means multiple views can be displayed at those coordinates... or something. Anyway, I like using .click() a lot better than the timeout. :-) ::: browser/modules/test/browser/browser_PageActions.js:302 (Diff revision 1) > + EventUtils.synthesizeMouseAtCenter(panelButtonNode, {}); > + await subviewShownPromise; > + Assert.equal(onActionCommandCallCount, 0); > + Assert.equal(onSubviewShowingCount, 1); > + onButtonCommandExpectedButtonID = panelViewButtonIDPanel; > + EventUtils.synthesizeMouseAtCenter(panelViewButtonNodePanel, {}); Specifically, the 2 synthesizeMouseAtCenter calls in this bit of code to open the subview and then close the panel work better for me if I just use node.click(). This doesn't fix the index out of bounds issue, that's something I'll look into separately, but that error seems mostly harmless.

(In reply to :Gijs from comment #153) > ::: browser/modules/test/browser/browser_PageActions.js:53 > (Diff revision 1) > > + Assert.ok(!!event, event); > > + Assert.ok(!!buttonNode, buttonNode); > > You use 2-param versions of Assert.ok, but the second param is usually not a > string but a node or event object, which doesn't serialize usefully. FWIW passing the object as the second param is exactly just to serialize it, to see what the object is when these asserts fail. (Although now that I think about it, that's probably not so useful here.) More below. > ::: browser/modules/test/browser/browser_PageActions.js:74 > (Diff revision 1) > > + Assert.ok(!!buttonNode, buttonNode); > > + Assert.equal(buttonNode.id, panelButtonID); > > + }, > > + })); > > + > > + Assert.equal(action.iconURL, iconURL); > > While I don't like making work for you, can we add short 'reason' bits to > the assertions in this test ("action iconURL should match" would already > help) ? Right now, when stuff fails (I played around with the patch because > of the setTimeout stuff), it's Really Hard to figure out where "1 == 0" > failures and such actually come from, or where in the code things broke. OK, I'll make these changes, but FWIW assertion reasons aren't useful in general IMO. It's busy work for the test writer (often they're just like "X should not be null" which is totally obvious from the test code/assert), and for the test debugger, you get line numbers and file names when assertions fail, and line numbers are about 100% more useful than grepping a file for reason strings. I say "in general" because there are cases where reason strings are helpful, when the test code/assert isn't self explanatory. (Mini-rant done.) > ::: browser/modules/test/browser/browser_PageActions.js:134 > (Diff revision 1) > > + // Open the panel, click the action's button. > > + await promisePageActionPanelOpen(); > > + Assert.equal(onShowingInPanelCallCount, 1); > > + onCommandExpectedButtonID = panelButtonID; > > + EventUtils.synthesizeMouseAtCenter(panelButtonNode, {}); > > + await promisePageActionPanelHidden; > > I think you're missing '()' here. Ah, thanks. > ::: browser/modules/test/browser/browser_PageActions.js:183 > (Diff revision 1) > > + // Without this, sometimes the subview isn't correctly shown. Something to > > + // do with how the panelview is added after the panel has already been shown. > > + // ("SlidingPanelViews :: index -1 out of bounds") > > + await new Promise(r => setTimeout(r, 1000)); > > I can get rid of this by replacing some synthesizeMouseAtCenters in the test > with just node.click(). I think the issue is that somehow synthesizeMouse > does not work well with the stacked panelviews (inside the panelmultiview) > at all times, because it tries to get a node based on coordinates, and the > stack means multiple views can be displayed at those coordinates... or > something. Anyway, I like using .click() a lot better than the timeout. :-) OK, thanks for debugging.

The 5-second timeout when handling the ViewShown event seems to be necessary, though (as confirmed by other subview tests, at least browser_page_action_menu.js). I notice that sometimes it takes an actual second for the subview to pop in. I'll push this to try, rebase again on the current tree, push to try one last time, and then land.

Try looks good. I'll rebase on the current m-c tip and push to review and try one last (hopefully) time.

Comment on attachment 8891613 [details] Bug 1374477 - Add a new test for Photon page actions, along with some related code changes. (I cannot figure out how to re-request review on an r+'ed commit in mozreview, so doing it through bugzilla.) Gijs, a few changes, at least one that merits another look (the last one). All are in the last mozreview commit, the test one. Please see the diff between revisions 3 and 4. * Make star-button-box hidden in the markup. Otherwise, if you remove it from the urlbar, when you open a new window (e.g., app startup), the star is briefly visible before being hidden. The other direction isn't a problem IMO: unhiding the star when it should be shown. (I hope this doesn't screw up tests. We'll see.) * After I updated my tree, I see that there's less space between the last urlbar icon and the egde of the urlbar. Not enough. So I added some margin to #urlbar-icons. * The biggest change: I was playing around and noticed that the bookmark star would come back after I removed it from the urlbar and restarted the app. The problme is that I'm not setting shownInUrlbar = false when the stored action IDs in the urlbar does not contain a given action. But that's not quite right either because if an action is brand new and its shownInUrlbar is true, then it should be shown even though it's not stored in the prefs. I can't think of a way around having to store which actions we've ever seen. If we haven't seen an action before (or it was removed), then obey its shownInUrlbar. Otherwise, use the shownInUrlbar value we previously stored. If you can think of something better, let me know.

Try looks good.

Gijs, if you r+ without comments, please autoland if you'd like.

Comment on attachment 8891613 [details] Bug 1374477 - Add a new test for Photon page actions, along with some related code changes. https://reviewboard.mozilla.org/r/162722/#review168128 This makes sense to me, though I have one nit. ::: browser/modules/PageActions.jsm:364 (Diff revisions 3 - 4) > - _actionIDsInUrlbar: [] > + _persistedActions: { > + // action ID => true, for actions that have ever been seen and not removed > + ids: {}, Can this just be an array and use array.includes(id) where we currently check for object.id ? Seems like that would be simpler.

I can make that change, but are you sure? Really ids should be a set because we're storing unique IDs and we want O(1) lookup. But since I'm stringifying this, an object with a simple ID => true mapping is the next best thing. If we make it an array, then an ordering is implied where none exists, and lookup becomes O(number of ever seen actions). Further, in terms of code, ids[id] (or id in ids) is simpler to write and read than ids.includes(id) IMO. I could convert the stringified object to a Set object and vice versa, but that seems unnecessary and more prone to bugs.

(In reply to Drew Willcoxon :adw from comment #179) > I can make that change, but are you sure? Really ids should be a set > because we're storing unique IDs and we want O(1) lookup. But since I'm > stringifying this, an object with a simple ID => true mapping is the next > best thing. > > If we make it an array, then an ordering is implied where none exists, and > lookup becomes O(number of ever seen actions). Further, in terms of code, > ids[id] (or id in ids) is simpler to write and read than ids.includes(id) > IMO. > > I could convert the stringified object to a Set object and vice versa, but > that seems unnecessary and more prone to bugs. The stringified thing is where my concern comes from - we're storing things in a pref, and the {} serialization takes up more space, making it easier for us to hit pref limits and warnings. The lookups aren't hot code, as far as I can tell. We could indeed convert on (de)serialization, but for the opposite reason (ie store as string, convert to object when deserializing, and convert back to array with Object.keys() when serializing). Hm, in fact, maybe we should move the storage of this and CUI to XULStore.jsm, which would also fix those issues. Follow-up fodder, I suppose. Anyway, I guess we can land this as-is.

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0e84cc659625 Add PageActions.jsm for Photon page actions. r=Gijs https://hg.mozilla.org/integration/autoland/rev/e8d29c386509 Add browser-pageActions.js for Photon page actions. r=Gijs https://hg.mozilla.org/integration/autoland/rev/01b509ec5e0d Remove page actions from browser.xul. r=Gijs https://hg.mozilla.org/integration/autoland/rev/f65406393199 Update Photon page action panel CSS. r=Gijs https://hg.mozilla.org/integration/autoland/rev/fce3303758ed Update consumers of Photon page action panel for changed identifiers. r=Gijs https://hg.mozilla.org/integration/autoland/rev/e86583843aa2 Photon SVG changes for page action panel. r=Gijs https://hg.mozilla.org/integration/autoland/rev/280149adf284 Add a new test for Photon page actions, along with some related code changes. r=Gijs

Comment on attachment 8888890 [details] Bug 1374477 - Add PageActions.jsm for Photon page actions. https://reviewboard.mozilla.org/r/159926/#review168502 ::: browser/modules/PageActions.jsm:787 (Diff revision 8) > + }, > + }, > + > + // separator > + { > + id: ACTION_ID_BOOKMARK_SEPARATOR, From what I understand of the spec, this separator is supposed to separate page actions visible in the URL bar and those only visible in the panel. So if you right click on the bookmark page action and click "Remove from address bar", the bookmark page action would appear under the separator instead. The bookmark page action isn't supposed to be a special button apart from the fact that it's visible by default.

(In reply to Tim Nguyen :ntim from comment #182) > Comment on attachment 8888890 [details] > Bug 1374477 - Add PageActions.jsm for Photon page actions. > > https://reviewboard.mozilla.org/r/159926/#review168502 > > ::: browser/modules/PageActions.jsm:787 > (Diff revision 8) > > + }, > > + }, > > + > > + // separator > > + { > > + id: ACTION_ID_BOOKMARK_SEPARATOR, > > From what I understand of the spec, this separator is supposed to separate > page actions visible in the URL bar and those only visible in the panel. > > So if you right click on the bookmark page action and click "Remove from > address bar", the bookmark page action would appear under the separator > instead. > > The bookmark page action isn't supposed to be a special button apart from > the fact that it's visible by default. I don't see anything that specifies this in https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/230998673 , so I don't understand where this idea came from. I just assumed pocket and bookmarked were grouped together, with a separator, because they do comparable things: they save a page somewhere. Copy link, email, send to device, share etc. all don't do that type of thing - they are for sharing with someone/something else, rather than saving for one's own uses. Aaron, if this is what you wanted, please can you file a followup bug?

No one has ever mentioned that possibility in all this time and discussions we've had about this... where in the world did you get that idea?

(In reply to :Gijs from comment #183) > I don't see anything that specifies this in > https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/230998673 , so I > don't understand where this idea came from. I just noticed both items in the URL bar (bookmark and pocket) were also above the separator in the page action menu. Moreover, the spec shows the "Remove from address bar" item on an item appearing above the separator, and "Add to address bar" on an item below the separator. So it seemed to me that the spec was insisting on the fact that items are placed above or below the separator depending on their visibility in the address bar.

The horizontal separator doesn't indicate an items position in the URL bar, it's only meant to group similar tools together and items in the doorhanger menu aren't dynamic is any way.

https://hg.mozilla.org/mozilla-central/rev/0e84cc659625 https://hg.mozilla.org/mozilla-central/rev/e8d29c386509 https://hg.mozilla.org/mozilla-central/rev/01b509ec5e0d https://hg.mozilla.org/mozilla-central/rev/f65406393199 https://hg.mozilla.org/mozilla-central/rev/fce3303758ed https://hg.mozilla.org/mozilla-central/rev/e86583843aa2 https://hg.mozilla.org/mozilla-central/rev/280149adf284

I assume this patch switched the page action menu and the star button in the url bar. If that's correct, here are the screenshots: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=87824406b9feb420a3150720707b424d7cee5915&newProject=mozilla-central&newRev=8b19670d12fde57d3aee50a5a7d1c734d9b709d5

(In reply to Johann Hofmann [:johannh] from comment #188) > I assume this patch switched the page action menu and the star button in the > url bar. Yup!