All permutations of these states should be addressed: - Tab selected/not-selected - Window Active/inactive - macOS Light/Dark mode
<rdar://problem/65293335>
Created attachment 403977 [details] Patch
Created attachment 403979 [details] Before/after in Big Sur/Catalina in both Light&Dark modes
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.
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
(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.
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.
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.
(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.
(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.
Created attachment 404277 [details] Patch
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.
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.
Comment on attachment 404277 [details] Patch r=me with a question and few nits. Please address prior to landing.
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.
Created attachment 404357 [details] Patch
ChangeLog entry in Source/WebInspectorUI/ChangeLog contains OOPS!.
Created attachment 404368 [details] Patch
Committed r264410: <https://trac.webkit.org/changeset/264410> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404368 [details].