1706895 - Context menu doesn't have icons for the extensions

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, defect)

Description

[juraj.masiar]

Attachment: Screenshot 2021-04-22 09.43.22.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:89.0) Gecko/20100101 Firefox/89.0

Steps to reproduce:

1. install Web Translate:
   https://addons.thunderbird.net/en-US/thunderbird/addon/web_translate/
2. open email
3. select text
4. right click

Actual results:

The extension item in the context menu doesn't have icon.

Expected results:

It should have an icon.
Also that double separator bar looks fishy.

Comment 1

[John Bieling (:TbSync)]

mozgegression showed it was this changeset:

https://hg.mozilla.org/comm-central/pushloghtml?fromchange=0bc379dfb5184277046debb04b0b7aec4796f841&tochange=01b42daa8a2675fd108dcfd4873dd75970425e2c

Probably the UI patch. Richard, could you have a look?

Comment 2

[Richard Marti (:Paenglab)]

Yes, this was done because Proton don't show icons and this would make the menuitem text alignment odd. But they added a [needsgutter] property that I can use now to show the icons when this property is set.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: 1706895-menuitem-icon.patch

When the menupopup has the property [needsgutter] set then show the icon because there is enough space to show it. When it's not set, then we have no space and the text is still correctly aligned.

This is a Windows 10 only bug with browser.proton.contextmenus.enabled on true, which is the default.

Comment 4

[John Bieling (:TbSync)]

Comment on attachment 9217712 [details] [diff] [review]
1706895-menuitem-icon.patch

Confirmed this fixes the icon issue.

@juraj.masiar: I will fix the additional separator in a follow-up patch/bug.

Comment 5

[Richard Marti (:Paenglab)]

Attachment: 1706895-menuitem-icon.patch

Patch updated after landing of bug 1706978.

Comment 6

[Richard Marti (:Paenglab)]

Comment on attachment 9217948 [details] [diff] [review]
1706895-menuitem-icon.patch

Changing to r+ as the previous patch was already reviewed.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/81f8cf11da4b
[Windows 10] Show the menuitem icon when the menupopup has the needed space in front. r=TBSync

Comment 8

[Richard Marti (:Paenglab)]

Comment on attachment 9217948 [details] [diff] [review]
1706895-menuitem-icon.patch

[Approval Request Comment]
Regression caused by (bug #): bug 1703988
User impact if declined: No extension icon in menu
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low

Comment 9

[John Bieling (:TbSync)]

This bug confuses me. We switch off the icons in menupopups, if there is no needsgutter attribute. The needsgutter attribute cannot be enforced, it is auto-controlled by this:

https://searchfox.org/comm-central/rev/b4465cecab137702144ec8a2b3f8d6779971f60f/mozilla/toolkit/content/widgets/menupopup.js#30

So only if there is a radio or checkbox type menuitem in there, the needsgutter attribute is added. It does not care about icons. If I have menupopups without checkboxes or icons (in the menus WebExtension API) and the icons are consequently not shown (but we need them). And adding a needsgutter attribute does not help, as the linked code removes it again.

Can we have a new class like menupopup-iconic, which I can apply to a menupopup, which overrides this behavior? So that needsgutter will not be used as a check to hide icons if the menupopup-iconic class is present?

Or can we limit this needsgutter behavior to some areas of TB but not everywhere?

Comment 10

[Richard Marti (:Paenglab)]

Proton seems to not like icons in menus. You need to ask someone from toolkit if this new class would be allowed.

Comment 11

[John Bieling (:TbSync)]

Hm, if I am not missing something then I would say it is our own code, who is causing this, not toolkit.

Toolkit does not have any CSS rule which applies display:none to elements who do not have needsgutter, that is only in our code:

https://searchfox.org/comm-central/search?q=needsgutter&path=&case=false&regexp=false

If I remove this, then I have my icons again.

Comment 12

[Richard Marti (:Paenglab)]

Attachment: menuitem-iconic.png

The CSS code is our, yes. This to fix the alignment in the menus, see screenshot if the icon, or the space for the icon, isn't hidden. This looks bad or not?

Comment 13

[John Bieling (:TbSync)]

How does that look on Linux?

What I think is strange, is that needsgutter is only added on Windows so Windows has no icons anymore, but Linux and Mac do. I wish we could do something about this.

For me it would be enough to change that CSS rule of ours so I can enforce icons in my WebExtension menus, but I hope we can find a different solution altogether.

Comment 14

[John Bieling (:TbSync)]

Attachment: linux.PNG

This is how tip on linux looks for me.

Comment 15

[Richard Marti (:Paenglab)]

TB itself doesn't use any icons in menus, except the chat staus icons, but in its own sub menu. Only extensions have icons.

Comment 16

[John Bieling (:TbSync)]

Attachment: windows.png

And this is how it looks on Windows.

Comment 17

[Richard Marti (:Paenglab)]

Alessandro, what do you think?

Comment 18

[John Bieling (:TbSync)]

Attachment: firefox.PNG

In Firefox nightly on Windows the menuitem-iconic icons are shown. Shifting them looks also strange but better than nothing at all. I have to check later how that looks on Linux.

Comment 19

[Wayne Mery (:wsmwk)]

Comment on attachment 9217948 [details] [diff] [review]
1706895-menuitem-icon.patch

[Triage Comment]
Approved for beta

Comment 20

[Rob Lemley [:rjl]]

Thunderbird 89.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/80299e59cb41

Comment 21

[Alessandro Castellani [:aleca]]

(In reply to Richard Marti (:Paenglab) from comment #12)

Created attachment 9218688 [details]
menuitem-iconic.png

The CSS code is our, yes. This to fix the alignment in the menus, see screenshot if the icon, or the space for the icon, isn't hidden. This looks bad or not?

Yeah, this looks ugly.

I honestly don't know what might be the optimal solution here.
I personally don't mind menu items that have icons being indented compared to adjacent items, but I can see how that might be visually jarring.
Checkboxes and radio buttons are another thing, and I don't know if not having the gutter space for all the adjacent items in the same menu will look weird.
Nonetheless, having a whole menu with an empty gutter space to the left when we know nothing will be added to it it's very strange.

Comment 22

[John Bieling (:TbSync)]

I can write a patch which will enforce gutter in all menupopups (on popupshowing) if a WebExtension has added a menu entry with an icon.

This will make windows and linux look the same and only add gutter if needed.

How does that sound?

Comment 23

[Richard Marti (:Paenglab)]

(In reply to Alessandro Castellani [:aleca] from comment #21)

Nonetheless, having a whole menu with an empty gutter space to the left when we know nothing will be added to it it's very strange.

Attachment 9218702 [details] shows also on Linux an empty space to the left also without the extension installed.

I'm planning a follow-up bug that adds the left space to all menupopups like the default Windows 10 menus have.