Scope bars used in Network tab, Console tab, and various other places. Here's one prominent example where the scope bar isn't accessible with Tab navigation: 1. Open Console tab 2. Click "Emulate User Gesture" checkbox 3. Press Tab Expected: The focus outline should be around the first selected scope bar item ("All", by default). Actual: It isn't clear where is the focus at all.
<rdar://problem/59828111>
Created attachment 391813 [details] Patch
Created attachment 391815 [details] [Image] With patch applied - focused scope bar item
Comment on attachment 391813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391813&action=review > Source/WebInspectorUI/UserInterface/Views/ScopeBar.js:39 > + for (let item of this._items) > + item.delegate = this; Why are we using a delegate for this? Other things with `WI.ScopeBar` and `WI.ScopeBarItem` just use event dispatch (e.g. `WI.ScopeBarItem.Event.SelectionChanged`). I'd prefer that approach to allowing a modifiable delegate. What about `WI.MultipleScopeBarItem`? > Source/WebInspectorUI/UserInterface/Views/ScopeBar.js:115 > + if (!this._element.contains(document.activeElement)) Is it not possible at all to do something like this inside `WI.ScopeBarItem`? Previously, `WI.ScopeBarItem` has never needed to know about anything "above" it (both in the DOM or in the reference tree), so it'd be nice to keep it that way. > Source/WebInspectorUI/UserInterface/Views/ScopeBarItem.js:120 > + _setSelected(selected) How about `_updateSelectedStyle`? > Source/WebInspectorUI/UserInterface/Views/ScopeBarItem.js:122 > + this._element.classList.toggle("selected", selected); In the past, I've been bitten by the fact that `classList.toggle` treats `undefined` as a flip-flop when given as the second argument <https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/toggle> Please add a `!!` to force a boolean for `!!selected`.
Comment on attachment 391813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391813&action=review >> Source/WebInspectorUI/UserInterface/Views/ScopeBar.js:39 >> + item.delegate = this; > > Why are we using a delegate for this? Other things with `WI.ScopeBar` and `WI.ScopeBarItem` just use event dispatch (e.g. `WI.ScopeBarItem.Event.SelectionChanged`). I'd prefer that approach to allowing a modifiable delegate. > > What about `WI.MultipleScopeBarItem`? I'm using a delegate because it's a clear 1-to-1 relation (ScopeBarItem has only one "parent" - ScopeBar). I think we should always use delegates for these cases, they're simpler. I can't use the existing WI.ScopeBarItem.Event.SelectionChanged event. I need to handle mouseDown specifically. I didn't address WI.MultipleScopeBarItem because I'm trying to fix the most glaring accessibility issues this week (Elements, Console, Sources, some of Network tab). WI.MultipleScopeBarItem is used in Storage and Timelines — I care less about them. >> Source/WebInspectorUI/UserInterface/Views/ScopeBar.js:115 >> + if (!this._element.contains(document.activeElement)) > > Is it not possible at all to do something like this inside `WI.ScopeBarItem`? Previously, `WI.ScopeBarItem` has never needed to know about anything "above" it (both in the DOM or in the reference tree), so it'd be nice to keep it that way. Not really, no. It checks if other scope bar items are selected in this ScopeBar. >> Source/WebInspectorUI/UserInterface/Views/ScopeBarItem.js:120 >> + _setSelected(selected) > > How about `_updateSelectedStyle`? I'm fine with `_updateSelected`. Update `_updateSelectedStyle` sounds like it only updates CSS. >> Source/WebInspectorUI/UserInterface/Views/ScopeBarItem.js:122 >> + this._element.classList.toggle("selected", selected); > > In the past, I've been bitten by the fact that `classList.toggle` treats `undefined` as a flip-flop when given as the second argument <https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/toggle> > > Please add a `!!` to force a boolean for `!!selected`. Good catch! It always seem to be a boolean here but I'll add it just in case.
Comment on attachment 391813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=391813&action=review >>> Source/WebInspectorUI/UserInterface/Views/ScopeBar.js:39 >>> + item.delegate = this; >> >> Why are we using a delegate for this? Other things with `WI.ScopeBar` and `WI.ScopeBarItem` just use event dispatch (e.g. `WI.ScopeBarItem.Event.SelectionChanged`). I'd prefer that approach to allowing a modifiable delegate. >> >> What about `WI.MultipleScopeBarItem`? > > I'm using a delegate because it's a clear 1-to-1 relation (ScopeBarItem has only one "parent" - ScopeBar). I think we should always use delegates for these cases, they're simpler. > > I can't use the existing WI.ScopeBarItem.Event.SelectionChanged event. I need to handle mouseDown specifically. > > I didn't address WI.MultipleScopeBarItem because I'm trying to fix the most glaring accessibility issues this week (Elements, Console, Sources, some of Network tab). WI.MultipleScopeBarItem is used in Storage and Timelines — I care less about them. Delegates are not appropriate just because of only having one client. Delegates are for delegating policy decisions and operations that are outside the control of the object. I've spent a lot of time ripping out delegates in ObjC code that should be event-based. Please don't add more. Additionally, classes that dispatch events are much easier to mock and observe than delegates in test code. If you need a protocol / interface object that's 1:1 with an instance, that's fine– call it a *Provider or something. But not a delegate.
Created attachment 392043 [details] Patch
Created attachment 392044 [details] Patch
Comment on attachment 392044 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392044&action=review > Source/WebInspectorUI/ChangeLog:9 > + Make resource type filter in the Network tab and message type filter in the Console tab focusable. This doesn't appear to work. When I focus the URL filter and press tab, I can focus "All" (or whatever is selected), but not any other scope bar items (focus jumps to the Group Media Requests checkbox). The same is true for the Console filter. > Source/WebInspectorUI/UserInterface/Views/ScopeBar.css:76 > + outline: auto -webkit-focus-ring-color; This doesn't look good in the Audit Tab. > Source/WebInspectorUI/UserInterface/Views/ScopeBarItem.js:138 > + if (!this.scopeBar?.element.contains(document.activeElement)) NIT: prefer using the private variable when inside a class, so you avoid an extra getter function call. ``` if (!this._scopeBar?.element.contains(document.activeElement)) ```
Comment on attachment 392044 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392044&action=review >> Source/WebInspectorUI/ChangeLog:9 >> + Make resource type filter in the Network tab and message type filter in the Console tab focusable. > > This doesn't appear to work. When I focus the URL filter and press tab, I can focus "All" (or whatever is selected), but not any other scope bar items (focus jumps to the Group Media Requests checkbox). The same is true for the Console filter. This is the expected behavior. It matches macOS radio buttons. For example, Finder's "Show items as icons, in a list, in columns, or in a gallery". Arrow right/left keys should select next/previous items (in LTR mode). This hasn't been implemented in this patch yet.
Created attachment 392654 [details] Patch
Created attachment 392818 [details] Patch I made Left Arrow and Right Arrow keys move the focus within the scope bar. This matches macOS radio buttons.
Comment on attachment 392818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392818&action=review r=me > Source/WebInspectorUI/UserInterface/Views/AuditTestGroupContentView.css:74 > + overflow: visible; Why not merge this with the above rule? The `WI.NavigationBar` will always use a `<nav>` because that's now it's created (`new WI.NavigationBar(document.createElement("nav"))`). > Source/WebInspectorUI/UserInterface/Views/ScopeBar.css:104 > +.scope-bar > li.multiple:focus-within { Would it be more accurate to say `.scope-bar > li.multiple > select:focus`? > Source/WebInspectorUI/UserInterface/Views/ScopeBar.js:184 > + if (event.code === "ArrowLeft" || event.code === "ArrowRight") { What's the difference between this (which only appears to be used once or twice in Web Inspector) and `keyIdentifier === "Left"` (which is used a good amount)? > Source/WebInspectorUI/UserInterface/Views/ScopeBar.js:200 > + if (WI.resolveLayoutDirectionForElement(this._element) === WI.LayoutDirection.RTL) When would this ever be different than `WI.resolvedLayoutDirection()`? > Source/WebInspectorUI/UserInterface/Views/ScopeBar.js:204 > + focusedIndex = (focusedIndex + this._items.length) % this._items.length; This logic means that we will wrap around the scope bar if you keep pressing left or keep pressing right. Is that what we want? If so, you don't need the `if` below because we will always be between `0` and `this._items.length` due to the `%`.
Comment on attachment 392818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=392818&action=review >> Source/WebInspectorUI/UserInterface/Views/ScopeBar.js:184 >> + if (event.code === "ArrowLeft" || event.code === "ArrowRight") { > > What's the difference between this (which only appears to be used once or twice in Web Inspector) and `keyIdentifier === "Left"` (which is used a good amount)? keyIdentifier is a non-standard deprecated property. All else equal being equal, we shouldn't be using it. https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/keyIdentifier >> Source/WebInspectorUI/UserInterface/Views/ScopeBar.js:200 >> + if (WI.resolveLayoutDirectionForElement(this._element) === WI.LayoutDirection.RTL) > > When would this ever be different than `WI.resolvedLayoutDirection()`? resolveLayoutDirectionForElement was made to handle LTR "islands" — regions that are always LTR even when the global layout is RTL. Going forward, we should use resolveLayoutDirectionForElement instead of resolvedLayoutDirection for most cases. >> Source/WebInspectorUI/UserInterface/Views/ScopeBar.js:204 >> + focusedIndex = (focusedIndex + this._items.length) % this._items.length; > > This logic means that we will wrap around the scope bar if you keep pressing left or keep pressing right. Is that what we want? If so, you don't need the `if` below because we will always be between `0` and `this._items.length` due to the `%`. Yes, to wrap around — this matches macOS. Good catch!
Created attachment 392844 [details] Patch
Comment on attachment 392844 [details] Patch Clearing flags on attachment: 392844 Committed r258057: <https://trac.webkit.org/changeset/258057>
All reviewed patches have been landed. Closing bug.