fix contextmenu for webext panels

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
RESOLVED FIXED
a year ago
8 days ago

People

(Reporter: chrisyeh96, Assigned: freaktechnik)

Tracking

(Blocks: 1 bug)

48 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

a year ago
Created attachment 8786607 [details]
buggy_extension.zip

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160726073904

Steps to reproduce:

Load a WebExtension-based add-on that registers a click handler on an element in a popup. (See attached example.) Right-click or middle-click on that element.


Actual results:

The right-click or middle-click is not registered.


Expected results:

The right-click or middle-click should have triggered the event handler to fire. In the case of the attached example, it should have created an alert.
Component: Untriaged → WebExtensions
Product: Firefox → Toolkit

Updated

11 months ago
Priority: -- → P3
Whiteboard: triaged

Updated

11 months ago
Blocks: 1237377

Updated

11 months ago
Duplicate of this bug: 1308748
(Assignee)

Comment 2

10 months ago
Created attachment 8804026 [details]
popup-click-tester.zip

I've written my own little extension to check what Chrome does and how Firefox currently behaves. The extension listens to both the click and the contextmenu events and prints the relevant button when it gets a click event.

Chrome will fire the click event for left and middle click and the contextmenu event for right click. It will also show a context menu, though it only consists of a single item "Inspect" plus any extension-inserted context menu items.

So I assume Firefox should behave the same here? This would involve creating a panel-specific context menu, as far as I know. I am not sure where the middle clicks get lost, but if that can be explained, I'd happily take this bug.
(Assignee)

Comment 3

10 months ago
Another thing I noticed both, when looking through what the SDK panel did and testing what chrome does: links are opened in a new tab when ctrl + clicked or middle clicked. Is that why the middle click event is not fired, because that event gets prevented?
(Assignee)

Comment 4

10 months ago
Chrome seems to show context-specific items for links etc. in popups.

kmag: forgot to needinfo you on my first comment here. Do you think Firefox should behave the same as chrome with middle & right clicks?
Flags: needinfo?(kmaglione+bmo)

Comment 5

7 months ago
Supporting the context menu should be fairly easy. It should mostly just be a matter of adding context="contentAreaContextMenu" to the browser.

I'm not sure exactly why the click events are blocked. It probably has something to do with the fact that the panel is a child of the toolbarbutton, but I don't see any obvious listeners on the toolbarbutton or toolbar that would block the events.

Can you check whether the behavior is any different between browserAction and pageAction popups?
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Updated

7 months ago
Assignee: nobody → martin
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 6

6 months ago
After some testing with the mouse button events I have not seen the click event getting fired for middle or right click, though the contextmenu event is fired for right-clicking. The mouseup and mousedown events get fired properly for all buttons.

To do that I compared behavior of the same page in a tab loaded from the extension, from the local file system, pageAction and browserAction popups and they all behaved the same.
Comment hidden (mozreview-request)

Comment 8

6 months ago
mozreview-review
Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

https://reviewboard.mozilla.org/r/113692/#review115276

::: browser/base/content/nsContextMenu.js:178
(Diff revision 1)
>    },
>  
>    initNavigationItems: function CM_initNavigationItems() {
>      var shouldShow = !(this.isContentSelected || this.onLink || this.onImage ||
>                         this.onCanvas || this.onVideo || this.onAudio ||
> -                       this.onTextInput || this.onSocial);
> +                       this.onTextInput || this.onSocial || this.onWebExtPopup);

Lets call this something better, e.g. addonView which is the value of webextension-view-type.
We also now have a sidebar, and have similar issues ( know at least with inspector.
It seems the patch you're producing here is related, but is not addressing the OP.  Can you create a second bug for the context menu (or change this bug, and create a second for the original issue).
Flags: needinfo?(martin)

Comment 10

6 months ago
mozreview-review
Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

https://reviewboard.mozilla.org/r/113692/#review115278

::: browser/base/content/nsContextMenu.js:253
(Diff revision 1)
>  
>      var shouldShow = !(this.isContentSelected ||
>                         this.onImage || this.onCanvas ||
>                         this.onVideo || this.onAudio ||
>                         this.onLink || this.onTextInput);
> -    var showInspect = !this.onSocial && gPrefService.getBoolPref("devtools.inspector.enabled");
> +    var showInspect = !this.onSocial && gPrefService.getBoolPref("devtools.inspector.enabled") && !this.onWebExtPopup;

Move the !this.onWebExtPopup prior to the getBoolPref

::: browser/base/content/nsContextMenu.js:669
(Diff revision 1)
>                                          .QueryInterface(Ci.nsIInterfaceRequestor)
>                                          .getInterface(Ci.nsIDOMWindowUtils)
>                                          .outerWindowID;
>      }
>      this.onSocial = !!this.browser.getAttribute("origin");
> +    this.onWebExtPopup = this.browser.getAttribute("webextension-view-type") === "popup";

