[webkit-reviews] review granted: [Bug 208277] Web Inspector: AXI: scope bars should be focusable when navigating by pressing Tab : [Attachment 392818] 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 208277: Web Inspector: AXI: scope bars should be focusable when navigating
by pressing Tab
https://bugs.webkit.org/show_bug.cgi?id=208277

Attachment 392818: Patch

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




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