Implement "tools_menu" context

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: Frontend
P3
enhancement
RESOLVED FIXED
a year ago
14 days ago

People

(Reporter: gkw, Assigned: zombie)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla56
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [design-decision-approved]triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
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/
(Reporter)

Updated

a year ago
OS: Unspecified → All
Hardware: Unspecified → All

Updated

a year ago
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.

Updated

10 months ago
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Priority: -- → P3

Updated

8 months ago
webextensions: --- → ?

Updated

7 months ago
Blocks: 1311472
(Assignee)

Comment 4

7 months ago
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 hidden (mozreview-request)

Comment 6

7 months ago
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)

Updated

7 months ago
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 8

7 months ago
Created attachment 8829847 [details]
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)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Blocks: 1333403
(Assignee)

Comment 10

7 months ago
(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+
(Assignee)

Updated

7 months ago
Attachment #8829254 - Flags: review?(aswan)
I'm not sure we should use the contextMenus API for this...

Comment 13

7 months ago
mozreview-review
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)

Updated

7 months ago
Attachment #8829254 - Flags: review?(kmaglione+bmo)

Comment 14

6 months ago
mozreview-review
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+
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Keywords: checkin-needed
Summary: Implement "main_menu" context → Implement "tools_menu" context
(Assignee)

Comment 16

6 months ago
(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

Updated

5 months ago
webextensions: ? → ---
(Assignee)

Comment 17

3 months ago
(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)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8829254 - Attachment is obsolete: true
(Assignee)

Updated

2 months ago
Attachment #8882010 - Flags: review?(kmaglione+bmo)

Comment 19

a month ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

26 days ago
mozreview-review-reply
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.
(Assignee)

Updated

26 days ago
Attachment #8889171 - Attachment is obsolete: true

Comment 23

26 days ago
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/509be072f2ea
Implement "tools_menu" context r=kmag

Comment 24

26 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/509be072f2ea
Status: ASSIGNED → RESOLVED
Last Resolved: 26 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Depends on: 1385528

Updated

14 days ago
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.