854 bytes, application/zip
905 bytes, application/zip
59 bytes, text/x-review-board-request
764.85 KB, image/gif
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.
And here's a version of the extension with an empty function as an onShown listener, which displays the expected behaviour.
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
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)
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.
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 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 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+
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.
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?
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
That was it. Thanks!
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/9a65efe88395 allow menus.refresh() without an onShown listener r=mixedpuppy
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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
You need to log in before you can comment on or make changes to this bug.