Closed Bug 1414566 Opened 7 years ago Closed 6 years ago

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

Categories

(WebExtensions :: General, defect, P3)

defect

Tracking

(firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: mtanalin, Assigned: tushararora, Mentored)

References

Details

(Keywords: dev-doc-complete, good-first-bug)

Attachments

(1 file)

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'
        }
    });
Priority: -- → P3
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.
> status-firefox57: --- → wontfix

It means that it will not be fixed for Firefox 57 :)
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).
Mentor: rob
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug
See Also: → 1321544
Product: Toolkit → WebExtensions
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.
Hey Rob! Can I start working on this bug?
Flags: needinfo?(rob)
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
Flags: needinfo?(rob)
Assignee: nobody → tushararora.cs
Status: NEW → ASSIGNED
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?
Flags: needinfo?(rob)
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).
Flags: needinfo?(rob)
See Also: → 1481179
Is Tushar still working on this? 
If not I am interested to work on this.
Flags: needinfo?(rob)
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.
Flags: needinfo?(rob)
Ok. Thank you!!
Attachment #9005082 - Flags: review?(rob)
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.
Attachment #9005082 - Flags: review?(rob)
Comment on attachment 9005082 [details]
Bug 1414566 browser.menus.update() does not support updating icon r=robwu

Rob Wu [:robwu] has approved the revision.
Attachment #9005082 - Flags: review+
Comment on attachment 9005082 [details]
Bug 1414566 browser.menus.update() does not support updating icon r=robwu

Shane Caraveo (:mixedpuppy) has approved the revision.
Attachment #9005082 - Flags: review+
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d8b1a021c0a8
browser.menus.update() does not support updating icon r=robwu,mixedpuppy
Keywords: checkin-needed
When you add the checkin-needed keyword, try to keep existing keywords.
Keywords: good-first-bug
https://hg.mozilla.org/mozilla-central/rev/d8b1a021c0a8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
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!
Flags: needinfo?(tushararora.cs)
> 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/
Flags: needinfo?(tushararora.cs)
Flags: qe-verify-
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.
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.
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).
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.

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.

You need to log in before you can comment on or make changes to this bug.