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

VERIFIED FIXED in Firefox 63

Status

enhancement
P3
normal
VERIFIED FIXED
11 months ago
10 months ago

People

(Reporter: robwu, Assigned: robwu)

Tracking

unspecified
mozilla63

Firefox Tracking Flags

(firefox63 verified)

Details

Attachments

(2 attachments)

Assignee

Description

11 months ago
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.
Assignee

Comment 1

11 months ago
str
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"

Updated

11 months ago
Flags: needinfo?(mixedpuppy)
Priority: -- → P3
(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.
Flags: needinfo?(mixedpuppy) → needinfo?(mconca)

Comment 3

11 months ago
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.
Flags: needinfo?(mconca)
Assignee

Updated

10 months ago
Assignee: nobody → rob
Status: NEW → ASSIGNED
Iteration: --- → 63.4 - Aug 20
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.
Attachment #8999552 - Flags: review+

Comment 6

10 months ago
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/6d03373ced3a
Stop extensions from changing the icon in the top-level menu r=mixedpuppy

Comment 7

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6d03373ced3a
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

Comment 8

10 months ago
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.