1704772 - [Windows 10] Addon context menu checkbox items show icons instead of checkboxes, are misaligned

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P2)

Description

[Mike Hommey [:glandium]]

Attachment: Screenshot

See attached screenshot.

OS: Windows 10, arm64.
Display: 1920x1080 @ 150% scaling.

Comment 1

[:Gijs (he/him)]

Which add-on is this (ie how can people reproduce), and what font and font size is in use here? (the browser devtools can tell you; you may want to turn on "Disable Popup Auto-Hide" in the menu there)

Edit: also what build ID of nightly is this?

Comment 2

[Mike Hommey [:glandium]]

about:buildconfig says

Built from https://hg.mozilla.org/mozilla-central/rev/b0d9f96ea571e00f4e0dbd78a089833a22ff2252

Addon is Rikaichamp. Installing the addon should be enough to get it in the menu. Interestingly, when the menu item is selected once to enable the feature, it shows an icon and then is aligned.

(In reply to :Gijs (he/him) from comment #1)

(the browser devtools can tell you; you may want to turn on "Disable Popup Auto-Hide" in the menu there)

I got the popup to not autohide, and got the inspector to select the item, but the font tab says there is no font used.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I'm more than happy to fix Rikaichamp if the add-on itself is to blame.

The context-menu is added here: https://github.com/birtles/rikaichamp/blob/4b6c9bec5acfdeac9dd15b6ec22b6cb8e1aca31a/src/background.ts#L416-L423

Comment 4

[:Gijs (he/him)]

(In reply to Mike Hommey [:glandium] from comment #2)

(In reply to :Gijs (he/him) from comment #1)

(the browser devtools can tell you; you may want to turn on "Disable Popup Auto-Hide" in the menu there)

I got the popup to not autohide, and got the inspector to select the item, but the font tab says there is no font used.

Can you check the computed style view for the item? You may need to turn on browser styles with the checkbox in that view, and then use the filter box for font.

The reason I'm asking is that there are other things that are misaligned in that menu (most obviously the icons at the top) and the menu uses em margins/paddings in some cases, and Windows allows font and font size configuration, and some of the defaults vary across Windows locales. Because I am not seeing the other misalignment, I kind of suspect the same will be true once I test with the add-on, and there is some dependency on the system font and font-size being different for this issue to occur.

Comment 5

[Mike Hommey [:glandium]]

font-family: Yu Gothic UI;
font-feature-settings: normal;
font-kerning: auto;
font-language-override: normal;
font-optical-sizing: auto;
font-size: 12px;
font-size-adjust: none;
font-stretch: 100%;
font-style: normal;
font-synthesis: weight style;
font-variant: normal;
font-variant-alternates: normal;
font-variant-caps: normal;
font-variant-east-asian: normal;
font-variant-ligatures: normal;
font-variant-numeric: normal;
font-variant-position: normal;
font-variation-settings: normal;
font-weight: 400

Comment 6

[Mike Hommey [:glandium]]

FWIW, it still happens if I add a CSS rule via the toolbox to change the font-family to Arial on the parent menupopup.

Comment 7

[:Gijs (he/him)]

(In reply to Mike Hommey [:glandium] from comment #6)

FWIW, it still happens if I add a CSS rule via the toolbox to change the font-family to Arial on the parent menupopup.

Thanks. Not that it matters, but the en-US default is Segoe UI. :-)

Anyway, having tried this out, the issue is, apparently, triggered by the add-on having a menuitem of type checkbox as well as an icon, which then actually never appears. This causes a few problems:

1. the icons at the top each get 36px padding which warps everything. They should be excluded from the selector at https://searchfox.org/mozilla-central/rev/3de2db87f3c9001ae478318d47a2ca3427574382/toolkit/themes/windows/global/menu.css#350 . Though the menugroup should probably get that same padding, minus the padding we compute for the first item to make the icon align... Or something. It's not really clear what the best end state there would be. AFAIK it is not possible to show these navigation items together with any checkable context menu item without involving add-ons.
2. the add-on item itself has no icon when not enabled. Then the icon element is sized as 0x0 but its container is still visible and still gets 12px margin, which off-sets the add-on item by those 12px compared to all the other items. From the link in comment #3 (thank you!) I don't understand off-hand why the icon is only there when enabled, or if it would be straightforward to detect this situation from within the CSS.
3. I don't really understand the add-on item; it claims to be a checkbox with an icon, but AFAICT we never show a checkbox, only an icon. I don't really understand why it specifies both, or why the icon is not there when the item is "unchecked". It looks like without proton, only the checkbox shows, never the icon.

Tomislav, for (2) and (3), do you know what the expectation is for checkable add-on items?

Comment 8

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #7)

1. the icons at the top each get 36px padding which warps everything. They should be excluded from the selector at https://searchfox.org/mozilla-central/rev/3de2db87f3c9001ae478318d47a2ca3427574382/toolkit/themes/windows/global/menu.css#350 . Though the menugroup should probably get that same padding, minus the padding we compute for the first item to make the icon align... Or something. It's not really clear what the best end state there would be. AFAIK it is not possible to show these navigation items together with any checkable context menu item without involving add-ons.

I guess this might not be clear without more context. For checkable menuitems, you need a "gutter" in which to display the checkbox, if you want the text of the remaining items to be aligned. For stylistic reasons, with proton on Win10, we only add this spacing if there is a visible item that can be checked (for this to happen it doesn't need to be checked when the menu appears). As noted in my previous comment, the group of 4 icons at the top isn't set up to deal with this, because out of the box this situation never happens.

The constraints for the group at the top are a bit annoying to work with. We want:

1. the leading edge of the first icon to be aligned with the text of the other menuitems
2. the trailing space after the last icon to be identical to the leading space before the first
3. the icons' visible hover boxes to be identical and centered around the icon
4. no dead space between the icons

And, at least when there was no gutter, the hover box around icons should touch the edges of the menu.

With the gutter being 36px wide, that would make for very very wide hover states (36 + 16 + 36 = 88px), and thus a very wide menu. I don't think that's desirable. Amy, what would you like us to do here?

Comment 9

[:Gijs (he/him)]

OK, per discussion with Amy, we just want to keep the navigation group looking the same, irrespective of the presence/absence of checkboxes. It'll lose horizontal alignment with the text but that's OK.

I'm still curious about the story wrt the add-on and the expectations around icon vs. checkbox, so keeping Tomislav's needinfo.

Comment 10

[Tomislav Jovanovic :zombie]

Attachment: context-menu-ext-check.png

(In reply to :Gijs (he/him) from comment #7)

2. the add-on item itself has no icon when not enabled. Then the icon element is sized as 0x0 but its container is still visible and still gets 12px margin, which off-sets the add-on item by those 12px compared to all the other items. From the link in comment #3 (thank you!) I don't understand off-hand why the icon is only there when enabled, or if it would be straightforward to detect this situation from within the CSS.
3. I don't really understand the add-on item; it claims to be a checkbox with an icon, but AFAICT we never show a checkbox, only an icon. I don't really understand why it specifies both, or why the icon is not there when the item is "unchecked". It looks like without proton, only the checkbox shows, never the icon.

Tomislav, for (2) and (3), do you know what the expectation is for checkable add-on items?

Sorry for the delay, yes I believe at least in Quantum design (ISTR it might have been different before 57), for extensions with only 1 menu item, which is a check box, we don't show the extension icon, just the check box.

Here's a screenshot of how it looks on Windows 10 checked and unchecked before Proton, and with Proton. It seems that all extension icons that were in the left gutter pre-Proton are now padded in, which is probably not what we want.

Comment 11

[Tomislav Jovanovic :zombie]

Attachment: context-menu-ext-normal.png

(In reply to :Gijs (he/him) from comment #9)

OK, per discussion with Amy, we just want to keep the navigation group looking the same, irrespective of the presence/absence of checkboxes. It'll lose horizontal alignment with the text but that's OK.

Oops, I kept this open while at meetings/dinner, and didn't see this comment. I just tested without the single-checkbox-item extension Rikaichamp, and it apparently already looks like that with icons (if I'm reading the description right). I guess making the checkbox case also indent the text would be good then.

Comment 12

[:Gijs (he/him)]

(In reply to Tomislav Jovanovic :zombie from comment #11)

Created attachment 9215563 [details]
context-menu-ext-normal.png

(In reply to :Gijs (he/him) from comment #9)

OK, per discussion with Amy, we just want to keep the navigation group looking the same, irrespective of the presence/absence of checkboxes. It'll lose horizontal alignment with the text but that's OK.

Oops, I kept this open while at meetings/dinner, and didn't see this comment. I just tested without the single-checkbox-item extension Rikaichamp, and it apparently already looks like that with icons (if I'm reading the description right). I guess making the checkbox case also indent the text would be good then.

Sorry, that comment was just about the back/fwd/refresh/star icons.

I think the case without the checkbox in your screenshot does look as expected, yes. Based on your description and the pre-proton state, I guess I would make the checkbox state have a checkmark instead of the add-on icon as it does now. But if we have that gutter anyway (for checkmarks), we might as well put the icons (for non-checkmark-items) in it and align the text, it'll look less sloppy than the current state.

Comment 13

[Tomislav Jovanovic :zombie]

Gijs asked me if I can help with this, so I investigated. The main issue seems to be that pre-Proton, we used default appearance for menu checkboxes, and hid the extension icon image element. But on Windows 10 with Proton, we're crafting a custom appearance, and that seems to need the image.menu-iconic-icon to be visible, see:
https://searchfox.org/mozilla-central/rev/759872688d/toolkit/themes/windows/global/menu.css#358,379,384

Unfortunately, the inherited image src attribute wins over the list-style-image css property for extension menu items, and checkbox is not rendered properly. I don't know if this can be solved through CSS somehow, but I think I could fix it in extensions framework code that generates menu items, by just not setting the image attribute in this case.

After that, this should a matter of adjusting padding and margins to properly line up everything.

Hey Molly, I think you introduced this new structure for checkboxes, does above sound reasonable, or is there a better way I'm not seeing?

Comment 14

[Molly Howell (she/her) (no longer active)]

Hi! Thanks for taking this on. Yes, I do think your assessment is accurate. I was not actually aware that this kind of menu item existed until I read this bug, so there's no allowance for it in the checkbox setup; it probably is easier to omit the src as you suggest than to alter the checkbox setup to make it work.

Comment 15

[Tomislav Jovanovic :zombie]

Attachment: Bug 1704772 - Fix extension context menu checkbox items and vertical alignment

1. Stop setting the image attribute for extension menu checkbox items. They
   don't work with the new custom chekbox appearance CSS, and not shown anyway.

2. When gutter is visible, stop padding menu items with an icon outside of it,
   and use the gutter as a place for said icons.

3. Adjust margin for menu items with icons to align the text with the rest of
   the menu items when the gutter is visible.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f12aed4ff646
Fix extension context menu checkbox items and vertical alignment r=Gijs

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/f12aed4ff646

Comment 18

[Bogdan Maris, Desktop QA]

Reproduced the initial issue using the Rikaichamp extension using old Nightly build from 2021-04-13, verified that using Firefox 89.0b1 and Rikaichamp extension the every label inside context menu is aligned correctly and a check is displayed when the extension is enabled.