Bug 214163 – Web Inspector: Tab bar colors of undocked Inspector should match Safari in Big Sur

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

All permutations of these states should be addressed:

- Tab selected/not-selected
- Window Active/inactive
- macOS Light/Dark mode

[Radar WebKit Bug Importer]

<rdar://problem/65293335>

[Nikita Vasilyev]

Created attachment 403977 [details]
Patch

[Nikita Vasilyev]

Created attachment 403979 [details]
Before/after in Big Sur/Catalina in both Light&Dark modes

[Blaze Burg]

Comment on attachment 403977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403977&action=review

Correctness issues:
- To match other system apps, the top border of the selected tab should not be visible.
- Please include screenshots that include display of :hover style, and when body.window-inactive.

Detailed comments inline.

> Source/WebInspectorUI/ChangeLog:14
> +        Rename `--tab-item-extra-light-border-color` to `--tab-bar-inactive-window-background`. It wasn't used used for borders.

Nit: used used

> Source/WebInspectorUI/UserInterface/Views/Main.css:580
> +    body[class].window-inactive #undocked-title-area {

What conflict is this resolving? Let's use :not(.window-inactive) if possible. Non-semantic hacks like body[class] will quickly get out of control if we use it more.

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:39
> +    --tab-bar-inactive-window-background: hsl(0, 0%, 92%);

