1268020 - Implement "tools_menu" context

Status: RESOLVED FIXED

Description

[Gary Kwong [:gkw] [:nth10sd]]

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/

Comment 1

[Andy McKay]

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.

Comment 2

[Andy McKay]

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.

Comment 3

[Andy McKay]

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

Comment 4

[Tomislav Jovanovic :zombie]

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

Comment 5

[Tomislav Jovanovic :zombie]

Attachment: Bug 1268020 - Implement "tools_menu" context

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

Comment 6

[Andy McKay]

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.

Comment 7

[Tomislav Jovanovic :zombie]

Comment on attachment 8829254 [details]
Bug 1268020 - Implement "tools_menu" context

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

Comment 8

[Tomislav Jovanovic :zombie]

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

Comment 9

[Tomislav Jovanovic :zombie]

Comment on attachment 8829254 [details]
Bug 1268020 - Implement "tools_menu" context

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

Comment 10

[Tomislav Jovanovic :zombie]

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

Comment 11

[Markus Jaritz [:designakt] (UX)]

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

Comment 12

[Kris Maglione [:kmag]]

I'm not sure we should use the contextMenus API for this...

Comment 13

[Andrew Swan [:aswan]]

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.

Comment 14

[Kris Maglione [:kmag]]

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.

Comment 15

[Tomislav Jovanovic :zombie]

Comment on attachment 8829254 [details]
Bug 1268020 - Implement "tools_menu" context

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

Comment 16

[Tomislav Jovanovic :zombie]

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

Comment 17

[Tomislav Jovanovic :zombie]

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

Comment 18

[Tomislav Jovanovic :zombie]

Attachment: Bug 1268020 - Implement "tools_menu" context

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

Comment 19

[Kris Maglione [:kmag]]

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.

Comment 20

[Tomislav Jovanovic :zombie]

Attachment: Bug 1268020 - Implement "tools_menu" context

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

Comment 21

[Tomislav Jovanovic :zombie]

Comment on attachment 8882010 [details]
Bug 1268020 - Implement "tools_menu" context

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

Comment 22

[Tomislav Jovanovic :zombie]

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.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/509be072f2ea
Implement "tools_menu" context r=kmag

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/509be072f2ea

Comment 25

[Will Bamberg [:wbamberg]]

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!

Comment 26

[YF (Yang)]

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

Comment 27

[Will Bamberg [:wbamberg]]

(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

Comment 28

[Rob Wu [:robwu]]

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.

Comment 29

[Will Bamberg [:wbamberg]]

Fair enough Rob, I've updated the page and filed https://github.com/mdn/browser-compat-data/pull/415.

Comment 30

[Tomislav Jovanovic :zombie]

Agreed, looks good now, thanks Rob and Will.