1568794 - Consolidate --theme-bg-yellow and --theme-contrast-background variables

Status: RESOLVED FIXED

(DevTools :: General, task, P3)

Description

[Florens Verschelde :fvsch]

We introduced the --theme-bg-yellow variable in bug 1541278, but I think we already had a --theme-contrast-background variable that served a very similar purpose?

We also introduced the .theme-bg-yellow-contrast class on top of the existing .theme-bg-contrast class.

Vikas, Micah, do you think we could unify both styles to avoid duplication? One design goal was to avoid orange highlights because orange is reserved for the Firefox brand and also it's kinda ugly as a background, but with the changes in bug 1541278 we're keeping it around for some things, which feels strange. Was there a specific reason for that?

I'd suggest keeping the --theme-contrast-background name, because it's maybe more semantic and less implementation-specific than just saying "yellow background". What if we want to change those highlights to green or some other hue in the future? (Small risk, but this very recent experience about changing orange backgrounds to yellow is a hint that sometimes things change ^^)

Another small risk with --theme-bg-yellow is that anyone who wants to make a warning message of some sort will now reach for this variable, and we'll end up in a situation where we can't change it because it impacts too many things. (We're working on landing theme variables for error and warning messages, and these too should have semantic names.)

Comment 1

[Florens Verschelde :fvsch]

On a "code style" note, we have a set of --theme-*-background variables, so creating a single --theme-bg-* variable is not ideal. If we need a new variable, we could rename it something like --theme-flash-background.

Comment 2

[Vikas Prasad Mahato [:vikas_mahato]]

Vikas, Micah, do you think we could unify both styles to avoid duplication?

Hi Florens,
I can try unifying both the styles.

Comment 3

[Florens Verschelde :fvsch]

That would be excellent. :)

A first step would be to review where we are using both variables to see if it's a similar use case and using only the --theme-contrast-background (or --theme-flash-highlight-background maybe, because "contrast" is a bit ambiguous in the first place) would make sense.

Comment 4

[Micah [:mtigley] (she/her)]

I think this would be fine. The only other consumer of --theme-contrast-background is the a11y inspector, which uses it for flashing accessible mutations much like the markup view.

Comment 5

[Florens Verschelde :fvsch]

Attachment: markup-drag-indicator-color-tweak.png

Turns out --theme-contrast-background is also used in the Markup view for the drag target border.
Currently in the light theme the node dragging border colors are:

• original location: black (too contrasted, I think we want more attention on the target location instead?)
• target location: orange (not contrasted enough?)

I looked at what Chrome does and it was a bit buggy in my tests but they're using a blue border for the target location, so I tried that + a medium gray for the original location.

Martin, does that look alright to you?

Comment 6

[Florens Verschelde :fvsch]

Attachment: markup-dom-update-background.png

Also changed the update highlight text color to white (contrast: 7.1) instead of black (contrast: 2.93) in the dark mode. Light mode was already okay.

Comment 7

[Florens Verschelde :fvsch]

Attachment: Bug 1568794 - Consolidate bg-yellow and contrast-background devtools theme variables; r=gl

Comment 8

[Martin Balfanz [:mbalfanz]]

Yes, this looks good to me! Esp. the text color change is a big improvement.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/49d28cc0bf99
Consolidate bg-yellow and contrast-background devtools theme variables; r=gl

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/49d28cc0bf99