Bug 201442 - Web Inspector: Dark Mode: Overview Timeline View has jarring white box-shadows in dark mode
Summary: Web Inspector: Dark Mode: Overview Timeline View has jarring white box-shadow...
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-09-03 17:42 PDT by Joseph Pecoraro
Modified: 2019-09-10 10:14 PDT (History)
5 users (show)

See Also:


Attachments
[IMAGE] Before (1.18 MB, image/png)
2019-09-03 17:43 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Before - Inactive Selection (954.46 KB, image/png)
2019-09-03 17:43 PDT, Joseph Pecoraro
no flags Details
[IMAGE] After (1.18 MB, image/png)
2019-09-03 17:44 PDT, Joseph Pecoraro
no flags Details
[IMAGE] After - Inactive Selection (952.78 KB, image/png)
2019-09-03 17:44 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (3.78 KB, patch)
2019-09-03 17:45 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (12.17 KB, patch)
2019-09-06 13:00 PDT, Joseph Pecoraro
drousso: review+
Details | Formatted Diff | Diff
[IMAGE] After - Odd Row (789.66 KB, image/png)
2019-09-06 13:00 PDT, Joseph Pecoraro
no flags Details
[IMAGE] After - Even Row (834.15 KB, image/png)
2019-09-06 13:00 PDT, Joseph Pecoraro
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-09-03 17:42:52 PDT
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
Comment 1 Radar WebKit Bug Importer 2019-09-03 17:43:14 PDT
<rdar://problem/54999794>
Comment 2 Joseph Pecoraro 2019-09-03 17:43:34 PDT
Created attachment 377938 [details]
[IMAGE] Before
Comment 3 Joseph Pecoraro 2019-09-03 17:43:51 PDT
Created attachment 377939 [details]
[IMAGE] Before - Inactive Selection
Comment 4 Joseph Pecoraro 2019-09-03 17:44:01 PDT
Created attachment 377940 [details]
[IMAGE] After
Comment 5 Joseph Pecoraro 2019-09-03 17:44:11 PDT
Created attachment 377941 [details]
[IMAGE] After - Inactive Selection
Comment 6 Joseph Pecoraro 2019-09-03 17:45:00 PDT
Created attachment 377942 [details]
[PATCH] Proposed Fix
Comment 7 Devin Rousso 2019-09-03 19:09:47 PDT
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”?
Comment 8 Joseph Pecoraro 2019-09-04 10:57:19 PDT
(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.
Comment 9 Devin Rousso 2019-09-06 11:24:55 PDT
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?
Comment 10 Joseph Pecoraro 2019-09-06 13:00:11 PDT
Created attachment 378221 [details]
[PATCH] Proposed Fix
Comment 11 Joseph Pecoraro 2019-09-06 13:00:33 PDT
Created attachment 378222 [details]
[IMAGE] After - Odd Row
Comment 12 Joseph Pecoraro 2019-09-06 13:00:44 PDT
Created attachment 378223 [details]
[IMAGE] After - Even Row
Comment 13 Devin Rousso 2019-09-09 06:39:55 PDT
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).
Comment 14 Joseph Pecoraro 2019-09-09 12:03:08 PDT
https://trac.webkit.org/changeset/249655/webkit