287.80 KB, image/png
46 bytes, text/x-phabricator-request
|Details | Review|
151.75 KB, image/png
The item came up during a quick application panel UI review: the color of the text of the empty screen in light theme should be --grey-90 instead of --grey-60. (I'll just refer to light theme values in the rest of this bug to keep it short). However the default color for devtools text is --theme-body-color which is grey-60 in light theme. The accessibility panel was suggested as a reference, and in the empty screen, the color is set by reusing --theme-toolbar-color, which happens to be grey-90. However, this is clearly not the intended role of this css variable so we should either: - set theme-body-color to grey-90 - create a new variable with a correct name. In this case we need guidelines to know when to use grey-60 rather than grey-90 Current usage of --theme-toolbar-color https://searchfox.org/mozilla-central/search?q=--theme-toolbar-color&case=false®exp=false&path=
Looking at the current `--theme-body-color*` and `--theme-content-color*` colors, the strongest contrast in light theme is grey-80 (#2a2a2e) on white. There are a few instances of darker text, either grey-90 or full black, either as --theme-toolbar-color or specific overrides in some stylesheets.
I’ve been working on this topic in UX issue #4 https://github.com/devtools-html/ux/issues/4 I looked at the base body color and its variants, and found that it was probably not enough contrasted in the dark theme, perhaps due to some values being inverted. Victoria suggested going for the second line in my proposal (the conservative fix, rather than the “let’s also make all text color one step stronger” fix).
I have a test fix on Phabricator: https://phabricator.services.mozilla.com/D4300 I’ll explain the rationale for those changes below. ## Current situation After looking at the code more, it seems that text colors are set in in group of variables: --theme-comment --theme-comment-alt --theme-body-color --theme-body-color-alt --theme-body-color-inactive --theme-content-color1 --theme-content-color2 --theme-content-color3 Plus, as you described, more specific variables like `--theme-toolbar-color` which may be abused because these generic colors were not contrasted enough (or some other reason). Looking at the values for both themes, there are some outstanding issues: 1. As mentioned before, in the dark theme, the base `--theme-body-color` has poor contrast. This is a problem because it’s used on all pages, with some tools specifying other colors for 90% of text and other tools specifying only 50% of colors at most. And the Settings page uses this base color for everything. 2. Both `--theme-body-color*` and `--theme-content-color*` color sets have 3 levels of contrast, but they don’t follow the same direction (decreasing or increasing contrast) in light and dark themes. This is bound to create situations where a contrasted choice in the light theme can look muted in the dark theme, and the other way round. 3. It’s not clear why there are 2 sets of colors, the “body” text colors and the “content” text colors, both with 3 levels of contrast. 4. Finally, `--theme-comment-alt` looks like it’s used mostly to draw borders in the old debugger, it could probably be removed. It's set very inconsistently in the light theme (muted) and the dark theme (contrasted). ## Test with conservative changes Following Victoria’s indications in https://github.com/devtools-html/ux/issues/4#issuecomment-415204138 I set up to improve the dark theme base color, and if possible avoid inconsistencies between the dark and light theme. For some colors, I kept the same contrast level but tried to align shades of gray with the Photon Grey palette: https://design.firefox.com/photon/visuals/color.html#grey A lot of pre-Photon gray values had a noticeable blue tint, for instance #708090. Photon grays are often just 1% saturation, e.g. #808082. Since we are currently using both, it results in a few inconsistencies. This is what I tried for the light theme: /* 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-content-color1: var(--grey-80); --theme-content-color2: var(--grey-60); --theme-content-color3: var(--grey-45); Both “body” and “content” color sets go from higher contrast to low contrast. Similarly for the dark theme: /* Text color */ --theme-comment: var(--grey-45); --theme-comment-alt: var(--grey-50); --theme-body-color: var(--grey-40); --theme-body-color-alt: var(--grey-45); --theme-body-color-inactive: var(--grey-50); --theme-content-color1: var(--grey-30); --theme-content-color2: var(--grey-40); --theme-content-color3: var(--grey-55); (I had to create the grey-45 and grey-55 inter-values, which are not part of Photon, because Photon has some big gaps in its Grey palette which make sense for a white background but not so much for a dark one.) Here are screenshots of the result for the dark theme: https://www.dropbox.com/sh/jeuja2449o205j6/AADRtc4dA3HzGFYYaBnvQiwVa?dl=0 The change is subtle and mostly impacts some secondary toolbar text like the “Browser styles” label in Inspector. It more dramatically affects the Settings page (yay!) and would affect the Network monitor too if we hadn’t fixed that a little while ago. ## Further possible changes It might be worthwhile to merge the “body” and “content” colors, into a single scale of grey tones. For example, for the light theme: /* Text color */ --theme-comment: var(--grey-50); --theme-content-color-base: var(--grey-60); /* or grey-70 */ --theme-content-color-high: var(--grey-90); --theme-content-color-low: var(--grey-50); /* or grey-60 */ --theme-content-color-inactive: var(--grey-40); /* or grey-45 */ That means replacing a lot of variable uses in the codebase, though.
Comment on attachment 9004097 [details] Bug 1464348 - Tweak base devtools colors to fix theme inconsistencies; r=jdescottes Julian Descottes [:jdescottes][:julian] has approved the revision.
Attachment #9004097 - Flags: review+
Assignee: nobody → florens
Status: NEW → ASSIGNED
Summary: Clarify usage of --theme-toolbar-color vs --theme-body-color → Fix contrast issues in DevTools dark theme
Comment on attachment 9004097 [details] Bug 1464348 - Tweak base devtools colors to fix theme inconsistencies; r=jdescottes Victoria: I realize this was already discussed on GitHub, but could you confirm you are ok with the patch here? Florens already added several screenshots. There is also a try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e1af31889ba862db78340d336e71ab4358c535a if you want a build.
Attachment #9004097 - Flags: ui-review?(victoria)
According to Julian’s comment on Phabricator, the text contrast for Settings, Netmonitor etc. in dark theme may have been regressed by: https://bugzilla.mozilla.org/show_bug.cgi?id=1421389 The purpose of that bug was to lower the contrast of scrollbars, especially in the Debugger. I think touching the body color might not be the best approach (given the text contrast issues), but it might be possible to target the scrollbar color and contrast more specifically. Happy to take a follow-up bug for scrollbars, or try to include this concern in this bug.
(In reply to Florens Verschelde from comment #7) > According to Julian’s comment on Phabricator, the text contrast for > Settings, Netmonitor etc. in dark theme may have been regressed by: > https://bugzilla.mozilla.org/show_bug.cgi?id=1421389 > > The purpose of that bug was to lower the contrast of scrollbars, especially > in the Debugger. Sidebars, not scrollbars :) Basically, all the sidebars had a much darker background before that bug. You can see a comparison screenshot at https://pbs.twimg.com/media/DPLwSvDVQAA03Bi.jpg:large
Woops, not enough sleep ^^
I see, so according to that screenshot: - we slightly upped the sidebar background's luminosity, which is alright; - and we lowered the body-color’s luminosity As a result we had a drop in contrast for text using the body color: - from 9.14:1 to 5.39:1 in sidebars - from 6.68:1 to 4.48:1 when on top of the body-background Looks like the changes went a bit too far, and restoring the #b1b1b3 base text color is indeed the right move. We should end up with these contrasts: - 8.03:1 in sidebars - 6.68:1 on top of the body-background For the record: - minimum accessible contrast is 4.50:1, and it will looked dimmed (try reading a lot of `color:#777;background:#fff;` text) - maximum contrast is 21:0 - it's good to shoot for values between 6:1 (base, a bit muted) and 15:1 (high contrast) You can check contrast ratios using tools such as https://webaim.org/resources/contrastchecker/
Comment on attachment 9004097 [details] Bug 1464348 - Tweak base devtools colors to fix theme inconsistencies; r=jdescottes Florens, thank you for this work, including the CSS improvements and the screenshots! Looking great. Re: that sidebar bug: Yes, last year I received a bunch of feedback that those original Debugger sidebars were too high-contrast. The sense I got was that the source list is only used in tiny bits at a time so people wanted it to mostly fade into the background for their main work. (Other sidebars in DevTools, like those in the Inspector, get a lot more usage, so at one point I considered two different sidebar styles.) It seems to me that the sidebar contrast would ideally be the same or a bit less than the body contrast. E.g. about 6:1? So something like #99999E, which could be awkwardly considered Gray-42. Let me know what you think of this. A few minor things: - I think the font slider tracks should be a step darker (perhaps the inactive color?) - The [...] icon in the markup view gets darker, but should actually become brighter (:gl recently worked on the badges though, so changes may already be on the way) Can't wait to see this patch land soon! :D
Attachment #9004097 - Flags: ui-review?(victoria) → ui-review+
For the [...] icon in markup view: with :gl's fixes it looks like we're okay now. Font slider tracks: made one step darker, and aligned the toggle colors to match (in dark theme only, because in the light theme the toggle thumb is white and the slider thumb is gray so I didn't touch that). Another change I made was changing the search box colors to use more neutral Photon Grey shades instead of the previous blue-grays. I considered #0c0c0d as a background (same color as the main tab bar), but it was a bit much, so I went half-way to #141416. If these changes are okay for you, I think we can land.
Attachment #9004873 - Flags: ui-review?(victoria)
Comment on attachment 9004873 [details] inspector-contrast.png Looks great! Thanks for making the slider changes and good catch on those search box colors!
Attachment #9004873 - Flags: ui-review?(victoria) → ui-review+
Ah also, I was wondering if you had a chance to try the sidebars with the #99999E base color I mentioned to match the main body contrast of about 6:1 (in case that helped balance the prominence of the two types of panels, especially in Debugger). Either way I think this should be fine.
I opened an issue in Debugger to follow up on that: https://github.com/devtools-html/debugger.html/issues/6899
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/7d47dca5962e Tweak base devtools colors to fix theme inconsistencies; r=jdescottes
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Sounds good, thanks Florens!
You need to log in before you can comment on or make changes to this bug.