Closed
Bug 1321544
Opened 8 years ago
Closed 7 years ago
Set a different icon for a contextMenus item
Categories
(WebExtensions :: Frontend, defect, P3)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla56
People
(Reporter: hi, Assigned: swapneshks, Mentored)
References
Details
(Keywords: dev-doc-complete, good-first-bug, Whiteboard: [design-decision-approved][contextMenus] triaged)
Attachments
(3 files, 3 obsolete files)
14.99 KB,
image/png
|
markus
:
ui-review+
|
Details |
59 bytes,
text/x-review-board-request
|
zombie
:
review+
|
Details |
20.64 KB,
text/plain
|
Details |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20161201030205
Expected results:
I understand from the documentation [1] that it is not possible to have different icons for different menu items.
But is there a way to have an icon for the addon, and another one for the menu items?
At the moment we are using two different icons for gtranslate [2], because the logo doesn’t represent the action and is too noisy to be used in that context.
[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus
[2] https://addons.mozilla.org/firefox/addon/gtranslate/
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
Updated•8 years ago
|
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Summary: (WebExtensions) Set a different icon for a contextMenus item → Set a different icon for a contextMenus item
Whiteboard: [design-decision-needed]
Comment 1•8 years ago
|
||
Unless I'm mistaken, I think I would need this to port Context Search, which expands the context menu to contain the search plugins from the Search Bar (CTRL+K).
I would be looking to replicate the current UI:
http://www.basson.at/images/addons/contextsearch.png
Updated•8 years ago
|
Whiteboard: [design-decision-needed] → [design-decision-needed][contextMenus]
Comment 3•8 years ago
|
||
Hi Pierre, this has been added to the agenda for the May 2 WebExtensions Triage meeting. Will you be able to join us?
Wiki: https://wiki.mozilla.org/Add-ons/Contribute/Triage#Next_Meeting
Agenda: https://docs.google.com/document/d/1vf8AaW8tKKbMn4KhsqEYhrYqVTUGaERHQmzanEp2Cls/edit#
Comment 5•8 years ago
|
||
It sounds good to me that in the sub-menu (as shown in the screenshot in comment 1) should have custom icons. We'd like to retain that the top level menu item still has the add-on icon on it so we clearly indicate to the user where the context menu item is coming from. Let's do it.
Priority: -- → P3
Whiteboard: [design-decision-needed][contextMenus] → [design-decision-approved][contextMenus]
Updated•8 years ago
|
Assignee | ||
Comment 6•8 years ago
|
||
Hi.. Can I take up this bug? (I am a beginner and have worked only on 8-9 moz bugs).
From an initial glance, it appears that the end result should be a combination of the following:
1. Menu snapshot in the documentation ([1] in the bug Description)
2. Screenshot in comment#1
Flags: needinfo?(tomica)
Comment 7•8 years ago
|
||
Hi Swapnesh, sure you can.
That sounds about right. Are you familiar with mozreview [1]? Let me know if you have any questions.
1) https://reviewboard.mozilla.org/
Assignee: nobody → swapneshks
Flags: needinfo?(tomica)
Assignee | ||
Comment 8•8 years ago
|
||
(In reply to Tomislav Jovanovic :zombie from comment #7)
> Are you familiar with mozreview [1]? Let me know
> if you have any questions.
>
> 1) https://reviewboard.mozilla.org/
Yes, I have learnt it just a few days back. :)
I have a few doubts regarding which files to modify. After digging a bit, I found the following files:
http://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/context_menus.json
http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#347
http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#630
I'm a bit unsure about how the frontend has to be modified to include the icon and what changes need to be made. Could you help me out a bit here?
Flags: needinfo?(tomica)
Comment 9•8 years ago
|
||
Yes, you've got the right files. Specifically, [1] is where we set the current icon on the top level menu item for each extension, and you would have to do something similar around [2], thought the icon will from `item`, not the manifest.
1) http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#57
2) http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#157
Flags: needinfo?(tomica)
Comment 10•8 years ago
|
||
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8869744 [details]
Bug 1321544 - Support icons for context menu items in MozReview;
In this patch, I've made only one addition and I feel that something more needs to be added.
Also, I tried testing the changes but couldn't figure out how to, i.e., how do I test the UI? (I found a mochitest browser/components/extensions/test/browser/browser_ext_browserAction_contextMenu.js that calls contextMenus.create, but how to replicate similar behaviour in a firefox build?)
Comment 13•8 years ago
|
||
Yeah, this will not be enough. First of all, you need to determine the correct icon dimensions, and to resolve the icon URL, similar to the code around line 60 I pointed you at [1].
For testing, you will either need an extension that creates several menu items with different icons, or even better, to do it from a mochitest.
After you make your code changes, you will need to build firefox, and then run the test with:
mach build faster
mach mochitest browser/components/extensions/test/browser/browser_ext_browserAction_contextMenu.js
(your test shouldn't really end up in that file, but it might be easier to start with modifying an existing test)
1) http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#57
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8869744 [details]
Bug 1321544 - Support icons for context menu items in MozReview;
https://reviewboard.mozilla.org/r/141314/#review144934
::: browser/components/extensions/ext-contextMenus.js:203
(Diff revision 1)
> element.setAttribute("id",
> `${makeWidgetId(item.extension.id)}_${item.id}`);
> }
>
> + if (item.icon) {
> + element.setAttribute("image", "url(" + item.icon + ")");
This is not CSS, no need for `url()`, but also, you need to resolve the icon URI.
Attachment #8869744 -
Flags: review?(tomica) → review-
Updated•8 years ago
|
Whiteboard: [design-decision-approved][contextMenus] → [design-decision-approved][contextMenus] triaged
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8869744 [details]
Bug 1321544 - Support icons for context menu items in MozReview;
I did some logging and according to https://pastebin.mozilla.org/9022817, the contextMenu items are stored as item.children and so, the changes in this patch are based on the same fact. Although, I am not sure how to set icon for each contextMenu item.
Comment 17•8 years ago
|
||
mozreview-review |
Comment on attachment 8869744 [details]
Bug 1321544 - Support icons for context menu items in MozReview;
https://reviewboard.mozilla.org/r/141314/#review147356
This is in the right direction, just need to correct a few things, and add tests after you get it going.
::: browser/components/extensions/ext-contextMenus.js:202
(Diff revision 2)
> if (item.id && item.extension && item.extension.id) {
> element.setAttribute("id",
> `${makeWidgetId(item.extension.id)}_${item.id}`);
> }
>
> - if (item.icon) {
> + for(let child of item.children) {
This method is called for each menu item, so you shouldn't go through the child items here, just work with `item.icon`.
::: browser/components/extensions/ext-contextMenus.js:204
(Diff revision 2)
> + let parentWindow = contextData.menu.ownerGlobal;
> + let extension = item.extension;
> +
> + let {icon} = IconDetails.getPreferredIcon(item.children[0].icons, extension,
> + 16 * parentWindow.devicePixelRatio);
> +
> + let resolvedURL = item.extension.baseURI.resolve(icon);
Let's extract this in a helper method instead of duplicating the code.
::: browser/components/extensions/schemas/context_menus.json:145
(Diff revision 2)
> "id": {
> "type": "string",
> "optional": true,
> "description": "The unique ID to assign to this item. Mandatory for event pages. Cannot be the same as another ID for this extension."
> },
> + "icons": {
Lets do something simpler and similar to the manifest: http://searchfox.org/mozilla-central/source/toolkit/components/extensions/schemas/manifest.json#81
::: browser/components/extensions/test/browser/browser-common.ini:69
(Diff revision 2)
> [browser_ext_devtools_network.js]
> [browser_ext_devtools_page.js]
> [browser_ext_devtools_panel.js]
> [browser_ext_geckoProfiler_symbolicate.js]
> [browser_ext_getViews.js]
> +[browser_ext_icon_contextMenu.js]
browser_ext_contextMenus_icons.js please, and we already that file, so you can just add your test to it.
::: browser/components/extensions/test/browser/browser_ext_icon_contextMenu.js:8
(Diff revision 2)
> + "browser_action": {
> + "default_popup": "popup.html",
> + },
Let's not complicate with `browser_action` stuff, and instead base your test on the one from `browser_ext_contextMenus_icon.js`.
::: browser/components/extensions/test/browser/browser_ext_icon_contextMenu.js:35
(Diff revision 2)
> + title: "Click me!",
> + icons: {
> + 48 : "moz-anno:favicon:https://www.mozilla.org/media/img/firefox/favicon.dc6635050bf5.ico",
> + },
> + contexts: ["all"],
> + });
You need to add at least two items to get a submenu effect, and not have the main item use your extension icon.
Attachment #8869744 -
Flags: review?(tomica) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8869744 [details]
Bug 1321544 - Support icons for context menu items in MozReview;
I've incorporated changes as per comment#17 in this patch expect the test update to have submenu, which I am not very sure of how to add. Could you guide me here on how to do that, as I am getting a bit confused regarding the implementation?
Flags: needinfo?(tomica)
Comment 20•8 years ago
|
||
mozreview-review |
Comment on attachment 8869744 [details]
Bug 1321544 - Support icons for context menu items in MozReview;
https://reviewboard.mozilla.org/r/141314/#review148024
::: commit-message-1e2fe:1
(Diff revision 3)
> +Bug 1321544 - Set different icon for contextMenus item in MozReview; r?zombie
nit: better message might be "Support icons for context menu items"
::: browser/components/extensions/ext-contextMenus.js:195
(Diff revision 3)
> -
> - let {icon} = IconDetails.getPreferredIcon(item.children[0].icons, extension,
> - 16 * parentWindow.devicePixelRatio);
> -
> - let resolvedURL = item.extension.baseURI.resolve(icon);
> - element.setAttribute("image", resolvedURL);
> + element.setAttribute("image", resolvedURL);
It looks like you would also need to set the the class to `menu-iconic`/`menuitem-iconic`.
::: browser/components/extensions/ext-contextMenus.js:279
(Diff revision 3)
> },
>
> itemsToCleanUp: new Set(),
> };
>
> +function getResolvedImageURL(item, contextData, icons) {
nit: `getResolvedIconURL`
also, `item` is only used to get the `extension`, so let's pass that directly as an argument instead.
::: browser/components/extensions/ext-contextMenus.js:284
(Diff revision 3)
> +function getResolvedImageURL(item, contextData, icons) {
> + let parentWindow = contextData.menu.ownerGlobal;
> + let extension = item.extension;
> +
> + let {icon} = IconDetails.getPreferredIcon(icons, extension,
> + 16 * parentWindow.devicePixelRatio);
nit: please align this with the `icons` argument.
::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:5
(Diff revision 3)
> /* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> /* vim: set sts=2 sw=2 et tw=80: */
> "use strict";
>
> add_task(async function() {
Let's name this function `test_root_icon`, and add your test as a separate function in this file, instead of modifying this one.
Attachment #8869744 -
Flags: review?(tomica) → review-
Comment 21•8 years ago
|
||
(In reply to Swapnesh Kumar Sahoo [:swapneshks] from comment #19)
> update to have submenu, which I am not very sure of how to add. Could you
> guide me here on how to do that,
Try by copying the existing test function in that file, and modifying it to add multiple menu items, each with a different icon.
Updated•8 years ago
|
Flags: needinfo?(tomica)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•8 years ago
|
||
Comment on attachment 8869744 [details]
Bug 1321544 - Support icons for context menu items in MozReview;
After the changes in this patch, I am able to see the icons for the child menu items. :)
However, the output on the console for the test is not very neat. I guess the new part of the test needs refining. Please let me know how the same can be done.
Flags: needinfo?(tomica)
Comment 24•8 years ago
|
||
mozreview-review |
Comment on attachment 8869744 [details]
Bug 1321544 - Support icons for context menu items in MozReview;
https://reviewboard.mozilla.org/r/141312/#review149878
This is looking good. After you are done with the next version, can you please also take a screenshot and attach it to ask for ux-review from :designakt.
::: browser/components/extensions/ext-contextMenus.js:193
(Diff revision 4)
> if (item.id && item.extension && item.extension.id) {
> element.setAttribute("id",
> `${makeWidgetId(item.extension.id)}_${item.id}`);
> }
>
> - for(let child of item.children) {
> + if(item.icons) {
nit: space after `if`. you should run eslint[1] on your code, it will tell you about these things.
1) https://developer.mozilla.org/en-US/docs/ESLint
::: browser/components/extensions/ext-contextMenus.js:196
(Diff revision 4)
> - let {icon} = IconDetails.getPreferredIcon(item.children[0].icons, extension,
> - 16 * parentWindow.devicePixelRatio);
> + if (element.localName == "menu") {
> + element.setAttribute("class", "menu-iconic");
> + } else if (element.localName == "menuitem") {
> + element.setAttribute("class", "menuitem-iconic");
> + }
Now this is repeated, so maybe we should include it in the `getResolvedIcon` function (and rename it, and make it take an element, and a method of `gMenuBuilder` and ...).
::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:76
(Diff revision 4)
> +
> +add_task(async function test_child_icon() {
> + let tab1 = await BrowserTestUtils.openNewForegroundTab(gBrowser,
> + "http://mochi.test:8888/browser/browser/components/extensions/test/browser/context.html?test=icons");
> +
> + let encodedImageData = "iVBORw0KGgoAAAANSUhEUgAAACQAAAAkCAYAAADhAJiYAAAC4klEQVRYhdWXLWzbQBSADQtDAwsHC1tUhUxqfL67lk2tdn+OJg0ODU0rLByqgqINBY6tmlbn7LMTJ5FaFVVBk1G0oUGjG2jT2Y7jxmmcbU/6iJ+f36fz+e5sGP9riCGm9hB37RG+scd4Yo/wsDXCZyIE2xuXsce4bY+wXkAsQtzYmExrfFgvkJkRbkzo1ehoxx5iXcgI/9iYUGt8WH9MqDXEcmNChmEYrRCf2SHWeYgQx3x0tLNRIeKQLTtEFyJEep4NTuhk8BC+yMrwEE3+iozo42d8gK7FAOkMsRiiN8QhW2ttSK5QTfRRV4QoymVeJMvPvDp7gCZigD613MN6yRFA3SWarow9QB9LCfG+NeF9qCtjAKOSQjCqVKhfVsiHEQ+grgx/lRGqUihAc1uL8EFD+KCRO+GrF4J61phcoRoPoEzkYhZYpykh5sMb7kOdIeY+jHKur4QI4Feh4AFX1nVeLxrAvQchGsBz5ls6wa2QdwcvIcE2863bTH79KOvsz/uUYJsp+J0pSzNlDckVqqVGUAF+n6uS7txcOl6wot4JVy70ufDLy4pWLUQVPE81pRI0mGe9oxLMHSeohHvMs/STUNaUK6vDPCvOyxMFDx4achehRDJmHnydnkPww5OFfLxrGIZBFDyYl4LpMzlTQFIP6AQx86w2UeYBccFpJrcKv5L9eGDtUAU6RIELqsB74uynjy/UBRF1gS5BTFxwQT1wTiXoUg9MH7m/3NZRRoi5IJytUbMgzv4Wc832+oQkiKgEehmyMkkpKsFkQV11QsRJL5rJYBLItQgRaUZEmnoZXsomz3vGiWw+I9KMF9SVFOqZEemZekli1jN3U/UOqhHHvC6oWWGElhfSpGdOk6+O9prdwvtLj5BjRsQxdRnot+Zeifpy/2/0stktKTRNLmbk0mwXyl8253fyojj+8rxOHNAhjjm5n0/5OOCGOKBzkrMO0Z75lvSAzKlrF32Z/3z8BqLAn+yMV7VhAAAAAElFTkSuQmCC";
You can use two or three simpler images instead of this one. Just create a solid red, green and blue PNG each, and use a tool to encode it:
http://jpillora.com/base64-encoder/
::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:121
(Diff revision 4)
> + browser.test.notifyPass("contextmenus-icons");
> + },
> + });
> +
> + let confirmContextMenuIcon = (childElement) => {
> + let expectedURL = new RegExp(String.raw`^moz-extension://[^/]+/extension\.png$`);
You can simply test `imageUrl.endsWith("extension.png")`.
::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:131
(Diff revision 4)
> + await extension.startup();
> + await extension.awaitFinish("contextmenus-icons");
> +
> + let extensionMenu = await openExtensionContextMenu();
> +
> + await openExtensionContextMenu();
Why are you calling this twice?
::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:146
(Diff revision 4)
> +
> + await extension.unload();
> +});
> +
> +function run_test() {
> + run_next_test();
What is this?
Comment 25•8 years ago
|
||
(In reply to Swapnesh Kumar Sahoo [:swapneshks] from comment #23)
> However, the output on the console for the test is not very neat. I guess
> the new part of the test needs refining. Please let me know how the same can
> be done.
I don't understand the question. What exactly are you having problems with?
Flags: needinfo?(tomica)
Updated•8 years ago
|
Attachment #8869744 -
Flags: review?(tomica)
Assignee | ||
Comment 26•8 years ago
|
||
Requesting UX review for context menu item icon...
Attachment #8874958 -
Flags: review?(mjaritz)
Assignee | ||
Comment 27•8 years ago
|
||
(In reply to Tomislav Jovanovic :zombie from comment #24)
> > + let extensionMenu = await openExtensionContextMenu();
> > +
> > + await openExtensionContextMenu();
>
> Why are you calling this twice?
Actually, it got left by mistake.
> :::
> browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:
> 146
> (Diff revision 4)
> > +
> > + await extension.unload();
> > +});
> > +
> > +function run_test() {
> > + run_next_test();
>
> What is this?
I was referring to xpcshell test wiki and by mistake added according to that.
> > However, the output on the console for the test is not very neat. I guess
> > the new part of the test needs refining. Please let me know how the same can
> > be done.
>
> I don't understand the question. What exactly are you having problems with?
I was seeing a hang in the test. But I guess it was due to openExtensionContextMenu() being called twice.
The new review for some reason got published on https://reviewboard.mozilla.org/r/141312/diff/5#index_header instead of https://reviewboard.mozilla.org/r/141314/diff/#index_header. Let me know what can be done to correct the same.
I've also requested a UX review as asked.
Flags: needinfo?(tomica)
Comment 29•8 years ago
|
||
Comment on attachment 8874958 [details]
context_menu_item_icon_test.png
This looks good. Thanks.
ui-r+, assuming it will look very similar on other platforms.
Attachment #8874958 -
Flags: review?(mjaritz) → ui-review+
Comment 30•8 years ago
|
||
(In reply to Swapnesh Kumar Sahoo [:swapneshks] from comment #27)
> The new review for some reason got published on
> https://reviewboard.mozilla.org/r/141312/diff/5#index_header instead of
> https://reviewboard.mozilla.org/r/141314/diff/#index_header. Let me know
> what can be done to correct the same.
I think the problem is that you are not preserving the MozReview-Commit-ID: between your patches.
Let's leave that review request, and start a second one, with something like this:
hg push -c . --reviewid bz://1321544/swapneshks2 review
Flags: needinfo?(tomica)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8869744 -
Attachment is obsolete: true
Assignee | ||
Comment 32•8 years ago
|
||
(In reply to Tomislav Jovanovic :zombie from comment #30)
> (In reply to Swapnesh Kumar Sahoo [:swapneshks] from comment #27)
> > The new review for some reason got published on
> > https://reviewboard.mozilla.org/r/141312/diff/5#index_header instead of
> > https://reviewboard.mozilla.org/r/141314/diff/#index_header. Let me know
> > what can be done to correct the same.
>
> I think the problem is that you are not preserving the MozReview-Commit-ID:
> between your patches.
>
> Let's leave that review request, and start a second one, with something like
> this:
>
> hg push -c . --reviewid bz://1321544/swapneshks2 review
Thanks for the guidance.. will keep mentioned things in mind in future.
New review request has been created.
Comment 33•8 years ago
|
||
mozreview-review |
Comment on attachment 8875368 [details]
Bug 1321544 - Support icons for context menu items in MozReview;
https://reviewboard.mozilla.org/r/146790/#review150954
The code looks good, but this is still somehow showing some extra stuff in the commit (including `browser_ext_icon_contextMenu.js`). Perhaps you need rebase this commit on top of central or similar?
Attachment #8875368 -
Flags: review?(tomica)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8875368 -
Attachment is obsolete: true
Assignee | ||
Comment 35•8 years ago
|
||
Comment on attachment 8875809 [details]
Bug 1321544 - Support icons for context menu items;
My local repository got messed up, so I cleaned it up and made a fresh commit & review request. Let me know if things look okay.
Comment 36•8 years ago
|
||
mozreview-review |
Comment on attachment 8875809 [details]
Bug 1321544 - Support icons for context menu items;
https://reviewboard.mozilla.org/r/147226/#review151498
Good work, almost there.
I'd like to test the case when there is only one menu item with a `icon` property, it should still get the extension (black) icon.
And after you update, please also add `mixedpuppy` for final review.
::: commit-message-6491f:1
(Diff revision 1)
> +Bug 1321544 - Support icons for context menu items in MozReview; r?zombie
Please remove "in MozReview" from the commit message.
::: browser/components/extensions/ext-contextMenus.js:278
(Diff revision 1)
> item.remove();
> }
> this.itemsToCleanUp.clear();
> },
>
> + setMenuItemIcon(element, extension, contextData, icons) {
nit: please move this above the `handleEvent` method
::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:73
(Diff revision 1)
> + let tab1 = await BrowserTestUtils.openNewForegroundTab(gBrowser,
> + "http://mochi.test:8888/browser/browser/components/extensions/test/browser/context.html?test=icons");
Please define a `PAGE` constant at the top of this file, and use it in both tests. Also, there's only one tab.
::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:111
(Diff revision 1)
> + id: "contextmenu-child1",
> + icons: {
> + 18: "red_icon.png",
> + },
> + });
> +
You can add just the first (red) menu item at start, open the menu and check that it has a black icon.
After that, send the message back to the extension to add the other two items (green and blue), and open the menu again to test all three (as you already are).
::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:140
(Diff revision 1)
> + await openExtensionContextMenu();
> +
> + let contextMenu = document.getElementById("contentAreaContextMenu");
I think the `openExtensionContextMenu()` above returns the context menu, so you can just set it there.
Attachment #8875809 -
Flags: review?(tomica)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 38•8 years ago
|
||
(In reply to Tomislav Jovanovic :zombie from comment #36)
>
> :::
> browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:
> 140
> (Diff revision 1)
> > + await openExtensionContextMenu();
> > +
> > + let contextMenu = document.getElementById("contentAreaContextMenu");
>
> I think the `openExtensionContextMenu()` above returns the context menu, so
> you can just set it there.
On trying out the following:
> let contextMenu = await openExtensionContextMenu();
I received "contextMenu is null" error while trying to use it. So, I went ahead with the older approach.
Rest, changes asked in comment#36 have been incorporated and new review request has been published.
Comment 39•8 years ago
|
||
mozreview-review |
Comment on attachment 8875809 [details]
Bug 1321544 - Support icons for context menu items;
https://reviewboard.mozilla.org/r/147226/#review151558
r=me with the two small issues addressed.
::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:148
(Diff revision 2)
> + let contextMenu = document.getElementById("contentAreaContextMenu");
> +
> + await extension.startup();
> + await extension.awaitFinish("contextmenus-icons");
> +
> + await openExtensionContextMenu();
When there is only one menu item, there is no "extension menu" to open, so this should be `await openContextMenu()`, and it should return a `contextMenu`.
::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:166
(Diff revision 2)
> + let contextMenuChild2 = contextMenu.getElementsByAttribute("label", "child2")[0];
> + confirmContextMenuIcon(contextMenuChild2, "blue_icon.png");
> +
> + let contextMenuChild3 = contextMenu.getElementsByAttribute("label", "child3")[0];
> + confirmContextMenuIcon(contextMenuChild3, "green_icon.png");
> +
Close the menu please.
Attachment #8875809 -
Flags: review?(tomica) → review+
Comment 40•8 years ago
|
||
mozreview-review |
Comment on attachment 8875809 [details]
Bug 1321544 - Support icons for context menu items;
https://reviewboard.mozilla.org/r/147226/#review151582
::: browser/components/extensions/schemas/context_menus.json:149
(Diff revision 2)
> },
> + "icons": {
> + "type": "object",
> + "optional" : true,
> + "patternProperties" : {
> + "^[1-9]\\d*$": { "type": "string" }
I think this could be a choice of ImageData or ExtensionURL rather than just string. However we use string for this elsewhere...
Attachment #8875809 -
Flags: review?(mixedpuppy) → review+
Comment 41•8 years ago
|
||
You should be able to discard the other reviewboard entries from the "review summary" page.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 43•7 years ago
|
||
Comment on attachment 8875809 [details]
Bug 1321544 - Support icons for context menu items;
Sorry for the delay in my replies and commits.. I have been a bit occupied with college related stuff lately...
Comment 44•7 years ago
|
||
mozreview-review |
Comment on attachment 8875809 [details]
Bug 1321544 - Support icons for context menu items;
https://reviewboard.mozilla.org/r/147226/#review152444
Did you try the suggestion from Shane above?
Other than that, this looks good, just add the `checkin-needed` keyword when you are done.
::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:154
(Diff revisions 2 - 3)
>
> await closeContextMenu();
>
> extension.sendMessage("add-additional-menu-items");
>
> + contextMenu = document.getElementById("contentAreaContextMenu");
`contextMenu` should be what `await openExtensionContextMenu()` returns.
Assignee | ||
Comment 45•7 years ago
|
||
Comment on attachment 8875809 [details]
Bug 1321544 - Support icons for context menu items;
This patch contains changes requested in comment#44
Some queries:
1. Do we need to push to try before I add the checkin-needed keyword?
2. I have tried to make changes to browser/components/extensions/schemas/context_menus.json:149 to accept ImageData or ExtensionURL for the icon. However, I get:
JavaScript error: resource://gre/modules/Schemas.jsm, line 1221: Error: Internal error: Type ExtensionURL not found
or
JavaScript Error: "ReferenceError: IMAGE_ARRAYBUFFER_BLACK is not defined"
I think we should stick with string. Let me know your views on this and I'll make appropriate changes.
Comment 46•7 years ago
|
||
I'm not totally sure how that's supposed to work, so string is fine for now.
And yes, please run on Try before asking for checkin.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 48•7 years ago
|
||
Assignee | ||
Comment 49•7 years ago
|
||
Assignee | ||
Comment 50•7 years ago
|
||
(In reply to Swapnesh Kumar Sahoo [:swapneshks] from comment #49)
> Please ignore comment#48.
>
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=febb406cabc784b860c9359c9afd1d565944d801
I think the try job looks fine. Can you have a look and let me know if we can add the checkin flag?
Flags: needinfo?(tomica)
Comment 52•7 years ago
|
||
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
Comment 53•7 years ago
|
||
Hey Swapnesh, can you please mark all the issues as resolved, and flag for checkin again?
Flags: needinfo?(swapneshks)
Assignee | ||
Comment 54•7 years ago
|
||
(In reply to Tomislav Jovanovic :zombie from comment #53)
> Hey Swapnesh, can you please mark all the issues as resolved, and flag for
> checkin again?
Done.
Flags: needinfo?(swapneshks)
Keywords: checkin-needed
Comment 55•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c3f7c2d4303f
Support icons for context menu items; r=mixedpuppy,zombie
Keywords: checkin-needed
Backed out for frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=111109376&repo=autoland
https://hg.mozilla.org/integration/autoland/rev/7cde810db1040bfa5566da5fc2243232e72b6ee1
Flags: needinfo?(swapneshks)
Assignee | ||
Comment 57•7 years ago
|
||
(In reply to Wes Kocher (:KWierso) from comment #56)
> Backed out for frequent failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=111109376&repo=autoland
>
> https://hg.mozilla.org/integration/autoland/rev/
> 7cde810db1040bfa5566da5fc2243232e72b6ee1
Sorry for the frequent failures issue caused.
I had a brief look at the issue and I think the solution is that something similar to [1] needs to be done (which I missed out).
Also, actually I have run into an unforeseeable problem. My laptop hard drive has crashed and I am waiting for a replacement hard drive (writing this comment from liveUSB drive). I will look into this issue as soon as my hard drive problem is solved.
[1] https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_contextMenus_checkboxes.js#75
Comment hidden (mozreview-request) |
Assignee | ||
Comment 59•7 years ago
|
||
I am sorry for all the problems. I had to create a fresh review request as I lost all my data with my hard disk.
This attached file is the log for the local mochitest run.
I've also given a Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8a37f3930c1eb6ca9df492ae49ab559b03e0c93
Let me know if we need to perform additional checks/tests.
Flags: needinfo?(swapneshks) → needinfo?(tomica)
Comment 60•7 years ago
|
||
mozreview-review |
Comment on attachment 8884774 [details]
Bug 1321544 - Support icons for context menu items;
https://reviewboard.mozilla.org/r/155656/#review161260
This will not work, did you run the test, either locally or via Try?
::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:156
(Diff revision 1)
> + confirmContextMenuIcon(contextMenuChild1, "black_icon.png");
> +
> + await closeContextMenu();
> +
> + extension.sendMessage("add-additional-menu-items");
> + await extension.awaitFinish("contextmenus-icons");
This will never resolve, you are not sending this in the message listener (also, don't think you can `notifyFinish()` on the same message twice.
Attachment #8884774 -
Flags: review?(tomica) → review-
Updated•7 years ago
|
Flags: needinfo?(tomica)
Assignee | ||
Comment 61•7 years ago
|
||
(In reply to Tomislav Jovanovic :zombie from comment #60)
> Comment on attachment 8884774 [details]
> Bug 1321544 - Support icons for context menu items;
>
> https://reviewboard.mozilla.org/r/155656/#review161260
>
> This will not work, did you run the test, either locally or via Try?
>
As per comment#59, it did seem to work (both locally and in Try).
Nonetheless, I think https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_contextMenus_checkboxes.js#56 looks like a better approach. I'll give it a try.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 63•7 years ago
|
||
Comment on attachment 8884774 [details]
Bug 1321544 - Support icons for context menu items;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=047218c94ca3bad6a5ced06f5c9aba4a19c41317
Comment 64•7 years ago
|
||
mozreview-review |
Comment on attachment 8884774 [details]
Bug 1321544 - Support icons for context menu items;
https://reviewboard.mozilla.org/r/155656/#review161834
Attachment #8884774 -
Flags: review?(tomica) → review+
Updated•7 years ago
|
Attachment #8875809 -
Attachment is obsolete: true
Assignee | ||
Comment 65•7 years ago
|
||
Can we set the checkin-needed flag on this or do I need to run some more tests?
Flags: needinfo?(tomica)
Comment 66•7 years ago
|
||
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/231631a953da
Support icons for context menu items; r=zombie
Comment 68•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee | ||
Comment 69•7 years ago
|
||
(In reply to Tomislav Jovanovic :zombie from comment #67)
> heh, someone has an overzealous search query.. ;)
:)
Thanks for the guidance, btw. Was fun and great learning experience!
Comment 70•7 years ago
|
||
Thank you for doing the actual work! ;)
Comment 71•7 years ago
|
||
Needs updates:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus/create
https://github.com/mdn/browser-compat-data/blob/master/webextensions/javascript-apis.json
In addition, is there a reliable ways to detect or avoid error in older versions? see https://github.com/M-Reimer/undoclosetab/pull/7.
Status: RESOLVED → VERIFIED
Keywords: dev-doc-needed
Comment 72•7 years ago
|
||
(In reply to YF (Yang) from comment #71)
> In addition, is there a reliable ways to detect or avoid error in older
> versions? see https://github.com/M-Reimer/undoclosetab/pull/7.
I found https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/getBrowserInfo.
Comment 73•7 years ago
|
||
I've added some stuff about `icons` here:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/create
...and updated the main page a bit too:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus#Icons
I have updated the compat data, but it won't be deployed until Monday: https://github.com/mdn/browser-compat-data/pull/379.
Please let me know if we need anything else!
Flags: needinfo?(tomica)
Comment 74•7 years ago
|
||
:zombie are you able to confirm if the top level menu item will always still be the extensions icon?
The documentation reads as if the icons will only represent the individual items.
When there is one item with an icon does it create a submenu? The icon indicating the extension seems to me like a core part of ensuring the user is aware which extension controls the menu.
Comment 75•7 years ago
|
||
(In reply to Will Bamberg [:wbamberg] from comment #73)
> I've added some stuff about `icons` here:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/create
> ...and updated the main page a bit too:
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus#Icons
>
> I have updated the compat data, but it won't be deployed until Monday:
> https://github.com/mdn/browser-compat-data/pull/379.
>
> Please let me know if we need anything else!
Even if it was only two week premature, this change in the doc has just cost me 2 hours of searching why this feature does not work. Please, do not publish documentation for versions that have not been released yet!
Comment 76•7 years ago
|
||
(In reply to Will Bamberg [:wbamberg] from comment #73)
> ...and updated the main page a bit too:
Thanks Will, looks good.
I added the detail about the "contextMenus" permissions for the alias namespace.
(In reply to Jonathan Kingston [:jkt] from comment #74)
> :zombie are you able to confirm if the top level menu item will always still
> be the extensions icon?
Yes.
https://searchfox.org/mozilla-central/search?q=confirmContextMenuIcon.%2Bblack®exp=true
> The documentation reads as if the icons will only represent the individual
> items.
"Custom icons can only be set for items appearing in submenus"
> When there is one item with an icon does it create a submenu?
No.
Flags: needinfo?(tomica)
Comment 77•7 years ago
|
||
Thanks :zombie!
> Please, do not publish documentation for versions that have not been released yet!
Many people want documentation for features that are not released yet, and of course many features are released in some browsers and not in others. The way we deal with this is using the "Browser compatibility" tables.
Unfortunately it's hard at the moment to precisely coordinate updates to the docs with updates to the compatibility data, so for a couple of days the docs talked about `icons`, but the data was not updated to show that it was new in Firefox 56. Sorry you got caught out by that. The data's now been updated.
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/create#Browser_compatibility
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Updated•6 years ago
|
status-firefox56:
fixed → ---
See Also: → 1481179
You need to log in
before you can comment on or make changes to this bug.
Description
•