Closed Bug 1321544 Opened 8 years ago Closed 7 years ago

Set a different icon for a contextMenus item

Categories

(WebExtensions :: Frontend, defect, P3)

Product:
WebExtensions
Component:
Frontend
Version:
53 Branch
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
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)

context_menu_item_icon_test.png
14.99 KB, image/png
designakt
: ui-review+
Details
Bug 1321544 - Support icons for context menu items;
59 bytes, text/x-review-board-request
zombie
: review+
Details
contextmenu-icon-log
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
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]
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
Whiteboard: [design-decision-needed] → [design-decision-needed][contextMenus]
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#
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]
Mentor: tomica
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug
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)
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)
(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)
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 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?)
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 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-
Whiteboard: [design-decision-approved][contextMenus] → [design-decision-approved][contextMenus] triaged
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 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 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 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-
(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.
Flags: needinfo?(tomica)
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 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?
(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)
Attachment #8869744 - Flags: review?(tomica)
Requesting UX review for context menu item icon...
Attachment #8874958 - Flags: review?(mjaritz)
(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 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+
(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)
Attachment #8869744 - Attachment is obsolete: true
(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 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)
Attachment #8875368 - Attachment is obsolete: true
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 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)
(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 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 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+
You should be able to discard the other reviewboard entries from the "review summary" page.
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 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.
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.
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.
(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)
Yes, looks good.
Flags: needinfo?(tomica)
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
Hey Swapnesh, can you please mark all the issues as resolved, and flag for checkin again?
Flags: needinfo?(swapneshks)
(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
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
(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
Attached file contextmenu-icon-log —
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 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-
Flags: needinfo?(tomica)
(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 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+
Attachment #8875809 - Attachment is obsolete: true
Can we set the checkin-needed flag on this or do I need to run some more tests?
Flags: needinfo?(tomica)
Pushed by kwierso@gmail.com: https://hg.mozilla.org/integration/autoland/rev/231631a953da Support icons for context menu items; r=zombie
heh, someone has an overzealous search query.. ;)
Flags: needinfo?(tomica)
https://hg.mozilla.org/mozilla-central/rev/231631a953da
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(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!
Thank you for doing the actual work! ;)
(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.
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)
: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.
(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!
(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&regexp=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)
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
See Also: → 1414566
Product: Toolkit → WebExtensions
status-firefox56: fixed → ---
See Also: → 1481179
See Also: → 1492969
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: