Set a different icon for a contextMenus item

NEW
Assigned to

Status

()

Toolkit
WebExtensions: Frontend
P3
normal
6 months ago
16 hours ago

People

(Reporter: Pierre Bertet, Assigned: swapneshks, Mentored)

Tracking

({good-first-bug})

53 Branch
good-first-bug
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [design-decision-approved][contextMenus] triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20161201030205




Expected results:

I understand from the documentation [1] that it is not possible to have different icons for different menu items.

But is there a way to have an icon for the addon, and another one for the menu items?

At the moment we are using two different icons for gtranslate [2], because the logo doesn’t represent the action and is too noisy to be used in that context.

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/contextMenus
[2] https://addons.mozilla.org/firefox/addon/gtranslate/

Updated

6 months ago
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
Component: WebExtensions: Untriaged → WebExtensions: Frontend
Summary: (WebExtensions) Set a different icon for a contextMenus item → Set a different icon for a contextMenus item
Whiteboard: [design-decision-needed]

Comment 1

5 months ago
Unless I'm mistaken, I think I would need this to port Context Search, which expands the context menu to contain the search plugins from the Search Bar (CTRL+K). 

I would be looking to replicate the current UI:
http://www.basson.at/images/addons/contextsearch.png

Updated

3 months ago
Whiteboard: [design-decision-needed] → [design-decision-needed][contextMenus]
Duplicate of this bug: 1337470
Hi Pierre, this has been added to the agenda for the May 2 WebExtensions Triage meeting. Will you be able to join us? 

Wiki: https://wiki.mozilla.org/Add-ons/Contribute/Triage#Next_Meeting

Agenda: https://docs.google.com/document/d/1vf8AaW8tKKbMn4KhsqEYhrYqVTUGaERHQmzanEp2Cls/edit#

Updated

21 days ago
Duplicate of this bug: 1361085

Comment 5

21 days ago
It sounds good to me that in the sub-menu (as shown in the screenshot in comment 1) should have custom icons. We'd like to retain that the top level menu item still has the add-on icon on it so we clearly indicate to the user where the context menu item is coming from. Let's do it.
Priority: -- → P3
Whiteboard: [design-decision-needed][contextMenus] → [design-decision-approved][contextMenus]
Mentor: tomica@gmail.com
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug
(Assignee)

Comment 6

8 days ago
Hi.. Can I take up this bug? (I am a beginner and have worked only on 8-9 moz bugs).

From an initial glance, it appears that the end result should be a combination of the following:
1. Menu snapshot in the documentation ([1] in the bug Description)
2. Screenshot in comment#1
Flags: needinfo?(tomica)
Hi Swapnesh, sure you can.

That sounds about right.  Are you familiar with mozreview [1]?  Let me know if you have any questions.

1) https://reviewboard.mozilla.org/
Assignee: nobody → swapneshks
Flags: needinfo?(tomica)
(Assignee)

Comment 8

7 days ago
(In reply to Tomislav Jovanovic :zombie from comment #7)
> Are you familiar with mozreview [1]?  Let me know
> if you have any questions.
> 
> 1) https://reviewboard.mozilla.org/

Yes, I have learnt it just a few days back. :)

I have a few doubts regarding which files to modify. After digging a bit, I found the following files:

http://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/context_menus.json
http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#347
http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#630

I'm a bit unsure about how the frontend has to be modified to include the icon and what changes need to be made. Could you help me out a bit here?
Flags: needinfo?(tomica)
Yes, you've got the right files.  Specifically, [1] is where we set the current icon on the top level menu item for each extension, and you would have to do something similar around [2], thought the icon will from `item`, not the manifest.

1) http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#57
2) http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#157
Flags: needinfo?(tomica)
> 2) http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#157

or better yet
http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#179
Comment hidden (mozreview-request)
(Assignee)

Comment 12

3 days ago
Comment on attachment 8869744 [details]
Bug 1321544 - Set different icon for contextMenus item in MozReview;

In this patch, I've made only one addition and I feel that something more needs to be added. 
Also, I tried testing the changes but couldn't figure out how to, i.e., how do I test the UI? (I found a mochitest browser/components/extensions/test/browser/browser_ext_browserAction_contextMenu.js that calls contextMenus.create, but how to replicate similar behaviour in a firefox build?)
Yeah, this will not be enough.  First of all, you need to determine the correct icon dimensions, and to resolve the icon URL, similar to the code around line 60 I pointed you at [1].

For testing, you will either need an extension that creates several menu items with different icons, or even better, to do it from a mochitest.

After you make your code changes, you will need to build firefox, and then run the test with:

    mach build faster
    mach mochitest browser/components/extensions/test/browser/browser_ext_browserAction_contextMenu.js

(your test shouldn't really end up in that file, but it might be easier to start with modifying an existing test)

1) http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-contextMenus.js#57

Comment 14

2 days ago
mozreview-review
Comment on attachment 8869744 [details]
Bug 1321544 - Set different icon for contextMenus item in MozReview;

https://reviewboard.mozilla.org/r/141314/#review144934

::: browser/components/extensions/ext-contextMenus.js:203
(Diff revision 1)
>        element.setAttribute("id",
>          `${makeWidgetId(item.extension.id)}_${item.id}`);
>      }
>  
> +    if (item.icon) {
> +      element.setAttribute("image", "url(" + item.icon + ")");

This is not CSS, no need for `url()`, but also, you need to resolve the icon URI.
Attachment #8869744 - Flags: review?(tomica) → review-

Updated

16 hours ago
Whiteboard: [design-decision-approved][contextMenus] → [design-decision-approved][contextMenus] triaged
You need to log in before you can comment on or make changes to this bug.