Closed
Bug 1414566
Opened 6 years ago
Closed 5 years ago
browser.menus.update() does not support updating icon
Categories
(WebExtensions :: General, defect, P3)
WebExtensions
General
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' } });
Updated•6 years ago
|
status-firefox57:
--- → wontfix
Priority: -- → P3
Comment 1•6 years ago
|
||
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 :)
Comment 3•5 years ago
|
||
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).
Updated•5 years ago
|
Product: Toolkit → WebExtensions
Comment 4•5 years ago
|
||
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.
status-firefox57:
wontfix → ---
Comment 6•5 years ago
|
||
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)
Updated•5 years ago
|
Assignee: nobody → tushararora.cs
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•5 years ago
|
||
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)
Comment 8•5 years ago
|
||
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)
Comment 9•5 years ago
|
||
Is Tushar still working on this? If not I am interested to work on this.
Flags: needinfo?(rob)
Comment 10•5 years ago
|
||
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)
Comment 11•5 years ago
|
||
Ok. Thank you!!
Assignee | ||
Comment 12•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Attachment #9005082 -
Flags: review?(rob)
Comment 13•5 years ago
|
||
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 14•5 years ago
|
||
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 15•5 years ago
|
||
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+
Assignee | ||
Updated•5 years ago
|
Keywords: good-first-bug → checkin-needed
Comment 16•5 years ago
|
||
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
Comment 17•5 years ago
|
||
When you add the checkin-needed keyword, try to keep existing keywords.
Keywords: good-first-bug
Comment 18•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d8b1a021c0a8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment 19•5 years ago
|
||
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)
Assignee | ||
Comment 20•5 years ago
|
||
> 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)
Comment 21•5 years ago
|
||
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/update and its compat table need to be updated.
Keywords: dev-doc-needed
Comment 22•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
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•4 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•