Closed Bug 1215376 Opened 9 years ago Closed 6 years ago

Implement onBeforeShow event

Categories

(WebExtensions :: Frontend, defect, P5)

Product:
WebExtensions
Component:
Frontend
Type:
defect

Tracking

(firefox56 ?, firefox60 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox56 --- ?
firefox60 --- verified

People

(Reporter: kmag, Assigned: robwu, Mentored)

References

Details

(Keywords: dev-doc-complete, good-first-bug, Whiteboard: [contextMenus]triaged)

Attachments

(15 files, 11 obsolete files)

content DOM-orientated contextMenu test extension
3.73 KB, application/x-xpinstall
Details
bug1215376_testcase@mozilla.org.xpi
2.10 KB, application/x-xpinstall
Details
menu-refresh.xpi
1.89 KB, application/x-xpinstall
Details
Bug 1215376 - allow itemToSelect to be null in menu closing test utility
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1215376 - Add onShown and onHidden to contextMenus/menus API
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1215376 - Ensure that menu elements have a unique ID
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1215376 - Refactor gMenuBuilder.build
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1215376 - Add menus.refresh to update menus
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1215376 - Always use old contextData upon refresh
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1215376 - Add more contextual information to onShown
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1215376 - refactor: re-use onShown event generator for onClicked
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1215376 - Fire menus.onShown for every menu
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1215376 - Test menu event order: onShown, onClicked, onHidden
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Bug 1215376 - fix linting and test issues from rebase
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
deley not saved.gif
234.52 KB, image/gif
Details 
+++ This bug was initially created as a clone of Bug #1211368 +++

(In reply to Bill McCloskey (:billm) from bug 1190667 comment #2)
> Thanks for pointing this out. We should add an onBeforeShow event. If the
> extension takes too long to respond, we can show the menu and add any
> extension-created items later if they appear.
Blocks: 1211368
Flags: blocking-webextensions-
I am adding a test extension with the relevant functionality.
Short workflow description:
1) add a mousedown|contextmenu listener on the content DOM
2) the listener generates data based on event target
3) data is sent to the background process
4) background process uses it to generated the menu

However, the displayed menu does not contain the generated items. Presumably, due to a race condition.
Whiteboard: [contextMenus] → [contextMenus][good first bug]
Kris, as you added the "good first bug" tag, could you please add some details about the scope of the bug and some guidance how and where to fix this?
Flags: needinfo?(kmaglione+bmo)
I added the tag because the bug had a mentor assigned, but Gabor is the mentor, not me.
Flags: needinfo?(kmaglione+bmo) → needinfo?(gkrizsanits)
Assignee: nobody → mwein2009
Iteration: --- → 47.1 - Feb 8
(In reply to Kris Maglione [:kmag] from comment #0)
> +++ This bug was initially created as a clone of Bug #1211368 +++
> 
> (In reply to Bill McCloskey (:billm) from bug 1190667 comment #2)
> > Thanks for pointing this out. We should add an onBeforeShow event. If the
> > extension takes too long to respond, we can show the menu and add any
> > extension-created items later if they appear.

Since there is a little bit of API design here, let's settle that first before Matt start working on this. We can do two things here:
1. While the context menu is shown we can keep a reference to the menu and store the context as well, and when an extension adds a new element to the menu or modifies an item, we can just do a quick update on the menu. We need to test this if this looks acceptable but if it does, this would be as simple as it can get and there is no need for a new API.
2. Add an onBeforeShow event sink to contextMenus and fire the event whenever a contextmenu pops up as the bug suggests. This should be blocking the contextMenu to be shown while an add-on code runs, and I'm not too happy about this. Not the mention out of process add-ons, which can further complicate things here because that would mean that there is a cross process blocking behavior. If it's done right it would not block the process just the contextmenu ofc but still... Anyway probably this can get tricky.
3. We can even add an extra event at DOM level, but I'm not a big fan of event without specs, so I don't even go there...

I'm leaning toward 1 but I want to hear your opinion first Bill, before I give some helping instructions how to implement this.
Flags: needinfo?(gkrizsanits) → needinfo?(wmccloskey)
I was looking into this with Matt the other day, and it seemed like the easiest thing would be  to call an onBeforeShow handler in the extension with the same (or similar) context data as we get from on-build-contextmenu. It gets more complicated if the extension needs more context than that, admittedly.

As for blocking the context menu, yeah, that's not ideal. I'm not sure how much OOP add-ons affect it, since we need to message the content process for context menus in any case (and we still rely a lot on CPOWs).

Bill mentioned in the comment where he suggested adding the event that we could impose a timeout on the reply, and then update the menu when we get it if it's already been shown. That doesn't sound like good first bug territory, but it could be done in a follow-up, once we actually have OOP extensions.
(In reply to Kris Maglione [:kmag] from comment #5)
> call an onBeforeShow handler in the extension with the same (or similar)
> context data as we get from on-build-contextmenu.

(N.B., data sanitized based on the extension's permissions, possibly including
content script permissions)
What's your reasoning against 1) exactly? Live updating the menu while it's open should solve all these difficulties, no?

(In reply to Kris Maglione [:kmag] from comment #5)
> I was looking into this with Matt the other day, and it seemed like the
> easiest thing would be  to call an onBeforeShow handler in the extension
> with the same (or similar) context data as we get from on-build-contextmenu.
> It gets more complicated if the extension needs more context than that,
> admittedly.
> 
> As for blocking the context menu, yeah, that's not ideal. I'm not sure how
> much OOP add-ons affect it, since we need to message the content process for
> context menus in any case (and we still rely a lot on CPOWs).
> 

Right now content process sends up a message to the chrome process and the parent
process calls into the ext-contextMenus function synchronously. This works now,
because the add-on is in the parent process as well, but we need to figure out
a new module for OOP add-ons. I'm not comfortable with blocking the parent process
at all, and adding a timeout to that is just a hack for damage control. Live
updating the menu would fix both issues. The other solution would be to send an async
message from the parent process to ext-contextMenus and when it thinks it's ready
with everything, message back to the parent that it can open the context menu.
This would delay the context menu opening but would not block the parent process.
Same timeout can be applied, but we must be careful about various things ofc like
an incoming second attempt for opening the context menu. Anyway, if we choose this
approach I don't think it's a good first bug.

> Bill mentioned in the comment where he suggested adding the event that we
> could impose a timeout on the reply, and then update the menu when we get it
> if it's already been shown. That doesn't sound like good first bug
> territory, but it could be done in a follow-up, once we actually have OOP
> extensions.

Again, I think it's a really bad idea to block the parent process from the
add-on.
Flags: needinfo?(wmccloskey) → needinfo?(kmaglione+bmo)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7)
> What's your reasoning against 1) exactly? Live updating the menu while it's
> open should solve all these difficulties, no?

I have nothing against 1). I'm just not sure it removes the need for an event.

Bill's proposal in the original bug was to add an event, wait for a short time
for the add-on to respond, and then live update with any changes that happened
later.

I don't really have anything against skipping the delay either, as long as the
UX isn't too awkward.

> Right now content process sends up a message to the chrome process and the
> parent process calls into the ext-contextMenus function synchronously. This
> works now, because the add-on is in the parent process as well, but we need
> to figure out a new module for OOP add-ons.

I'm not sure OOP add-ons change anything. As I understand it, OOP add-ons
will live in the regular content process, so it should be possible to trigger
their listeners synchronously before calling out to the parent process.
Granted, that could always change and it probably wouldn't be great to design
an API that absolutely required it.

> I'm not comfortable with blocking the parent process at all, and adding a
> timeout to that is just a hack for damage control.

I think it's more a matter of trying to balance the overall experience.
There's a question of whether we want to add a delay capped to a nearly
imperceptible timeout if we know an add-on may want to add entries, or show
the context menu immediately and then (after a very short delay) add new
entries to it after it opens. I think the latter would be more obvious to the
user. And it would require a bit more UI work, because I'm not sure we could
get away with it without transitions.

I'm not sure whether it would be a good or bad effect, though. I'd be willing
to try it and find out.

> The other solution would be to send an async message from the parent process
> to ext-contextMenus and when it thinks it's ready with everything, message
> back to the parent that it can open the context menu. This would delay the
> context menu opening but would not block the parent process.

Yeah, I was assuming it would be done with something along those lines.
Flags: needinfo?(kmaglione+bmo)
Alright, I think we're on the same page more or less let me summarize the todos here:

1. Add support for live updating the menu
2. Add an onBeforeShow event that blocks
3. Set up a timer with really short timeout that unblocks the context-menu

optional after 1: check if without an onBeforeShow event live updating the menu
right after it opens looks acceptable or weird.

And let's take care of the OOP issue in another bug.
3 should be also carried over to follow up bug, but in the related docs we must mention
that onBeforeShow comes with a timeout.
Whiteboard: [contextMenus][good first bug] → [contextMenus][good first bug]triaged
Sounds good to me.
Here is a similar feature request (with use cases from users) in Chromium: https://crbug.com/60758

At this early stage in API design, try to avoid APIs that are known to have severe disadvantages (performance, OOP support, ...).

Assuming that firing up an isolated JavaScript context in the browser process is inexpensive, I suggest to implement dynamic context menus by executing some static JavaScript code/function in an isolated JavaScript environment (without much privileges). If extensions become OOP, then latency due to IPC between the browser and extension process won't have an impact, since the context menu script can be run in the browser process.
PAC scripts are existing example of this concept: https://en.wikipedia.org/wiki/Proxy_auto-config.

Here is an example of how it could work (you can probably think of better names, but this is to show the idea):

chrome.contextMenus.create({
  id: 'my-context-menu',
  title: 'Call me',
  beforeShowFile: 'doSomething.js',
  onClicked: function(info, tab) {
    alert('');
  },
});

// doSomething.js
function ShouldShowContextMenu(context) {
  if (context.type === 'selection' &&
      context.selectionText.includes('my secret')) {
    return true;
  }
  // ( ... some other things ... )
  return false;
}

Instead of returning false, you could also return an object, which could be used to change the rendered context menu, if feasible/desirable.
Iteration: 47.1 - Feb 8 → 47.2 - Feb 22
Iteration: 47.2 - Feb 22 → ---
Assignee: mwein → nobody
Assignee: nobody → rob
This beforeShowFile prototype shows that the title can be changed and context menu items can be suppressed (the tests pass). There is still a long way to go before it can be merged though. At the very least, the script needs to be moved to a separate thread to prevent bad behaving scripts from blocking the browser process.

Kris, could you take a look?
Flags: needinfo?(kmaglione+bmo)
Attachment #8723939 - Flags: review?(kmaglione+bmo)
Keywords: good-first-bug
Whiteboard: [contextMenus][good first bug]triaged → [contextMenus]triaged
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Flags: blocking-webextensions-
Priority: -- → P5
Attachment #8723939 - Flags: review?(kmaglione+bmo)
This bug would be still keeping me from transitioning to web-ext.

Since there does not seem to be any traction/ideas for it, and no actual time left for waiting, I think I'll have to disable/rethink the related functionality
:zombie proposed I explained my use-case here.

In the test pilot containers experiment we are trying to add an assignment feature which is assigning a container based upon a hostname.

The user can once they have a container tab open for a site can assign a container with a checkbox menu item "Always open in this container". The state of the check would be based upon if they have assigned to the container.

User flow is:

1.  User opens banking container
2.  User opens myamazingbank.com
3.  User right clicks and sees a context menu item "Always open in this container"
4.  User clicks menu item
5.  User is given a notification they have assigned myamazingbank.com to banking container.
6.  User opens another tab in a personal container and types myamazingbank.com
7.  Tab closes
8.  Tab opens in Banking
9.  User is presented with a warning about myamazingbank.com about to open
10. User clicks confirm
11. User is taken to the page


The context menu should not show for:
- about: urls
- non containers
- private windows

To add a layer of complexity we would also like to hide this option when the current container doesn't match what they have assigned for this hostname. In the userflow 9. would permit them to change what the container they open just for this tab (Assign will still be in place but they open their work gmail just for one tab).

So as :zombie suggested we could:
- Use documentUrlPatterns to filter based on all assigned URLs
- Add a filter for isContextId and isIncognito

However this still doesn't allow us to display text/checked state on when a URL pattern matches an assignment to the current userContextId.

Having a blocking event handler would not only make the current code much easier (which checks for page loads, contextId assignments, tab changes and window focuses) it would also fix a case where user right clicks to focus into a different window (this actually works in Chrome also without any promise based code etc). See the following for more info on the window focus issue: https://github.com/jonathanKingston/extension-debugging/tree/master/context-menu-window-focus-issue
Priority: P5 → --
Added a simpler test case to this bug. It should work on google.com(untested) and google.de if you do a right click on input field.

But after playing with this, i have more questions as before.

1.) The context menu create works always on 2nd try and stays present then for next click. And of course if you click outside the input field to remove the menu entry it works also on 2nd try. There is always a delay of one click.

2.) More interesting is for test i called chrome.contextMenus.removeAll() after creation with setTimeout() of 500ms to remove all entries again. You can guess what happened. I see never the menu item, there is enough time to remove it before the menu will be showed. For 900ms i see the menu entry sometimes, but also very often not.

