Summary: Use system accent color throughout UI. https://webkit.org/b/186609 exposed more semantic system colors to CSS. Using the system accent and highlight colors for some Inspector UI will provide a more seamless experience on macOS Mojave: System accent color (-apple-system-control-accent): - Breakpoints - ScopeBar items - NavigationBar button glyphs and radio buttons System highlight color (-apple-system-selected-content-background): - Table/TreeOutline selection
<rdar://problem/47327971>
Created attachment 359300 [details] Patch
Created attachment 359303 [details] [Image] Blue (default)
Created attachment 359304 [details] [Image] Purple
Created attachment 359305 [details] [Image] Pink
Created attachment 359306 [details] [Image] Orange
Created attachment 359307 [details] [Image] Breakpoints - green
Comment on attachment 359300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359300&action=review I think we need to extract the -apple- values into variables so that other ports can customize them. Other than that, this patch looks good to me. Nikita, please take a look as well. > Source/WebInspectorUI/ChangeLog:18 > + to pick up the system accent color. Yay! > Source/WebInspectorUI/UserInterface/Views/BreakpointTreeElement.css:33 > + filter: grayscale(); Nice idea. > Source/WebInspectorUI/UserInterface/Views/BreakpointTreeElement.js:164 > + this.status.classList.toggle(WI.BreakpointTreeElement.StatusImageResolvedStyleClassName, this._breakpoint.resolved && WI.debuggerManager.breakpointsEnabled); Nice cleanup. > Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:65 > + background-color: -apple-system-unemphasized-selected-content-background; o_o
(In reply to Brian Burg from comment #8) > Comment on attachment 359300 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=359300&action=review > > I think we need to extract the -apple- values into variables so that other > ports can customize them. Other than that, this patch looks good to me. > Nikita, please take a look as well. These are defined in CSSValueKeywords.in behind a guard, #if defined(WTF_PLATFORM_MAC) && WTF_PLATFORM_MAC so accommodations will need to be made.
Comment on attachment 359300 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359300&action=review > Source/WebInspectorUI/UserInterface/Views/RadioButtonNavigationItem.css:64 > + filter: brightness(0.8); This makes the default blue (light mode, blue highlight color, blue accent color) look too dark.
I agree, I'll need to tweak the colors a little.
Created attachment 359320 [details] [Image] Light mode - blue of selected tab is too dark The blue color of the selected tab is too dark. Ideally, we'd want to make it darker only for certain accent colors (e.g. yellow). I don't know how to do this. If we can't, we should decide what do we care the most: matching system appearance or making custom accent colors more readable. I'm leaning towards the former.
Created attachment 359322 [details] [Image] Accent color comparisons In general the light mode colors look too dark, and the dark mode selected and hover colors are too similar.
Created attachment 359323 [details] [Image] Bug — Network tab
I agree with Brian that we need to break out the -apple-system- values. The approach I'm taking is to have a set of variables that parallel the system ones, that are initialized to the hardcoded values we've been using. Then on ports that support it (just Mojave for now), we override then. I wanted to do something like this in Variables.css: :root { --system-control-accent: hsl(212, 92%, 54%); .... } body:matches(.mojave) { --system-control-accent: -apple-system-control-accent; ... } :root { --glyph-color: var(--system-control-accent); ... } But of course this doesn't work; the Mojave override occurs at the body level, which has no effect on the variables scoped to the document (:root). I think this approach will require declaring all custom CSS properties at the body level rather than :root. Any thoughts? Alternative approaches?
(In reply to Matt Baker from comment #15) > I agree with Brian that we need to break out the -apple-system- values. The > approach I'm taking is to have a set of variables that parallel the system > ones, that are initialized to the hardcoded values we've been using. Then on > ports that support it (just Mojave for now), we override then. > > I wanted to do something like this in Variables.css: > > :root { > --system-control-accent: hsl(212, 92%, 54%); > .... > } > > body:matches(.mojave) { > --system-control-accent: -apple-system-control-accent; > ... > } > > :root { > --glyph-color: var(--system-control-accent); > ... > } > > But of course this doesn't work; the Mojave override occurs at the body > level, which has no effect on the variables scoped to the document (:root). > I think this approach will require declaring all custom CSS properties at > the body level rather than :root. > > Any thoughts? Alternative approaches? Why not just: :root { --glyph-color: hsl(212, 92%, 54%); } body:matches(.mojave) { --glyph-color: -apple-system-control-accent; }
Created attachment 362263 [details] Patch
(In reply to Matt Baker from comment #17) > Created attachment 362263 [details] > Patch Getting pretty close. Still need to tweak a couple of things. ScopeBar/Radio button backgrounds for the inactive-window case are tricky.
I think we should use the accent colors directly whenever possible. That way we can get changes in the system for free and always match.
Comment on attachment 362263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362263&action=review > Source/WebInspectorUI/UserInterface/Views/Variables.css:173 > +body:matches(.mojave) { This will need updated every year when we have new systems. You should invert this can make it something like: body.mac-platform:not(.sierra, .high-sierra) Also you don't need to use :matches(.classname), since body.classname is the same thing. Using :matches() is for more complex selectors to avoid duplication. See: https://css-tricks.com/almanac/selectors/m/matches/
Comment on attachment 362263 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362263&action=review >> Source/WebInspectorUI/UserInterface/Views/Variables.css:173 >> +body:matches(.mojave) { > > This will need updated every year when we have new systems. You should invert this can make it something like: body.mac-platform:not(.sierra, .high-sierra) > > Also you don't need to use :matches(.classname), since body.classname is the same thing. Using :matches() is for more complex selectors to avoid duplication. See: https://css-tricks.com/almanac/selectors/m/matches/ Actually that should be: body.mac-platform:not(.sierra):not(.high-sierra), since :not() is not like :matches().
Created attachment 362947 [details] Patch
Created attachment 362954 [details] Patch
Most recent changes: 1) Added system accent to - Console prompt ">" - Inline widgets (spring/curve and variable) - Rendering Frames timeline selected frame marker - DOM breakpoints 2) Radio/scope button background color is no longer darkened. 3) Pressed (:active) radio/scope button is now slightly less dark.
Created attachment 362956 [details] [Image] Light mode - default accent
Created attachment 362957 [details] [Image] Dark mode - green accent
Comment on attachment 362954 [details] Patch Attachment 362954 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11285357 New failing tests: imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html
Created attachment 362963 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
(In reply to Build Bot from comment #28) > Created attachment 362963 [details] > Archive of layout-test-results from ews125 for ios-simulator-wk2 > > The attached test failures were seen while running run-webkit-tests on the > ios-sim-ews. > Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6 Test failures are unrelated.
I think this is good to land! In the follow ups we should address: — Selected console messages should use the selected accent/highlight color. We may have to disable syntax highlighting for selected messages — to be determined. — In Dark Mode with the default accent color, our icon glyphs are too dark. The active glyph of Inspect Element is particularly bad.
Created attachment 363035 [details] [Image] Inspect Element button, Dark Mode, Blue accent
Comment on attachment 362954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362954&action=review > Source/WebInspectorUI/ChangeLog:96 > + Use system accent color for selection and hover styles. Since it isn't > + yet possible to derive new colors from the accent color in CSS, fake it > + with a ::before pseudo-element that can have have `filter` or `opacity` > + effects applied to it without altering the button text color. It's possible to derive accent colors with getComputedStyle, adjust them with JS, and then store them as CSS variables. I think it's worth exploring as a follow-up.
(In reply to Nikita Vasilyev from comment #31) > Created attachment 363035 [details] > [Image] Inspect Element button, Dark Mode, Blue accent We got pretty close to matching the equivalent UI used for Xcode's docking buttons by adjusting the accent color with brightness/saturate filters. It's a little hacky, and I'll need to confirm it works for all the system accents in both light/dark mode. Will try this in a follow up.
Comment on attachment 362954 [details] Patch Clearing flags on attachment: 362954 Committed r242118: <https://trac.webkit.org/changeset/242118>
All reviewed patches have been landed. Closing bug.