[webkit-reviews] review denied: [Bug 210447] Web Inspector: AXI: tabs should be focusable when navigating by pressing Tab : [Attachment 396318] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 210447: Web Inspector: AXI: tabs should be focusable when navigating by
pressing Tab
https://bugs.webkit.org/show_bug.cgi?id=210447

Attachment 396318: Patch

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




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 396318
  --> https://bugs.webkit.org/attachment.cgi?id=396318
Patch

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

r-, as this needs work for the detached/undocked state

Overall though, most of the logic/look/feel seems fine :)

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:694
> +    _tabBarItemActivated(event, byMouse)

Instead of passing a boolean flag `byMouse` (this could definitely use a better
name, like `fromMouse`), why not just look at `event.type.startsWith("mouse")`
or even just `event.type === "mosuedown"`?

NIT: I like to put non event listener member functions above event listener
callbacks (makes it clearer what's actual logic code vs just an event listener
"pipe" to some logic), so I'd move this after `_finishExpandingTabsAfterClose`

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:714
> +	       // FIXME: WI.ContextMenu.createFromEvent doesn't support keyDown
event.

Why can't we fix this?	We should just be able to modify
`InspectorFrontendHost::dispatchEventAsContextMenuEvent` to support a
`KeyboardEvent`, likely just using the middle of the `Event::target` as a
replacement for `MouseRelatedEvent::absoluteLocation`.