| Submitter | Diff | Changes | Open Issues | Last Updated |
|---|---|---|---|---|
| Loading... | ||||
| Error loading review requests: | ||||
|
26.31 KB,
image/png
|
designakt
:
ui-review+
|
Details |
|
59 bytes,
text/x-review-board-request
|
kmag
:
review+
|
Details | Review |
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
|
||
|
||
Updated•a year ago
|
||
|
|
||
Comment 1•a year 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•a year 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.
|
|
||
Comment 3•a year 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•10 months ago
|
||
|
|
||
Updated•8 months ago
|
||
|
(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
|
||
| 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)
| Comment hidden (mozreview-request) |
|
|
(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.
|
||
Comment 11•7 months ago
|
||
|
|
(Assignee) | |
Updated•7 months ago
|
||
|
||
Comment 12•7 months ago
|
||
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.
|
|
||
Updated•7 months ago
|
||
|
|
||
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.
| Comment hidden (mozreview-request) |
|
|
(Assignee) | |
Updated•6 months ago
|
||
|
|
(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)
|
|
||
Updated•5 months ago
|
||
|
|
(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"
| Comment hidden (mozreview-request) |
|
|
(Assignee) | |
Updated•2 months ago
|
||
|
|
(Assignee) | |
Updated•2 months ago
|
||
|
|
||
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.
| 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
|
||
|
||
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
|
|
||
Updated•14 days ago
|
||
Description
•