1532172 - Almost invisible separators in the context menu of a downloaded item in the Library when dark theme is enabled

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P3)

Description

[Itiel]

Attachment: Separators in the context menu of a downloaded item, from the Downloads

This is a regression caused by bug 1453722.

STR:

1. New profile, enable dark theme
2. Download any file
3. Click the Downloads icon on the toolbar > right click the downloaded item > observe separators color and style -- expected
4. Click the Library icon > Downloads > right click the downloaded item > observe separators color and style -- bad

There are actually 2 issues here:

1. The separators are almost invisible -- this is regression from bug 1453722
2. Their style is not the same as other separators in the Firefox UI (not so much padding before and after) -- this goes back to when the Downloads section was first introduced in the Library

See attached.

Comment 1

[Itiel]

Attachment: Separators in the context menu of a downloaded item, from Library > Downloads

Comment 2

[Dão Gottwald [:dao]]

Bug 1453722 just changed --panel-separator-color. The problem here is that this color shouldn't be used in the first place in the natively styled context menu.

I think there are two ways to approach this (short of making context menus dark which we'll likely do at some point). The first one would be to somehow exclude the context menu here:

https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/browser/themes/shared/customizableui/panelUI.inc.css#1129-1133

The other option would be to put panelDownloadsContextMenu in mainPopupSet instead of the panel.

Comment 3

[Tim Spurway [:tspurway]]

Felipe, do you think we should be tracking this for 66 uplift?

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

If you are able to land a fix soon it can still make it into tomorrow's beta build. Past that I don't really want to uplift.

Comment 5

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #2)

The other option would be to put panelDownloadsContextMenu in mainPopupSet instead of the panel.

This (having a context menu for a panel that is not inside that panel in the DOM structure) has historically caused flickering issues on Windows classic.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Since the regression is from 62, I don't think we need to track it for 66 or worry about uplifting.

Comment 7

[Dão Gottwald [:dao]]

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

(In reply to Dão Gottwald [::dao] from comment #2)

The other option would be to put panelDownloadsContextMenu in mainPopupSet instead of the panel.

This (having a context menu for a panel that is not inside that panel in the DOM structure) has historically caused flickering issues on Windows classic.

I'll move this then since it seems that the fix isn't going to be download panel specific. Probably still a P3 but I'll reset the priority to be sure.

I wonder if we should use the child selector here, or if this would miss separators that this rule wants to match:

https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/browser/themes/shared/customizableui/panelUI.inc.css#1130-1133

Comment 8

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #7)

I wonder if we should use the child selector here, or if this would miss separators that this rule wants to match:

https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/browser/themes/shared/customizableui/panelUI.inc.css#1130-1133

I think there's probably .panel-subview-body things in the way in some cases, and in the bookmarks menu button I don't know if we set these classes on the submenus (ie nested bookmark folders etc.) as well.

I think the only menuseparator items we want to affect here are going to be in the bookmarks menu button. I'd be fine with making the menuseparator rules specific to that. Does that sound like it'd work?

Comment 9

[Dão Gottwald [:dao]]

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

(In reply to Dão Gottwald [::dao] from comment #7)

I wonder if we should use the child selector here, or if this would miss separators that this rule wants to match:

https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/browser/themes/shared/customizableui/panelUI.inc.css#1130-1133

I think there's probably .panel-subview-body things in the way in some cases,

That alone doesn't sound like a problem, as the selectors could just include that. The previous selector already is #widget-overflow-mainView > .panel-subview-body > toolbarseparator. Perhaps we could just drop #widget-overflow-mainView > from that?

I think the only menuseparator items we want to affect here are going to be in the bookmarks menu button. I'd be fine with making the menuseparator rules specific to that. Does that sound like it'd work?

Or use menupopup.PanelUI-subView there?

Comment 10

[:Gijs (he/him)]

So just adding the class (.PanelUI-subView) doesn't help, because we don't set that class on the submenus.

In fact, the styling for the submenus looks noticeably broken on mac, because the submenu items don't get moz-appearance: none anymore. I'll see if I can find a regression window for that tomorrow or if I'm just imagining that it used to work that way...

Comment 11

[:Gijs (he/him)]

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

So just adding the class (.PanelUI-subView) doesn't help, because we don't set that class on the submenus.

In fact, the styling for the submenus looks noticeably broken on mac, because the submenu items don't get moz-appearance: none anymore. I'll see if I can find a regression window for that tomorrow or if I'm just imagining that it used to work that way...

This seems to have regressed when we wrote photon, and I guess isn't super important if we still haven't fixed it.

Comment 12

[Ricardo Silva Veloso]

On Firefox 75.0 / GNOME 3.34, using system's dark theme (on Firefox's Default theme), context menus are dark, but separators are almost invisible. On Firefox's Dark theme, the separators on bookmarks bar becomes invisible too (doesn't affect the context menus).

Comment 13

[Ricardo Silva Veloso]

Attachment: Screenshot showing a context menu in system's light theme

Comment 14

[Ricardo Silva Veloso]

Attachment: Screenshot showing a context menu in system's dark theme

Comment 15

[Ricardo Silva Veloso]

Attachment: Screenshot showing a context menu in system's light theme and Firefox's Dark theme

Comment 16

[:Gijs (he/him)]

(In reply to Ricardo Silva Veloso from comment #12)

On Firefox 75.0 / GNOME 3.34, using system's dark theme (on Firefox's Default theme), context menus are dark, but separators are almost invisible. On Firefox's Dark theme, the separators on bookmarks bar becomes invisible too (doesn't affect the context menus).

This is a different issue, please file a separate bug.

Comment 17

[Itiel]

Attachment: Bug 1532172 - Fix separators in the context menu for downloaded files in the Library r?gijs

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb3258d652a4
Fix separators in the context menu for downloaded files in the Library r=dao

Comment 19

[Alexandru Michis [:malexandru]]

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