Bug 209760

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.

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) }

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

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.

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?

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.

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

Created attachment 394999 [details] Patch

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.

Created attachment 395000 [details] Patch Fixed typo.

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

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.

Committed r259277: <https://trac.webkit.org/changeset/259277> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395007 [details].

<rdar://problem/61087781>