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
<rdar://problem/54999794>
Created attachment 377938 [details] [IMAGE] Before
Created attachment 377939 [details] [IMAGE] Before - Inactive Selection
Created attachment 377940 [details] [IMAGE] After
Created attachment 377941 [details] [IMAGE] After - Inactive Selection
Created attachment 377942 [details] [PATCH] Proposed Fix
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”?
(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 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?
Created attachment 378221 [details] [PATCH] Proposed Fix
Created attachment 378222 [details] [IMAGE] After - Odd Row
Created attachment 378223 [details] [IMAGE] After - Even Row
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).
https://trac.webkit.org/changeset/249655/webkit