1299371 - fix contextmenu for webext panels

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[chrisyeh96]

Attachment: 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.

Comment 2

[Martin Giger [:freaktechnik]]

Attachment: 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.

Comment 3

[Martin Giger [:freaktechnik]]

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?

Comment 4

[Martin Giger [:freaktechnik]]

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?

Comment 5

[Kris Maglione [:kmag]]

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?

Comment 6

[Martin Giger [:freaktechnik]]

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 7

[Martin Giger [:freaktechnik]]

Attachment: Bug 1299371 - Show context menu in WebExtension popups;

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

Comment 8

[Shane Caraveo (:mixedpuppy)]

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.

Comment 9

[Shane Caraveo (:mixedpuppy)]

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).

Comment 10

[Shane Caraveo (:mixedpuppy)]

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

[Shane Caraveo (:mixedpuppy)]

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.

Comment 12

[Martin Giger [:freaktechnik]]

(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.

Comment 13

[Shane Caraveo (:mixedpuppy)]

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.

Comment 14

[Kris Maglione [:kmag]]

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.

Comment 15

[Martin Giger [:freaktechnik]]

(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?

Comment 16

[Shane Caraveo (: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.

Comment 17

[Shane Caraveo (: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.

Comment 18

[Shane Caraveo (:mixedpuppy)]

This bug has morphed into fixing the contextmenu, so updating title.

Comment 19

[Martin Giger [:freaktechnik]]

Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113692/diff/1-2/

Comment 20

[Martin Giger [:freaktechnik]]

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

[Shane Caraveo (:mixedpuppy)]

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.

Comment 22

[Kris Maglione [:kmag]]

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.

Comment 23

[Martin Giger [:freaktechnik]]

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?

Comment 24

[Shane Caraveo (: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.

Comment 25

[Shane Caraveo (: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.

Comment 26

[Martin Giger [:freaktechnik]]

Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113692/diff/2-3/

Comment 27

[Martin Giger [:freaktechnik]]

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

[Shane Caraveo (:mixedpuppy)]

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

Comment 29

[Shane Caraveo (:mixedpuppy)]

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.

Comment 30

[Martin Giger [:freaktechnik]]

Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113692/diff/3-4/

Comment 31

[Martin Giger [:freaktechnik]]

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.

Comment 32

[Martin Giger [:freaktechnik]]

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.

Comment 33

[Kris Maglione [:kmag]]

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.

Comment 34

[Martin Giger [:freaktechnik]]

(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.

Comment 35

[Martin Giger [:freaktechnik]]

Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113692/diff/4-5/

Comment 36

[Martin Giger [:freaktechnik]]

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

[Shane Caraveo (:mixedpuppy)]

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

Comment 38

[Shane Caraveo (:mixedpuppy)]

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

Comment 39

[Martin Giger [:freaktechnik]]

Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113692/diff/5-6/

Comment 40

[Martin Giger [:freaktechnik]]

Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

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

Comment 41

[Martin Giger [:freaktechnik]]

I only see intermittents in the try run. Ready for checkin.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e08459e83d3f
Show context menu in WebExtension popups; r=mixedpuppy

Comment 43

[Iris Hsiao [:ihsiao]]

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

Comment 44

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd1b3c997089
Backed out changeset e08459e83d3f for multiple mochitests time out

Comment 45

[Martin Giger [:freaktechnik]]

Comment on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113692/diff/6-7/

Comment 46

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6692deb12e207eb5e65ad35049e59cfe33dd822a

Comment 47

[Shane Caraveo (:mixedpuppy)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8db3ccba8b51
Show context menu in WebExtension popups; r=mixedpuppy

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8db3ccba8b51

Comment 50

[fess61]

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.