This is not needed and it's not a semantic variable. We already have --tab-bar-background, it would be better to override that variable with this selector.

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:175
> +body:not(.big-sur):not(.docked) .tab-bar > .tabs > .item {

Nit: this can combine classes inside :not...

body:not(.big-sur, .docked) .tab-bar > .tabs > .item

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:276
> +

Please combine the declaration of background-color into one rule, and add a separate rule just for background-image.

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:413
> +    body[class] .tab-bar {

Ditto regarding above comment. We shouldn't do body[class].

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:423
> +        --tab-bar-inactive-window-background: hsl(0, 0%, 8%);

Why do we need a separate variable for when `.window-inactive`?  Can't we just modify `--tab-bar-background` in a `body.window-inactive` rule?

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:430
> +    body:not(.big-sur):not(.docked) .tab-bar {

Nit: combine two :not into one

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:443
>          background-image: linear-gradient(to bottom, hsl(0, 0%, 12%), hsl(0, 0%, 10%));

Nit: combine two :not into one

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:458
> +    body:not(.big-sur):not(.docked) .tab-bar > .tabs:not(.animating) > .item:not(.selected, .disabled):hover {

Nit: combine two :not into one

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:502
> +    body:not(.big-sur):not(.docked).window-inactive .tab-bar > .tabs > .item:not(.disabled).selected {

Nit: combine two :not into one

> Source/WebInspectorUI/UserInterface/Views/Variables.css:205
> +    --window-inactive-background: hsl(0, 0%, 8%);

This is only referenced as a value in TabBar.css, please move it out of the global file into TabBar.css. And as previously noted, this should go away in favor of --background-color being conditionally overridden when body.window-inactive is true.

[Nikita Vasilyev]

Comment on attachment 403977 [details]
Patch

Some comments here. I'll answer/address the rest later.

View in context: https://bugs.webkit.org/attachment.cgi?id=403977&action=review

>> Source/WebInspectorUI/UserInterface/Views/Main.css:580
>> +    body[class].window-inactive #undocked-title-area {
> 
> What conflict is this resolving? Let's use :not(.window-inactive) if possible. Non-semantic hacks like body[class] will quickly get out of control if we use it more.

`body.window-inactive #undocked-title-area` has the same weight as `body.big-sur #undocked-title-area` or `body:not(.big-sur) #undocked-title-area`.

An alternative to `body[class].window-inactive #undocked-title-area` would be:
body.big-sur.window-inactive #undocked-title-area,
body:not(.big-sur).window-inactive #undocked-title-area

>> Source/WebInspectorUI/UserInterface/Views/TabBar.css:175
>> +body:not(.big-sur):not(.docked) .tab-bar > .tabs > .item {
> 
> Nit: this can combine classes inside :not...
> 
> body:not(.big-sur, .docked) .tab-bar > .tabs > .item

`:not(.big-sur):not(.docked)` means NOT .big-sur AND NOT .docked
`body:not(.big-sur, .docked)` means NOT .big-sur OR NOT .docked

[Nikita Vasilyev]

(In reply to Brian Burg from comment #4)
> Comment on attachment 403977 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403977&action=review
> 
> Correctness issues:
> - To match other system apps, the top border of the selected tab should not
> be visible.

Yes, this is correct. I won't have time to fix it today so I'd prefer to address it as a separate bug.

[Nikita Vasilyev]

Comment on attachment 403977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403977&action=review

>> Source/WebInspectorUI/UserInterface/Views/TabBar.css:39
>> +    --tab-bar-inactive-window-background: hsl(0, 0%, 92%);
> 
> This is not needed and it's not a semantic variable. We already have --tab-bar-background, it would be better to override that variable with this selector.

Just curious, why are you saying it isn't a semantic variable? HIG says "A semantic color conveys its purpose rather than its appearance or color values.". https://developer.apple.com/design/human-interface-guidelines/ios/visual-design/color/#dynamic-system-colors
So, unlike --tab-item-extra-light-border-color, --tab-bar-inactive-window-background seems more semantic to me.

Note that even if I override --tab-bar-background with a selector, I'd still have keep a rule to set `background-image: none`, e.g. `body:not(.big-sur):not(.docked).window-inactive .tab-bar > .tabs > .item`.

Also, when inspecting an element, I find it hard to understand what's going on when variable overrides are used excessively.

>> Source/WebInspectorUI/UserInterface/Views/Variables.css:205
>> +    --window-inactive-background: hsl(0, 0%, 8%);
> 
> This is only referenced as a value in TabBar.css, please move it out of the global file into TabBar.css. And as previously noted, this should go away in favor of --background-color being conditionally overridden when body.window-inactive is true.

The idea is that it would be used outside of TabBar.css going forward but I can move it to TabBar.css for now.

[Blaze Burg]

Comment on attachment 403977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403977&action=review

>>> Source/WebInspectorUI/UserInterface/Views/TabBar.css:39
>>> +    --tab-bar-inactive-window-background: hsl(0, 0%, 92%);
>> 
>> This is not needed and it's not a semantic variable. We already have --tab-bar-background, it would be better to override that variable with this selector.
> 
> Just curious, why are you saying it isn't a semantic variable? HIG says "A semantic color conveys its purpose rather than its appearance or color values.". https://developer.apple.com/design/human-interface-guidelines/ios/visual-design/color/#dynamic-system-colors
> So, unlike --tab-item-extra-light-border-color, --tab-bar-inactive-window-background seems more semantic to me.
> 
> Note that even if I override --tab-bar-background with a selector, I'd still have keep a rule to set `background-image: none`, e.g. `body:not(.big-sur):not(.docked).window-inactive .tab-bar > .tabs > .item`.
> 
> Also, when inspecting an element, I find it hard to understand what's going on when variable overrides are used excessively.

Sure, it's semantic in the sense it doesn't name a color by value. However, we don't have similar counterparts for inactive-window state, so introducing one case where we do is unnecessary, and will cause every use site to need two rules.

I would love to hear ideas for improving variables support if you find our current UI hard to use. But it's not a reason to avoid a variable override.

>>> Source/WebInspectorUI/UserInterface/Views/TabBar.css:175
>>> +body:not(.big-sur):not(.docked) .tab-bar > .tabs > .item {
>> 
>> Nit: this can combine classes inside :not...
>> 
>> body:not(.big-sur, .docked) .tab-bar > .tabs > .item
> 
> `:not(.big-sur):not(.docked)` means NOT .big-sur AND NOT .docked
> `body:not(.big-sur, .docked)` means NOT .big-sur OR NOT .docked

OK

>>> Source/WebInspectorUI/UserInterface/Views/Variables.css:205
>>> +    --window-inactive-background: hsl(0, 0%, 8%);
>> 
>> This is only referenced as a value in TabBar.css, please move it out of the global file into TabBar.css. And as previously noted, this should go away in favor of --background-color being conditionally overridden when body.window-inactive is true.
> 
> The idea is that it would be used outside of TabBar.css going forward but I can move it to TabBar.css for now.

Let's keep it in TabBar.css until that comes to pass.

[Blaze Burg]

(In reply to Nikita Vasilyev from comment #6)
> (In reply to Brian Burg from comment #4)
> > Comment on attachment 403977 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=403977&action=review
> > 
> > Correctness issues:
> > - To match other system apps, the top border of the selected tab should not
> > be visible.
> 
> Yes, this is correct. I won't have time to fix it today so I'd prefer to
> address it as a separate bug.

I'm not going to review this patch without this fix. We don't knowingly commit bugs.

[Nikita Vasilyev]

(In reply to Brian Burg from comment #9)
> (In reply to Nikita Vasilyev from comment #6)
> > (In reply to Brian Burg from comment #4)
> > > Comment on attachment 403977 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=403977&action=review
> > > 
> > > Correctness issues:
> > > - To match other system apps, the top border of the selected tab should not
> > > be visible.
> > 
> > Yes, this is correct. I won't have time to fix it today so I'd prefer to
> > address it as a separate bug.
> 
> I'm not going to review this patch without this fix. We don't knowingly
> commit bugs.

Okay, I’m working on this.

[Nikita Vasilyev]

Created attachment 404277 [details]
Patch

[Nikita Vasilyev]

Created attachment 404279 [details]
Before/after in Big Sur/Catalina in both Light&Dark modes

On each screenshot, the 3rd tab (Sources) is hovered by the mouse cursor.

[Blaze Burg]

Comment on attachment 404277 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404277&action=review

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:151
> +    z-index: calc(var(--tab-bar-border-z-index) + 1);

Does this affect being able to drag to resize the bottom docked frame?

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:173
> +    border-top: 1px solid var(--tab-item-medium-border-color);
> +    border-bottom: 1px solid var(--tab-item-medium-border-color);

Nit: use var(--tab-item-medium-border-style) instead of embedding in the shorthand rule.

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:176
> +    --tab-item-background: hsl(0, 0%, 90%);

Is this used? I would expect it to always get overridden by the below rules.

[Blaze Burg]

Comment on attachment 404277 [details]
Patch

r=me with a question and few nits. Please address prior to landing.

[Nikita Vasilyev]

Comment on attachment 404277 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=404277&action=review

>> Source/WebInspectorUI/UserInterface/Views/TabBar.css:151
>> +    z-index: calc(var(--tab-bar-border-z-index) + 1);
> 
> Does this affect being able to drag to resize the bottom docked frame?

It doesn't. I just made z-index consistent across docked and undocked modes.

>> Source/WebInspectorUI/UserInterface/Views/TabBar.css:173
>> +    border-bottom: 1px solid var(--tab-item-medium-border-color);
> 
> Nit: use var(--tab-item-medium-border-style) instead of embedding in the shorthand rule.

Thanks!

>> Source/WebInspectorUI/UserInterface/Views/TabBar.css:176
>> +    --tab-item-background: hsl(0, 0%, 90%);
> 
> Is this used? I would expect it to always get overridden by the below rules.

No, good catch.

[Nikita Vasilyev]

Created attachment 404357 [details]
Patch

[EWS]

ChangeLog entry in Source/WebInspectorUI/ChangeLog contains OOPS!.

[Nikita Vasilyev]

Created attachment 404368 [details]
Patch

[EWS]

Committed r264410: <https://trac.webkit.org/changeset/264410>

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