Closed Bug 1568794 Opened 8 months ago Closed 6 months ago

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

Categories

(DevTools :: General, task, P3)

task

Tracking

(firefox71 fixed)

RESOLVED FIXED
Firefox 71
Tracking Status
firefox71 --- fixed

People

(Reporter: fvsch, Assigned: fvsch)

Details

Attachments

(3 files)

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

Flags: needinfo?(vikasmahato0)
Flags: needinfo?(mtigley)

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.

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

Hi Florens,
I can try unifying both the styles.

Flags: needinfo?(vikasmahato0)

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.

Priority: -- → P3

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.

Flags: needinfo?(mtigley)

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?

Flags: needinfo?(mbalfanz)

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.

Assignee: nobody → florens
Status: NEW → ASSIGNED

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

Flags: needinfo?(mbalfanz)
Pushed by florens@fvsch.com:
https://hg.mozilla.org/integration/autoland/rev/49d28cc0bf99
Consolidate bg-yellow and contrast-background devtools theme variables; r=gl
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 71
You need to log in before you can comment on or make changes to this bug.