1737690 - DevTools theme notice uses dark-on-dark text

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P3)

Description

[Harry Twyford]

Attachment: Screen Shot 2021-10-25 at 5.28.15 PM.png

What were you doing?

I was on https://en.wikipedia.org/w/index.php?title=Special:CreateAccount&returnto=Main+Page and opened DevTools. I had Firefox Dark mode enabled.

What happened?

I got a message saying DevTools now follow the Firefox theme. The message was dark-on-dark. Screenshot attached.

What should have happened?

White text.

Comment 1

[Harry Twyford]

I had system Dark theme applied on macOS. Also, it looks like the same message in the Browser Toolbox uses white text, as expected.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:Honza, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I can't reproduce the issue on my machines (Win10), using Firefox Nightly (96)
The message I see is having white text on dark background.

Julian, can you repro?

Honza

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Solved this offline.

Tested on Win10 and Mac OS and we can't repro the issue.

Harry, is this still reproducible?
If yes, can you provide detailed steps to reproduce?
Do you have any Firefox custom theme installed?
Does the MacOS dark theme have an impact?

Honza

Comment 5

[Julian Descottes [:jdescottes]]

As discussed with Honza, even if we can't reproduce our current CSS for this notification is fragile:

.notificationbox .notification[data-type="new"] {
  color: -moz-DialogText;
  background-color: var(--theme-selection-background-hover);
}

We are mixing -moz-DialogText with a DevTools variable. We should rather only use DevTools variables. Let's make the change right now.
Keeping the needinfo for :harry, could still be interesting to understand why we didn't spot this in our tests.

Comment 6

[Harry Twyford]

Hm, I can no longer reproduce. But it makes sense that the cause of this is mixing a system CSS variable with a custom one. I probably had some combination of OS and Firefox theme settings that caused -moz-DialogText to be a light mode color but var(--theme-selection-background-hover); to be dark.

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Harry Twyford [:harry] from comment #6)

Hm, I can no longer reproduce. But it makes sense that the cause of this is mixing a system CSS variable with a custom one. I probably had some combination of OS and Firefox theme settings that caused -moz-DialogText to be a light mode color but var(--theme-selection-background-hover); to be dark.

Thank you for the update, Harry! And yes, agree, let's introduce CSS var for the bk color.
Julian is already working on it.

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1737690 - [devtools] Use a devtools css variable for the auto theme notification text color

Using --theme-contrast-color. Sadly --theme-contrast-background looks pretty bad in dark theme, so we can't use the background color.
But the text color itself is fine against --theme-selection-background-hover, so let's use this

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14b3fdb44b3c
[devtools] Use a devtools css variable for the auto theme notification text color r=Honza

Comment 10

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/14b3fdb44b3c

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 9249998 [details]
Bug 1737690 - [devtools] Use a devtools css variable for the auto theme notification text color

Beta/Release Uplift Approval Request

• User impact if declined: A devtools notification about the new "auto" theme might be unreadable (black on black). This is a minor thing, but we introduced this notification in cycle 95, so it would be great to fix this minor CSS issue.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): There is no risk here, we just update a css property to use a DevTools variable
• String changes made/needed:

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9249998 [details]
Bug 1737690 - [devtools] Use a devtools css variable for the auto theme notification text color

Low risk, approved for 95 beta 6, thanks.

Comment 13

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e4186ddea3b5

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1735359