If this remains a boolean, then we need to also check === "sidebar".  I'm assuming pageAction is also "popup".

Comment 11

6 months ago
mozreview-review
Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

https://reviewboard.mozilla.org/r/113692/#review115280

::: browser/base/content/nsContextMenu.js:253
(Diff revision 1)
>  
>      var shouldShow = !(this.isContentSelected ||
>                         this.onImage || this.onCanvas ||
>                         this.onVideo || this.onAudio ||
>                         this.onLink || this.onTextInput);
> -    var showInspect = !this.onSocial && gPrefService.getBoolPref("devtools.inspector.enabled");
> +    var showInspect = !this.onSocial && gPrefService.getBoolPref("devtools.inspector.enabled") && !this.onWebExtPopup;

Also, look through nsContextMenu for "onSocial", we need to do the same checks/limitations for addon views.
(Assignee)

Comment 12

6 months ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> It seems the patch you're producing here is related, but is not addressing
> the OP.  Can you create a second bug for the context menu (or change this
> bug, and create a second for the original issue).

Thanks for the quick feedback!
The bug related to the context menu was duped into this one, thus the patch on this bug (see https://bugzilla.mozilla.org/show_bug.cgi?id=1308748). As I stated in comment #6 I've not been able to identify unexpected behavior relating the click events based on how they behave in a normal tab.
Flags: needinfo?(martin)
I'm thinking now that onSocial and onWebExt should be changed and combined.  The issue here is that the browser is not a tabbrowser browser.  Other features (e.g. inspector) only work with tabbrowser browsers.  So...

isTabBrowser = !!gBrowser.getTabForBrowser(browser)

replace all uses of onSocial with isTabBrowser.

That should address the problem for webextensions as well as any other non-tab-browser.
Flags: needinfo?(martin)

Updated

6 months ago
Attachment #8838929 - Flags: review?(kmaglione+bmo)

Comment 14

6 months ago
mozreview-review
Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

https://reviewboard.mozilla.org/r/113692/#review115674

::: browser/components/extensions/ExtensionPopups.jsm:269
(Diff revision 1)
> +
> +      // Sets the context information for context menus.
> +      mm.loadFrameScript("chrome://browser/content/content.js", true);

Please load this in the same place we load ext-browser.content.js, and in the same way.
(Assignee)

Comment 15

6 months ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> I'm thinking now that onSocial and onWebExt should be changed and combined. 
> The issue here is that the browser is not a tabbrowser browser.  Other
> features (e.g. inspector) only work with tabbrowser browsers.  So...
> 
> isTabBrowser = !!gBrowser.getTabForBrowser(browser)
> 
> replace all uses of onSocial with isTabBrowser.
> 
> That should address the problem for webextensions as well as any other
> non-tab-browser.

That seems like a nicer way to solve the features that rely on the tab browser (back/forward, inspector, sharing). However for onSocial there are other cases that aren't directly related to it being a tab browser (bookmarking, page source).

I also assume inspection should be fixed for webextensions in the future to improve developer experience.

onSocial is also used for telemetry - I assume the webextensions one would also be used for that?
Flags: needinfo?(martin) → needinfo?(mixedpuppy)
onSocial shouldn't be doing anything with view source, did you mean page share where you said page source?

I looked over things again. I don't think we want to bookmark addon pages, sidebar, etc., nor would we want to share those.  However, for WebExt, we may just want to verify that the url is not moz-extension, and allow bookmarks and share if it is not.  That way, e.g. you can bookmark links in a sidebar, useful for sidebar tabmanagers.

For the telemetry stuff, it would be good to know that it is a webext panel, the share panel (only social panel left) or some other non-tabbrowser panel.  So I would keep onSocial and onWebExt, add isTabBrowser.  In _getTelemetryPageContextInfo add onWebExt and isTabBrowser.  Use isTabBrowser in all other places where onSocial is used except bookmarks/share.  Add validation that it is not a moz-extension uri for bookmark/share.
Flags: needinfo?(mixedpuppy)
BTW, there have been other non-social non-webext non-tabbrowser panels before, and I wont be surprised to see them in the future.  So addressing this might help avoid future breakage as well which is a nice bonus.
This bug has morphed into fixing the contextmenu, so updating title.
Summary: Middle-click and right-click are not recognized by the "click" event handler in webextension popups → fix contextmenu for webext panels
Comment hidden (mozreview-request)
(Assignee)

Comment 20

5 months ago
mozreview-review-reply
Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

https://reviewboard.mozilla.org/r/113692/#review115278

> If this remains a boolean, then we need to also check === "sidebar".  I'm assuming pageAction is also "popup".

I've added it for now, however it now has the side effect of disabling the info context menu items in sidebars. I haven't managed to test this patch with sidebars, however I'd assume the info popups work there.

Comment 21

5 months ago
mozreview-review
Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

https://reviewboard.mozilla.org/r/113692/#review121746

::: browser/base/content/nsContextMenu.js:257
(Diff revision 2)
>                         this.onLink || this.onTextInput);
> -    var showInspect = !this.onSocial && gPrefService.getBoolPref("devtools.inspector.enabled");
> +    var showInspect = this.inTabBrowser && gPrefService.getBoolPref("devtools.inspector.enabled");
>      this.showItem("context-viewsource", shouldShow);
>      this.showItem("context-viewinfo", shouldShow);
> +    // The page info is broken for WebExtension popups.
> +    this.setItemAttr("context-viewinfo", "disabled", this.inWebExtBrowser);

