Bug 214366 - Web Inspector: on Big Sur, match OS background, text, and border colors
Summary: Web Inspector: on Big Sur, match OS background, text, and border colors
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: Nikita Vasilyev
URL:
Keywords: InRadar
: 214321 214364 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-07-15 11:30 PDT by Nikita Vasilyev
Modified: 2020-07-30 17:12 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.51 KB, patch)
2020-07-23 16:02 PDT, Nikita Vasilyev
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
[Image] Before/After (965.27 KB, image/png)
2020-07-23 16:03 PDT, Nikita Vasilyev
no flags Details
Patch (10.98 KB, patch)
2020-07-28 12:40 PDT, Nikita Vasilyev
nvasilyev: commit-queue-
Details | Formatted Diff | Diff
[Image] Before/After (1.27 MB, image/png)
2020-07-28 12:41 PDT, Nikita Vasilyev
no flags Details
Patch (23.88 KB, patch)
2020-07-30 14:37 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Image] Before/After (1.25 MB, image/png)
2020-07-30 14:56 PDT, Nikita Vasilyev
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2020-07-15 11:30:29 PDT
Now that the tab bar is lighter, sidebars' backgrounds look out of place.

Same goes for:
- Popovers
- Graphics tab content view
Comment 1 Radar WebKit Bug Importer 2020-07-15 11:30:39 PDT
<rdar://problem/65617290>
Comment 2 Nikita Vasilyev 2020-07-21 05:20:45 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.
Comment 3 Nikita Vasilyev 2020-07-23 16:02:35 PDT
Created attachment 405091 [details]
Patch
Comment 4 Nikita Vasilyev 2020-07-23 16:03:32 PDT
Created attachment 405092 [details]
[Image] Before/After
Comment 5 Devin Rousso 2020-07-24 10:31:33 PDT
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 6 Nikita Vasilyev 2020-07-24 10:52:30 PDT
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
Comment 7 Nikita Vasilyev 2020-07-24 10:59:14 PDT
(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 🧐
Comment 8 Devin Rousso 2020-07-24 11:01:59 PDT
(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 😅
Comment 9 Nikita Vasilyev 2020-07-24 11:15:54 PDT
(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.
Comment 10 Nikita Vasilyev 2020-07-28 11:58:18 PDT
*** Bug 214364 has been marked as a duplicate of this bug. ***
Comment 11 Nikita Vasilyev 2020-07-28 12:40:27 PDT
Created attachment 405394 [details]
Patch
Comment 12 Nikita Vasilyev 2020-07-28 12:41:38 PDT
Created attachment 405395 [details]
[Image] Before/After
Comment 13 Nikita Vasilyev 2020-07-28 13:15:32 PDT
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.
Comment 14 Nikita Vasilyev 2020-07-30 10:58:41 PDT
*** Bug 214321 has been marked as a duplicate of this bug. ***
Comment 15 Devin Rousso 2020-07-30 11:43:49 PDT
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 16 Devin Rousso 2020-07-30 11:44:46 PDT
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 17 Nikita Vasilyev 2020-07-30 11:55:52 PDT
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.
Comment 18 Nikita Vasilyev 2020-07-30 13:57:24 PDT
(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.
Comment 19 Nikita Vasilyev 2020-07-30 14:37:48 PDT
Created attachment 405620 [details]
Patch
Comment 20 Nikita Vasilyev 2020-07-30 14:56:55 PDT
Created attachment 405626 [details]
[Image] Before/After
Comment 21 Devin Rousso 2020-07-30 16:56:06 PDT
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 22 Nikita Vasilyev 2020-07-30 17:06:40 PDT
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.
Comment 23 EWS 2020-07-30 17:12:12 PDT
Committed r265120: <https://trac.webkit.org/changeset/265120>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405620 [details].