Bug 194966 – Web Inspector: Dark Mode: Network Overview Graph segments have distracting white box shadow

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 362779 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 362780 [details]
[IMAGE] Before

[Joseph Pecoraro]

Created attachment 362782 [details]
[IMAGE] After

[Nikita Vasilyev]

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.

[Joseph Pecoraro]

Created attachment 362787 [details]
[PATCH] Proposed Fix

Use variables now.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 362788 [details]
[PATCH] Proposed Fix

[Nikita Vasilyev]

Patch doesn't apply to ToT.

[Joseph Pecoraro]

Created attachment 362811 [details]
[PATCH] Proposed Fix

[Devin Rousso]

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.

[Matt Baker]

(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

[Joseph Pecoraro]

(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.

[Devin Rousso]

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.

[Joseph Pecoraro]

> 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.

[Joseph Pecoraro]

https://trac.webkit.org/r242072

[Radar WebKit Bug Importer]

<rdar://problem/48391381>