Closed Bug 1299371 Opened 8 years ago Closed 7 years ago

fix contextmenu for webext panels

Categories

(WebExtensions :: Untriaged, defect, P3)

Product:
WebExtensions
Component:
Untriaged
Version:
48 Branch
Type:
defect

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: chrisyeh96, Assigned: freaktechnik)

References

(Blocks 1 open bug)

Details

(Whiteboard: triaged)

Attachments

(3 files)

buggy_extension.zip
915 bytes, application/zip
Details
popup-click-tester.zip
1.01 KB, application/x-zip-compressed
Details
Bug 1299371 - Show context menu in WebExtension popups;
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details
Attached file 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
Priority: -- → P3
Whiteboard: triaged
Attached file 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.
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?
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)
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: nobody → martin
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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 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 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 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.
(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)
Attachment #8838929 - Flags: review?(kmaglione+bmo)
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.
(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 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 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)
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.
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 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: → 1353060
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.
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.
(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 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 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 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 on attachment 8838929 [details]
Bug 1299371 - Show context menu in WebExtension popups;

https://reviewboard.mozilla.org/r/113692/#review135424
I only see intermittents in the try run. Ready for checkin.
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e08459e83d3f
Show context menu in WebExtension popups; r=mixedpuppy
Keywords: checkin-needed
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd1b3c997089
Backed out changeset e08459e83d3f for multiple mochitests time out
Flags: needinfo?(martin)
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8db3ccba8b51
Show context menu in WebExtension popups; r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/8db3ccba8b51
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: