Bug 210447

Created attachment 396318 [details] Patch

Created attachment 396320 [details] [Video] With patch applied

Comment on attachment 396318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396318&action=review r-, as this needs work for the detached/undocked state Overall though, most of the logic/look/feel seems fine :) > Source/WebInspectorUI/UserInterface/Views/TabBar.js:694 > + _tabBarItemActivated(event, byMouse) Instead of passing a boolean flag `byMouse` (this could definitely use a better name, like `fromMouse`), why not just look at `event.type.startsWith("mouse")` or even just `event.type === "mosuedown"`? NIT: I like to put non event listener member functions above event listener callbacks (makes it clearer what's actual logic code vs just an event listener "pipe" to some logic), so I'd move this after `_finishExpandingTabsAfterClose` > Source/WebInspectorUI/UserInterface/Views/TabBar.js:714 > + // FIXME: WI.ContextMenu.createFromEvent doesn't support keyDown event. Why can't we fix this? We should just be able to modify `InspectorFrontendHost::dispatchEventAsContextMenuEvent` to support a `KeyboardEvent`, likely just using the middle of the `Event::target` as a replacement for `MouseRelatedEvent::absoluteLocation`.

(In reply to Devin Rousso from comment #3) > Comment on attachment 396318 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396318&action=review > > r-, as this needs work for the detached/undocked state It works in the detached/undocked state. Our code review tool doesn't correctly show what CSS rule I changed. It shows: body.docked.bottom .tab-bar > .tabs > .flexible-space { But I actually changed .tab-bar > .tabs > .item {

Created attachment 396342 [details] [Image] Screenshot of issue after Patch is applied (In reply to Nikita Vasilyev from comment #4) > (In reply to Devin Rousso from comment #3) > > Comment on attachment 396318 [details] > > Patch > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=396318&action=review > > > > r-, as this needs work for the detached/undocked state > > It works in the detached/undocked state. > > Our code review tool doesn't correctly show what CSS rule I changed. It shows: > > body.docked.bottom .tab-bar > .tabs > .flexible-space { > > But I actually changed > > .tab-bar > .tabs > .item { I realize that. I had applied the patch locally and was commenting on how it looked visually.

Created attachment 396347 [details] Patch

Created attachment 396348 [details] [Image] With patch applied - undocked

Comment on attachment 396347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396347&action=review (In reply to Nikita Vasilyev from comment #7) > Created attachment 396348 [details] > [Image] With patch applied - undocked Are we trying to exactly (or close) match Safari (or the rest of macOS)? If so, i still think the focus ring when detached/undocked needs work. It's overlapping the left border and seems to be clipped on the top/bottom. > Source/WebInspectorUI/UserInterface/Views/TabBar.css:159 > + outline-offset: calc(var(--focus-outline-offset) - 2px); Where does this magic `2px` come from? > Source/WebInspectorUI/UserInterface/Views/Variables.css:186 > + --focus-ring-width: 3px; How is this magic `3px` determined? Is it the same for all ports? Should we be using this for the other `outline-offset` you've added/used recently?

Created attachment 396467 [details] [Image] Focused tab in Safari (In reply to Devin Rousso from comment #8) > Comment on attachment 396347 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396347&action=review > > (In reply to Nikita Vasilyev from comment #7) > > Created attachment 396348 [details] > > [Image] With patch applied - undocked > Are we trying to exactly (or close) match Safari (or the rest of macOS)? I'm trying to do something sensible here. I think Safari has a bug here.

I made this page https://codepen.io/NV/pen/jObbRWe with webkit-focus-ring on various elements. There's a 2px gap between div/span's borders and webkit-focus-ring. That gap is the WebKit magic I'm compensating for. Perhaps it's a WebKit bug - I'd expect no gap by default. Chrome has a slightly different focus-ring style and only 1px gap. -- As for now, the default outline-offset should be -2px for most cases. I'll update --focus-outline-offset. However, it shouldn't always be -2px. Here are some User Agent styles: ``` input:focus, textarea:focus, keygen:focus, select:focus { outline-offset: -2px; } input:matches([type="button"], [type="checkbox"], [type="file"], [type="hidden"], [type="image"], [type="radio"], [type="reset"], [type="search"], [type="submit"]):focus, input[type="file"]:focus::-webkit-file-upload-button { outline-offset: 0px; } ```

Created attachment 396468 [details] [Image] Focused tab in STP 104 (In reply to Nikita Vasilyev from comment #9) > Created attachment 396467 [details] > [Image] Focused tab in Safari > > (In reply to Devin Rousso from comment #8) > > Comment on attachment 396347 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=396347&action=review > > > > (In reply to Nikita Vasilyev from comment #7) > > > Created attachment 396348 [details] > > > [Image] With patch applied - undocked > > Are we trying to exactly (or close) match Safari (or the rest of macOS)? > > I'm trying to do something sensible here. I think Safari has a bug here. Interesting. This is what I see in STP 104. Pressing tab again causes the [X] to appear and have a focus ring around that.

(In reply to Devin Rousso from comment #11) > Created attachment 396468 [details] > [Image] Focused tab in STP 104 > > (In reply to Nikita Vasilyev from comment #9) > > Created attachment 396467 [details] > > [Image] Focused tab in Safari > > > > (In reply to Devin Rousso from comment #8) > > > Comment on attachment 396347 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=396347&action=review > > > > > > (In reply to Nikita Vasilyev from comment #7) > > > > Created attachment 396348 [details] > > > > [Image] With patch applied - undocked > > > Are we trying to exactly (or close) match Safari (or the rest of macOS)? > > > > I'm trying to do something sensible here. I think Safari has a bug here. > Interesting. This is what I see in STP 104. Pressing tab again causes the > [X] to appear and have a focus ring around that. This looks good. I'll need to add markup to make it look like this in WI.

Created attachment 397025 [details] Patch

Created attachment 397026 [details] [Image] Focused tab - undocked

Created attachment 397028 [details] [Image] Focused tab - docked body.docked .tab-bar > .tabs > .item { outline-offset: calc(var(--focus-ring-outline-offset) - 1px); } To answer where is this -1px coming from. It's an adjustment to make it look better in this particular case.

Comment on attachment 397025 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397025&action=review > Source/WebInspectorUI/ChangeLog:49 > + Remove two "flex-space" elements. They were used to center an element between them. I use `justify-content: center` on the parent element instead. Please remove the associated CSS too :) > Source/WebInspectorUI/ChangeLog:52 > + Don't steal focus on click (default macOS behavior). Don't we _want_ to do that? In Safari, if I focus a tab and then click on it (or a different tab), the focus ring disappears. > Source/WebInspectorUI/UserInterface/Views/GeneralTabBarItem.js:48 > + this._displayNameElement.textContent = displayName; This should go below the `super.displayName = displayName;`. We should also use `this.displayName` in the assignment so that we take advantage of the fallback assignment inside `super.displayName`. Actually, why isn't this just in `WI.TabBarItem.prototype.set displayName`? `_displayNameElement` is defined there anyways, and it's technically private to that class too. > Source/WebInspectorUI/UserInterface/Views/TabBar.css:157 > +.tab-bar > .tabs > .item > .container { This should be `.tab-bar > .tabs > .item .icon` (way down), as it refers to a child of `.item`, while the following rules refers to `.item` itself. > Source/WebInspectorUI/UserInterface/Views/TabBar.css:167 > +body:not(.docked) .tab-bar > .tabs > .item:focus > .container { Ditto (157) > Source/WebInspectorUI/UserInterface/Views/TabBar.css:317 > Style: please remove this unnecessary newline while you're at it :) > Source/WebInspectorUI/UserInterface/Views/TabBar.css:323 > +.tab-bar > .tabs > .item .name:empty { Nice =D > Source/WebInspectorUI/UserInterface/Views/TabBar.css:331 > +.tab-bar > .tabs > .item .name > .content { I don't think anything will match this selector anymore. We can probably remove it. > Source/WebInspectorUI/UserInterface/Views/TabBarItem.js:40 > + let containerElement = this._element.createChild("div", "container"); I think "content" would be better than "container", as it provides more description as to what it is. > Source/WebInspectorUI/UserInterface/Views/TabBarItem.js:141 > + event.preventDefault(); If you do need to keep this (see WebInspectorUI/ChangeLog:52), could we use CSS instead (something like `pointer-events: none;`)?

Comment on attachment 397025 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397025&action=review >> Source/WebInspectorUI/ChangeLog:49 >> + Remove two "flex-space" elements. They were used to center an element between them. I use `justify-content: center` on the parent element instead. > > Please remove the associated CSS too :) Thanks. >> Source/WebInspectorUI/UserInterface/Views/GeneralTabBarItem.js:48 >> + this._displayNameElement.textContent = displayName; > > This should go below the `super.displayName = displayName;`. > > We should also use `this.displayName` in the assignment so that we take advantage of the fallback assignment inside `super.displayName`. > > Actually, why isn't this just in `WI.TabBarItem.prototype.set displayName`? `_displayNameElement` is defined there anyways, and it's technically private to that class too. It isn't there only because you want to show it for GeneralTabBarItem but not for PinnedTabBarItem. >> Source/WebInspectorUI/UserInterface/Views/TabBar.css:331 >> +.tab-bar > .tabs > .item .name > .content { > > I don't think anything will match this selector anymore. We can probably remove it. Yeah, this is from the old tab bar. >> Source/WebInspectorUI/UserInterface/Views/TabBarItem.js:141 >> + event.preventDefault(); > > If you do need to keep this (see WebInspectorUI/ChangeLog:52), could we use CSS instead (something like `pointer-events: none;`)? Sorry, this part was incomplete. Updating.

Created attachment 397303 [details] Patch

Comment on attachment 397025 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397025&action=review >>> Source/WebInspectorUI/UserInterface/Views/GeneralTabBarItem.js:48 >>> + this._displayNameElement.textContent = displayName; >> >> This should go below the `super.displayName = displayName;`. >> >> We should also use `this.displayName` in the assignment so that we take advantage of the fallback assignment inside `super.displayName`. >> >> Actually, why isn't this just in `WI.TabBarItem.prototype.set displayName`? `_displayNameElement` is defined there anyways, and it's technically private to that class too. > > It isn't there only because you want to show it for GeneralTabBarItem but not for PinnedTabBarItem. That's just because of how we use it right now. There's no reason to prevent a `WI.PinnedTabBarItem` from having a `displayName`. I'd move the `this._displayNameElement.textContent = displayName;` (which avoids the private violation) to `WI.TabBarItem.prototype.set displayName` and move the `displayName` values from `WI.SearchTabContentView.tabInfo` and `WI.SettingsTabContentView.tabInfo`.

Comment on attachment 397303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397303&action=review > Source/WebInspectorUI/UserInterface/Views/TabBar.css:157 > + Style: unnecessary newline > Source/WebInspectorUI/UserInterface/Views/TabBar.css:266 > + /* Default focus styles */ this comment doesn't really add anything, so I'd remove it > Source/WebInspectorUI/UserInterface/Views/TabBarItem.js:141 > + // Move the focus to the clicked element only when the focus is already inside the scope bar element. oops, copypasta > Source/WebInspectorUI/UserInterface/Views/TabBarItem.js:142 > + if (!this._parentTabBar?.element.contains(document.activeElement)) I'd drop the `?.`, as we should be able to safely assume that `this._parentTabBar` exists > Source/WebInspectorUI/UserInterface/Views/TabBarItem.js:143 > + event.preventDefault(); It seems very weird to me that we move the focus ring at all. This doesn't appear to match the Safari tab bar. Why are we doing this?

Comment on attachment 397303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397303&action=review >> Source/WebInspectorUI/UserInterface/Views/TabBarItem.js:143 >> + event.preventDefault(); > > It seems very weird to me that we move the focus ring at all. This doesn't appear to match the Safari tab bar. Why are we doing this? This's consistent with the ScopeBar and NavigationBar logic. I can remove it — I think either way is fine.

Created attachment 397426 [details] Patch

Comment on attachment 397426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397426&action=review > Source/WebInspectorUI/UserInterface/Views/GeneralTabBarItem.js:50 > + this._displayNameElement.textContent = this.displayName; This is a private violation as `_displayNameElement` is a private member of `WI.TabBarItem`. > Source/WebInspectorUI/UserInterface/Views/TabBar.css:138 > NIT: I would not be against removing all the extra whitespace =D > Source/WebInspectorUI/UserInterface/Views/TabBarItem.js:141 > + event.preventDefault(); Why exactly is this needed? You mention in the ChangeLog that it's supposed to keep focus on the console prompt, but we already seem to do that without this. There's also already logic for saving/restoring focus when switching tabs (see `WI.TabBrowser.prototype._saveFocusedNodeForTabContentView` and `WI.TabBrowser.prototype._restoreFocusedNodeForTabContentView`).

Comment on attachment 397426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397426&action=review >> Source/WebInspectorUI/UserInterface/Views/TabBar.css:138 >> > > NIT: I would not be against removing all the extra whitespace =D We should really have a tool for this. >> Source/WebInspectorUI/UserInterface/Views/TabBarItem.js:141 >> + event.preventDefault(); > > Why exactly is this needed? You mention in the ChangeLog that it's supposed to keep focus on the console prompt, but we already seem to do that without this. There's also already logic for saving/restoring focus when switching tabs (see `WI.TabBrowser.prototype._saveFocusedNodeForTabContentView` and `WI.TabBrowser.prototype._restoreFocusedNodeForTabContentView`). Huh, you're correct. In this case preventDefault doesn't matter here. In general, on macOS clicking a button does NOT move the focus. For example, in Finder, when focused on the search field, clicking on a tab or a toolbar button doesn't move focus. To match this behavior, I've been adding `event.preventDefault()` on views that are buttons, tabs, and various other clickable elements.

Created attachment 397939 [details] Patch

(In reply to Nikita Vasilyev from comment #25) > Created attachment 397939 [details] > Patch It seems that you reuploaded the same patch as attachment 397426 [details] 😅

Created attachment 397991 [details] Patch Oh, whoops 😅

Comment on attachment 397991 [details] Patch Clearing r? and cq? because this patch has become stale. Please address review comments and upload a new patch :-)