Bug 208277 – Web Inspector: AXI: scope bars should be focusable when navigating by pressing Tab

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

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.

[Radar WebKit Bug Importer]

<rdar://problem/59828111>

[Nikita Vasilyev]

Created attachment 391813 [details]
Patch

[Nikita Vasilyev]

Created attachment 391815 [details]
[Image] With patch applied - focused scope bar item

[Devin Rousso]

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`.

[Nikita Vasilyev]

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.

[Blaze Burg]

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.

[Nikita Vasilyev]

Created attachment 392043 [details]
Patch

[Nikita Vasilyev]

Created attachment 392044 [details]
Patch

[Devin Rousso]

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))
```

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 392654 [details]
Patch

[Nikita Vasilyev]

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.

[Devin Rousso]

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 `%`.

[Nikita Vasilyev]

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!

[Nikita Vasilyev]

Created attachment 392844 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 392844 [details]
Patch

Clearing flags on attachment: 392844

Committed r258057: <https://trac.webkit.org/changeset/258057>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.