Closed Bug 1268020 Opened 5 years ago Closed 4 years ago

Implement "tools_menu" context

Categories

(WebExtensions :: Frontend, enhancement, P3)

Product:
WebExtensions
Component:
Frontend
Type:
enhancement

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: gkw, Assigned: zombie)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved]triaged)

Attachments

(2 files, 2 obsolete files)

tools-menu.png
26.31 KB, image/png
designakt
: ui-review+
Details
Bug 1268020 - Implement "tools_menu" context
59 bytes, text/x-review-board-request
kmag
: review+
Details 
It should be possible to create a new menu item using WebExtensions. (Not context menus)

e.g. Add a new entry under View, similar to the ViewAbout extension. [1]

[1] https://addons.mozilla.org/en-US/firefox/addon/viewabout/
OS: Unspecified → All
Hardware: Unspecified → All
Whiteboard: [design-decision-needed]triaged
This seems a perfectly reasonable thing to do. Can we re-use the context menu code and API for this?

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus

It seems mostly a matter of placing the items somewhere in the menus as opposed to the context menu.
There's a couple of concerns around this:
* adding in menu items is acceptable
* ideally menu items should occur in a well know location (maybe at the bottom, or only in tools), if possible
* modifying or deleting menu items (other than those created by the add-on) is not acceptable
* we should make clear that the menu item is related to the add-on so that end-users can be clear that it is caused by and related to an add-on, it was suggested we follow the same plan as context menus (the button is the add-on name, and if you have more than one item they become sub menus of the addon name, see example: https://www.dropbox.com/s/z96m970rhgo2yrm/Screenshot%202016-07-27%2011.32.58.png?dl=0

With those restrictions in mind, this seems like a idea.
Whiteboard: [design-decision-needed]triaged → [design-decision-approved]triaged
(In reply to Andy McKay [:andym] from comment #2)
> With those restrictions in mind, this seems like a idea.

..this seems like a *good* idea.
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Priority: -- → P3
webextensions: --- → ?
Blocks: 1311472
I see several approaches from the API design point of view:

1) Use the browser.contextMenus API, with a new context: "mainmenu"
2) Create browser.menus API, different from contextMenus (for some reason)
3) Create browser.menus, the same as contextMenus, use it just for context: "mainmenu"
4) browser.menus, document it as the preferred for all menus, make contextMenus just an alias.

Unfortunately, neither of these seems as the obvious choice (though I vote against both 2 and 3).
1) or 4) seems like the way to go. browser.contextMenus mainmenu seems a bit off since its not a context menu but avoids making yet another namespace for the API.
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Attached image tools-menu.png — 
Hi Markus, this is very similar to every other way we allow extensions to add (context) menu items, but I still wanted to keep you in the loop:

 - There is a separator before any web extension items
 - Each extension gets to add a maximum of one menu item
 - If it tries to add more, they get collected into a submenu with extension's name
 - Extension icon can be used as menu item icon (not shown, this is from a test)
Attachment #8829847 - Flags: ui-review?(mjaritz)
Blocks: 1333403
(In reply to Andy McKay [:andym] from comment #6)
> 1) or 4) seems like the way to go. browser.contextMenus mainmenu seems a bit
> off since its not a context menu but avoids making yet another namespace for
> the API.

Ok, since work needed for 1 is a proper subset of work needed for 4, I'm scoping this down, and filed bug 1333403 so that API design can be discussed in the meeting.
Summary: Add menu item using WebExtensions → Implement "main_menu" context
Comment on attachment 8829847 [details]
tools-menu.png

Thanks for looping me in. This looks fine.
(Assuming entries can only be added to the end of the tools menu, not to any other menu item.)
Attachment #8829847 - Flags: ui-review?(mjaritz) → ui-review+
Attachment #8829254 - Flags: review?(aswan)
I'm not sure we should use the contextMenus API for this...
Comment on attachment 8829254 [details]
Bug 1268020 - Implement "tools_menu" context

https://reviewboard.mozilla.org/r/106376/#review108140

I don't see any obvious problems but I don't know this code well enough to do an in-depth review, sorry.  I'm going to punt this one back to Kris.
Attachment #8829254 - Flags: review?(aswan)
Attachment #8829254 - Flags: review?(kmaglione+bmo)
Comment on attachment 8829254 [details]
Bug 1268020 - Implement "tools_menu" context

https://reviewboard.mozilla.org/r/106376/#review118898

I'm still a bit leery about using the contextMenus API for this, but I guess I can live with it. r=me with the changes below.

