[webkit-reviews] review granted: [Bug 209617] Web Inspector: RTL: ArrowLeft and ArrowRight keys select wrong navigation bar items : [Attachment 394659] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 209617: Web Inspector: RTL: ArrowLeft and ArrowRight keys select wrong
navigation bar items
https://bugs.webkit.org/show_bug.cgi?id=209617

Attachment 394659: Patch

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




--- Comment #5 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 394659
  --> https://bugs.webkit.org/attachment.cgi?id=394659
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`.