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

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

# STEPS TO REPRODUCE:
1. inspect any page
2. detach/undock Web Inspector
3. go to the Timelines Tab
4. close Web Inspector
5. open Web Inspector
6. press space
 => expect a timeline recording to start, but instead the Dock Side button is "clicked" causing Web Inspector to dock to the side

[Nikita Vasilyev]

There's something specific about undocked mode that makes it focus on the first focusable DOM element. I don't quite understand it yet.

On Safari shipped with macOS Catalina it focuses on the global search field. It was the first focusable DOM element at the time.

[Nikita Vasilyev]

I attempted to duct tape the issue.

I appended the following to WI.updateDockedState:

    if (side === WI.DockConfiguration.Undocked) {
        console.info(document.activeElement); // activeElement is the dock to right button
        document.activeElement?.blur();
        console.info(document.activeElement); // activeElement is document.body
    }

To my surprise, this didn't even work!

    if (side === WI.DockConfiguration.Undocked) {
        console.info(document.activeElement);
        document.activeElement?.blur();
        console.info(document.activeElement);

        setTimeout(() => {
            console.log(document.activeElement); // activeElement is the dock to right button, again!
        }, 500)
    }

[Nikita Vasilyev]

If this's a regression than it broke over a year ago.

[Nikita Vasilyev]

Created attachment 394986 [details]
Patch

This is the least ugliest fix I could come up with.

---

I've tried restoring focus in WI.TabBar.prototype.didLayoutSubtree. That fixed the problem described in the bug, but it didn't fix this:

1. Inspect any page
2. Dock Web Inspector
3. Go to the Timelines Tab
4. Undock Web Inspector (by pressing Command-Shift-D, for example)

My current patch does fix it.

---

I tried restoring focus on requestAnimationFrame. It didn't work.

I've tried restoring focus after a timeout. It introduced flickering. Also, it was even uglier than my current patch.

[Devin Rousso]

Comment on attachment 394986 [details]
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?

[Nikita Vasilyev]

Comment on attachment 394986 [details]
Patch

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

>> 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"`.

I don't. Frankly, I don't test on WebKitGTK.

>> 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.

Whoops, you're right. The keyword is *visible*. (When I stare into something for 3 hours straight I make mistakes like this.)

>> 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?

If some change in WebKit happens to fix this bug, this code would oddly move the focus to the previously focused element. The idea was mitigate that by checking if it's in the `WI.tabBar`.

I observed focus changes _only_ after `WI.updateDockedState` is called. Furthermore, I couldn't pinpoint the exact code that triggered the focus change, so I couldn't add a callback.

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #5)
> 
> Amazing that this hasn't been noticed till now (o.0)

When it was focusing on the search field it didn't stand out as it does now.

[Nikita Vasilyev]

Created attachment 394999 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 394999 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:909
> +            let firstFocusableElement = document.querySelector("[tabIndex='0']:not(.hidden)");
> +            if (firstFocusableElement === event.target) {

How about this? I think this is more future-proof.

[Nikita Vasilyev]

Created attachment 395000 [details]
Patch

Fixed typo.

[Devin Rousso]

Comment on attachment 394999 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:908
> +            let firstFocusableElement = document.querySelector("[tabIndex='0']:not(.hidden)");

I realize that CSS selectors aren't case sensitive, but we should try to keep CSS/DOM things alike.  As such, this should be `tabindex` (lowercase).

>> Source/WebInspectorUI/UserInterface/Base/Main.js:909
>> +            if (firstFocusableElement === event.target) {
> 
> How about this? I think this is more future-proof.

Yeah, that sounds reasonable.

This would break if we ever added a non-0 `tabindex`, however.  What if instead of listening for `"focusin"`, we set a global flag when updating the docking configuration that calls `preventDefault()` on the next `"focus"` event?  From what you've seen, it will come after `WI.updateDockedState` is called, so maybe we can just do that instead of having to worry about whether or not the newly-incorretly-focused node is the node we expect?

Ideally, it'd be great to understand why this is happening at all, and perhaps figure out a fix for it in the backed (e.g. set some preference on the webview/window used by Web Inspector).

[Nikita Vasilyev]

Created attachment 395007 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Base/Main.js:909
>>> +            if (firstFocusableElement === event.target) {
>> 
>> How about this? I think this is more future-proof.
> 
> Yeah, that sounds reasonable.
> 
> This would break if we ever added a non-0 `tabindex`, however.  What if instead of listening for `"focusin"`, we set a global flag when updating the docking configuration that calls `preventDefault()` on the next `"focus"` event?  From what you've seen, it will come after `WI.updateDockedState` is called, so maybe we can just do that instead of having to worry about whether or not the newly-incorretly-focused node is the node we expect?
> 
> Ideally, it'd be great to understand why this is happening at all, and perhaps figure out a fix for it in the backed (e.g. set some preference on the webview/window used by Web Inspector).

Focus event isn't preventable.

[EWS]

Committed r259277: <https://trac.webkit.org/changeset/259277>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395007 [details].

[Radar WebKit Bug Importer]

<rdar://problem/61087781>