Closed Bug 1253418 Opened 4 years ago Closed 3 years ago

Support browser_action and page_action contexts in browser.contextMenus API

Categories

(WebExtensions :: Frontend, defect, P2)

defect

Tracking

(firefox53 verified)

VERIFIED FIXED
mozilla53
Tracking Status
firefox53 --- verified

People

(Reporter: kmag, Assigned: zombie)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [contextMenus][berlin]triaged)

Attachments

(3 files)

No description provided.
Whiteboard: [contextMenus] → [contextMenus][berlin]
I am porting one of my add-ons (Navigate Up) to use the new WebExtensions interface.

Previously, I have ported ‘Print Edit’ to Google Chrome and also re-written ‘Tile Tabs’ for Google Chrome.

Many of my add-ons rely on having a drop-down menu associated with the add-on button/icon – either a toolbar button with type  ‘menu-button’ - or an icon in the url bar or search bar with a right-click menu attached. 

This is not a problem when porting add-ons to Chrome, because page/browser action buttons have a right click menu that displays the first 6 items added to the context menu.  However, this feature is not available in WebExtensions, because chrome.contextMenus.create does not support ‘page_action’ or ‘browser_action’ contexts.

For me, and I suspect many add-on developers, this is a big issue, and for users this is an unnecessary incompatibility with Chrome.  I don’t see why WebExtensions cannot append context menu items to the normal right-click toolbar customize menu.

When implemented, I assume this feature will end up in chrome.contextMenu rather than browser.contextMenus.
Priority: -- → P2
Whiteboard: [contextMenus][berlin] → [contextMenus][berlin]triaged
Component: WebExtensions: Untriaged → WebExtensions: Frontend
(In reply to dw-dev from comment #1)
> I am porting one of my add-ons (Navigate Up) to use the new WebExtensions
> interface.

Likewise, I am learning add-on programming by porting some abandoned but useful add-ons.

> For me, and I suspect many add-on developers, this is a big issue, and for

This is the primary reason I did not port to the SDK, because it likewise did not allow context menus on action buttons.  And it is the primary reason I have not tried to port any other add-ons to WebExtensions.

The action buttons are useful when the mouse-over is used for most frequently accessed feature (look at live stats in a popup), and perhaps a left click to look at a less frequently accessed feature (open a new page or tab for additional, more detailed info), and the context menu used for the least frequently accessed feature (options).  This provides maximum info with minimal hand movements, clicks or load times.

A beautiful example of this technique is found in the add-on "Forecastfox (fix version)", which is written in XUL/Overlay, but has also been ported to Google Chrome.  I do not know what the developer plans, but lack of this feature means he either postpones porting from XUL (possibly the add-on disappearing from newer Firefox versions), or he has to redesign the interface to be sub-optimal, and then must maintain two (possibly non-trivially differing) code bases for the same add-on, which use the "same" WebExtensions API.

> When implemented, I assume this feature will end up in chrome.contextMenu
> rather than browser.contextMenus.

For my own use, I am far less concerned about where it ends up than with the underlying functionality.  However, it would make sense to keep it in the same place as WebExtensions to make porting to other browsers (and Mozilla's own "copy-pasted" documentation maintenance) that much easier.
Blocks: 1316020
No longer blocks: 1316020
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Gah, didn't mean to do that, sorry for the spam.
Attachment #8811535 - Flags: review?(aswan)
Attachment #8811535 - Flags: review?(aswan) → review?(kmaglione+bmo)
Attachment #8811535 - Flags: review?(kmaglione+bmo)
Hey Markus, can you provide some feedback here please?

This bug is about allowing web extensions to add context menus to their *own* page_action and browser_action buttons. 

First question is weather extension's menu should replace or add to the toolbar context menu.  Kris thinks they should be added to the existing menu.

If that's ok with you, that leaves the question of weather extension's items should go above or below the existing context menu items?

I went with above, as that puts more "context-specific" menu items to the top.  Do you agree with that?

Also note that the number of items the extension is allowed to add is limited to 6, so there is no threat of our existing items getting pushed out off the screen.
Attachment #8812496 - Flags: feedback?(mjaritz)
This is what Chrome does, for reference.
Attachment #8811535 - Flags: review?(kmaglione+bmo)
Attachment #8812496 - Flags: feedback?(mjaritz) → ui-review?(mjaritz)
Comment on attachment 8811535 [details]
bug 1253418 - implement contextMenu page_action and browser_action contexts

https://reviewboard.mozilla.org/r/93622/#review94388

Looks good. I just want to make sure that we check that the `popupshowing` event is actually generated by a context menu, so we don't wind up causing weird errors down the road.

::: browser/components/extensions/ext-browserAction.js:234
(Diff revision 3)
>            }
>          }
>          break;
> +
> +      case "popupshowing":
> +        const menu = event.target;

