[webkit-reviews] review granted: [Bug 209760] Web Inspector: the Dock Side navigation item is automatically focused when Web Inspector is opened detached, preventing any global spacebar shortcuts from working : [Attachment 394986] 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 209760: Web Inspector: the Dock Side navigation item is automatically
focused when Web Inspector is opened detached, preventing any global spacebar
shortcuts from working
https://bugs.webkit.org/show_bug.cgi?id=209760

Attachment 394986: Patch

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




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

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

r=me, with some comments

It may also be worth noting that on the first open (which is where I noticed
this), in tabs where we restore the focus from the previous session if able
(e.g. the previously selected node in the Elements Tab, the previously selected
resource in the Source Tab, etc.), the focus shifted away from the first
focusable item very quickly, although you could usually see the flash of a
focus ring.  Furthermore, in every tab except the Timelines Tab and the Audit
Tab, we default to focusing the console prompt (r252213), so this bug only
appeared to fully manifest (focus stays with the first visible element) on
those tabs.

With that having been said, however, this bug did always show when switching
docking configurations after Web Inspector is already open.

Amazing that this hasn't been noticed till now (o.0)

> Source/WebInspectorUI/UserInterface/Base/Main.js:905
> +    if (side === WI.DockConfiguration.Undocked) {

Do we know if this is macOS-only behavior?  We should probably only do this if
`WI.Platform.name === "mac"`.

> Source/WebInspectorUI/UserInterface/Base/Main.js:906
> +	   // When undocking, the second focusable element steals focus. Undo
this.

This doesn't sound right.  I would imagine that it's the first visible
focusable element.

> Source/WebInspectorUI/UserInterface/Base/Main.js:907
> +	   // <http://webkit.org/b/209760>

Including a link to this bug is unnecessary, as you can just blame this line to
get that info.	Please remove.

> Source/WebInspectorUI/UserInterface/Base/Main.js:909
> +	       if (WI.tabBar.element.contains(event.target)) {

Why are we checking to see if it's in the `WI.tabBar`?	If we know it steals
focus, we shouldn't need to check where it moves the focus.

```
    document.body.addEventListener("focusin", (event) => {
	if (WI.previousFocusElement)
	    WI.previousFocusElement.focus();
	else
	    event.target.blur();
    });
```

Also, this code implies that the focus change happens _after_
`WI.updateDockedState` is called.  Is that guaranteed, or could we somehow
"miss" the focus event because we added the event listener too late?