Set a different icon for a contextMenus item

NEW
Assigned to

Status

()

Toolkit
WebExtensions: Frontend
P3
normal
7 months ago
2 days ago

People

(Reporter: Pierre Bertet, Assigned: swapneshks, Mentored)

Tracking

({good-first-bug})

53 Branch
good-first-bug
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

MozReview Requests

()

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

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Updated

7 months ago
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit

Updated

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

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

5 months ago
Whiteboard: [design-decision-needed] → [design-decision-needed][contextMenus]

Updated

4 months ago
Duplicate of this bug: 1337470
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#

Updated

2 months ago
Duplicate of this bug: 1361085

Comment 5

2 months 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]
Mentor: tomica@gmail.com
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug
(Assignee)

Comment 6

a month 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)
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

a month 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)
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)
> 2) http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#157

or better yet
http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#179
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a month 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?)
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

a month 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

a month ago
Whiteboard: [design-decision-approved][contextMenus] → [design-decision-approved][contextMenus] triaged
Comment hidden (mozreview-request)
(Assignee)

Comment 16

29 days 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

28 days 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

27 days 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

26 days 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-
(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 hidden (mozreview-request)
(Assignee)

Comment 23

22 days 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

19 days 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?
(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)
(Assignee)

Comment 26

19 days ago
Created attachment 8874958 [details]
context_menu_item_icon_test.png

Requesting UX review for context menu item icon...
Attachment #8874958 - Flags: review?(mjaritz)
(Assignee)

Comment 27

19 days 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)

Updated

18 days ago
Duplicate of this bug: 1370498
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)
Comment hidden (mozreview-request)
(Assignee)

Updated

18 days ago
Attachment #8869744 - Attachment is obsolete: true
(Assignee)

Comment 32

18 days 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

18 days 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

17 days ago
Attachment #8875368 - Attachment is obsolete: true
(Assignee)

Comment 35

17 days 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

17 days 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

16 days 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

16 days 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

16 days 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+
You should be able to discard the other reviewboard entries from the "review summary" page.
Comment hidden (mozreview-request)
(Assignee)

Comment 43

14 days 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

13 days 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

11 days 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.
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

11 days ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ec62d15a6c0c985d1d2d52758264eb3c733a353
(Assignee)

Comment 49

11 days ago
Please ignore comment#48.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=febb406cabc784b860c9359c9afd1d565944d801
(Assignee)

Comment 50

6 days 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)
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
You need to log in before you can comment on or make changes to this bug.