fix(datagrid): move focus to grid on explicit table open (#1490)#1497
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9df84b1c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| super.viewDidMoveToWindow() | ||
| guard coordinator?.tabType == .table, let window else { return } | ||
| let current = window.firstResponder | ||
| guard current == nil || current === window else { return } |
There was a problem hiding this comment.
When a table is opened from the sidebar by keyboard, the sidebar outline/list is still the window's first responder while the new grid is inserted. This guard only allows focusing when firstResponder is nil or the window itself, so viewDidMoveToWindow() skips makeFirstResponder(self) in the primary reported flow and arrow keys continue not to reach KeyHandlingTableView until the user clicks the grid. The focus check needs to allow replacing focus from the previous sidebar/tab content while still avoiding active sheets or in-grid controls.
Useful? React with 👍 / 👎.
datlechin
changed the title
There was a problem hiding this comment.
💡 Codex Review
Focus the grid when reusing an existing table view
If the current tab is reusable (notably a preview table tab), this branch replaces the tab contents in the existing DataGridView and returns, so no viewDidMoveToWindow() fires to consume the pending focus request. In the common preview-tabs flow where the sidebar still owns first responder and the user explicitly opens a different table into the preview tab, the new table loads but arrow keys remain in the sidebar; the pending flag also remains set for a later unrelated grid.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| perform { | ||
| if let clickedTable { | ||
| coordinator?.openTableTab(clickedTable, showStructure: true) | ||
| coordinator?.openTableTab(clickedTable, showStructure: true, activateGridFocus: true) |
There was a problem hiding this comment.
When this path opens the Structure view, the rendered TableStructureView grid is configured without tabType, so KeyHandlingTableView.viewDidMoveToWindow() never consumes pendingGridFocusOnOpen. The flag then stays set until the next data-tab grid appears; for example, after choosing Show Structure, a later single-selection preview open from the sidebar (which passes activateGridFocus: false) can unexpectedly yank focus from the sidebar into that unrelated grid.
Useful? React with 👍 / 👎.
…bound stale intent (#1490)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d3f7e2113
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| if activateGridFocus { | ||
| pendingGridFocusOnOpen = true |
There was a problem hiding this comment.
When preview tabs are enabled and an explicit open reuses the currently selected preview table tab, this only sets pendingGridFocusOnOpen, then reuseActiveTab mutates the same tab in place. The existing DataGridView is not remounted in that path, so KeyHandlingTableView.viewDidMoveToWindow() does not run to consume the pending flag, leaving focus in the sidebar after double-click/Return on a different table in the preview tab. This path needs to focus the attached grid (or otherwise consume the pending intent) after reusing the tab.
Useful? React with 👍 / 👎.
| pendingGridFocusOnOpen = false | ||
| window.makeKeyAndOrderFront(nil) |
There was a problem hiding this comment.
When the quick switcher opens a table that already exists in another window, this branch clears the focus request on the current coordinator and only raises the sibling window. Because no grid is attached in the current window and the sibling coordinator is never asked to focus its grid, the sibling window keeps whatever first responder it previously had (for example the editor or sidebar), so arrow keys still do not navigate the selected table after this explicit open.
Useful? React with 👍 / 👎.
1 participant
Part of #1490 (keyboard, focus, accessibility). Revised after testing feedback and HIG research.
What changed vs the first attempt
The first version auto-focused the grid on appear (
viewDidMoveToWindow). That is wrong: Apple's HIG ("Focus and selection") says "Avoid changing focus without people's interaction," and auto-focusing fought the sidebar's selection-driven browsing (arrowing the sidebar loads each table; stealing focus on load would break browsing). In testing, focus stayed on the sidebar and arrows navigated the table list.HIG-correct model (what this implements)
Implementation
MainContentCoordinator.pendingGridFocusOnOpen: a one-shot intent set only byopenTableTab(activateGridFocus: true).openTableTab: on the fast-path (table already active, e.g. a promoted preview) it focuses the grid directly; otherwise it sets the one-shot, which the freshly built grid consumes.KeyHandlingTableView.viewDidMoveToWindowconsumes the one-shot (consumePendingGridFocus()) and makes itself first responder. It no longer focuses on mere appearance.activateGridFocus: true: the database tree'sprimaryAction(Return/double-click), the sidebar double-click handler inMainSplitViewController, favorites open, and the context-menu open. Selection-driven opens (handleTableSelectionChange, the tree's selectiononChange) do not.SQLEditorCoordinator!isDestroyedguard from the first commit stays (correct on its own: a torn-down editor must not grab focus).Verify (keyboard only)
Not in this PR (separate follow-up)
Tab moving focus between panes via the key view loop. HIG research confirms that is the other half of the native model, but bridging Tab across two
NSHostingControllerpanes and the representable table (which owns Tab for cell nav) is a larger, separate change.Focus/responder behavior is not unit-testable here (consistent with the codebase); verification is manual.