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.
Created attachment 362779 [details] [PATCH] Proposed Fix
Created attachment 362780 [details] [IMAGE] Before
Created attachment 362782 [details] [IMAGE] After
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.
Created attachment 362787 [details] [PATCH] Proposed Fix Use variables now.
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.
Created attachment 362788 [details] [PATCH] Proposed Fix
Patch doesn't apply to ToT.
Created attachment 362811 [details] [PATCH] Proposed Fix
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.
(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
(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 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.
> 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.
https://trac.webkit.org/r242072
<rdar://problem/48391381>