1481179 - menus.update allows icon update if the extension has no icons

Status: VERIFIED FIXED

(WebExtensions :: Frontend, enhancement, P3)

Description

[Rob Wu [:robwu]]

According to MDN:

> 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

When an extension does not have an icon in manifest.json, then the menus.update API allows the update anyway, contrary to the documentation.

I think that the intent of the documented requirement is that extensions cannot spoof the menu icon in the top-level menu.

Should we allow extensions that have no default icon to create top-level menu items with arbitrary icons? If not, the fix excluding tests is just a few lines of code and I can submit a patch.

Comment 1

[Rob Wu [:robwu]]

Attachment: menus-update-icon.zip

Steps to reproduce:
1. Load add-on
2. Right-click on any web page.

Expected:
- Menu item without icon, "<-- Should not have a red icon" at bottom

Actual:
- Menu item with red icon, "[red icon] <-- Should not have a red icon"

Comment 2

[Shane Caraveo (:mixedpuppy)]

(In reply to Rob Wu [:robwu] from comment #0)

> I think that the intent of the documented requirement is that extensions
> cannot spoof the menu icon in the top-level menu.

I thought the intent is that an extension, when it has more than one menu and thus a parent "extension" menuitem that we created, it cannot set the icon for that parent menu.  I don't recall any restriction on setting the icons for the menuitems, whether a single top level or multiple submenu.  I was not very involved in the api at that time.

> Should we allow extensions that have no default icon to create top-level
> menu items with arbitrary icons? If not, the fix excluding tests is just a
> few lines of code and I can submit a patch.

Lets kick this can over to mconca.

Comment 3

[Mike Conca [:mconca]]

I think the original assumption was that the top-level menu item would use the extension icon defined in the manifest, and that every extension would define an icon.

The way this works now seems to be: if 1) you don't define an extension icon in the manifest, 2) create a menu item with an icon, and 3) only create *one* menu item, then the top-level menu item will show the menu item's icon. If there is an extension icon in the manifest, it seems to always be used. And if I create more than one menu item, the top level menu uses the extension icon in the manifest if there is one, otherwise, it leaves it blank.

This isn't completely terrible, but creates a pretty inconsistent user experience and will be confusing to document for developers.

I'd rather see us fix this to match the docs:  icons for created menus items only apply when they are in a submenu. The top-level menu always displays the extension icon.  If there is no extension icon in the manifest, it should be blank.

Comment 4

[Rob Wu [:robwu]]

Attachment: Bug 1481179 - Stop extensions from changing the icon in the top-level menu

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8999552 [details]
Bug 1481179 - Stop extensions from changing the icon in the top-level menu

Shane Caraveo (:mixedpuppy) has approved the revision.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d03373ced3a
Stop extensions from changing the icon in the top-level menu r=mixedpuppy

Comment 7

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/6d03373ced3a

Comment 8

[Vlad Jiman]

Verified on Windows 10 x64 bit using Firefox Nightly 63.0a1 (08/14/2018), the issue is not reproducible on the latest update, it was reproducing before updating to the latest build.