Bug 197955 - Web Inspector: Dark Mode: Fix colors in network table waterfall container
Summary: Web Inspector: Dark Mode: Fix colors in network table waterfall container
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-16 12:27 PDT by Jamal Nasser
Modified: 2019-05-17 16:08 PDT (History)
4 users (show)

See Also:


Attachments
[Screenshot] Bug (86.12 KB, image/png)
2019-05-16 12:27 PDT, Jamal Nasser
no flags Details
[Screenshot] Patch applied (130.42 KB, image/png)
2019-05-16 12:28 PDT, Jamal Nasser
no flags Details
Patch (1.95 KB, patch)
2019-05-16 12:44 PDT, Jamal Nasser
no flags Details | Formatted Diff | Diff
Patch (3.13 KB, patch)
2019-05-17 13:09 PDT, Jamal Nasser
no flags Details | Formatted Diff | Diff
Patch (3.12 KB, patch)
2019-05-17 13:17 PDT, Jamal Nasser
no flags Details | Formatted Diff | Diff
Patch (2.39 KB, patch)
2019-05-17 13:41 PDT, Jamal Nasser
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jamal Nasser 2019-05-16 12:27:45 PDT
Created attachment 370055 [details]
[Screenshot] Bug

The waterfall-container area under the network tab is currently using colors that don't suit the dark mode well.
Specifically, changed the blue circles to be brighter and the 'fullscreen' grey area to be dimmer. The latter is also now using a preexisting color variable instead of a custom color.
Comment 1 Jamal Nasser 2019-05-16 12:28:24 PDT
Created attachment 370056 [details]
[Screenshot] Patch applied
Comment 2 Jamal Nasser 2019-05-16 12:44:11 PDT
Created attachment 370060 [details]
Patch
Comment 3 Devin Rousso 2019-05-16 14:11:58 PDT
Comment on attachment 370060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370060&action=review

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:211
> +    background-color: var(--dashboard-background-color);

We probably don't want to reuse a variable like this, since the fullscreen event area has nothing to do with the dashboard.  Also, FWIW I think we might want to make it slightly lighter, almost halfway between what you have now `hsl(0, 0%, 36%)` and what was there before `hsla(0, 0%, 75%, 0.75)`.  Maybe a regular `grey` or a `hsla(0, 0%, 50%)`?
Comment 4 Devin Rousso 2019-05-16 14:12:22 PDT
Comment on attachment 370060 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370060&action=review

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:187
> +    background-color: var(--selected-background-color-active);

This color is WAY better :)
Comment 5 Jamal Nasser 2019-05-17 13:09:03 PDT
Created attachment 370137 [details]
Patch
Comment 6 Jamal Nasser 2019-05-17 13:13:55 PDT
(In reply to Devin Rousso from comment #3)
> Comment on attachment 370060 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370060&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:211
> > +    background-color: var(--dashboard-background-color);
> 
> We probably don't want to reuse a variable like this, since the fullscreen
> event area has nothing to do with the dashboard.  Also, FWIW I think we
> might want to make it slightly lighter, almost halfway between what you have
> now `hsl(0, 0%, 36%)` and what was there before `hsla(0, 0%, 75%, 0.75)`. 
> Maybe a regular `grey` or a `hsla(0, 0%, 50%)`?

50% is a little too bright in my opinion. This is what 40% looks like: https://i.imgur.com/d8Q1V8A.png

I think that fits better than 50%:
https://i.imgur.com/Rz0RRuV.png
Comment 7 Jamal Nasser 2019-05-17 13:17:51 PDT
Created attachment 370139 [details]
Patch
Comment 8 Devin Rousso 2019-05-17 13:33:56 PDT
Comment on attachment 370139 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370139&action=review

r=me, nice fix!

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:211
> +    background-color: var(--network-waterfall-fullscreen-area-color);

There's no reason to make a variable if it's only really used once (or even twice).  Please just inline it.
Comment 9 Jamal Nasser 2019-05-17 13:41:17 PDT
Created attachment 370143 [details]
Patch
Comment 10 WebKit Commit Bot 2019-05-17 16:07:27 PDT
Comment on attachment 370143 [details]
Patch

Clearing flags on attachment: 370143

Committed r245484: <https://trac.webkit.org/changeset/245484>
Comment 11 WebKit Commit Bot 2019-05-17 16:07:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2019-05-17 16:08:24 PDT
<rdar://problem/50909676>