fix(windows): leave a connection window's first focus to its tab so Cmd+W closes the tab - #2981
Merged
Merged
Conversation
…md+W closes the tab
…ions strip collapses
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
After relaunching with restored tabs, pressing
Cmd+Wbefore clicking anything closed the whole connection window instead of the current tab. Clicking the editor or the grid first madeCmd+Wclose the tab as expected.It is not specific to restore. Any connection window nobody has clicked into yet does it, including one opened fresh from the welcome window.
Root cause
A connection window never said where its first keyboard focus goes, so AppKit picked it once, as the window was first put on screen, from the views that existed at that moment. The editor, the grid and the object list are SwiftUI and are not built yet at that point. The only view that could take focus was the connections strip's list.
That list qualified even when the strip was "hidden", because collapsing the strip only set its width constraint to 0 and never set
isHidden. AppKit treats a zero-width view as visible, so the list stayed a key view. The strip's controller answersperformClose:itself and closes its highlighted entry, and for a connection's only entry that means closing the connection. SoCmd+Wwent to the strip, not to the window.The editor (
SQLEditorCoordinator.installEditorServices) and the object list (SidebarOutlineView.adoptFirstResponderIfVacant) already take focus when nobody holds it. The strip already held it, so both gave way.Measured in a sandboxed Debug build of
main, readingAXFocusedUIElementbefore pressingCmd+W:Cmd+Wdidworkspace-rail(zero width, invisible)workspace-railworkspace-railProbes against AppKit on macOS 27 confirmed the rest: the pick happens at
makeKeyAndOrderFrontwhether or notautorecalculatesKeyViewLoopis set; views added later never take it; a visible strip wins too; and hiding a view that holds focus hands it on.The fix
Two changes, each with its own job:
NavigationSidebarViewController). It is shown before it grows and hidden once it has shrunk, so the animation is unchanged. If the keyboard is in the strip when it starts to collapse, focus moves to the next key view, which is what AppKit does itself when a focused view is hidden. The window keeps focus only if no other key view exists. This takes the invisible list out of the key view loop, the responder chain and the accessibility tree. That covers one connection, Show connections turned off, and View > Hide Connections.EditorWindow,MainSplitViewController+Focus).initialFirstResponderis the container the selected tab's content is shown in. The container takes no focus itself, so the window keeps it when it appears, and the editor or object list adopts it once built, which is the vacancy contract both already follow. This is what covers a strip that is already on screen at first show, which hiding cannot reach.The strip keeps its documented
Cmd+Wfor when you deliberately put the keyboard in it, perdocs/features/keyboard-shortcuts.mdx.Tests
ConnectionWindowInitialFocusTests(unit):Against the old code, four of the first five fail. The fifth is the shown-strip control, which passes either way.
CloseTabBeforeFirstClickUITests(UI):Cmd+Wwithout clicking: the table tab closes and the connection stays open.Cmd+Wwithout clicking: one tab closes and both connections stay in the strip.Before / After
Two connections restored into one window, nothing clicked, then
Cmd+W.Start: two entries in the strip, the "Query 1" tab selected.
Before: the "Chinook (Sample)" connection closed, and the strip went with it.
After: only the "Query 1" tab closed. Both connections are still in the strip.
Verification
verify.sh build: PASS.verify.sh test ConnectionWindowInitialFocusTests ConnectionWindowChromeTests: PASS, 21/21.mainfails in four of its first five cases.swiftlint lint --stricton every changed file: 0 violations.TABLEPRO_UI_TEST_SANDBOXDebug build, one and two restored connections: focus lands in the SQL editor,Cmd+Wcloses the tab, and every connection stays open.CloseTabBeforeFirstClickUITestsandWindowFocusUITests. The runner stopped at macOS's "Enable UI Automation" password prompt (Timed out while enabling automation mode, zero cases executed), so CI's UI shards are their first run./code-reviewread the diff instead. Its one finding was that collapsing a focused strip left the keyboard with the window. That is fixed in the second commit and has its own test.Not changed
The strip still closes its highlighted entry on
Cmd+Wwhen you have put the keyboard in it yourself, askeyboard-shortcuts.mdxdocuments.