Closed
Bug 1268020
Opened 5 years ago
Closed 4 years ago
Implement "tools_menu" context
Categories
(WebExtensions :: Frontend, enhancement, P3)
WebExtensions
Frontend
Tracking
(firefox56 fixed)
RESOLVED
FIXED
mozilla56
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)
26.31 KB,
image/png
|
designakt
:
ui-review+
|
Details |
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/
![]() |
Reporter | |
Updated•5 years ago
|
OS: Unspecified → All
Hardware: Unspecified → All
Updated•5 years ago
|
Whiteboard: [design-decision-needed]triaged
Comment 1•5 years ago
|
||
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•5 years ago
|
||
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
Comment 3•5 years ago
|
||
(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•4 years ago
|
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Priority: -- → P3
Updated•4 years ago
|
webextensions: --- → ?
Assignee | ||
Comment 4•4 years 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•4 years 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•4 years ago
|
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•4 years ago
|
||
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 | ||
Comment 10•4 years 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 11•4 years ago
|
||
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•4 years ago
|
Attachment #8829254 -
Flags: review?(aswan)
Comment 12•4 years ago
|
||
I'm not sure we should use the contextMenus API for this...
Comment 13•4 years 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•4 years ago
|
Attachment #8829254 -
Flags: review?(kmaglione+bmo)
Comment 14•4 years 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•4 years ago
|
Keywords: checkin-needed
Summary: Implement "main_menu" context → Implement "tools_menu" context
Assignee | ||
Comment 16•4 years 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•4 years ago
|
webextensions: ? → ---
Assignee | ||
Comment 17•4 years 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•4 years ago
|
Attachment #8829254 -
Attachment is obsolete: true
Assignee | ||
Updated•4 years ago
|
Attachment #8882010 -
Flags: review?(kmaglione+bmo)
Comment 19•4 years 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•4 years 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•4 years ago
|
Attachment #8889171 -
Attachment is obsolete: true
Comment 23•4 years ago
|
||
Pushed by tomica@gmail.com: https://hg.mozilla.org/integration/autoland/rev/509be072f2ea Implement "tools_menu" context r=kmag
Comment 24•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/509be072f2ea
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•4 years ago
|
Keywords: dev-doc-needed
Comment 25•3 years ago
|
||
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)
Comment 26•3 years ago
|
||
(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•3 years ago
|
||
(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•3 years ago
|
||
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•3 years ago
|
||
Fair enough Rob, I've updated the page and filed https://github.com/mdn/browser-compat-data/pull/415.
Assignee | ||
Comment 30•3 years ago
|
||
Agreed, looks good now, thanks Rob and Will.
Flags: needinfo?(tomica)
Updated•3 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Updated•3 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•