Bug 214366 – Web Inspector: on Big Sur, match OS background, text, and border colors

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Now that the tab bar is lighter, sidebars' backgrounds look out of place.

Same goes for:
- Popovers
- Graphics tab content view

[Radar WebKit Bug Importer]

<rdar://problem/65617290>

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 405091 [details]
Patch

[Nikita Vasilyev]

Created attachment 405092 [details]
[Image] Before/After

[Devin Rousso]

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`?

[Nikita Vasilyev]

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

[Nikita Vasilyev]

(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 🧐

[Devin Rousso]

(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 😅

[Nikita Vasilyev]

(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.

[Nikita Vasilyev]

*** Bug 214364 has been marked as a duplicate of this bug. ***

[Nikita Vasilyev]

Created attachment 405394 [details]
Patch

[Nikita Vasilyev]

Created attachment 405395 [details]
[Image] Before/After

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

*** Bug 214321 has been marked as a duplicate of this bug. ***

[Devin Rousso]

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

[Devin Rousso]

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

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

(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.

[Nikita Vasilyev]

Created attachment 405620 [details]
Patch

[Nikita Vasilyev]

Created attachment 405626 [details]
[Image] Before/After

[Devin Rousso]

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).

[Nikita Vasilyev]

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.

[EWS]

Committed r265120: <https://trac.webkit.org/changeset/265120>

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