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.
Created attachment 394651 [details] Patch
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?
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.
Created attachment 394659 [details] Patch Looks much nicer now.
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`.
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.
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.
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?
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.
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.
Created attachment 394664 [details] Patch
Created attachment 394667 [details] Patch
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`.
Created attachment 394675 [details] Patch
Committed r259094: <https://trac.webkit.org/changeset/259094> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394675 [details].
<rdar://problem/60943785>