1414566 - browser.menus.update() does not support updating icon

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[Marat Tanalin]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171102181127

Steps to reproduce:

`browser.menus.update()` does not support updating icon.

This can be worked around by removing the existing menu item with `browser.menus.remove()` and adding a new item with a different icon via `browser.menus.create()`, this is just unreasonably more complicated than updating the icon of the existing menu item directly.

Fwiw, same with the `command` property.

Also, the limitation is apparently not documented: `id` is the only property documented as disallowed on update:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/menus/update

> The same as the createProperties object passed to menus.create(), except that id can't be set.


Actual results:

With code like this in background script:

    browser.menus.update('example-id', {
        icons: 'example.png'
    });

Console says:

> Error: Type error for parameter updateProperties (Unexpected property "icons") for menus.update.

Same with an object as a value for `icons`:

    browser.menus.update('example-id', {
        icons: {
            '32' : 'example.png'
        }
    });

Comment 1

[Duncan Moore]

Why is this bug marked as wontfix?

The work around (remove() then create() again) is not that helpful, since the menu item is effectively moved to the bottom of the list.

At the very least the documentation should be corrected to say that icons cannot be updated.

Comment 2

[kernp25]

> status-firefox57: --- → wontfix

It means that it will not be fixed for Firefox 57 :)

Comment 3

[Rob Wu [:robwu]]

menus.update should support all properties that menus.create recognizes (except for "id").
However, "icons" property is missing from the "update" method, but it is defined for the "create" method: https://searchfox.org/mozilla-central/rev/6a27ff48dda8376d1f819c534510eca7d9af5818/browser/components/extensions/schemas/menus.json#169-175
("command" is also missing, but this bug is about "icons").

To fix this, add the missing property to browser/components/extensions/schemas/menus.json
and add unit tests to verify that browser.menus.update({icons: ... }) works as expected, e.g. to browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js

The tests should test the following, in order:
- browser.menus.update(id, {icons: ...}) updates the menu icon.
- browser.menus.update(id, { /* without icons */ }); should not change the menu icon.
- browser.menus.update(id, {icons: null}) should reset the icon to the default (missing) icon.

After following the above suggestions, the first two tests will pass and the last test will fail.
That is because the menu icon is only updated if it is set to a non-null object value: https://searchfox.org/mozilla-central/rev/6a27ff48dda8376d1f819c534510eca7d9af5818/browser/components/extensions/parent/ext-menus.js#234-235
To support resetting the menu icon to the default state, also perform the following changes:
- Use the "optional": "omit-key-if-missing" instead of "optional": true in the "icons" property in menus.json that you added above.
- Change ext-menus.js from:
    if (item.icons) {
      this.setMenuItemIcon(element, item.extension, contextData, item.icons);
    }
  to:
  if ("icons" in item) {
    if (item.icons) {
      this.setMenuItemIcon(element, item.extension, contextData, item.icons);
    } else {
      this.removeMenuItemIcon(element);
    }
  }
- Implement removeMenuItemIcon, which undoes the changes from setMenuItemIcon (i.e. remove the "class" and "image" attributes).

Comment 4

[Caitlin Neiman [:caitmuenster]]

If this is your first contribution, please refer to https://wiki.mozilla.org/Add-ons/Contribute/Code on how to get started. 

This bug's mentor is :robwu.

Comment 5

[Tushar Arora [:tushararora]]

Hey Rob! Can I start working on this bug?

Comment 6

[Rob Wu [:robwu]]

Hi Tushar! Yes, go ahead. See comment 3 for more details.

You are probably already familiar with the development process, since you have contributed patches before.
If you need a reminder, here is documentation to get started:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction

Comment 7

[Tushar Arora [:tushararora]]

Hi Rob! I went through the file that contains tests and I can't really understand what's happening here - https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js#152-154

In background script the first function in the order of execution creates a contextMenu item child with `red_icon.png` icon. Now on the lines I mentioned above we're first calling `openContextMenu()` and then checking if `child1` has an icon `black_icon.png`.

I am curious as to how did contextMenu item is an element with a label called `child1` and why does it have an icon as `black_icon.png` as these lines indicate otherwise https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js#131-139?

Comment 8

[Rob Wu [:robwu]]

