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

VERIFIED FIXED in Firefox 63

Status

P3
normal
VERIFIED FIXED
9 months ago
7 months ago

People

(Reporter: Kwan, Assigned: pts+bmo)

Tracking

unspecified
mozilla63

Firefox Tracking Flags

(firefox63 verified)

Details

Attachments

(4 attachments)

(Reporter)

Description

9 months ago
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.
(Reporter)

Comment 1

9 months ago
And here's a version of the extension with an empty function as an onShown listener, which displays the expected behaviour.
(Reporter)

Comment 2

9 months ago
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.
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.
Component: Untriaged → Frontend
Priority: -- → P3
Comment hidden (mozreview-request)
(Assignee)

Comment 5

9 months ago
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.
Attachment #8990274 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 6

9 months ago
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

8 months ago
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.
(Assignee)

Comment 8

8 months ago
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.)
Attachment #8990274 - Flags: review?(kmaglione+bmo) → review?(mixedpuppy)

Comment 9

8 months ago
mozreview-review
Comment on attachment 8990274 [details]
Bug 1469705: allow menus.refresh() without an onShown listener

https://reviewboard.mozilla.org/r/255298/#review268618
Attachment #8990274 - Flags: review?(mixedpuppy) → review+

Comment 10

8 months ago
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.
(Assignee)

Comment 11

8 months ago
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

8 months ago
I have assigned the bug to you. Let me know if it works. Otherwise I can easily land the patch on behalf of you.
Assignee: nobody → pts+bmo
Status: NEW → ASSIGNED
(Assignee)

Comment 13

8 months ago
That was it.  Thanks!
Keywords: checkin-needed

Comment 14

8 months ago
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a65efe88395
allow menus.refresh() without an onShown listener r=mixedpuppy
Keywords: checkin-needed

Comment 15

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9a65efe88395
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

Comment 16

7 months ago
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.
Status: RESOLVED → VERIFIED

Updated

7 months ago
status-firefox63: fixed → verified

Comment 17

7 months ago
Posted image Postfix video
You need to log in before you can comment on or make changes to this bug.