Bug 201442 – Web Inspector: Dark Mode: Overview Timeline View has jarring white box-shadows in dark mode

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

[Joseph Pecoraro]

Overview Timeline View has jarring white box-shadows in dark mode

* Steps to Reproduce:
1. Inspect this page
2. Show Timeline tab
3. Reload
4. Show Overview Timeline
  => White box-shadows

[Radar WebKit Bug Importer]

<rdar://problem/54999794>

[Joseph Pecoraro]

Created attachment 377938 [details]
[IMAGE] Before

[Joseph Pecoraro]

Created attachment 377939 [details]
[IMAGE] Before - Inactive Selection

[Joseph Pecoraro]

Created attachment 377940 [details]
[IMAGE] After

[Joseph Pecoraro]

Created attachment 377941 [details]
[IMAGE] After - Inactive Selection

[Joseph Pecoraro]

Created attachment 377942 [details]
[PATCH] Proposed Fix

[Devin Rousso]

I actually prefer the white ring in dark mode. I think it differentiates the records MUCH more clearly than a darker ring. Perhaps we could use a light grey, rather than white/“black”?

[Joseph Pecoraro]

(In reply to Devin Rousso from comment #7)
> I actually prefer the white ring in dark mode.
> I think it differentiates the records MUCH more clearly than a darker ring.
> Perhaps we could use a light grey, rather than white/“black”?

The intent of the box shadow is to separate near-by records. The fact that some records get it but not others makes the white jarring. The color of the record itself makes it clear that there is a record at all.

[Devin Rousso]

Comment on attachment 377942 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.css:63
> +    --timeline-box-shadow-color: var(--timeline-odd-background-color);

NIT: I'd make this name more specific (e.g. `--timeline-record-bar-box-shadow-color`) so its indented usage is clearer.

> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.css:64
> +    --timeline-box-shadow-selected-inactive-color: hsl(0, 0%, 83%);

Could we just use `--selected-background-color-unfocused` instead (as in remove this variable entirely and use that instead)?

> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.css:67
> +.timeline-view.overview > .data-grid tr:nth-child(even) {

This makes me think that the box-shadow's color should match the background color of that row, which doesn't seem to be the case in the screenshots.  That's what the Network timeline does in the overview, and I personally like that quite a lot.  Perhaps we can just use `--even-zebra-stripe-row-background-color` and `--odd-zebra-stripe-row-background-color` instead?

[Joseph Pecoraro]

Created attachment 378221 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 378222 [details]
[IMAGE] After - Odd Row

[Joseph Pecoraro]

Created attachment 378223 [details]
[IMAGE] After - Even Row

[Devin Rousso]

Comment on attachment 378221 [details]
[PATCH] Proposed Fix

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

r-me, nice fix!

We spoke about this in person, but I think we should switch it such that the first row is the one with the "odd" background color (e.g. it's different from the background color of the surrounding content, like a navigation bar).  This way, it's easier to identify where the table begins, because the first row has a different background color.  I'm also fine with that being a followup.

> Source/WebInspectorUI/ChangeLog:27
> +        (:root):

I think this is missing a `* UserInterface/Views/Main.css:`

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1145
> +        this._dataTableElement.classList.toggle("even-first-zebra-stripe", !!(topHiddenRowCount % 2));

We should probably add a comment like "if there are an odd number of rows hidden, meaning that the first visible row must be an even row".

> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.css:97
> +        --timeline-record-bar-selected-inactive-box-shadow-color: hsla(0, 0%, 35%);

This can just be `hsl()`.

> Source/WebInspectorUI/UserInterface/Views/OverviewTimelineView.css:101
> +        --timeline-record-bar-selected-inactive-box-shadow-color: hsla(0, 0%, 38%);

Ditto (97).

> Source/WebInspectorUI/UserInterface/Views/Table.js:1130
> +        this._listElement.classList.toggle("even-first-zebra-stripe", !!(topHiddenRowCount % 2));

Ditto (DataGrid.js:1145).

[Joseph Pecoraro]

https://trac.webkit.org/changeset/249655/webkit