::: browser/components/extensions/ext-contextMenus.js:311
(Diff revision 3)
> +  if (contextData.onTab) {
> +    contexts.add("tab");
> +  }
> +
> +  if (contextData.inMainMenu) {
> +    contexts.add("main_menu");

Please call this "tools_menu". Something similar for the context data, too.

::: browser/components/extensions/test/browser/head.js:274
(Diff revision 3)
> +  const node = win.document.getElementById("tools-menu");
> +  const menu = win.document.getElementById("menu_ToolsPopup");
> +  const shown = BrowserTestUtils.waitForEvent(menu, "popupshowing");
> +  if (AppConstants.platform === "macosx") {
> +    // We can't open the main menu on OSX, so mocking instead.
> +    menu.dispatchEvent(new MouseEvent("popupshowing"));

Hmm... Well, I guess as long as it works...

Please make sure to dispatch the appropriate "popupshown", "popuphiding", and "popuphidden" events afterwards, though.
Attachment #8829254 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
Summary: Implement "main_menu" context → Implement "tools_menu" context
(In reply to Kris Maglione [:kmag] from comment #14)
> I'm still a bit leery about using the contextMenus API for this,

We can potentially disallow just the "tools_menu" on browser.contextMenus if go ahead with browser.menus (bug 1333403), but if you think that is worth doing (I don't FWIW *), we (you) need to make that decision *now*, as we shouldn't ship in this state then.


(* shipping two just ever-so slightly different APIs for some "API purity" reasons is more trouble, collectively, than it's worth IMO)
Flags: needinfo?(kmaglione+bmo)
Keywords: checkin-needed
webextensions: ? → ---
(In reply to Tomislav Jovanovic :zombie from comment #16)
> We can potentially disallow just the "tools_menu" on browser.contextMenus if
> go ahead with browser.menus (bug 1333403), 

"Yes"
Flags: needinfo?(kmaglione+bmo)
Attachment #8829254 - Attachment is obsolete: true
Attachment #8882010 - Flags: review?(kmaglione+bmo)
Comment on attachment 8882010 [details]
Bug 1268020 - Implement "tools_menu" context

https://reviewboard.mozilla.org/r/153096/#review164406

::: browser/components/extensions/test/browser/browser_ext_menus.js:6
(Diff revision 1)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
> +/* global ExtensionTestUtils */

This shouldn't be necessary.
Attachment #8882010 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8882010 [details]
Bug 1268020 - Implement "tools_menu" context

https://reviewboard.mozilla.org/r/153096/#review164406

> This shouldn't be necessary.

Yeah, I was fighting eslint that day, and forgot to take it out.
Attachment #8889171 - Attachment is obsolete: true
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/509be072f2ea
Implement "tools_menu" context r=kmag
https://hg.mozilla.org/mozilla-central/rev/509be072f2ea
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Keywords: dev-doc-needed
I've added "tools_menu" here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/create, and updated the example to add a tools menu item (and icons): https://github.com/mdn/webextensions-examples/tree/master/menu-demo

Let me know if we need anything else though!
Flags: needinfo?(tomica)
(In reply to Will Bamberg [:wbamberg] from comment #25)
> I've added "tools_menu" here:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/create,
> and updated the example to add a tools menu item (and icons):
> https://github.com/mdn/webextensions-examples/tree/master/menu-demo
> 
> Let me know if we need anything else though!

Adding the notes to https://github.com/mdn/browser-compat-data/blob/master/webextensions/javascript-apis.json.
(In reply to YF (Yang) from comment #26)
> (In reply to Will Bamberg [:wbamberg] from comment #25)
> > I've added "tools_menu" here:
> > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/create,
> > and updated the example to add a tools menu item (and icons):
> > https://github.com/mdn/webextensions-examples/tree/master/menu-demo
> > 
> > Let me know if we need anything else though!
> 
> Adding the notes to
> https://github.com/mdn/browser-compat-data/blob/master/webextensions/
> javascript-apis.json.

Compat data is under ContextType: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/ContextType
The tools_menu ContextType is only available under the menus namespace, not under contextMenus. This has been documented at the menus.create docs, but not at ContextType.

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/ContextType

It would probably be good to add it both at the "tools_menu" description in the main article, and the compat table at the bottom.
Fair enough Rob, I've updated the page and filed https://github.com/mdn/browser-compat-data/pull/415.
Agreed, looks good now, thanks Rob and Will.
Flags: needinfo?(tomica)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.