We should check that this is actually a context menu and not, say, a panel.

::: browser/components/extensions/test/browser/head.js:21
(Diff revision 3)
>   *          promiseContentDimensions alterContent
>   */
>  
> -var {AppConstants} = Cu.import("resource://gre/modules/AppConstants.jsm");
> -var {CustomizableUI} = Cu.import("resource:///modules/CustomizableUI.jsm");
> +const {AppConstants} = Cu.import("resource://gre/modules/AppConstants.jsm");
> +const {CustomizableUI} = Cu.import("resource:///modules/CustomizableUI.jsm");
> +const {promiseEvent} = Cu.import("resource://gre/modules/ExtensionUtils.jsm", {});

Please use `BrowserTestUtils.waitForEvent` instead, in tests.
Attachment #8811535 - Flags: review?(kmaglione+bmo)
Comment on attachment 8811535 [details]
bug 1253418 - implement contextMenu page_action and browser_action contexts

https://reviewboard.mozilla.org/r/93622/#review94388

> Please use `BrowserTestUtils.waitForEvent` instead, in tests.

I was sure `BrowserTestUtils.waitForEvent` was content-only, possibly because I half-remember seeing `waitForContentEvent` all the time.
Comment on attachment 8811535 [details]
bug 1253418 - implement contextMenu page_action and browser_action contexts

https://reviewboard.mozilla.org/r/93622/#review94392

Thanks!

::: browser/components/extensions/ext-browserAction.js:238
(Diff revisions 3 - 4)
>        case "popupshowing":
>          const menu = event.target;
>          const trigger = menu.triggerNode;
>          const node = window.document.getElementById(this.id);
>  
> -        if (trigger && node && isAncestorOrSelf(node, trigger)) {
> +        if (menu.tagName === "menupopup" && node && isAncestorOrSelf(node, trigger)) {

s/tagName/localName/
Attachment #8811535 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8812496 [details]
fx-action-context-above.png

Hi Tomislav,
thanks for the great summary. The way you propose to add them sounds fine to me.
I like that Chrome keeps one entry above the inserted list to let users know what extension it is. (But as we do not have that, we can have the custom functions on top.)

What worries me in you screenshot is that I see an arrow next to the first custom entry. That looks like it makes possible room for a lot of sub-menus. I do not think we should support sub-menus, but keep the entries to functional menu items. Allowing for hierarchy would make it too easy to add more complexity instead of doing the hard work of structuring the content right.
Attachment #8812496 - Flags: ui-review?(mjaritz) → ui-review-
I have two points of concern:

1. I agree that having one entry (with the extension name) above the inserted list of menu items, like Chrome, would be nice.  This first entry is non-clickable in Chrome, so I don't see any reason why the same cannot be done in Firefox.

2. I don't see any reason why the inserted menu items should not have sub-menu items.  This is supported in Chrome and in some cases would be very useful and legitimate (for example, look at my Tile Tabs WE add-on on both Chrome and Firefox).  Doing things differently to Chrome and breaking the symmetry between the button menu and the context menu does not make any sense.
Hi Markus, thanks for the reply.

I'm not sure how to argue against ux-review-, but taking into account:
1) what dw-dev said, and
2) this would break Chrome API compatibility,
3) we already support sub-menus in other places where extensions can add context menu items,
4) in fact, we explicitly use a sub-menu [1] to group all the extension's menu items in a content context menu, where multiple extensions can add items.

I'm asking you to reconsider.

In Chrome's contextMenu API [2], that we inherited and are committed to supporting [3], context menu items (and submenus) can be shared across different contexts.  It would be especially confusing if the same submenu worked in the content context menu, but failed in the action context menu.

1) https://mdn.mozillademos.org/files/13362/context-menu.png
2) https://developer.chrome.com/extensions/contextMenus
3) https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus
Flags: needinfo?(mjaritz)
Comment on attachment 8812496 [details]
fx-action-context-above.png


