firefox-devtools / ux

Firefox DevTools UX Community
Mozilla Public License 2.0
103 stars 21 forks source link

Limit grayscale text colors to 2 or 3 shades #48

Closed fvsch closed 5 years ago

fvsch commented 5 years ago

I'd love to remove some of the duplication in text colors. We currently have 8 (height!) global CSS variables for shades of grey text: https://searchfox.org/mozilla-central/source/devtools/client/themes/variables.css#47-55

With a bit of work, we can probably end up with a consistent set of 3 colors:

(We did similar work for icon colors in bug 1521340, where we landed on 3 colors: base, dimmed, and active (blue).)

@mcroud I would love your input on this. @nt1m As always, if you have some time, this might interest you too :)

nt1m commented 5 years ago

I'm all for anything that can simplify the current set of variables :)

fvsch commented 5 years ago

Usage count:

I would like to remove:

DPX-designer commented 5 years ago

Consolidating some of the lesser used variables to make a more refined set sounds beneficial to everyone. The proposal is to remove variables that are collectively used 56 times in the codebase. I'm not sure how we can determine which were created for a specific purpose or if we can clearly establish there is no reason for them to exist? (maybe we can!).

I wonder what the best method would be to preview the results of key use cases after we assign it a different grey shade? Not that I would expect a huge difference mind but a visual before and after screengrab would serve to document how we go about this?

Maybe I'm thinking this is a bigger job than it actually is 😆

fvsch commented 5 years ago

@mcroud We can try to identify where some old variables are used most and work on those tools specifically (I suspect all the --theme-content-color ones are older and used mostly in Inspector/Network/Memory). I wouldn't be too worried about regressing some colors or contrasts, we'll probably see it if it's a strong issue. In any case we can try to make progress panel-by-panel in a handful of bugs, if we think that lessens the risk.

The important part would be to try to come up with a palette of grayscale text colors we want to use, and their intended semantics. For instance:

  1. Primary/highlight (most contrasted with background, e.g. Grey 10 in the dark theme; used for example for selected tabs)
  2. Base text color (e.g. Grey 40 in the dark theme; personally I'd bump it to Grey 30 but that suggestion was not accepted when I worked on it last summer)
  3. Dimmed text color (used for secondary information, code comments) (maybe disabled stuff? but oftentimes disabled elements are just opacity: 0.5)

Our base color is, in practice, --theme-body-color. We can look at the other variables and see which ones are more contrasted and which ones are dimmer, and try to reverse-engineer (er, reverse-design) the intended semantics. Unify stuff, disregard what is too rare or specific or doesn't make sense, and here we go.

A lot of colors are abused, for example --theme-comment and --theme-comment-alt are sometimes used for icon colors.

fvsch commented 5 years ago

Here's a proposed conservative change:

   /* Text color */
   --theme-comment: var(--grey-50);
-  --theme-comment-alt: var(--grey-40);
   --theme-body-color: var(--grey-60);
   --theme-body-color-alt: var(--grey-50);
   --theme-body-color-inactive: var(--grey-40);
+  --theme-body-color-strong: var(--grey-80);
-  --theme-content-color1: var(--grey-80);
-  --theme-content-color2: var(--grey-60);
-  --theme-content-color3: var(--grey-45);

Rationale:

Not counting the "comment" color, this keeps 4 contrast steps for gray colors:

  1. High contrast ("strong")
  2. Base = medium-high contrast (--theme-body-color)
  3. Dimmed, barely passing WCAG contrast guidelines = for secondary text
  4. Inactive, not passing WCAG contrast guidelines = for inactive elements

Ideally, 3 steps would be enough, but I'm not sure what we should change, so let's keep it like this for now.

violasong commented 5 years ago

This sounds good! (Though I still think it's worth getting my "gray 43" #a4a4a4 in for markup's inactive attr :). Monochrome syntax highlighting gets kinda delicate!)

Btw, I was thinking about how you mentioned the Copy All Changes button looked a bit disabled. What if we change --theme-body-color to Gray 70? I'm trying it out now and it looks good to me.

fvsch commented 5 years ago

So we shipped those changes (including the --theme-body-color to Gray 70), it's not perfect everywhere but I think we can close this now that we have fewer variables for that in variables.css.