1469705 - Existence of an empty menus.onShown listener function affects other code adjusting the menu.

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P3)

Description

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Test extension, buggy, no onShown listener

I'm not sure if the summary is well worded, or if this is even a bug or expected.
I've noticed that for a particular extension of mine, which only creates a menuitem based on a message from a contentScript that examines the target element on the contextmenu event, it won't work properly unless I create an _empty_ menus.onShown listener.

I've created a simplified text extension to demonstrate.

STR:
1) Install attached test extension
2) Right click on any element in a webpage

ER:
Menuitem appears with the element's localName

AR:
Menuitem doesn't appear, or does appear but displays the localName of the element clicked during the immediately proceeding contextmenu.

Comment 1

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Test extension v2, works, empty onShown listener

And here's a version of the extension with an empty function as an onShown listener, which displays the expected behaviour.

Comment 2

[Ian Moody [:Kwan] (UTC+1)]

Oh, and I later realised that since I'm on Linux perhaps (lack of) OOP extensions might make a difference so tried turning it on, but didn't notice any difference in results.

Comment 3

[Kris Maglione [:kmag]]

We currently ignore refresh() calls unless the extension has an onShow() listener, so this code only works when the create() call happens before the menu is shown.

This is a pretty silly optimization, though. If the extension is calling refresh(), it's probably doing it for a reason.

Comment 4

[Peter Simonyi]

Attachment: Bug 1469705: allow menus.refresh() without an onShown listener

Review commit: https://reviewboard.mozilla.org/r/255298/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255298/

Comment 5

[Peter Simonyi]

Comment on attachment 8990274 [details]
Bug 1469705: allow menus.refresh() without an onShown listener

Mozreview is showing a bit of a mess as it tries to see how the test refresh_menus_with_browser_action became refresh_without_onShown even though they aren't related.  refresh_without_onShown is a new test for this bug's case, based largely on the preceding refresh_without_menus_at_onShown.  refresh_menus_with_browser_action is mostly moved into a helper testRefreshOther so that it can run for other context types; this is checking that one extension's refresh() call doesn't affect other extensions' menu items.

Comment 6

[Peter Simonyi]

Relatedly, onHidden is only emitted when there is an onShown listener, or the extension had an item visible in the menu at some point while the menu was visible.  It seems harmless enough, but I wonder whether it too is waiting for some obscure use case like this one.

Comment 7

[Rob Wu [:robwu]]

My intent was that extensions should only be able to modify (menus.refresh) and detect menu visibility changes (menus.onHidden) if they know that a menu has been shown (menus.onShown).
The "oncontextmenu" event does not guarantee that a menu is actually being shown because the page can call event.preventDefault() to prevent the menu from appearing.
Regardless, it is a way to predict that a menu might be shown, so allowing menus.refresh() to work without menus.onShown should be fine, I guess.


(In reply to Peter Simonyi from comment #6)
> Relatedly, onHidden is only emitted when there is an onShown listener, or
> the extension had an item visible in the menu at some point while the menu
> was visible.  It seems harmless enough, but I wonder whether it too is
> waiting for some obscure use case like this one.

This too is intentional. Even if menus.refresh() drops the requirement of the existence of a menus.onShown listener, I believe that extensions have no business in knowing that a context menu has disappeared if they don't know that the context menu existed in the first place.
However, if others disagree, then this is the way to enable the onHidden event for any extension:
- Remove https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/browser/components/extensions/parent/ext-menus.js#336-341
- Remove the comment at https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/browser/components/extensions/parent/ext-menus.js#388-391
- Replace https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/browser/components/extensions/parent/ext-menus.js#419-421
  with logic to dispatch to all onHidden listeners (look for gOnShownSubscribers in the file for an example; you would be introducing a new gOnHiddenSubscribers set).

Peter, you might want to reassign the review to :mixedpuppy , who has reviewed other patches for the menus API.

Comment 8

[Peter Simonyi]

Comment on attachment 8990274 [details]
Bug 1469705: allow menus.refresh() without an onShown listener

Thanks Rob.  (I had flagged Kris because of comment 3, but it was just a guess.)

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8990274 [details]
Bug 1469705: allow menus.refresh() without an onShown listener

https://reviewboard.mozilla.org/r/255298/#review268618

Comment 10

[Rob Wu [:robwu]]

Try run is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dd7986197ecf329da815a180da6538444a8adc5

Add the checkin-needed keyword if you are ready to land the patch.

Comment 11

[Peter Simonyi]

I think it's ready to land.  I don't have permission to edit keywords though -- maybe I need to be the assignee to do that?

Comment 12

[Rob Wu [:robwu]]

I have assigned the bug to you. Let me know if it works. Otherwise I can easily land the patch on behalf of you.

Comment 13

[Peter Simonyi]

That was it.  Thanks!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a65efe88395
allow menus.refresh() without an onShown listener r=mixedpuppy

Comment 15

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/9a65efe88395

Comment 16

[Victor Carciu]

Verified as fixed using Windows 10x64 and macOS 10.13.2 in latest Firefox Nightly (63.0a1 (2018-08-30)).

Also, I can confirm that I reproduced the initial issue in Firefox 61.

I will attach a postfix video.

Comment 17

[Victor Carciu]

Attachment: Postfix video