Ok, that's a bummer.  Maybe just do:

this.inWebExtBrowser = this.browser.getAttribute("webextension-view-type");

Then where you want to disable for popups, inWebExtBrowser === "popup"

Dito for viewimageinfo.
Attachment #8838929 - Flags: review?(mixedpuppy)
Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

Let me know if this still needs a review from me after it gets an r+ from mixedpuppy.
Attachment #8838929 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 23

5 months ago
I've finally tested my patch with a sidebar action. Turns out the sidebar doesn't have proper context menus with this patch - all items get displayed. I wonder if this would still be considered in-scope of this bug.

Further I tested context menus with remote WebExtensions enabled, which leads to no context menus being shown - should I not worry about the remote state?
Flags: needinfo?(mixedpuppy)
I'm guessing that the patch has something wrong.  If you load a sidebar extension without your patch applied does the context menu have that problem?  If you can update the patch on this bug I can take a look.
Flags: needinfo?(mixedpuppy)
(In reply to Shane Caraveo (:mixedpuppy) from comment #24)
> I'm guessing that the patch has something wrong.  If you load a sidebar
> extension without your patch applied does the context menu have that
> problem?  If you can update the patch on this bug I can take a look.

I should add, I tested (without patch) and get context menu's I expect in the sidebar, thus my assumption its a change in the patch.
Blocks: 1342708
Comment hidden (mozreview-request)
(Assignee)

Comment 27

5 months ago
The latest version of the patch fixes the context menu in the sidebar and also only disables the info boxes in popups. I've also added more insight to the comments why they are disabled for popups.

Further it seems the context menu breaks with remote extensions because the value for https://dxr.mozilla.org/mozilla-central/rev/00a166a8640dffa2e0f48650f966d75ca3c1836e/browser/base/content/content.js#114 is null.

Lastly currently view source is broken for popups because of a remoteness mismatch. I assume that's because the popups aren't remote yet?

Comment 28

5 months ago
mozreview-review
Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

https://reviewboard.mozilla.org/r/113692/#review128282

::: browser/base/content/nsContextMenu.js:691
(Diff revision 3)
>                                          .getInterface(Ci.nsIDOMWindowUtils)
>                                          .outerWindowID;
>      }
>      this.onSocial = !!this.browser.getAttribute("origin");
> +    this.webExtBrowserType = this.browser.getAttribute("webextension-view-type");
> +    this.inWebExtBrowser = this.webExtBrowserType === "popup" ||

I think this could just be inWebExtBrowser = !!webExtBrowserType
Attachment #8838929 - Flags: review?(mixedpuppy)
Depends on: 1353073
Martin, try the patch in bug 1353073 and see if that fixes your issue.  I did not run into the null value you mentioned, but the bug describes what I did run into.
Flags: needinfo?(martin)
See Also: → bug 1353060
Blocks: 1353136
Comment hidden (mozreview-request)
(Assignee)

Comment 31

5 months ago
mozreview-review-reply
Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

https://reviewboard.mozilla.org/r/113692/#review128282

> I think this could just be inWebExtBrowser = !!webExtBrowserType

I looked at the usages of the type attribute. It seems this will also make dev tools panes affected by these changes if they were to get a context menu, which should be fine either way.
(Assignee)

Comment 32

