Closed Bug 1252925 Opened 4 years ago Closed 4 years ago

Unfocused text color in light theme has very low contrast

Categories

(DevTools :: Responsive Design Mode, defect, P1)

defect

Tracking

(firefox47 affected, firefox48 fixed)

RESOLVED FIXED
Firefox 48
Iteration:
48.2 - Apr 4
Tracking Status
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

Details

(Whiteboard: [multiviewport] [mvp-rdm])

Attachments

(3 files)

Attached image Light Theme
The text color used with the light theme in, e.g. the dimension inputs and device select when unfocused is very light, almost invisible on the also light background.

Compare this to the dark theme, where we actually get light text on dark background.  Should we change to a different theme color?  Do we need a custom color instead?
Flags: qe-verify-
Priority: -- → P3
Whiteboard: [multiviewport][triage][ux] → [multiviewport] [ux]
:helenvholmes, what do you think about this?
Flags: needinfo?(hholmes)
Let's bump up the contrast on light theme to #999797. I agree, it is kind of light.

That doesn't seem to map to anything in our variables file right now; we might consider something like "--theme-body-color-inactive" for both themes.
Flags: needinfo?(hholmes)
Summary: [UX] Unfocused text color in light theme has very low contrast → Unfocused text color in light theme has very low contrast
Whiteboard: [multiviewport] [ux] → [multiviewport]
Priority: P3 → P2
Whiteboard: [multiviewport] → [multiviewport] [mvp-rdm]
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Iteration: --- → 48.2 - Apr 4
Priority: P2 → P1
Comment on attachment 8734402 [details]
MozReview Request: Bug 1252925 - Add inactive color var, cleanup toolbar styles. r=gl,bgrins

https://reviewboard.mozilla.org/r/42217/#review38797

The new variable seems fine with me.

Helen, we should separately review all the places where --theme-comment or --theme-splitter-color is used for text coloring but --theme-body-color-inactive would be a better fit.  In particular.. should --theme-comment go away and just get replaced by --theme-body-color-inactive?

::: devtools/client/responsive.html/index.css
(Diff revision 1)
>  
>  #global-toolbar {
> -  background-color: var(--theme-toolbar-background);
> -  border: 1px solid var(--theme-splitter-color);
>    border-radius: 2px;
> -  color: var(--theme-body-color-alt);

Looks like colors were removed on #global-toolbar and .viewport-toolbar but not re-added to .toolbar.  Is that OK?  It looks like the new -inactive color is added to some `.viewport-*` classes so maybe that's covering all of the contents inside of the viewport toolbar but not sure if that covers colors in global-toolbar
Attachment #8734402 - Flags: review?(bgrinstead) → review+
Attachment #8734402 - Flags: ui-review?(hholmes) → ui-review+
(In reply to Brian Grinstead [:bgrins] from comment #5)
> Helen, we should separately review all the places where --theme-comment or
> --theme-splitter-color is used for text coloring but
> --theme-body-color-inactive would be a better fit.  In particular.. should
> --theme-comment go away and just get replaced by --theme-body-color-inactive?

I agree. We might consider a stepped approach for colors? We've talked a bit about having a system for all of our grays:

--theme-gray-lightest
--theme-gray-lighter
--theme-gray-light
--theme-gray // middle value
--theme-gray-dark
--theme-gray-darker
--theme-gray-darkest

... something along those lines.
Comment on attachment 8734402 [details]
MozReview Request: Bug 1252925 - Add inactive color var, cleanup toolbar styles. r=gl,bgrins

Instead of introducing this new "toolbar" class, why not use directly the built-in devtools-toolbar class ?
Comment on attachment 8734402 [details]
MozReview Request: Bug 1252925 - Add inactive color var, cleanup toolbar styles. r=gl,bgrins

https://reviewboard.mozilla.org/r/42217/#review39521

I am noticing differences in the color of the global toolbar compare to the existing colors because of the removal of color: var(--theme-body-color-alt);
Attachment #8734402 - Flags: review?(gl) → review+
(In reply to Tim Nguyen [:ntim] from comment #7)
> Comment on attachment 8734402 [details]
> MozReview Request: Bug 1252925 - Add inactive color var, cleanup toolbar
> styles. r=gl,bgrins
> 
> Instead of introducing this new "toolbar" class, why not use directly the
> built-in devtools-toolbar class ?

It's not clear to me how devtools-toolbar helps here.  If there's a reason to switch to devtools-toolbar, let's have a separate bug for that task.
https://reviewboard.mozilla.org/r/42217/#review38797

> Looks like colors were removed on #global-toolbar and .viewport-toolbar but not re-added to .toolbar.  Is that OK?  It looks like the new -inactive color is added to some `.viewport-*` classes so maybe that's covering all of the contents inside of the viewport toolbar but not sure if that covers colors in global-toolbar

I had removed color on purpose to simplify the two toolbars, but I guess this makes the global toolbar diverge from Helen's mockups.  I'll restore the previous color for it.  The viewport toolbor should not need the color set, since it flows down from the root.
https://hg.mozilla.org/mozilla-central/rev/9ea54d71bf06
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.