Closed Bug 1704772 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Menus, defect, P2)

Desktop
Windows 10
defect

Tracking

()

VERIFIED FIXED
89 Branch
Tracking Status
firefox89 --- verified

People

(Reporter: glandium, Assigned: zombie)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Whiteboard: [proton-context-menus] [priority:2b])

Attachments

(4 files)

Attached image Screenshot (deleted) —

See attached screenshot.

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

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?

Flags: needinfo?(mh+mozilla)

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.

Flags: needinfo?(mh+mozilla)

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

(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.

Flags: needinfo?(mh+mozilla)

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

Flags: needinfo?(mh+mozilla)

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

(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?

Flags: needinfo?(tomica)

(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?

Flags: needinfo?(amlee)
OS: Unspecified → Windows 10
Hardware: Unspecified → Desktop
Summary: Addon context menu item is misaligned → [Windows 10] Addon context menu item is misaligned
Whiteboard: [proton-context-menus]

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.

Flags: needinfo?(amlee)
Attached image context-menu-ext-check.png (deleted) —

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

  1. 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.
  2. 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.

Flags: needinfo?(tomica)
Attached image context-menu-ext-normal.png (deleted) —

(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.

(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.

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?

Assignee: nobody → tomica
Flags: needinfo?(mhowell)

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.

Flags: needinfo?(mhowell)
Priority: -- → P2
Whiteboard: [proton-context-menus] → [proton-context-menus] [priority:2b]
Summary: [Windows 10] Addon context menu item is misaligned → [Windows 10] Addon context menu checkbox items show icons instead of checkboxes, are misaligned
  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.

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/f12aed4ff646 Fix extension context menu checkbox items and vertical alignment r=Gijs
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch

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.

Status: RESOLVED → VERIFIED
Regressions: 1810306
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: