Bug 209617 – Web Inspector: RTL: ArrowLeft and ArrowRight keys select wrong navigation bar items

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Steps:
0. Enable RTL mode
1. Open Elements tab
2. In the right sidebar, focus on "Styles" navigation bar item
3. Press Arrow Left key

Expected:
Item on the left is selected ("Computed").

Actual:
Selection didn't change.

[Nikita Vasilyev]

Created attachment 394651 [details]
Patch

[Devin Rousso]

Comment on attachment 394651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394651&action=review

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:383
> +        if (delta === -1) {

Why not combine these into one loop?

[Nikita Vasilyev]

Comment on attachment 394651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394651&action=review

>> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:383
>> +        if (delta === -1) {
> 
> Why not combine these into one loop?

I can. I simply didn't touch what was working.

[Nikita Vasilyev]

Created attachment 394659 [details]
Patch

Looks much nicer now.

[Devin Rousso]

Comment on attachment 394659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394659&action=review

r=me, with a few changes

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:371
> +        if (event.code !== "ArrowLeft" && event.code !== "ArrowRight")

NIT: I'd pull this out into an `isLeftArrow` variable so you can avoid the repeated comparison below

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:377
> +        let selectedIndex = this._navigationItems.indexOf(this._selectedNavigationItem);

NIT: I'd put this closer to the `while`, where it's used

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:-381
> -            if (selectedNavigationItemIndex === -1)
> -                selectedNavigationItemIndex = this._navigationItems.length;

I think we still need this logic, or at least something like it.
```
    if (selectedIndex === -1)
        selectedIndex = (this._navigationItems.length + delta) % this._navigationItems.length;
```

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:383
> +        while (true) {

I generally try to avoid `while (true)` as they can often easily turn into an infinite loop, but the bounding in this case looks fine.  To be really safe, I'd ether add `console.assert(selectedIndex)` or move the first `if` to just be the condition of the `while` itself.

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:392
> +                this.selectedNavigationItem?.element.focus();

I don't think we need the `?.`, as it shouldn't be possible for a `WI.RadioButtonNavigationItem` to not have an `element`.

[Nikita Vasilyev]

Comment on attachment 394659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394659&action=review

>> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:392
>> +                this.selectedNavigationItem?.element.focus();
> 
> I don't think we need the `?.`, as it shouldn't be possible for a `WI.RadioButtonNavigationItem` to not have an `element`.

It checks for `this.selectedNavigationItem` which can be null. Not the element.

[Nikita Vasilyev]

Comment on attachment 394659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394659&action=review

>> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:-381
>> -                selectedNavigationItemIndex = this._navigationItems.length;
> 
> I think we still need this logic, or at least something like it.
> ```
>     if (selectedIndex === -1)
>         selectedIndex = (this._navigationItems.length + delta) % this._navigationItems.length;
> ```

Currently in NavigationBar, focus doesn't cycle from the last item to the 1st. It *does* cycle in ScopeBar. I have a slight preference for not cycling the focus.

[Devin Rousso]

Comment on attachment 394659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394659&action=review

>>> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:392
>>> +                this.selectedNavigationItem?.element.focus();
>> 
>> I don't think we need the `?.`, as it shouldn't be possible for a `WI.RadioButtonNavigationItem` to not have an `element`.
> 
> It checks for `this.selectedNavigationItem` which can be null. Not the element.

Err, yes, my mistake 😅

Regardless, it shouldn't be possible for `this.selectedNavigationItem` to not exist in these conditions.  `WI.NavigationBar.prototype.set selectedNavigationItem` will only result in a `null` value for `this.selectedNavitationItem` if:
 - the given `WI.NavigationItem` is attached to a different `WI.NavigationBar`
    => this isn't possible because we're grabbing the `WI.NavigationItem` from `this._navigationItems`, meaning `this` is already the right parent
 - the given `WI.NavigationItem` is `null` or not a `WI.RadioButtonNavigationItem`
    => this isn't possible because of the `if` condition that had to have been true for this code to be run

As such, I think we should be safe to remove it.  Or am I missing something?

[Devin Rousso]

Comment on attachment 394659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394659&action=review

>>> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:-381
>>> -                selectedNavigationItemIndex = this._navigationItems.length;
>> 
>> I think we still need this logic, or at least something like it.
>> ```
>>     if (selectedIndex === -1)
>>         selectedIndex = (this._navigationItems.length + delta) % this._navigationItems.length;
>> ```
> 
> Currently in NavigationBar, focus doesn't cycle from the last item to the 1st. It *does* cycle in ScopeBar. I have a slight preference for not cycling the focus.

I don't think that's what this does.  I think this just makes it so that if nothing is currently selected (or if somehow the selected `WI.NavigationItem` is not in the list of `_navigationItems`), it selects either the first or the last `WI.NavigationItem` depending on what key was pressed.

[Nikita Vasilyev]

Comment on attachment 394659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394659&action=review

>>>> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:-381
>>>> -                selectedNavigationItemIndex = this._navigationItems.length;
>>> 
>>> I think we still need this logic, or at least something like it.
>>> ```
>>>     if (selectedIndex === -1)
>>>         selectedIndex = (this._navigationItems.length + delta) % this._navigationItems.length;
>>> ```
>> 
>> Currently in NavigationBar, focus doesn't cycle from the last item to the 1st. It *does* cycle in ScopeBar. I have a slight preference for not cycling the focus.
> 
> I don't think that's what this does.  I think this just makes it so that if nothing is currently selected (or if somehow the selected `WI.NavigationItem` is not in the list of `_navigationItems`), it selects either the first or the last `WI.NavigationItem` depending on what key was pressed.

Oh, you're right.

You just pointed out that this.selectedNavitationItem should never be null here, so this shouldn't be something that could happen.

[Nikita Vasilyev]

Created attachment 394664 [details]
Patch

[Nikita Vasilyev]

Created attachment 394667 [details]
Patch

[Devin Rousso]

Comment on attachment 394659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394659&action=review

>>>>> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:-381
>>>>> -                selectedNavigationItemIndex = this._navigationItems.length;
>>>> 
>>>> I think we still need this logic, or at least something like it.
>>>> ```
>>>>     if (selectedIndex === -1)
>>>>         selectedIndex = (this._navigationItems.length + delta) % this._navigationItems.length;
>>>> ```
>>> 
>>> Currently in NavigationBar, focus doesn't cycle from the last item to the 1st. It *does* cycle in ScopeBar. I have a slight preference for not cycling the focus.
>> 
>> I don't think that's what this does.  I think this just makes it so that if nothing is currently selected (or if somehow the selected `WI.NavigationItem` is not in the list of `_navigationItems`), it selects either the first or the last `WI.NavigationItem` depending on what key was pressed.
> 
> Oh, you're right.
> 
> You just pointed out that this.selectedNavitationItem should never be null here, so this shouldn't be something that could happen.

I pointed out that `this.selctedNavigationItem` can't be `null` _below_.  I think this is completely possible for it to be `null` here, such as in the case that a `WI.NavigationBar` doesn't have any `WI.RadioButtonNavigationItem`.

[Nikita Vasilyev]

Created attachment 394675 [details]
Patch

[EWS]

Committed r259094: <https://trac.webkit.org/changeset/259094>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394675 [details].

[Radar WebKit Bug Importer]

<rdar://problem/60943785>