Bug 194966 - Web Inspector: Dark Mode: Network Overview Graph segments have distracting white box shadow
Summary: Web Inspector: Dark Mode: Network Overview Graph segments have distracting wh...
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: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-22 16:19 PST by Joseph Pecoraro
Modified: 2019-02-25 22:40 PST (History)
7 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (1.85 KB, patch)
2019-02-22 16:20 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[IMAGE] Before (321.46 KB, image/png)
2019-02-22 16:20 PST, Joseph Pecoraro
no flags Details
[IMAGE] After (321.00 KB, image/png)
2019-02-22 16:20 PST, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (6.69 KB, patch)
2019-02-22 16:48 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (6.68 KB, patch)
2019-02-22 16:51 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (5.92 KB, patch)
2019-02-22 18:45 PST, Joseph Pecoraro
drousso: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-02-22 16:19:14 PST
Dark Mode: Network Overview Graph segments have distracting white box shadow

The box shadow is supposed to be the background of the timeline (white in light mode) and needs to be updated for dark mode.
Comment 1 Joseph Pecoraro 2019-02-22 16:20:05 PST
Created attachment 362779 [details]
[PATCH] Proposed Fix
Comment 2 Joseph Pecoraro 2019-02-22 16:20:20 PST
Created attachment 362780 [details]
[IMAGE] Before
Comment 3 Joseph Pecoraro 2019-02-22 16:20:34 PST
Created attachment 362782 [details]
[IMAGE] After
Comment 4 Nikita Vasilyev 2019-02-22 16:31:55 PST
Comment on attachment 362779 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/NetworkTimelineOverviewGraph.css:49
> +    .timeline-overview-graph.network > .graph-row > .timeline-record-bar > .segment:not(.inactive) {
> +        box-shadow: hsl(0, 0%, 21%) 1px 0 0;

This is fine, but we could also use a variable:

    .timeline-overview-graph.network > .graph-row > .timeline-record-bar > .segment:not(.inactive) {
        --box-shadow-color: white;
        box-shadow: var(--box-shadow-color) 1px 0 0;
    }

    @media (prefers-color-scheme: dark) {
         .timeline-overview-graph.network > .graph-row > .timeline-record-bar > .segment:not(.inactive) {
            --box-shadow-color: hsl(0, 0%, 21%);
        }
    }

Same for the 2nd rule.
Comment 5 Joseph Pecoraro 2019-02-22 16:48:49 PST
Created attachment 362787 [details]
[PATCH] Proposed Fix

Use variables now.
Comment 6 Joseph Pecoraro 2019-02-22 16:50:18 PST
Comment on attachment 362787 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineOverviewGraph.css:57
> +    background-color: var(--timeline-alternate-background-color);

Typo this should be --timeline-even.
Comment 7 Joseph Pecoraro 2019-02-22 16:51:23 PST
Created attachment 362788 [details]
[PATCH] Proposed Fix
Comment 8 Nikita Vasilyev 2019-02-22 16:54:41 PST
Patch doesn't apply to ToT.
Comment 9 Joseph Pecoraro 2019-02-22 18:45:40 PST
Created attachment 362811 [details]
[PATCH] Proposed Fix
Comment 10 Devin Rousso 2019-02-22 20:14:47 PST
Comment on attachment 362811 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/Variables.css:117
> +    --timeline-odd-background-color: var(--background-color-content);

I don't think we want to do this for light-mode.  We should keep it as `white`.  making it match the background color will cause it to "blend in" a bit.  I found that a border/shadow with this color almost made it seem like records were getting cut in half by other records (e.g. in the overview), rather than being laid on top of each other.

> Source/WebInspectorUI/UserInterface/Views/Variables.css:118
> +    --timeline-even-background-color: hsl(0, 0%, 96%);

Even and odd are weird choice of name.  How about "primary" and "alternate"?  I get that in these cases it makes sense (e.g. `:nth-child(even)` or `:nth-child(odd)` are actually used), but to someone just looking at Variables.css it might seem confusing.  At the very least, more description as to what is even/odd (e.g. legend items, or network record "bubbles") would be great.
Comment 11 Matt Baker 2019-02-25 09:57:33 PST
(In reply to Devin Rousso from comment #10)
> Comment on attachment 362811 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362811&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/Variables.css:117
> > +    --timeline-odd-background-color: var(--background-color-content);
> 
> I don't think we want to do this for light-mode.  We should keep it as
> `white`.  making it match the background color will cause it to "blend in" a
> bit.  I found that a border/shadow with this color almost made it seem like
> records were getting cut in half by other records (e.g. in the overview),
> rather than being laid on top of each other.
> 
> > Source/WebInspectorUI/UserInterface/Views/Variables.css:118
> > +    --timeline-even-background-color: hsl(0, 0%, 96%);
> 
> Even and odd are weird choice of name.  How about "primary" and "alternate"?
> I get that in these cases it makes sense (e.g. `:nth-child(even)` or
> `:nth-child(odd)` are actually used), but to someone just looking at
> Variables.css it might seem confusing.  At the very least, more description
> as to what is even/odd (e.g. legend items, or network record "bubbles")
> would be great.

The words even and odd also match the system semantic color names:

-apple-system-even-alternating-content-background
-apple-system-odd-alternating-content-background
Comment 12 Joseph Pecoraro 2019-02-25 10:55:55 PST
(In reply to Devin Rousso from comment #10)
> Comment on attachment 362811 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362811&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/Variables.css:117
> > +    --timeline-odd-background-color: var(--background-color-content);
>
> I don't think we want to do this for light-mode.  We should keep it as
> `white`.  making it match the background color will cause it to "blend in" a
> bit.

This doesn't change things. --background-color-content is already white in light mode.

    UserInterface/Views/Variables.css
    50:    --background-color-content: white;

> > Source/WebInspectorUI/UserInterface/Views/Variables.css:118
> > +    --timeline-even-background-color: hsl(0, 0%, 96%);
> 
> Even and odd are weird choice of name.  How about "primary" and "alternate"?
> I get that in these cases it makes sense (e.g. `:nth-child(even)` or
> `:nth-child(odd)` are actually used), but to someone just looking at
> Variables.css it might seem confusing.  At the very least, more description
> as to what is even/odd (e.g. legend items, or network record "bubbles")
> would be great.

I wasn't aware that this matches the -apple-system naming pattern, but I also support the odd/even. primary/alternate is fine but odd/even makes it very obvious that during a `*:nth-child(even)` that you're going to use the even variant and not look up what to use.
Comment 13 Devin Rousso 2019-02-25 13:45:44 PST
Comment on attachment 362811 [details]
[PATCH] Proposed Fix

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

rs=me

> Source/WebInspectorUI/UserInterface/Views/NetworkTimelineOverviewGraph.css:40
> +    box-shadow: var(--timeline-odd-background-color) 1px 0 0;

Rather than repeat the `box-shadow` declaration, you could have another variable that just defines the color, and just change that variable when `:nth-child(even)`.

    box-shadow: var(--active-segment-box-shadow-color) 1px 0 0;

    --active-segment-box-shadow-color: var(--timeline-odd-background-color);
    --active-segment-box-shadow-color: var(--timeline-even-background-color);

> Source/WebInspectorUI/UserInterface/Views/Variables.css:285
> +        --timeline-even-background-color: hsl(0, 0%, 25%);

I think we should add a comment here explaining that if `--background-color-content` gets changed, this should also be adjusted to match.
Comment 14 Joseph Pecoraro 2019-02-25 20:10:26 PST
> I think we should add a comment here explaining that if
> `--background-color-content` gets changed, this should also be adjusted to
> match.

If the odd color changes then we should probably adjust the even color, yes. I don't think a comment will help with that much. More likely if we (very unlikely) change --background-color-content we should see that we are changing the odd color. even may not need to change at all.
Comment 15 Joseph Pecoraro 2019-02-25 22:37:41 PST
https://trac.webkit.org/r242072
Comment 16 Radar WebKit Bug Importer 2019-02-25 22:40:49 PST
<rdar://problem/48391381>