Because:

> Custom icons can only be set for items appearing in submenus.
> https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/create

In the test, a black icon is set in the manifest file:
https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js#94-96

So even though the menu was created with a red icon, the icon still uses the black manifest icon.

The implementation of customizing the icon is here, in function customizeElement:
https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/browser/components/extensions/parent/ext-menus.js#198,234-236

customizeElement is called by buildSingleElement,
buildSingleElement is called by buildElementWithChildren.
buildElementWithChildren is called for submenu items, but also by createTopLevelElement.
createTopLevelElement first creates the menu element and overwrites the icon with the one from the manifest file:
https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/browser/components/extensions/parent/ext-menus.js#119-120,129-132


PS. When you reference source code on Searchfox, I recommend to use permalinks (by clicking on "Go to latest version" in Searchfox). This ensures that the link always refers to the version of the code that you are seeing, and not to the latest version (which may look completely different in the future).

Comment 9

[Arshad Kazmi [:arshadkazmi42]]

Is Tushar still working on this? 
If not I am interested to work on this.

Comment 10

[Rob Wu [:robwu]]

Tushar hasn't followed up in almost 4 weeks. Let's allow a few days for him to respond (in case they are busy). If no reply is given by the end of the weekend, I'll make the bug available again.

Since you seem to be interested in bugs, I will follow up by mail to find another contribution opportunity for you.

Comment 11

[Arshad Kazmi [:arshadkazmi42]]

Ok. Thank you!!

Comment 12

[Tushar Arora [:tushararora]]

Attachment: Bug 1414566 browser.menus.update() does not support updating icon r=robwu

Comment 13

[Rob Wu [:robwu]]

Comment on attachment 9005082 [details]
Bug 1414566 browser.menus.update() does not support updating icon r=robwu

Thanks for the follow-up Tushar.

For your information, there is no need to manually add the r? flag to attachments.
With the new Phabricator review system, review notifications are entirely handled by Phabricator, and only the final review approval (r+) is mirrored to Bugzilla here.

Comment 14

[Rob Wu [:robwu]]

Comment on attachment 9005082 [details]
Bug 1414566 browser.menus.update() does not support updating icon r=robwu

Rob Wu [:robwu] has approved the revision.

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9005082 [details]
Bug 1414566 browser.menus.update() does not support updating icon r=robwu

Shane Caraveo (:mixedpuppy) has approved the revision.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8b1a021c0a8
browser.menus.update() does not support updating icon r=robwu,mixedpuppy

Comment 17

[Rob Wu [:robwu]]

When you add the checkin-needed keyword, try to keep existing keywords.

Comment 18

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d8b1a021c0a8

Comment 19

[Caitlin Neiman [:caitmuenster]]

Thanks for the patch, Tushar! Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you!

Comment 20

[Tushar Arora [:tushararora]]

> Would you be interested in creating an account on mozillians.org? I'd be
> happy to vouch for you!

Thank you so much Caitlin. Here's my mozillians account - https://mozillians.org/en-US/u/tushararora/

Comment 21

[Rob Wu [:robwu]]

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/update and its compat table need to be updated.

Comment 22

[Chris Mills [:cmills]]

Note to docs team:

I've added a note to the Fx 64 rel notes to cover this:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#API_changes

See above comment for what else needs to be done.

Comment 23

[Alexander K]

Hi everyone.

It seems the top-level icon is still cannot be changed.

Imagine an extension which has a toggle icon in address bar. 
Then you want to reflect the same toggle icon in a context menu and... you can't.

This is exactly where I've stuck and Google brought me here.

Comment 24

[Rob Wu [:robwu]]

The top-level icon can intentionally not be changed. To help users with understanding the source of the menu item, the top-level icon matches the extension's primary icon (as declared in the manifest file).

Comment 25

[Alexander K]

Yes, I can understand logic behind such decision. 
But let me explain how I see it.
When you click "three dots icon" in address bar, there is a menu, and you CAN update icon of your extension.
A context menu is also a menu, but you CANNOT do the same.

Comment 26

[Irene Smith]

Changed the existing statement on the update method to state:

In addition, icons can only be changed on menu commands, not on the top-level context menu. The top-level icon matches the extension's primary icon as declared in the extension's manifest file.