(In reply to dw-dev from comment #15)
> 2. I don't see any reason why the inserted menu items should not have
> sub-menu items.  This is supported in Chrome and in some cases would be very
> useful and legitimate (for example, look at my Tile Tabs WE add-on on both
> Chrome and Firefox).  Doing things differently to Chrome and breaking the
> symmetry between the button menu and the context menu does not make any
> sense.

(In reply to Tomislav Jovanovic :zombie from comment #16)
> In Chrome's contextMenu API, that we inherited and are committed to
> supporting, context menu items (and submenus) can be shared across
> different contexts.  It would be especially confusing if the same submenu
> worked in the content context menu, but failed in the action context menu.

I agree, we should not break symmetry with Chrome lightly. Does Chrome have any limitations on the structure or depth of custom entries hierarchy? (Other than no more than six on the top level.)
Can developers create one sub-level, or also sub-sub-levels? Or deeper?



(In reply to dw-dev from comment #15)
> 1. I agree that having one entry (with the extension name) above the
> inserted list of menu items, like Chrome, would be nice.  This first entry
> is non-clickable in Chrome, so I don't see any reason why the same cannot be
> done in Firefox.

It is clickable and links to the Chrome Store entry of the extension.
(We would probably need to make it non-clickable for unlisted extensions)
But that is a topic for another bug.
Flags: needinfo?(mjaritz)
Attachment #8812496 - Flags: ui-review- → ui-review?
(In reply to Markus Jaritz  from comment #17)
 
> I agree, we should not break symmetry with Chrome lightly. Does Chrome have
> any limitations on the structure or depth of custom entries hierarchy?
> (Other than no more than six on the top level.)
> Can developers create one sub-level, or also sub-sub-levels? Or deeper?

I am not aware of any limitations other than the 6 items at the top level. I have certainly created menus with two sub-levels, although none of my WebExtensions add-ons currently has more than one sub-level.

> It is clickable and links to the Chrome Store entry of the extension.
> (We would probably need to make it non-clickable for unlisted extensions)
> But that is a topic for another bug.

Markus, apologies, as you say, the first item (extension name) is clickable on Chrome and links to the extension's page in the Chrome Store. I currently have all of my add-ons installed in developer mode on Chrome, which makes the first item non-clickable.
Comment on attachment 8812496 [details]
fx-action-context-above.png

(In reply to dw-dev from comment #18)
> I am not aware of any limitations other than the 6 items at the top level. I
> have certainly created menus with two sub-levels, although none of my
> WebExtensions add-ons currently has more than one sub-level.

Thanks. Then let's implement as shown.
Attachment #8812496 - Flags: ui-review? → ui-review+
Thanks Markus.

Since this already had r+ on code, it's good to go.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7065b2e2f667
implement contextMenu page_action and browser_action contexts r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7065b2e2f667
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
I have tested browser_action context menus using Nightly 53.0a1 (2016-11-25) and my Tile Tabs WE add-on.

Unfortunately, there are a couple of serious bugs with sub-menus, which I have reported in Bug 1320439.
Flags: needinfo?(tomica)
> Unfortunately, there are a couple of serious bugs with sub-menus, which I
> have reported in Bug 1320439.

Thanks.
Flags: needinfo?(tomica)
Could you provide a WebExtension for this bug so I can verify it? Thanks!
Flags: needinfo?(tomica)
You should be able to use "Tile Tabs WE" (for WebExtension):

https://addons.mozilla.org/en-US/firefox/addon/tile-tabs-we/
Flags: needinfo?(tomica)
I can reproduce this issue on Firefox 53.0a1 (20161123030208) under Windows 7 64-bit and Ubuntu 16.04 LTS 32-bit.

This issue is verified as fixed on Firefox 53.0a1 (20161204030210) under Windows 7 64-bit and Ubuntu 16.04 LTS 32-bit, after right clicking on the WebExtension’s icon a menu with 6 items is displayed to the context menu.
Here is a video: http://screencast.com/t/4Gbxxkcq 

Thanks! Tomislav Jovanovic.
Status: RESOLVED → VERIFIED
I've noted this in https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus/ContextType. 

Please let me know if this covers it!
Flags: needinfo?(tomica)
Seems good, thanks.
Flags: needinfo?(tomica)
I added a reference to the contextMenus API at
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserAction/onClicked and
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pageAction/onClicked
since those are likely the most natural places where developers would look for such APIs.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.