Summary: | Web Inspector: on Big Sur, match OS background, text, and border colors | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | hi, inspector-bugzilla-changes, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2020-07-15 11:30:29 PDT
Retitling. I initially planned to implement Big Sur styles as a series of small patches. This has been hard to do without breaking things between patches. If I change background colors, I need to update text colors, border colors, glyph colors, and everything, really, to keep the UI usable. Now I plan to do only a few large patches. Created attachment 405091 [details]
Patch
Created attachment 405092 [details]
[Image] Before/After
Comment on attachment 405091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405091&action=review I'm not sure what change exactly causes this, but the Styles panel in the details sidebar in the Elements Tab in [Image] Before/After (attachment 405092 [details]) looks a lot harder to understand with this change. The fact that the background color of the panel itself (including the "Inherited from ...") is the same as the background color of locked rules makes it hard to distinguish the two. Is this design following some part of macOS Big Sur? > Source/WebInspectorUI/ChangeLog:27 > + Don't use hairline dividers. MacOS Catalina and Big Sur don't use hairline borders. There are a few other places these are used in Web Inspector. Should we remove those as well? > Source/WebInspectorUI/UserInterface/Views/DividerNavigationItem.css:29 > + background-color: var(--text-color-quaternary); We really shouldn't be using a variable named `--text-color` in a `background-color` property. I realize that this is done elsewhere, but it reads like a bug :( Also, this is not the same color as it was before. Is that intended? > Source/WebInspectorUI/UserInterface/Views/TabBar.css:95 > + background: var(--border-color); `background-color`? Comment on attachment 405091 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405091&action=review >> Source/WebInspectorUI/ChangeLog:27 >> + Don't use hairline dividers. MacOS Catalina and Big Sur don't use hairline borders. > > There are a few other places these are used in Web Inspector. Should we remove those as well? For most cases, yes. I still think there are cases where they make sense — tiny UI elements, second level sections. >> Source/WebInspectorUI/UserInterface/Views/DividerNavigationItem.css:29 >> + background-color: var(--text-color-quaternary); > > We really shouldn't be using a variable named `--text-color` in a `background-color` property. I realize that this is done elsewhere, but it reads like a bug :( > > Also, this is not the same color as it was before. Is that intended? --text-color-quaternary corresponds to Apple HIG quaternaryLabel. When Mojave was introduced, it was suggested to use for dividers and separators. Variables.css: /* Dividers, separators, background fills */ --text-color-quaternary: hsl(0, 0%, 85%); My CSS comment was directly from HIG. However! Now there's `separator` color and we should add a variable for it as well. https://developer.apple.com/design/human-interface-guidelines/ios/visual-design/color/#dynamic-system-colors (In reply to Devin Rousso from comment #5) > Comment on attachment 405091 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405091&action=review > > I'm not sure what change exactly causes this, but the Styles panel in the > details sidebar in the Elements Tab in [Image] Before/After (attachment > 405092 [details]) looks a lot harder to understand with this change. The > fact that the background color of the panel itself (including the "Inherited > from ...") is the same as the background color of locked rules makes it hard > to distinguish the two. Is this design following some part of macOS Big Sur? Yes, this is a good point. The hard thing about the dark mode in Big Sur is that most panel backgrounds depend on the desktop background, unlike the light mode. I'll look more into this 🧐 (In reply to Nikita Vasilyev from comment #7) > (In reply to Devin Rousso from comment #5) > > Comment on attachment 405091 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=405091&action=review > > > > I'm not sure what change exactly causes this, but the Styles panel in the details sidebar in the Elements Tab in [Image] Before/After (attachment 405092 [details]) looks a lot harder to understand with this change. The fact that the background color of the panel itself (including the "Inherited from ...") is the same as the background color of locked rules makes it hard to distinguish the two. Is this design following some part of macOS Big Sur? > > Yes, this is a good point. The hard thing about the dark mode in Big Sur is that most panel backgrounds depend on the desktop background, unlike the light mode. I'll look more into this 🧐 oh sorry I should've been clearer I was referring to light mode, although dark mode does also have this issue 😅 (In reply to Devin Rousso from comment #8) > (In reply to Nikita Vasilyev from comment #7) > > (In reply to Devin Rousso from comment #5) > > > Comment on attachment 405091 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=405091&action=review > > > > > > I'm not sure what change exactly causes this, but the Styles panel in the details sidebar in the Elements Tab in [Image] Before/After (attachment 405092 [details]) looks a lot harder to understand with this change. The fact that the background color of the panel itself (including the "Inherited from ...") is the same as the background color of locked rules makes it hard to distinguish the two. Is this design following some part of macOS Big Sur? > > > > Yes, this is a good point. The hard thing about the dark mode in Big Sur is that most panel backgrounds depend on the desktop background, unlike the light mode. I'll look more into this 🧐 > > oh sorry I should've been clearer I was referring to light mode, although > dark mode does also have this issue 😅 The "Inherited From" section header background is coming from --panel-background-color, which is coming from Big Sur. The locked (non-editable) style section background is custom defined. I didn't make it lighter and I think I should. *** Bug 214364 has been marked as a duplicate of this bug. *** Created attachment 405394 [details]
Patch
Created attachment 405395 [details]
[Image] Before/After
Comment on attachment 405394 [details] Patch The patch doesn't apply because it depends on bug 214563. It should apply just fine on top of the patch from bug 214563. *** Bug 214321 has been marked as a duplicate of this bug. *** Comment on attachment 405394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405394&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.css:136 > + background-color: hsl(0, 0%, 97%); This is actually used in a few different places. Perhaps we could create a `--background-color-intermediate` or `--background-color-section` or something and share this logic everywhere? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.css:166 > + background-color: hsl(0, 0%, 23%); /* --background-color-code + 2% lightness */ ditto (:136) > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.css:170 > + background-color: hsl(0, 0%, 19%); /* --background-color-code + 2% lightness */ ditto (:136) > Source/WebInspectorUI/UserInterface/Views/Variables.css:400 > + --background-color-content: hsl(0, 0%, 17%); > + --background-color-code: hsl(0, 0%, 17%); Aside: if these variables have the asme value, why do we have two separate variables? o.0 Comment on attachment 405394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405394&action=review > Source/WebInspectorUI/UserInterface/Views/Variables.css:395 > + /* Big Sur is 3-4% darker than Catalina. */ this comment is unnecessary Comment on attachment 405394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405394&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.css:136 >> + background-color: hsl(0, 0%, 97%); > > This is actually used in a few different places. Perhaps we could create a `--background-color-intermediate` or `--background-color-section` or something and share this logic everywhere? I like the idea of creating a new variable. The semantics here are different. It isn't just a section — it's a non-editable section. This color is intentionally only slightly darker/lighter from --background-color-content. What about --background-color-content-not-editable? Or just --background-color-not-editable? >> Source/WebInspectorUI/UserInterface/Views/Variables.css:400 >> + --background-color-code: hsl(0, 0%, 17%); > > Aside: if these variables have the asme value, why do we have two separate variables? o.0 Good point. They used to be different but not anymore. I'll remove --background-color-code. (In reply to Nikita Vasilyev from comment #17) > Comment on attachment 405394 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=405394&action=review > > >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.css:136 > >> + background-color: hsl(0, 0%, 97%); > > > > This is actually used in a few different places. Perhaps we could create a `--background-color-intermediate` or `--background-color-section` or something and share this logic everywhere? > > I like the idea of creating a new variable. The semantics here are > different. It isn't just a section — it's a non-editable section. > This color is intentionally only slightly darker/lighter from > --background-color-content. What about > --background-color-content-not-editable? Or just > --background-color-not-editable? After looking more into this, I agree that it should be `--background-color-intermediate`. SpreadsheetCSSStyleDeclarationSection.css is the only case where it's used to indicate non-editable section. In 5 other instances it has different semantics. Created attachment 405620 [details]
Patch
Created attachment 405626 [details]
[Image] Before/After
Comment on attachment 405620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405620&action=review r=me, nice refactoring. I have some comments, but most of them can be done as followups as they are about codebase health/cohesion. > Source/WebInspectorUI/UserInterface/Views/BreakpointPopoverController.css:86 > border: 1px solid hsl(0, 0%, 78%); While you're at it, should this change too? > Source/WebInspectorUI/UserInterface/Views/GraphicsOverviewContentView.css:76 > background-color: hsl(0, 0%, 14%); Do we have a variable for this (and in light mode)? > Source/WebInspectorUI/UserInterface/Views/LocalResourceOverridePopover.css:59 > border: 1px solid hsl(0, 0%, 78%); While you're at it, should this change too? > Source/WebInspectorUI/UserInterface/Views/URLBreakpointPopover.css:42 > border: 1px solid hsl(0, 0%, 78%); While you're at it, should this change too? > Source/WebInspectorUI/UserInterface/Views/Variables.css:378 > + body.big-sur { please combine this with the rule below (or move that one up here) > Source/WebInspectorUI/UserInterface/Views/Variables.css:397 > + --background-color-secondary: hsl(0, 0%, 23%); > + --background-color-tertiary: hsl(0, 0%, 27%); Now that we have `--background-color-intermediate`, I feel like we should remove these (especially since they only exist in dark mode) or at least give them better names (and add them to light mode). Comment on attachment 405620 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405620&action=review I acknowledge your comment about hardcoded colors but I'd like to fix them as a follow up. >> Source/WebInspectorUI/UserInterface/Views/Variables.css:378 >> + body.big-sur { > > please combine this with the rule below (or move that one up here) This is intentionally placed before `body.window-inactive` so both Catalina and Big Sur have the same glyph colors for inactive window. >> Source/WebInspectorUI/UserInterface/Views/Variables.css:397 >> + --background-color-tertiary: hsl(0, 0%, 27%); > > Now that we have `--background-color-intermediate`, I feel like we should remove these (especially since they only exist in dark mode) or at least give them better names (and add them to light mode). I agree that there's opportunity to refactor. --background-color-tertiary is used only in one place. However, --background-color-intermediate isn't the right substitute. --background-color-intermediate (in dark mode) is slightly lighter than --background-color-content. --background-color-{secondary, tertiary} is slightly lighter than --background-color, a different variable. Committed r265120: <https://trac.webkit.org/changeset/265120> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405620 [details]. |