Poor contrast of DOMContentLoaded summary text in dark theme

RESOLVED FIXED in Firefox 57

Status

DevTools
Netmonitor
P3
normal
RESOLVED FIXED
a year ago
a month ago

People

(Reporter: sebo, Assigned: jdescottes)

Tracking

55 Branch
Firefox 57
Dependency tree / graph

Firefox Tracking Flags

(firefox55 wontfix, firefox56 fix-optional, firefox57 fixed)

Details

MozReview Requests

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

Attachments

(3 attachments)

(Reporter)

Description

a year ago
Created attachment 8855694 [details]
Screenshot showing the poor contrast

The contrast of the blue DOMContentLoaded summary text on the dark gray background in the dark theme makes it hard to read.

Therefore the color should be changed to some lighter color.

Sebastian

Comment 1

a year ago
I've intentionally used #00f as colour because that's exactly the colour used in the waterfall ticks.
Definitively agree it could be improved though.

Comment 2

a year ago
> I've intentionally used #00f as colour because that's exactly the colour used in the waterfall ticks.

Then the waterfall ticks should be changed as well I guess. This is so much into the face (or eyes ;) that I wonder why this bug is not P1 (or at least P2). Really severe UI bug, I cannot even read the "DOMContentLoaded" without moving very near to the monitor. At the same time, it's probably easy to fix I guess (or hope).

Maybe check Chrome, there the colors are readable in the dark theme, too. 

The Red is not great either in my opinion, could be lighter too in the dark theme.

Comment 3

a year ago
(In reply to Jens from comment #2)
> > I've intentionally used #00f as colour because that's exactly the colour used in the waterfall ticks.
> 
> Then the waterfall ticks should be changed as well I guess. This is so much
> into the face (or eyes ;) that I wonder why this bug is not P1 (or at least
> P2). Really severe UI bug, I cannot even read the "DOMContentLoaded" without
> moving very near to the monitor. At the same time, it's probably easy to fix
> I guess (or hope).

Changing the waterfall ticks isn't that trivial though.

Comment 4

a year ago
(In reply to Tim Nguyen :ntim from comment #3)
> Changing the waterfall ticks isn't that trivial though.

Oh :-/ I'm curious why? I can't think of anything else than a CSS or JS variable to define a color. Even if it's a hard-coded color string I don't understand why this should be hard? Are we talking about the same thing, that is, single blue line?

Comment 5

a year ago
I recommend below settings...

[netmonitor.css]

.status-bar-label.dom-content-loaded {
  color: var(--theme-highlight-blue);
}

.status-bar-label.load {
  color: var(--theme-highlight-red);
}
Comment hidden (mozreview-request)
(Assignee)

Comment 7

11 months ago
(In reply to Tim Nguyen :ntim from comment #1)
> I've intentionally used #00f as colour because that's exactly the colour
> used in the waterfall ticks.
> Definitively agree it could be improved though.

The current colors are really not readable in dark theme so I think we should fix this asap and potentially uplift. 

We can have a follow up to make the colors of the lines match exactly the text. I think we would be fine hardcoding two colors for the lines in devtools/client/netmonitor/src/constants.js, one for light and one for dark theme. And listen to the theme-changed event to switch between the two. 

The patch here simply applies the suggestion from the last magicp's comment.
(Assignee)

Updated

11 months ago
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: -- → P3

Comment 8

11 months ago
mozreview-review
Comment on attachment 8899956 [details]
Bug 1354472 - use readable colors for netmonitor in dark theme;

https://reviewboard.mozilla.org/r/171286/#review176462

IMO, we should really have a fix for the waterfall ticks landing at the same time, but this part looks good to me.
Attachment #8899956 - Flags: review?(ntim.bugs) → review+

Comment 9

11 months ago
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/49655de50701
use readable colors for netmonitor in dark theme;r=ntim
https://hg.mozilla.org/mozilla-central/rev/49655de50701
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Reporter)

Comment 11

11 months ago
(In reply to Tim Nguyen :ntim from comment #8)
> IMO, we should really have a fix for the waterfall ticks landing at the same
> time, but this part looks good to me.

Agree. Only changing the colors in the status bar causes an inconsistency between them. So, IMO, the patch should not have landed without changing those, too.

Unfortunately, as Julian already noted, they are defined as constants in JS[1]. Best approach would be to somehow reuse the colors defined in CSS.

Sebastian

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/constants.js#235,238
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

11 months ago
Depends on: 1393395

Comment 12

11 months ago
(In reply to Sebastian Zartner [:sebo] from comment #11)
> Unfortunately, as Julian already noted, they are defined as constants in
> JS[1]. Best approach would be to somehow reuse the colors defined in CSS.
> 
> Sebastian
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/
> src/constants.js#235,238


I opened bug 1393395 for this.

Updated

11 months ago
Status: REOPENED → RESOLVED
Last Resolved: 11 months ago11 months ago
Resolution: --- → FIXED
(In reply to Sebastian Zartner [:sebo] from comment #11)
> (In reply to Tim Nguyen :ntim from comment #8)
> > IMO, we should really have a fix for the waterfall ticks landing at the same
> > time, but this part looks good to me.
> 
> Agree. Only changing the colors in the status bar causes an inconsistency
> between them. So, IMO, the patch should not have landed without changing
> those, too.
> 

I want to uplift this, that's why I don't want to have a bigger patch with JS changes here.

Regarding the inconsistency, the colors are still shades of blue and red. They're not the same shade of blue/red, agreed, but is it really inconsistent? In my opinion having unreadable text is a bug. Having the exact same shade for ticks and text is an improvement.

Sebastian: is there a particular theme/setting which makes it feel inconsistent?
Flags: needinfo?(sebastianzartner)
status-firefox55: affected → wontfix
status-firefox56: --- → fix-optional

Comment 14

11 months ago
Created attachment 8900703 [details]
Inconsistent colors

> Regarding the inconsistency, the colors are still shades of blue and red. They're not the same shade of blue/red, agreed, but is it really inconsistent? In my opinion having unreadable text is a bug. Having the exact same shade for ticks and text is an improvement.

It does look really inconsistent in the dark theme.

The red color looks OK, but the blue looks really different from the corresponding waterfall tick.
(Reporter)

Comment 15

11 months ago
(In reply to Julian Descottes [:jdescottes] from comment #13)
> Sebastian: is there a particular theme/setting which makes it feel
> inconsistent?

As all themes use the same colors, the inconsistency exists in all three of them. As Tim says, the colors look quite different.

> I want to uplift this, that's why I don't want to have a bigger patch with
> JS changes here.

Ok, so I think a good compromise is to just change the two constants to the values used in the CSS variables[1], i.e.

  DOMCONTENTLOADED_TICKS_COLOR_RGBA: [0, 136, 204, 128]
  LOAD_TICKS_COLOR_RGBA: [237, 38, 85, 128]

(Maybe with adjusted alpha value if they are too pale.)

Sebastian

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/variables.css#43,48
Flags: needinfo?(sebastianzartner)

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.