Unfocused text color in light theme has very low contrast

RESOLVED FIXED in Firefox 48

Status

()

Firefox
Developer Tools: Responsive Design Mode
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jryans, Assigned: jryans)

Tracking

Trunk
Firefox 48
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox47 affected, firefox48 fixed)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

Created attachment 8725753 [details]
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?

Updated

2 years ago
Flags: qe-verify-
(Assignee)

Comment 1

2 years ago
Created attachment 8725754 [details]
Dark Theme

Updated

2 years ago
Priority: -- → P3
Whiteboard: [multiviewport][triage][ux] → [multiviewport] [ux]
(Assignee)

Comment 2

2 years ago
: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)

Updated

2 years ago
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]

Updated

2 years ago
Priority: P3 → P2
Whiteboard: [multiviewport] → [multiviewport] [mvp-rdm]
(Assignee)

Updated

2 years ago
Assignee: nobody → jryans
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
Created attachment 8734402 [details]
MozReview Request: Bug 1252925 - Add inactive color var, cleanup toolbar styles. r=gl,bgrins

Review commit: https://reviewboard.mozilla.org/r/42217/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42217/
Attachment #8734402 - Flags: review?(gl)
Attachment #8734402 - Flags: review?(bgrinstead)
(Assignee)

Updated

2 years ago
Attachment #8734402 - Flags: ui-review?(hholmes)

Updated

2 years ago
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 7

2 years ago
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+
(Assignee)

Comment 9

2 years ago
(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.

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9ea54d71bf06
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.