This means i have a time interval of 500ms to add and remove the entries and this is not enough. For my feeling 500ms are already a very long period to get all managed but not for this task.
What is the plan here? Fx 57 comes every day one day closer and we do not know what we shall do here. Shall we wait? Shall we switch to HTML5 context menus? Specially on webpages which loads some content later as pageload event fires i would prefer not to use HTML5 context menus, because then we need again additional observers. P5 was removed here, what do you think about this bug?
Played with HTML5 context menu today to get rid of the problems here. All was fine until a reach a IFRAME source - no HTML5 context menu entries on Iframes...no real alternatives here.
Further rationale to this bug: https://github.com/mozilla/testpilot-containers/issues/520

For the "tab" context state we would like to show the state of the clicked tab rather than the current tab. This is actually not possible at all.
I found that the HTML5 workaround works also on iframes, if the menu is placed in the iframe and not in the levels above. And if i use where i need it but not on jsfiddle(tried it for testing, but no way). Will switch all here to HTML5 context menus now.
Hi Gabor,

What's needed to finish implementation of this bug?

I've been looking for mentored bugs outside screenshots to work on, and this seems pretty important for webextensions in general, and for two test pilot experiments in particular (min vid and containers).

I'd be happy to steal from :robwu, provided I can get an updated TODO list - I'm sure a lot has changed in the year this has been sitting idle, particularly with e10s and OOP webextensions.