5 months ago
The changes introduced by the latest patch version that aren't related to this bug seem to be coming from foreign commits. I assume it's due to the try version of m-c being behind or similar. I checked the local diff of my commit and it does not include those changes.
That happens when you rebase in-between patch revisions. Mozreview used to try to hide unrelated changes in those cases, but it doesn't trust itself enough to, anymore.
(Assignee)

Comment 34

5 months ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #29)
> Martin, try the patch in bug 1353073 and see if that fixes your issue.  I
> did not run into the null value you mentioned, but the bug describes what I
> did run into.

It indeed fixes context menus in the sidebar for me with remote WebExtensions.
Flags: needinfo?(martin)
No longer blocks: 1353136
Comment hidden (mozreview-request)
(Assignee)

Comment 36

4 months ago
mozreview-review
Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

https://reviewboard.mozilla.org/r/113692/#review133340

::: browser/base/content/test/contextMenu/browser_contextmenu_mozextension.js:20
(Diff revision 5)
> +  /* import-globals-from ../general/contextmenu_common.js */
> +  Services.scriptloader.loadSubScript(contextmenu_common, this);
> +});
> +
> +add_task(function* test_link() {
> +  // TODO enable share button, so the share context menu should be displayed but

I don't know how to enable the sharing context menu items. I found out, that they were disable because there's no provider installed, but I'm not sure if there's a simple way to install one for these tests. I didn't really find out more from the social tests.

Comment 37

4 months ago
mozreview-review
Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

https://reviewboard.mozilla.org/r/113692/#review133440

::: browser/base/content/test/contextMenu/browser_contextmenu_mozextension.js:20
(Diff revision 5)
> +  /* import-globals-from ../general/contextmenu_common.js */
> +  Services.scriptloader.loadSubScript(contextmenu_common, this);
> +});
> +
> +add_task(function* test_link() {
> +  // TODO enable share button, so the share context menu should be displayed but

In test setup/teardown you can add/remove a provider similar to: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/social/browser_share.js#139
Attachment #8838929 - Flags: review?(mixedpuppy)

Comment 38

4 months ago
mozreview-review
Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

https://reviewboard.mozilla.org/r/113692/#review133442

Very nice.  Lets add the share check and run it through try (if not already), then r+

::: browser/components/extensions/test/browser/head.js:11
(Diff revision 5)
>   *          getBrowserActionWidget
>   *          clickBrowserAction clickPageAction
>   *          getBrowserActionPopup getPageActionPopup
>   *          closeBrowserAction closePageAction
>   *          promisePopupShown promisePopupHidden
> - *          openContextMenu closeContextMenu openContextMenuInSidebar
> + *          openContextMenu closeContextMenu openContextMenuInSidebar openContextMenuInPopup

nit: wrap these
Attachment #8838929 - Flags: review+
Comment hidden (mozreview-request)
(Assignee)

Comment 40

4 months ago
mozreview-review
Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

https://reviewboard.mozilla.org/r/113692/#review135424
(Assignee)

Comment 41

4 months ago
I only see intermittents in the try run. Ready for checkin.
Keywords: checkin-needed

Comment 42

4 months ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e08459e83d3f
Show context menu in WebExtension popups; r=mixedpuppy
Keywords: checkin-needed

Comment 43

4 months ago
sorry had to back this out for multiple mochitests time out, some of them like
https://treeherder.mozilla.org/logviewer.html#?job_id=93645772&repo=autoland&lineNumber=32288
https://treeherder.mozilla.org/logviewer.html#?job_id=93631903&repo=autoland&lineNumber=32533
https://treeherder.mozilla.org/logviewer.html#?job_id=93615615&repo=autoland&lineNumber=32351
Flags: needinfo?(martin)

Comment 44

4 months ago
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd1b3c997089
Backed out changeset e08459e83d3f for multiple mochitests time out
(Assignee)

Updated

4 months ago
Flags: needinfo?(martin)
Comment hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6692deb12e207eb5e65ad35049e59cfe33dd822a
running a try comparison:
baseline: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a464e848a49115a008be81db54f8f547f5994a5
with patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca57f939482792d2f1dfffb91d5cd3158c724623

Comment 48

4 months ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8db3ccba8b51
Show context menu in WebExtension popups; r=mixedpuppy

Comment 49

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8db3ccba8b51
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 50

8 days ago
popup-click-tester.zip

osVersion: "Windows_NT 6.1"
User Agent: "Mozilla/5.0 (Windows NT 6.1; rv:56.0) Gecko/20100101 Firefox/56.0",
Build ID: 20170810180547

The right-click or middle-click is not registered.

Expected results:

The right-click or middle-click should have triggered the event handler to fire.
You need to log in before you can comment on or make changes to this bug.