Closed Bug 1694384 Opened 7 months ago Closed 7 months ago

Poor contrast on hover/active with proton appmenu with dark system theme.

Categories

(Firefox :: Menus, defect, P1)

Desktop
Linux
defect

Tracking

()

VERIFIED FIXED
88 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox86 --- unaffected
firefox87 --- disabled
firefox88 --- verified

People

(Reporter: emilio, Assigned: mconley)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, Whiteboard: [proton-hamburger-menu])

Attachments

(2 files, 1 obsolete file)

Attached image image.png

https://searchfox.org/mozilla-central/rev/f47a4b67643b3048ef9a2e2ac0c34edf6d1ebff3/browser/themes/shared/customizableui/panelUI.inc.css#64-67 is specifying a hardcoded background but inheriting a system color. That's not great if the system color is light :)

Component: CSS Parsing and Computation → Menus
Product: Core → Firefox

Not sure if you want to take something like this or do something else,
really your call, but I did this so I may as well post it.

The opacity change helps with the regular dark theme as well.

Assignee: nobody → emilio
Status: NEW → ASSIGNED
Attachment #9204844 - Attachment is obsolete: true

This is assigned, but it looks like a straight dupe of bug 1693010?

Severity: -- → S3
OS: Unspecified → Linux
Priority: -- → P1
Hardware: Unspecified → Desktop
See Also: → 1693010
Whiteboard: [proton-hamburger-menu]

Yeah, I had a patch but quoting Mike:

The Proton dark theme is still being defined, and we should have a clearer path forward for how to apply it later this week. I'm going to resign from this review until then.

So I decided to abandon the revision from now, we can revive it later if needed.

Assignee: emilio → nobody
Status: ASSIGNED → NEW

This should, I think, offer some relief to folks using the current built-in
dark theme.

Assignee: nobody → mconley
Status: NEW → ASSIGNED

Hey ntim, is this the right idea? What I've got here is roughly:

:root {
  (proven colors that we've shipped before for things)
}

@supports -moz-bool-pref("browser.proton.enabled") {
  :root {
    (non color things)
  }

  :root:not(:-moz-lwtheme) {
    (Proton default theme color values)
  }
}

This seems to improve the AppMenu for the built-in themes. Am I on the right track here?

Flags: needinfo?(ntim.bugs)

Speaking with ntim, I believe he's recommending that I do the following:

  1. In browser/themes/linux/customizableui/panelUI.css, make sure that the colors set on the :root are from the "known proven set" of colours that work on Linux.
  2. Inside of panelUI.inc.css, wrap the :root:not(:-moz-lwtheme) collection of colours with a @media not (prefers-contrast) query
Flags: needinfo?(ntim.bugs)
Attachment #9205542 - Attachment description: Bug 1694384 - Separate Proton AppMenu colors into a separate rule block. r?ntim! → Bug 1694384 - Separate Proton AppMenu colors into a separate rule block in an include file. r?ntim!
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/172ab28afcf1
Separate Proton AppMenu colors into a separate rule block in an include file. r=harry
Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
Duplicate of this bug: 1693909
Duplicate of this bug: 1693010
Duplicate of this bug: 1693904

Verified - Fixed in release 88, beta 89.0b5 and latest Nightly 90.0a1 (04-28-2021) using Ubuntu 20.04 and Windows 10 with dark system theme.

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