[webkit-reviews] review denied: [Bug 214163] Web Inspector: Tab bar colors of undocked Inspector should match Safari in Big Sur : [Attachment 403977] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Nikita Vasilyev <nvasilyev at apple.com>'s
request for review:
Bug 214163: Web Inspector: Tab bar colors of undocked Inspector should match
Safari in Big Sur
https://bugs.webkit.org/show_bug.cgi?id=214163

Attachment 403977: Patch

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




--- Comment #4 from Brian Burg <bburg at apple.com> ---
Comment on attachment 403977
  --> https://bugs.webkit.org/attachment.cgi?id=403977
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.