(:robwu, please let me know if you'd like to finish this one or work together to finish it)

Thanks!

Jared
Flags: needinfo?(rob)
Flags: needinfo?(gkrizsanits)
I have thought of multiple alternatives to implement this feature, each with pros and cons. I'll discuss this (with :mixedpuppy) at the SF all-hands. If you want to join the discussion, mail me and I will let you know when we meet.

Briefly, among my thoughts:
1. whether and how to block creation of the context menu
2. or whether to allow context menu labels to be updated after they are shown
3. whether to have a default (fallback) label that is updated after the menu item is shown.
Flags: needinfo?(rob)
(In reply to Jared Hirsch [:_6a68] (please use 'needinfo' :-) from comment #23)
> Hi Gabor,
> 
> What's needed to finish implementation of this bug?
> 
> I've been looking for mentored bugs outside screenshots to work on, and this
> seems pretty important for webextensions in general, and for two test pilot
> experiments in particular (min vid and containers).
> 
> I'd be happy to steal from :robwu, provided I can get an updated TODO list -
> I'm sure a lot has changed in the year this has been sitting idle,
> particularly with e10s and OOP webextensions.
> 
> (:robwu, please let me know if you'd like to finish this one or work
> together to finish it)
> 
> Thanks!
> 
> Jared

It's been a while since I worked on anything add-on related, so I don't think I'm the right person to make any decision here. Based on :robwu's comment I'm removing the needinfo flag, but if you need any help during the SF allhands, let me know. Although I'm pretty sure :kmag would be a lot more knowledgeable person here to ask for guidance than me :)
Flags: needinfo?(gkrizsanits)
Hey Kris - I guess you've inherited this bug! Any thoughts on which of Rob's suggestions in comment 24 might be the best starting point?
I got here by this thread that I opened https://discourse.mozilla-community.org/t/change-label-in-the-contextmenu-before-the-opening/16620

There is a chance to have that feature soon?
Flags: needinfo?(kmaglione+bmo)
An example of extension that will use this feature (based on the previous discourse thread) https://github.com/mte90/gulpease
There won't be a blocking onBeforeShown event.
I'm going to implement the following two events instead:

- browser.contextMenus.onShown
- browser.contextMenus.onHidden

And the following API extensions:

- The contextMenus.create/update/remove/removeAll APIs can be used while a contextMenu is being shown (e.g. during/after the onShown event).
- Context menu items can be updated at any time, with no restrictions if possible (i.e. already-shown menu items can be updated).
- For adding/removal, it depends on whether the menu is already shown.
- When the menu is hidden, the behavior will remain unchanged (items can freely be added/removed).
- When the menu is shown, new items are appended to the end (and when there are too many items, I will either throw an error or combine the menu items, whichever is easier/makes more sense).
- When the menu is shown, items in the top-level menu CANNOT BE REMOVED, to avoid bad UX by unexpected shifting of menu items. Items in submenus can freely be removed (even if this means that menu items would shift around. UX is a lesser concern here because it is presumably clear to the user that the menu is managed by the add-on).

I will not start with the implementation right away, so if this API does not meet your needs, please speak up now before the API is set in stone.
Status: NEW → ASSIGNED
I think that in this way is more cool and more powerful that the initial suggestion :-)
(In reply to Rob Wu [:robwu] from comment #29)
> There won't be a blocking onBeforeShown event.
> ...

> - When the menu is shown, items in the top-level menu CANNOT BE REMOVED, to
> avoid bad UX by unexpected shifting of menu items. Items in submenus can
> freely be removed (even if this means that menu items would shift around. UX
> is a lesser concern here because it is presumably clear to the user that the
> menu is managed by the add-on).
> 
> I will not start with the implementation right away, so if this API does not
> meet your needs, please speak up now before the API is set in stone.

I would use this API to dynamically remove/add my ONLY extension item (=top level menu item) to the top level context menu. If you were to impose restrictions on the "top level" context menu like this, the API isn't usable for me.

Examples:
* I want to add a context menu item to "a" elements that contain a certain string (domain). If they do not contain that string, I want to hide/remove the context menu item (which is probably in the top level context menu).
* I want to check for the current page URL. If it contains "addons.mozilla.org", "about:" or something internal, I want to hide/remove my context menu item. (On a side note, I would like to note that not allowing context menu items by default on such pages would break many extensions that can still do something useful without a content script)
Flags: needinfo?(rob)
(In reply to Geoffrey De Belie (Smile4ever) from comment #31)
> (In reply to Rob Wu [:robwu] from comment #29)
> > - When the menu is shown, items in the top-level menu CANNOT BE REMOVED, to
> > avoid bad UX by unexpected shifting of menu items. Items in submenus can
> > freely be removed (even if this means that menu items would shift around. UX
> > is a lesser concern here because it is presumably clear to the user that the
> > menu is managed by the add-on).
> 
> I would use this API to dynamically remove/add my ONLY extension item (=top
> level menu item) to the top level context menu. If you were to impose
> restrictions on the "top level" context menu like this, the API isn't usable
> for me.
> 
> Examples:
> * I want to add a context menu item to "a" elements that contain a certain
> string (domain). If they do not contain that string, I want to hide/remove
> the context menu item (which is probably in the top level context menu).
> * I want to check for the current page URL. If it contains
> "addons.mozilla.org", "about:" or something internal, I want to hide/remove
> my context menu item. (On a side note, I would like to note that not
> allowing context menu items by default on such pages would break many
> extensions that can still do something useful without a content script)

All of your use cases can be implemented by *adding* menu items in the onShown event (and to clean up, removing the menus in the onHidden event).
Flags: needinfo?(rob)
Wont we see menu items shuffle around a lot on show if the events are not blocking or would it execute all the handler before showing (Ie promises won't finish but normal code will)?
Priority: -- → P5
See Also: → 1367160
I'm glad to see there's a plan in place, but this is important to at least min-vid for 57, so I'm curious if there's a timeline to implement the plan? If not, could you outline high-level details and I can help with implementation?
Flags: needinfo?(rob)
The extension with 50k users I'm porting needs this too. Use case: when the user has some text selected and opens the root context menu item for the extension I need to do a translate service lookup and update the child context menu entry with the new text.

https://addons.mozilla.org/en-US/firefox/addon/gtranslate/
(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #34)
> I'm glad to see there's a plan in place, but this is important to at least
> min-vid for 57, so I'm curious if there's a timeline to implement the plan?
> If not, could you outline high-level details and I can help with
> implementation?

Deadline is Firefox 57.
I intend to submit a set of patches mid-august (in 2-3 weeks from now).
Flags: needinfo?(rob)
(In reply to winflip from comment #35)
> The extension with 50k users I'm porting needs this too. Use case: when the
> user has some text selected and opens the root context menu item for the
> extension I need to do a translate service lookup and update the child
> context menu entry with the new text.
> 
> https://addons.mozilla.org/en-US/firefox/addon/gtranslate/

But, the tooltip what the add-on (gtranslate) uses will not work.

https://bugzilla.mozilla.org/show_bug.cgi?id=1332270

Maybe you can talk with them about this?
Flags: needinfo?(rob)
(In reply to Rob Wu [:robwu] from comment #36)
> (In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #34)
> > I'm glad to see there's a plan in place, but this is important to at least
> > min-vid for 57, so I'm curious if there's a timeline to implement the plan?
> > If not, could you outline high-level details and I can help with
> > implementation?
> 
> Deadline is Firefox 57.
> I intend to submit a set of patches mid-august (in 2-3 weeks from now).

Any luck with these? It is now mid September.
(In reply to Worcester12345 from comment #39)
> (In reply to Rob Wu [:robwu] from comment #36)
> > (In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #34)
> > > I'm glad to see there's a plan in place, but this is important to at least
> > > min-vid for 57, so I'm curious if there's a timeline to implement the plan?
> > > If not, could you outline high-level details and I can help with
> > > implementation?
> > 
> > Deadline is Firefox 57.
> > I intend to submit a set of patches mid-august (in 2-3 weeks from now).
> 
> Any luck with these? It is now mid September.

The patches aren't ready yet, other tasks took longer than expected.
But these other tasks are done now, so I can focus on this bug and have some review back-and-forths early next week.
Flags: needinfo?(rob)
(In reply to Rob Wu [:robwu] from comment #40)

> I can focus on this bug and have some
> review back-and-forths early next week.

Great, thanks.
status-firefox56: --- → ?
I have pushed 5 patches for review.
Patch 1, 3 and 4 are refactorings (easy to review).
Kris, we discussed patch 3 on IRC, so I'm assigning that one to you.

Patch 2 introduces the menus.onShown / menus.onHidden events.
Patch 5 introduces the menus.refresh method.

(see individual commit messages for a quick description).


While those patches are pending review, I will work on providing more event details to the onShown event, and try to see if it makes sense to allow extensions to use menus.refresh even if they haven't shown any menu items yet.
Comment on attachment 8907370 [details]
Bug 1215376 - allow itemToSelect to be null in menu closing test utility

https://reviewboard.mozilla.org/r/179042/#review184170
Attachment #8907370 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8907373 [details]
Bug 1215376 - Refactor gMenuBuilder.build

https://reviewboard.mozilla.org/r/179048/#review184176
Attachment #8907373 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8907371 [details]
Bug 1215376 - Add onShown and onHidden to contextMenus/menus API

https://reviewboard.mozilla.org/r/179044/#review184574

::: browser/components/extensions/ext-menus.js:681
(Diff revision 1)
>      let {extension} = context;
>  
> +    const menus = {
> +      onShown: new EventManager(context, "menus.onShown", fire => {
> +        let listener = (event, data) => {
> +          fire.sync(data);

why sync?  Are we blocking the appearance/use of the context menu in any way?

::: browser/components/extensions/ext-menus.js:690
(Diff revision 1)
> +          extension.off("webext-menu-shown", listener);
> +        };
> +      }).api(),
> +      onHidden: new EventManager(context, "menus.onHidden", fire => {
> +        let listener = () => {
> +          fire.sync();

ditto
Flags: needinfo?(tomica)
Comment on attachment 8907371 [details]
Bug 1215376 - Add onShown and onHidden to contextMenus/menus API

https://reviewboard.mozilla.org/r/179044/#review184574

> why sync?  Are we blocking the appearance/use of the context menu in any way?

Whether `fire.sync` or `fire.async` is used, the APIs are always asynchronous.

I used `fire.sync` because I didn't need an extra promise. In the case of `onShown`, it is even desirable to notify the extension ASAP, to minimize the latency of updating the menu.
Comment on attachment 8907374 [details]
Bug 1215376 - Add menus.refresh to update menus

https://reviewboard.mozilla.org/r/179050/#review184580

::: browser/components/extensions/ext-menus.js:332
(Diff revision 1)
> +      return;
> +    }
> +
> +    if (contextData.onBrowserAction || contextData.onPageAction) {
> +      if (extension !== contextData.extension) {
> +        Cu.reportError(`Extension (${extension.id}) tried to modify action menu of another extension (${contextData.extension.id})`);

How would this be possible if the extension id is prefixed on the menuitems?  This should be done in  a way that there is never be a possibility to hit this situation.

::: browser/components/extensions/test/browser/browser_ext_menus_refresh.js:199
(Diff revision 1)
> +    },
> +  });
> +  await BrowserTestUtils.removeTab(tab);
> +});
> +
> +// TODO: Add tests:

will you be adding these now?
(In reply to Rob Wu [:robwu] from comment #51)
> Comment on attachment 8907371 [details]
> Bug 1215376 - Add onShown and onHidden to contextMenus/menus API

> I used `fire.sync` because I didn't need an extra promise. In the case of
> `onShown`, it is even desirable to notify the extension ASAP, to minimize
> the latency of updating the menu.

If I have a couple/few addons modifying during onShown, and the first one happens to take a long time, is that going to slow down the whole boat?
Comment on attachment 8907374 [details]
Bug 1215376 - Add menus.refresh to update menus

https://reviewboard.mozilla.org/r/179050/#review184580

> How would this be possible if the extension id is prefixed on the menuitems?  This should be done in  a way that there is never be a possibility to hit this situation.

I have just removed this check + error reporting here, and instead added a unit test to ensure that calling `refresh()` from one extension won't update the action menu of another extension (the `gShownMenuItems.has(extension)` check a few lines back stops this from happening at the moment).
(In reply to Shane Caraveo (:mixedpuppy) from comment #53)
> (In reply to Rob Wu [:robwu] from comment #51)
> > Comment on attachment 8907371 [details]
> > Bug 1215376 - Add onShown and onHidden to contextMenus/menus API
> 
> > I used `fire.sync` because I didn't need an extra promise. In the case of
> > `onShown`, it is even desirable to notify the extension ASAP, to minimize
> > the latency of updating the menu.
> 
> If I have a couple/few addons modifying during onShown, and the first one
> happens to take a long time, is that going to slow down the whole boat?

They cannot slow down each other. Even without e10s, fire.sync will be delivered asynchronously.
fire.sync -> ParentAPIManager -> (async via message manager and MessageChannel) -> ChildAPIManager -> event in extension code.
Four more patches to get in a final state:
- onShown fires for any menu, even if the extension has no menu item.
- onShown receives contextual information (most of the data that is also passed to menus.onClicked) IF it has the host permissions for the page (either host permissions, or activeTab).

For reviewer (:mixedpuppy):
- patch 5 - small change based on review feedback.
NEW
- patch 6 - easy to review - few-line change + tests
- patch 7 - affects only one place (implementation of onShown), but review is a bit more complicated if you want to take a good look at the tests.
- patch 8 - easy to review (refactor to use the new helper function from patch 7)
- patch 9 - small change, straightforward patch.
Attachment #8907371 - Flags: review?(mixedpuppy)
Attachment #8907374 - Flags: review?(mixedpuppy)
This onShown event also works for submenus?
Flags: needinfo?(rob)
(In reply to kernp25 from comment #62)
> This onShown event also works for submenus?

The onShown event is only dispatched when a menu is opened (e.g. by right-clicking on a page). It is not triggered again when a submenu is opened.
Flags: needinfo?(rob)
But, can you make it also work for submenus?

Like with onClicked event also fires for submenus.

The gtranslate add-on needs this feature.
(In reply to kernp25 from comment #64)
> But, can you make it also work for submenus?

Not as part of this bug. We are already close to the feature cut-off for Firefox 57, and adding feature creep will certainly delay this feature.

If you strongly believe that such a feature is needed, create a new feature request and explain your use case (and in particular why it cannot be achieved with the current APIs).
Comment on attachment 8907371 [details]
Bug 1215376 - Add onShown and onHidden to contextMenus/menus API

https://reviewboard.mozilla.org/r/179044/#review185772

::: browser/components/extensions/ext-browser.json:100
(Diff revision 1)
>    "menusInternal": {
>      "url": "chrome://browser/content/ext-menus.js",
>      "schema": "chrome://browser/content/schemas/menus.json",
>      "scopes": ["addon_parent"],
>      "paths": [
> +      ["contextMenus"],

Did you actually try if this works with both namespaces?  The reason I had to do use the `menusInternal` namespace was because of some issues with events and permissions (and it seems the comment explaining this got lost in one of the rewrites).

::: browser/components/extensions/ext-menus.js:32
(Diff revision 1)
>  
>  // Map[Extension -> MenuItem]
>  var gRootItems = new Map();
>  
> +// Map[Extension -> ID[]]
> +// Menu IDs that were eligible for being shown in the current menu.

eligible or actually shown?

::: browser/components/extensions/ext-menus.js:701
(Diff revision 1)
> +      }).api(),
> +    };
> +
>      return {
> +      contextMenus: menus,
> +      menus,

You need to check if extension has each of the permissions before injecting the api namespace.  In fact, what's the reason for doing these different from the onClicked event (emitting `menusInternal` events and forwarding them from `ext-c-menus.js`)?

::: browser/components/extensions/schemas/menus.json:404
(Diff revision 1)
> +          {
> +            "name": "info",
> +            "type": "object",
> +            "description": "Information about the context of the menu action and the created menu items",
> +            "properties": {
> +              "menuIds": {

missing `contexts` array?

::: browser/components/extensions/test/browser/browser_ext_menus_events.js:22
(Diff revision 1)
> +// Registers a context menu using menus.create(menuCreateParams) and checks
> +// whether the menus.onShown and menus.onHidden events are fired as expected.
> +// doOpenMenu must open the menu and its returned promise must resolve after the
> +// menu is shown. Similarly, doCloseMenu must hide the menu.
> +async function testShowHideEvent({
> +  menuCreateParams, doOpenMenu, doCloseMenu, expectedShownEvent,

nit: please start and align the arguments after the perens.

    async function test({arg1, arg2,
                         arg3, arg4}) {

::: browser/components/extensions/test/browser/browser_ext_menus_events.js:26
(Diff revision 1)
> +async function testShowHideEvent({
> +  menuCreateParams, doOpenMenu, doCloseMenu, expectedShownEvent,
> +}) {
> +  async function background() {
> +    function awaitMessage(expectedId) {
> +      return new Promise(resolve => {

An info/log message here about what is being waited for could be useful to debug potential test timeouts on Try.

::: browser/components/extensions/test/browser/browser_ext_menus_events.js:28
(Diff revision 1)
> +}) {
> +  async function background() {
> +    function awaitMessage(expectedId) {
> +      return new Promise(resolve => {
> +        browser.test.onMessage.addListener(function listener(id, msg) {
> +          if (id === expectedId) {

I think this could be an assert? (also helpful in the log)

::: browser/components/extensions/test/browser/browser_ext_menus_events.js:48
(Diff revision 1)
> +
> +    const [tab] = await browser.tabs.query({active: true});
> +    await browser.pageAction.show(tab.id);
> +
> +    let menuId;
> +    await new Promise(resolve => {

nice

::: browser/components/extensions/test/browser/browser_ext_menus_events.js:87
(Diff revision 1)
> +  let shownEvent = await extension.awaitMessage("onShown-event-data");
> +
> +  // menuCreateParams.id is not set, therefore a numeric ID is generated.
> +  is(JSON.stringify(shownEvent.menuIds), `[${menuId}]`, "expected menuIds in onShown");
> +
> +  for (let [key, expectedValue] of Object.entries(expectedShownEvent)) {

Can you use `Assert.deepEqual()` here?

::: browser/components/extensions/test/browser/browser_ext_menus_events.js:91
(Diff revision 1)
> +
> +  for (let [key, expectedValue] of Object.entries(expectedShownEvent)) {
> +    let actualValue = shownEvent[key];
> +    if (Array.isArray(actualValue)) {
> +      // Serialize arrays so that they can be compared with is().
> +      actualValue += "";

nit: or use `.join()`

::: browser/components/extensions/test/browser/browser_ext_menus_events.js:98
(Diff revision 1)
> +    }
> +    is(actualValue, expectedValue, `property "${key}" of onShown event`);
> +  }
> +
> +  // See comment at RECOGNIZED_MENUS_ONSHOWN_EVENT_KEYS.
> +  for (let key of Object.keys(shownEvent)) {

nit: this could also be checked via `deepEqual()`, though remember to sort the arrays.

::: browser/components/extensions/test/browser/browser_ext_menus_events.js:120
(Diff revision 1)
> +    background() {
> +      let events = [];
> +      browser.menus.onShown.addListener(data => events.push(data));
> +      browser.menus.onHidden.addListener(() => events.push("onHidden"));
> +      browser.test.onMessage.addListener(() => {
> +        browser.test.assertEq("[]", JSON.stringify(events),

nit: just assert length is 0.

::: browser/components/extensions/test/browser/browser_ext_menus_events.js:148
(Diff revision 1)
> +    },
> +    expectedShownEvent: {
> +      contexts: ["page", "all"],
> +    },
> +    async doOpenMenu() {
> +      await openContextMenu("body");

nit: no need for the async/await, just return the result promise.

::: browser/components/extensions/test/browser/browser_ext_menus_events.js:332
(Diff revision 1)
> +    async doCloseMenu() {
> +      await closeExtensionContextMenu();
> +    },
> +  });
> +});
> +

I never thought I'd say this in a review, but..  ;)

I'm not sure all these tests add much value, as it seems only a couple variants would exercise all the code tested here.  This will just run longer, use more resources and extend potential for unrelated intermittents.
Comment on attachment 8907374 [details]
Bug 1215376 - Add menus.refresh to update menus

https://reviewboard.mozilla.org/r/179050/#review185788

What changed that we are now also removing top-level menu items after they are already shown?  I though we agreed that is visually disruptive and most if not all usecases are covered with just adding menu items?

::: browser/components/extensions/ext-menus.js:150
(Diff revision 2)
>  
>      this.xulMenu.appendChild(rootElement);
>      this.itemsToCleanUp.add(rootElement);
>    },
>  
> +  removeSeparatorIfNoTopLevelItems() {

I though we weren't removing, only adding top level items?

::: browser/components/extensions/ext-menus.js:387
(Diff revision 2)
>      for (let [extension, menuIds] of gShownMenuItems.entries()) {
>        let info = Object.assign({menuIds}, commonMenuInfo);
>        extension.emit("webext-menu-shown", info);
>      }
> +
> +    this.contextData = contextData;

Seems you are only using this as a flag, so please just use a boolean property like `.visible`, no need to keep things alive.
Comment on attachment 8907374 [details]
Bug 1215376 - Add menus.refresh to update menus

https://reviewboard.mozilla.org/r/179050/#review185788

> Seems you are only using this as a flag, so please just use a boolean property like `.visible`, no need to keep things alive.

Sorry, I missed the obvious use to rebuild menus.
Comment on attachment 8907374 [details]
Bug 1215376 - Add menus.refresh to update menus

https://reviewboard.mozilla.org/r/179050/#review185798

::: browser/components/extensions/test/browser/browser_ext_menus_refresh.js:8
(Diff revision 2)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +const PAGE = "http://mochi.test:8888/browser/browser/components/extensions/test/browser/context.html";
> +
> +function loadExtensionWithMenusApi() {

This could use a short docs comment.

::: browser/components/extensions/test/browser/browser_ext_menus_refresh.js:53
(Diff revision 2)
> +    return document.getElementById(xulId);
> +  };
> +
> +  return extension;
> +}
> +

same as above

::: browser/components/extensions/test/browser/browser_ext_menus_refresh.js:54
(Diff revision 2)
> +  };
> +
> +  return extension;
> +}
> +
> +async function testRefreshMenusWhileVisible({

nit: again, this is not common in our code style.

::: browser/components/extensions/test/browser/browser_ext_menus_refresh.js:123
(Diff revision 2)
> +  await extension.callMenuApi("removeAll");
> +  await extension.callMenuApi("refresh");
> +  elem = extension.getXULElementByMenuId("def");
> +  is(elem, null, "all menu items should be gone");
> +
> +  // Create a new one - this should be allowed because the extension knows that

I don't understand this comment, nor what this is testing.
Comment on attachment 8908232 [details]
Bug 1215376 - Always use old contextData upon refresh

https://reviewboard.mozilla.org/r/179874/#review185802

::: browser/components/extensions/test/browser/browser_ext_menus_refresh.js:311
(Diff revision 1)
> +  await BrowserTestUtils.loadURI(tab.linkedBrowser, PAGE + "?2");
> +  await BrowserTestUtils.browserStopped(tab.linkedBrowser);
> +
> +  await extension.callMenuApi("refresh");
> +
> +  // Even after switching tabs, the old context should be used.

Hmm.  I'm not sure this is the correct behavior.  Can you explain your reasoning please?
Comment on attachment 8907371 [details]
Bug 1215376 - Add onShown and onHidden to contextMenus/menus API

https://reviewboard.mozilla.org/r/179044/#review185772

> Did you actually try if this works with both namespaces?  The reason I had to do use the `menusInternal` namespace was because of some issues with events and permissions (and it seems the comment explaining this got lost in one of the rewrites).

Yes, it works. The tests confirm that the menus API work, and I manually checked that `contextMenus` without the `menus` permission works too.

> eligible or actually shown?

eligible. In the case of action menus, any items in excess of the maximum number of top-level items is discarded, but they still appear in this list. Other than that, the menu items in this list are equal to the menu items that are present in the context menu.

> You need to check if extension has each of the permissions before injecting the api namespace.  In fact, what's the reason for doing these different from the onClicked event (emitting `menusInternal` events and forwarding them from `ext-c-menus.js`)?

The explicit permission check in `ext-c-menus.js` is unnecessary. `Schemas.jsm` will only expose an API to an extension if the extension has the right permission (as defined in the "permissions" key of a node in the extension schema).

I am defining the API in `ext-menus.js` instead of `ext-c-menus.js` because the `onShown` and `onHidden` events do not require any maintenance in the content process. Extension functions should be in `ext-X.js` by default, and only be added to `ext-c-X.js` if the logic cannot be implemented in the parent.

I don't see a reason for `onClicked` being in `ext-c-menus.js`. It can probably be implemented in the same way as `onShown` and `onHidden`.

> missing `contexts` array?

Fixed.
The schema for parameters is not used anywhere in Firefox, hence this missing value did not cause any errors.
I guess that we should check the callback results in DEBUG mode to avoid mistakes like these in the future.

> Can you use `Assert.deepEqual()` here?

Yes, that's exactly what I needed. Thanks!

> nit: just assert length is 0.

I'm intentionally doing `[]`, so that if somehow the array is not empty, that the unexpected events are included in the log for ease of debugging.

> nit: no need for the async/await, just return the result promise.

In some tests, the logic is a bit more complex than returning one promise (e.g. using ContentTask to set up the page before opening the menu). In these cases, async/await is required. For consistency, I am using async/await everywhere.

> I never thought I'd say this in a review, but..  ;)
> 
> I'm not sure all these tests add much value, as it seems only a couple variants would exercise all the code tested here.  This will just run longer, use more resources and extend potential for unrelated intermittents.

I want to have a test that verifies that the event data is set as expected. Without these tests, the behavior could unknowingly change in the future. I tried to merge tests where possible (image + link, editable + selection) (and despite thinking that they are the same, I observed a difference in output for image+link vs link, hence in a later commit I add an individual test for "link").
Comment on attachment 8907374 [details]
Bug 1215376 - Add menus.refresh to update menus

https://reviewboard.mozilla.org/r/179050/#review185788

At our previous discussion, I assumed that Firefox has logic to allow menu items to be added without affecting the position of other menu items (e.g. adding a triangle at the bottom that expands the menu when hovered/clicked). This is not the case. Adding and removing a top-level item can be equally disruptive, and blocking the one while allowing the other does not make much sense.

For example, if you right-click at the bottom of the page, then the menu points upwards. When an item is appended at the end, the top menu items shift upwards. In case of removal, all menu items shift downwards. In either case, there is some visual disruption.

When the top of a page is right-clicked, it doesn't matter whether you add or remove a menu item; in both cases menu items aren't shifting (unless there is another extension that has appended menu items at the bottom).

> I though we weren't removing, only adding top level items?

See my answer above. TL;DR adding/removing are equivalent in terms of potential nuisance.
Comment on attachment 8908232 [details]
Bug 1215376 - Always use old contextData upon refresh

https://reviewboard.mozilla.org/r/179874/#review185802

> Hmm.  I'm not sure this is the correct behavior.  Can you explain your reasoning please?

I updated the comment. All items in the menu is based on the contextual information from the time that the menu is opened. It would be weird to mix the two. If an extension really wants to have context-independent menu items, then it should create menu items without a restrictive `documentUrlPatterns`
Comment on attachment 8908233 [details]
Bug 1215376 - Add more contextual information to onShown

https://reviewboard.mozilla.org/r/179876/#review186000

::: browser/components/extensions/ext-menus.js:446
(Diff revision 1)
>    }
>  
>    return contexts;
>  };
>  
> +function addMenuEventInfo(info, contextData, includeSensitiveData) {

This is 90% of `getClickInfo`, please extract/adapt that.

::: browser/components/extensions/ext-menus.js:802
(Diff revision 1)
> +          // The menus.onShown event is fired before the user has consciously
> +          // interacted with an extension, so we require permissions before
> +          // exposing sensitive contextual data.
> +          let includeSensitiveData =
> +            extension.tabManager.hasActiveTabPermission(contextData.tab) ||
> +            extension.whiteListedHosts.matches(contextData.inFrame ? contextData.frameUrl : contextData.pageUrl);

nit: operator precedence is a bit confusing, please add parens or split into a separate statement to make it easier to read.

::: browser/components/extensions/test/browser/browser_ext_menus_events.js:19
(Diff revision 1)
> -const RECOGNIZED_MENUS_ONSHOWN_EVENT_KEYS = [
> -  "menuIds",
> -  "contexts",
> -];
> +      // Serialize arrays so that they can be compared with is().
> +      actualValue = JSON.stringify(actualValue);
> +      expectedValue = JSON.stringify(expectedValue);
> +    }
> +    is(actualValue, expectedValue, `property "${key}" of onShown event`);
> +  }

nit: again Assert.deepEqual()
Comment on attachment 8908234 [details]
Bug 1215376 - refactor: re-use onShown event generator for onClicked

https://reviewboard.mozilla.org/r/179878/#review186030

Ah, makes more sense now.
I went though all the patches, and this seems good in general, just a few more specifics below.

(also, nice work Rob, code was fairly easy to follow)


(In reply to Rob Wu [:robwu] from comment #71)
> ...
> I don't see a reason for `onClicked` being in `ext-c-menus.js`. It can
> probably be implemented in the same way as `onShown` and `onHidden`.

I'm pretty sure I had reasons when I added the second namespace, though something could have changed and I forgot the details in the meantime.


> I guess that we should check the callback results in DEBUG mode to avoid
> mistakes like these in the future.
yeah, I plan to do that followup to bug 1363886 after the 57 scramble.


> I'm intentionally doing `[]`, so that if somehow the array is not empty,
> that the unexpected events are included in the log for ease of debugging.

Good thinking.


> I want to have a test that verifies that the event data is set as expected.

Yeah, this makes more sense after reviewing later patches.(In reply to Rob Wu [:robwu] from comment #72)


> For example, if you right-click at the bottom of the page, then the menu
> points upwards. When an item is appended at the end, the top menu items
> shift upwards. In case of removal, all menu items shift downwards. In either
> case, there is some visual disruption.

This is rather unfortunate.  :(

I think we should still try to come up with some restrictions to limit/minimize UX disruptions, for example only allowing refresh() for ~1s after the menu is opened.  But that could be a followup after 57, just make sure the docs warn the rules could change here.

Also, did you ask/plan to ask for ux-review?
Flags: needinfo?(tomica)
Attachment #8723939 - Attachment is obsolete: true
Attached file menu-refresh.xpi — 
Test case for manual testing.

1. Load the add-on via about:debugging
2. Open the menu (Right-click at the bottom of the page to open the menu).

   --> Check that a menu item "Dynamic menu item" is shown at the bottom.
   --> Visually, it seemed that the menu has always been there.

3. Change the delay to 1000 via the input field.
4. Open the menu.

   --> Check that a menu item "Dynamic menu item" appears after 1 second.
   --> Now there is an obvious visual change in the menu. The menu item is added after 1 second, causing all menu items to shift up (if the menu was opened from the bottom of the page).

5. Tick the checkbox to create a pre-existing menu.
6. Open the menu.

   --> Check that the menu item at the bottom is replaced with a menu item with a triangle (indicating that there is a submenu). The submenu has the original item "pre-existing menu item" and the new "Dynamic menu item".
   --> Because the menu item already existed, the position of other menu items is stable (they don't shift positions).
Could I get a UX review?

First get a build with the patch:

Try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a6bc2154356
Linux  : https://queue.taskcluster.net/v1/task/TJizTTKxTG6KaVESYuAb9A/runs/0/artifacts/public/build/target.tar.bz2
macOS  : https://queue.taskcluster.net/v1/task/A8gYWtv7TrqM7RcrInm5Zg/runs/0/artifacts/public/build/target.dmg
Windows: https://queue.taskcluster.net/v1/task/OJWmL60ETzW1UvzecPa8ZA/runs/0/artifacts/public/build/target.zip

Then run the test case from comment 94.
Extensions can have up to one top-level menu item in the main menu.

Under normal use (e.g. the flow described from comment 16), the menu is added so fast that the user cannot perceive the shifting of menu items (steps 1-2 of my manual test case).

The potential UX issue is when an add-on adds or removes a top-level menu item after a noticeable delay, which causes menu items to shift (steps 3-4).

Is the feature introduced here acceptable?
Do I have to limit how often/how soon an extension can modify a menu item?
If so, can that be done in a follow-up, so that this feature here can already land in Fx57?
Flags: needinfo?(ux-review)
Redirecting the UX review request to Emanuela...
Flags: needinfo?(ux-review) → needinfo?(emanuela)
Comment on attachment 8907371 [details]
Bug 1215376 - Add onShown and onHidden to contextMenus/menus API

https://reviewboard.mozilla.org/r/179044/#review187646

::: browser/components/extensions/ext-menus.js:308
(Diff revision 3)
> +      return;
> +    }
> +    let commonMenuInfo = {
> +      contexts: Array.from(getMenuContexts(contextData)),
> +    };
> +    // TODO(robwu): Add more contextual information.

Is this a followup bug?
Attachment #8907371 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8907374 [details]
Bug 1215376 - Add menus.refresh to update menus

https://reviewboard.mozilla.org/r/179050/#review187648

::: browser/components/extensions/ext-menus.js:319
(Diff revision 4)
>  
> +  rebuildMenu(extension) {
> +    let {contextData} = this;
> +    if (!contextData) {
> +      // This happens if the menu is not visible.
> +      // This also happens if there are no visible extension menu items.

nit: This happens if the menu is not visible or there are no visible extension menu items.
Attachment #8907374 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8908232 [details]
Bug 1215376 - Always use old contextData upon refresh

https://reviewboard.mozilla.org/r/179874/#review187748

::: browser/components/extensions/test/browser/browser_ext_menus_refresh.js:326
(Diff revision 3)
> +
> +  await extension.callMenuApi("refresh");
> +
> +  // Even after switching tabs, the tab at the time of opening the menu item
> +  // should still be used to determine which menu to show, because all other
> +  // menu items are also based on that contextual information.

This is a confusing comment in general.  Actually I wonder why the context menu would remain open on tab switch in the first place, since context is lost at that point.  I'm also not certain how to produce this situation in the real world.  Trying to switch tabs results in the context menu closing, so this feels artificial.  OTOH I don't think this causes a problem either.
Attachment #8908232 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8907371 [details]
Bug 1215376 - Add onShown and onHidden to contextMenus/menus API

https://reviewboard.mozilla.org/r/179044/#review187646

> Is this a followup bug?

This is the next item in your review queue on this bug, "Bug 1215376 - Add more contextual information to onShown".
Comment on attachment 8908232 [details]
Bug 1215376 - Always use old contextData upon refresh

https://reviewboard.mozilla.org/r/179874/#review187748

> This is a confusing comment in general.  Actually I wonder why the context menu would remain open on tab switch in the first place, since context is lost at that point.  I'm also not certain how to produce this situation in the real world.  Trying to switch tabs results in the context menu closing, so this feels artificial.  OTOH I don't think this causes a problem either.

The context menu is not on the page, but the browserAction button. To reproduce, right-click anywhere in the browser Chrome (e.g. browserAction button) and wait for the navigation to commit. The navigation could have been initiated in many ways, e.g. cicking on a link to a slow website before opening the context menu, or a page-initiated timed redirects (e.g. ad walls, interstitial pages, finished form submission, ...).
Comment on attachment 8908232 [details]
Bug 1215376 - Always use old contextData upon refresh

https://reviewboard.mozilla.org/r/179874/#review187748

> The context menu is not on the page, but the browserAction button. To reproduce, right-click anywhere in the browser Chrome (e.g. browserAction button) and wait for the navigation to commit. The navigation could have been initiated in many ways, e.g. cicking on a link to a slow website before opening the context menu, or a page-initiated timed redirects (e.g. ad walls, interstitial pages, finished form submission, ...).

Just getting my head back into this.  So this isn't about the tab at all, but about the content in the tab.  Maybe change the comment to something along the lines of "Refresh will not update the context menu after the onShown event has occurred.  Context menus will not update as a result of navigation events, etc."
Comment on attachment 8908233 [details]
Bug 1215376 - Add more contextual information to onShown

https://reviewboard.mozilla.org/r/179876/#review188542
Attachment #8908233 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8908234 [details]
Bug 1215376 - refactor: re-use onShown event generator for onClicked

https://reviewboard.mozilla.org/r/179878/#review188546
Attachment #8908234 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8908235 [details]
Bug 1215376 - Fire menus.onShown for every menu

https://reviewboard.mozilla.org/r/179880/#review188552
Attachment #8908235 - Flags: review?(mixedpuppy) → review+
I've addressed the nits and added one more commit that consists of a test that checks the event order.
Comment on attachment 8912334 [details]
Bug 1215376 - Test menu event order: onShown, onClicked, onHidden

https://reviewboard.mozilla.org/r/183664/#review188878

Overall I'm not convinced of the need for these tests.  I don't see how onclicked could happen prior to onshown or after onhidden.  If there is a convincing reason to beleive otherwise, there is probably some other bug.  I'd rather not add tests for the sake of adding tests.

::: browser/components/extensions/test/browser/browser_ext_menus_event_order.js:12
(Diff revision 1)
> +add_task(async function test_menus_click_event_sequence() {
> +  async function background() {
> +    let events = [];
> +
> +    browser.menus.onShown.addListener(() => { events.push("onShown"); });
> +    browser.menus.onHidden.addListener(() => { events.push("onHidden"); });

this seems unecessary, you can push the value in the other listener

::: browser/components/extensions/test/browser/browser_ext_menus_event_order.js:15
(Diff revision 1)
> +
> +    browser.menus.onShown.addListener(() => { events.push("onShown"); });
> +    browser.menus.onHidden.addListener(() => { events.push("onHidden"); });
> +    // This onClicked listener is registered before the 'onclick' parameters in
> +    // the browser.menus.create calls below, so it should fire first.
> +    browser.menus.onClicked.addListener(() => { events.push("onClicked"); });

again seems redundant
Attachment #8912334 - Flags: review?(mixedpuppy) → review-
(In reply to Rob Wu [:robwu] from comment #108)
> Comment on attachment 8908233 [details]
> Bug 1215376 - Add more contextual information to onShown
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/179876/diff/3-4/

this is weird, rb is not showing me any changes.
Comment on attachment 8912334 [details]
Bug 1215376 - Test menu event order: onShown, onClicked, onHidden

https://reviewboard.mozilla.org/r/183664/#review189398

Chatted on irc.  Still very skeptical that these could happen out of order, but ok.
Attachment #8912334 - Flags: review?(mixedpuppy) → review+
I'm an addon developer and have reading through this bug because I need a solution for it for my addon (any addon that wants to show a context menu for tab headers with tab-dependent menu content would need it). I'm writing for two reasons:

1) I'm voicing my concerns about the current direction the solution is headed. As I understand, the currently proposed implementation allows the menu contents to be changed after being shown, and hopes that the visual changes in the menu will happen fast enough to be unnoticeable for the user. I also understand first tests have shown this is the case. But is it always? Is it also unnoticeable with high system load? Is it unnoticeable if 20+ other plugins are also acting on the same event? Is it unnoticeable on old computers? Why is this solution better than an "onBeforeShown" event?

2) I might be too late because it seems the bug's solution is only waiting for a final review. But if not, I'd be willing to lend my help in a possible implementation, since not only is this something I need, but it is also tagged as "good-first-bug" (which I'm not sure but I suppose means that it is a good tarting task for somebody who has not yet worked on Firefox code).
(In reply to Karoly Pados from comment #116)
> I'm an addon developer and have reading through this bug because I need a
> solution for it for my addon (any addon that wants to show a context menu
> for tab headers with tab-dependent menu content would need it). I'm writing
> for two reasons:
> 
> 1) I'm voicing my concerns about the current direction the solution is
> headed. As I understand, the currently proposed implementation allows the
> menu contents to be changed after being shown, and hopes that the visual
> changes in the menu will happen fast enough to be unnoticeable for the user.
> I also understand first tests have shown this is the case. But is it always?
> Is it also unnoticeable with high system load? Is it unnoticeable if 20+
> other plugins are also acting on the same event? Is it unnoticeable on old
> computers? Why is this solution better than an "onBeforeShown" event?

There is no theoretical guarantee that the menu change cannot be perceived (in fact, if an extension is deliberately "slow" in requesting a menu change, then the user may be able to notice a change).
The solution is better than an "onBeforeShown" event, because the latter would imply that the menu is not shown until all extensions have signaled that the menu is ready. If the extension is slow enough for a regular menu update to be noticeable, then it is definitely slow enough to make the browser feel slow in delaying the appearance of the menu.


> 2) I might be too late because it seems the bug's solution is only waiting
> for a final review. But if not, I'd be willing to lend my help in a possible
> implementation

Before we got to the current state, I discussed alternatives with other members of the team, and the possibility of a blocking "before shown" event was ruled out, and I agree with.
There are plenty of other bugs if you want a good first bug ;)
Comment on attachment 8907372 [details]
Bug 1215376 - Ensure that menu elements have a unique ID

https://reviewboard.mozilla.org/r/179046/#review202450

::: browser/components/extensions/ext-menus.js:451
(Diff revision 3)
> +  get elementId() {
> +    let id = this.id;
> +    // If the ID is an integer, it is auto-generated and globally unique.
> +    // If the ID is a string, it is only unique within one extension and the
> +    // ID needs to be concatenated with the extension ID.
> +    if (typeof id != "number") {

Nit: !==
Attachment #8907372 - Flags: review?(kmaglione+bmo) → review+
Any update?
(In reply to Geoffrey De Belie (Smile4ever) from comment #120)
> Any update?

I lost the laptop on which I had submitted the patches. Although I managed to import my patches from mozreview on my new laptop, I am not able to update the original changes in mozreview (to address the nit from comment 118).

When I try to upload a set of patches, a new mozreview changeset is created, and I haven't found a way around this yet. Occasionally I try to investigate why this happens, but I haven't succeeded yet and this is not a high priority for me.
Blocks: 1385202
Hey Rob,

Do you still need a ux-review, considering this issue is currently not a high priority for you?
Flags: needinfo?(emanuela) → needinfo?(rob)
Hi Emanuela,

Yes, we would like ux-review.  Lets see if Rob can get the patches updated so a new build can be made available, if not I'll see if I can resurrect them.  

Rob, if you cannot get to this in the next couple weeks please ni? me.
Flags: needinfo?(emanuela)
Comment on attachment 8907370 [details]
Bug 1215376 - allow itemToSelect to be null in menu closing test utility

Obsoleted by https://bugzilla.mozilla.org/attachment.cgi?id=8940025
Attachment #8907370 - Attachment is obsolete: true
Comment on attachment 8907371 [details]
Bug 1215376 - Add onShown and onHidden to contextMenus/menus API

Obsoleted by https://bugzilla.mozilla.org/attachment.cgi?id=8940026
Attachment #8907371 - Attachment is obsolete: true
Comment on attachment 8907372 [details]
Bug 1215376 - Ensure that menu elements have a unique ID

Obsoleted by https://bugzilla.mozilla.org/attachment.cgi?id=8940027
Attachment #8907372 - Attachment is obsolete: true
Comment on attachment 8907373 [details]
Bug 1215376 - Refactor gMenuBuilder.build

Obsoleted by https://bugzilla.mozilla.org/attachment.cgi?id=8940028
Attachment #8907373 - Attachment is obsolete: true
Comment on attachment 8907374 [details]
Bug 1215376 - Add menus.refresh to update menus

Obsoleted by https://bugzilla.mozilla.org/attachment.cgi?id=8940030
Attachment #8907374 - Attachment is obsolete: true
Comment on attachment 8908232 [details]
Bug 1215376 - Always use old contextData upon refresh

Obsoleted by https://bugzilla.mozilla.org/attachment.cgi?id=8940030
Attachment #8908232 - Attachment is obsolete: true
Comment on attachment 8908233 [details]
Bug 1215376 - Add more contextual information to onShown

Obsoleted by https://bugzilla.mozilla.org/attachment.cgi?id=8940031
Attachment #8908233 - Attachment is obsolete: true
Comment on attachment 8908234 [details]
Bug 1215376 - refactor: re-use onShown event generator for onClicked

Obsoleted by https://bugzilla.mozilla.org/attachment.cgi?id=8940033
Attachment #8908234 - Attachment is obsolete: true
Comment on attachment 8908235 [details]
Bug 1215376 - Fire menus.onShown for every menu

Obsoleted by https://bugzilla.mozilla.org/attachment.cgi?id=8940033
Attachment #8908235 - Attachment is obsolete: true
Comment on attachment 8912334 [details]
Bug 1215376 - Test menu event order: onShown, onClicked, onHidden

Obsoleted by https://bugzilla.mozilla.org/attachment.cgi?id=8940034
Attachment #8912334 - Attachment is obsolete: true
I have rebased the patches and added a new patch to fix new linting issues and an unexpected test failure (https://bugzilla.mozilla.org/attachment.cgi?id=8940035).

Please finish the ux review before the 59 feature freeze if you can.
The review instructions were already provided 4 months ago in https://bugzilla.mozilla.org/show_bug.cgi?id=1215376#c95 .
Once the build completes, you can grab the latest binaries from https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a41cba9907b
If the build is not yet available, then you can also use the binaries from 4 months ago, since the core functionality has not changed.
Flags: needinfo?(rob)
Ok, here we are!

> Under normal use (e.g. the flow described from comment 16), the menu is
> added so fast that the user cannot perceive the shifting of menu items
> (steps 1-2 of my manual test case).


Agree, without any delay works well!


> The potential UX issue is when an add-on adds or removes a top-level menu
> item after a noticeable delay, which causes menu items to shift (steps 3-4).
> 
> Is the feature introduced here acceptable?
> Do I have to limit how often/how soon an extension can modify a menu item?

If we have a delay, especially a significant one like 1s, we should try a different solution. 
One question that I have is: are we aware of what's happening? Because, if we are, we can display a disable item (it can show the extension's name if we don't know what the label is going to be yet) and replace it with the actual item when the item is loaded.
Flags: needinfo?(emanuela)
(In reply to emanuela [ux team] from comment #147)
> > The potential UX issue is when an add-on adds or removes a top-level menu
> > item after a noticeable delay, which causes menu items to shift (steps 3-4).
> > 
> > Is the feature introduced here acceptable?
> > Do I have to limit how often/how soon an extension can modify a menu item?
> 
> If we have a delay, especially a significant one like 1s, we should try a
> different solution. 
> One question that I have is: are we aware of what's happening? Because, if
> we are, we can display a disable item (it can show the extension's name if
> we don't know what the label is going to be yet) and replace it with the
> actual item when the item is loaded.

We don't know in advance whether an extension would add or remove a menu item.
When a menu item is added, there is a clear visual indicator that the menu item originates from an extension (namely the extension icon in front of the menu item).
When a menu item is removed, an observant user can notice that a menu item with the extension icon has disappeared.

It is technically feasible to require a placeholder, but it should not be needed in the expected common case (where the menu is updated fast enough by the extension).
If you believe that it should be impossible for extensions to have any effect on the ordering of menus, I could push another patch to add a disabled placeholder menu item if the extension item is removed, and not render menu items if an extension is not showing any items. But I'd like to only do that if the benefit for UX outweighs the negative aspect of the menu item always being visible even in contexts where it is not relevant.

What weights stronger in your opinion?
Flags: needinfo?(emanuela)
(In reply to Rob Wu [:robwu] from comment #148)
> It is technically feasible to require a placeholder, but it should not be
> needed in the expected common case (where the menu is updated fast enough by
> the extension).

In the case of the gtranslate extension, the context menu item is the translation of the selected text. Fetching this from the translation service is a noticeable delay.

https://addons.mozilla.org/en-US/firefox/addon/gtranslate/
I will use for a similar behaviour (https://addons.mozilla.org/it/firefox/addon/gulpease-index/), to show in the contextmenu an updated value in real time based on where is the cursor.
So, my concern is not on the effect on the ordering of menus, but on the delay.
Any delay mayor of  500 milliseconds will affect the perceived performance of Firefox, which is something we want to avoid ;)

We can think to leave the patch as it is right now and set some metrics to identify how often and for how long the delay happens. Then, with more information, we'll think about a more appropriate solution.

Or, when the extension is using this API, we always display the disable item and replace (basically what I suggest in #147).

Whatever direction we decide to take, I recommend measuring the delays to improve this in the (next) future.
Flags: needinfo?(emanuela)
Comment on attachment 8940035 [details]
Bug 1215376 - fix linting and test issues from rebase

https://reviewboard.mozilla.org/r/210306/#review216250

::: browser/components/extensions/test/browser/browser_ext_menus_events.js:30
(Diff revision 1)
>      function awaitMessage(expectedId) {
>        return new Promise(resolve => {
>          browser.test.log(`Waiting for message: ${expectedId}`);
>          browser.test.onMessage.addListener(function listener(id, msg) {
> +          // Temporary work-around for https://bugzil.la/1428213
> +          // TODO(robwu): Remove this _wasCalled check once bug 1428213 is fixed.

TODO Bug 1428213: remove workaround for onMessage.removeListener
Attachment #8940035 - Flags: review?(mixedpuppy) → review+
All of these patches had r+ before and are just refreshed on current code.  Rob lost the ability to update the original reviewboard patches and posted new patches, obsoleting the originals.  I'll r+ the patches after a quick comparison.
Comment on attachment 8940025 [details]
Bug 1215376 - allow itemToSelect to be null in menu closing test utility

https://reviewboard.mozilla.org/r/210286/#review216260
Attachment #8940025 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8940026 [details]
Bug 1215376 - Add onShown and onHidden to contextMenus/menus API

https://reviewboard.mozilla.org/r/210288/#review216262
Attachment #8940026 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8940027 [details]
Bug 1215376 - Ensure that menu elements have a unique ID

https://reviewboard.mozilla.org/r/210290/#review216264
Attachment #8940027 - Flags: review+
Comment on attachment 8940028 [details]
Bug 1215376 - Refactor gMenuBuilder.build

https://reviewboard.mozilla.org/r/210292/#review216266
Attachment #8940028 - Flags: review?(mixedpuppy) → review+
Attachment #8940027 - Flags: review?(kmaglione+bmo)
Comment on attachment 8940029 [details]
Bug 1215376 - Add menus.refresh to update menus

https://reviewboard.mozilla.org/r/210294/#review216270
Attachment #8940029 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8940030 [details]
Bug 1215376 - Always use old contextData upon refresh

https://reviewboard.mozilla.org/r/210296/#review216274
Attachment #8940030 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8940031 [details]
Bug 1215376 - Add more contextual information to onShown

https://reviewboard.mozilla.org/r/210298/#review216280
Attachment #8940031 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8940032 [details]
Bug 1215376 - refactor: re-use onShown event generator for onClicked

https://reviewboard.mozilla.org/r/210300/#review216286
Attachment #8940032 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8940033 [details]
Bug 1215376 - Fire menus.onShown for every menu

https://reviewboard.mozilla.org/r/210302/#review216288
Attachment #8940033 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8940034 [details]
Bug 1215376 - Test menu event order: onShown, onClicked, onHidden

https://reviewboard.mozilla.org/r/210304/#review216290
Attachment #8940034 - Flags: review?(mixedpuppy) → review+
(In reply to emanuela [ux team] from comment #151)
> set some metrics to
> identify how often and for how long the delay happens. Then, with more
> information, we'll think about a more appropriate solution.

Shane, I don't know how to collect metrics. If you agree with this suggestion, do you want to create a follow-up bug with relevant details?

And can the current patches land as-is?
Flags: needinfo?(mixedpuppy)
I'm fine with handling the metrics in a followup bug.  My only caveat would be to land the metrics collection in the same release as the patches here.
Flags: needinfo?(mixedpuppy)
I can provide some guidance for collecting metrics in the follow-up bug, once it's been filed.
(In reply to Shane Caraveo (:mixedpuppy) from comment #166)
> I'm fine with handling the metrics in a followup bug.  My only caveat would
> be to land the metrics collection in the same release as the patches here.

(In reply to Bob Silverberg [:bsilverberg] from comment #167)
> I can provide some guidance for collecting metrics in the follow-up bug,
> once it's been filed.

By "followup bug", does that imply there is a pending fix for this one? I don't totally get this side of things, but is there a patch currently available for this bug, and if so, what is the status on testing it and getting it applied?
Blocks: 1432837
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/fe32678add2b
allow itemToSelect to be null in menu closing test utility r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/f9235f957ef6
Add onShown and onHidden to contextMenus/menus API r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/85211c321bec
Ensure that menu elements have a unique ID r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/23f6217f3569
Refactor gMenuBuilder.build r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/fd01166c4bd4
Add menus.refresh to update menus r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/c9d140ddac4d
Always use old contextData upon refresh r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/0ba0a66a945e
Add more contextual information to onShown r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/06def03114c1
refactor: re-use onShown event generator for onClicked r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/f473f26cba67
Fire menus.onShown for every menu r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/6257020951f6
Test menu event order: onShown, onClicked, onHidden r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/6a8c0e1a251c
fix linting and test issues from rebase r=mixedpuppy
Attached image deley not saved.gif — 
When is the timer started? When you click the right mouse button? If during your timer you click the right button again it is not reset and the timer is taken from when the first click was performed.
Flags: needinfo?(rob)
From the moment that the menu is shown.

The example (https://bugzilla.mozilla.org/attachment.cgi?id=8909724) uses a simple boolean (isShowing) to determine whether the menu is being shown. To avoid the issue that you observed (menu items appearing too soon), you should actually generate a unique ID, store it in a global, and after the asynchronous operation (the timer) finishes, check whether the global is still matching that value.

In terms of code, something like this:

var lastMenuInstanceId = 0;
var nextMenuInstanceId = 1;

browser.menus.onShown.addListener(async function(info, tab) {
  var menuInstanceId = nextMenuInstanceId++;
  lastMenuInstanceId = menuInstanceId;

  await .... ; // TODO: Some async operation here.

  // After completing the async operation, check whether the menu is still shown.
  if (menuInstanceId !== lastMenuInstanceId) {
    return; // Menu was closed and shown again.
  }
  // Now use menus.create/update + menus.refresh.
});

browser.menus.onHidden.addListener(function() {
  lastMenuInstanceId = 0;
});
Flags: needinfo?(rob)
Was able to reproduce the issue in Firefox 59.0a1 (20171201220040) on Windows 10 64Bit.
As per https://bugzilla.mozilla.org/show_bug.cgi?id=1215376#c172 this is an issue of coding not the feature.
Tested and verified on Windows 10 64Bit and Mac OS  X 10.13.2 in Firefox 60.0a1 (20180220103456).
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
Is there a way to put a listener for this event on a context menu submenu? I don't want my code to run every time the context menu is opened, only when the user opens the context menu and then opens my extenion's context menu subitem.
(In reply to winflip from comment #174)
> Is there a way to put a listener for this event on a context menu submenu? I
> don't want my code to run every time the context menu is opened, only when
> the user opens the context menu and then opens my extenion's context menu
> subitem.

Please see comment 63.
(In reply to kernp25 from comment #175)
> (In reply to winflip from comment #174)
> > Is there a way to put a listener for this event on a context menu submenu? I
> > don't want my code to run every time the context menu is opened, only when
> > the user opens the context menu and then opens my extenion's context menu
> > subitem.
> 
> Please see comment 63.

Created a bug https://bugzilla.mozilla.org/show_bug.cgi?id=1440635
Is this a bug? I do something like this:

browser.menus.onShown.addListener((info, tab) => {
	browser.menus.update(resultId, {title: "Something"});
    }
});

and the updated menuitem title is only visible the NEXT time that the context menu is opened. Whereas I would like its text to change immediately. Is this a bug or is it intended to work this way?
menus.refresh() solved it.
The introduction of onHidden states:
"For compatibility with other browsers, Firefox makes this event available via the contextMenus namespace as well as the menus namespace."

Firefox is the only browser that implements these events, so this sentence should be rephrased.


Updating the menu is relatively simple to implement. Creating a new menu (and hiding it) is probably more complicated, because the API is asynchronous, and it is very well possible that after resuming an `await`, that the menu has been closed already.
It would be helpful if you can provide an example that shows how one can create and hide a menu that accounts for this timing issue.

At the very least (even if you decide to not add an example), the documentation should clearly mention the caveats with using asynchronous APIs in the onShown/onHidden event handler. I.e. before calling any `menus` API method, they need to check whether the menu is still being shown. See comment 172 for a more detailed (but still concise) write-up.
Flags: needinfo?(rob)
menus.create and menus.update are asynchronous too though, aren't they? Does this complexity apply to them, or does "TODO: Some async operation here." in comment 172, refer more to something like fetch, that could realistically take much longer? It appears so, because comment 172 later says "// Now use menus.create/update + menus.refresh."
The menus events / API methods are synchronized (API calls handled in order).
The menus API is one of the few extension APIs that has this order implicitly embedded in the API design, because the menus.create API returns an integer to identify the newly created menu, so one can do things like this:

let menuId = browser.menus.create(...);
browser.menus.update(menuId, ...); // Note: Not waiting for returned promise.
browser.menus.update(menuId, ...); // Note: Not waiting for returned promise again.
browser.menus.refresh();


But if you do yield to JavaScript's event loop at any point, then you should check whether onShown / onHidden has been called in between. This even holds if you use a context menu method, such as "await browser.menus.update(...)"

So in short, an onShown handler that invokes a menus method *asynchronously* should always check whether the menu that it's handling is still active. (This also holds for onHidden).
I've added this stuff to https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/onShown. Please let me know if that covers it. Thanks!
Flags: needinfo?(rob)
Product: Toolkit → WebExtensions
Depends on: 1473720
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: