Skip to content

Put keyboard focus back on the terminal when a sheet or dialog closes - #686

Closed
coneilen wants to merge 1 commit into
mainfrom
coneilen-terminal-focus-restore
Closed

coneilen wants to merge 1 commit into
mainfrom
coneilen-terminal-focus-restore

Conversation

@coneilen

Copy link
Copy Markdown
Collaborator

Summary

On the Dev Box, Ctrl+Shift+W did not close the terminal tab after a cancelled Ctrl+Shift+N sheet and an Alt+Shift+D split in the same session, though it worked in isolation. The terminal key route only recognizes a terminal key by the window the message is sent to (App.onTerminalKeyRoute requires workspace.ownsSurfaceWindow(message.hwnd); MainWindow.dispatchMessage). When keyboard focus is on the shell window itself instead of a terminal child, the route is .default, and Ctrl+Shift+W falls through to the accelerator table, where it is the Worktrees command. This restores keyboard focus to the selected terminal child when a sheet or dialog closes.

What I could and could not establish, honestly:

  • Reproduced in-process: a stand-in sheet (a real top-level window; the owner disabled, then torn down with the production ModalTeardown.dismiss) leaves GetFocus() on the shell window even though the app already has WM_ACTIVATE and WM_SETFOCUS handlers that call focusRestoredPane, with the shell window active and foreground and no error status. I did not find out why those handlers do not restore it in the test process, and I cannot say it is the same mechanism as on the Dev Box.
  • Not reproduced: the real node-creation sheet (it needs product settings and a project), the Dev Box, and any focus loss caused by the split itself. In the test the split (handleAction(.split_horizontal)) left focus on a live terminal child; I am not claiming the split was a cause. The Alt+Shift+D accelerator and the close-tab WM_COMMAND were not delivered through the window procedure either (see below).

Changes

  • ModalTeardown.dismiss, the common teardown of every sheet and dialog (node and edge sheets, settings, onboarding, jump palette, repository dialogs, worktree dialogs and others), now calls an optional hook after the owner is reactivated. UpdateInstallDialog uses dismissWith with its own Api and is not covered.
  • App registers the hook at startup. restoreFocusAfterModal puts focus back on the selected terminal child only when the workspace is what the user was looking at (the workspace surface or its panel), the toolbar does not hold focus, focus is not already on a terminal child, and focus is on the shell window or nowhere (a control the user focused deliberately keeps it). It uses the same focusRestoredPane as the existing WM_SETFOCUS/WM_ACTIVATE policy.

What each test proves

  • live terminal keyboard: Ctrl+Shift+W still closes the tab after a cancelled sheet and a split (real Winghostty surface windows, fake zmx): after the stand-in sheet is torn down, focus is on a terminal child; handleAction(.split_horizontal) gives two panes with focus on a live terminal; LiveKeyboard.pressFocused sends Ctrl+Shift+W to whatever has focus and the shell posts the close-tab command; handleAction(.close_tab) closes the pane and focus is on a live terminal.
  • the real teardown tells the shell which owner was reactivated, after the dialog is gone (ModalTeardown.zig): two real windows, a probe hook, dismiss calls it once with the owner after the dialog window is destroyed.

RED before the fix: the focus test, with the hook unregistered, failed at its first assertion (focus was not on a terminal child after the sheet). The hook test uses the new hook, so it would only fail to compile before the change (not behavioral evidence).

What is NOT tested

  • The real New Loop sheet, any other real dialog, or the Dev Box. The sheet is a stand-in window running only the production ModalTeardown.dismiss.
  • That App registers the hook when it really starts (App.run): the live test registers the same callback the way the shell does, but the production registration line is not executed by a test.
  • The accelerator path itself: the Alt+Shift+D accelerator is not exercised (the split is invoked through handleAction), and the close-tab command posted by the route is observed, then applied by calling handleAction(.close_tab); neither WM_COMMAND is delivered through the shell window procedure in this fixture.
  • Focus stealing from a control that legitimately holds it is guarded by a rule (focus on the shell window or nowhere) but only that case and the already-on-terminal case are covered.
  • Isolation: process-only scratch USERPROFILE, APPDATA, LOCALAPPDATA, TEMP, TMP and GRAPHCODE_SUPPORT_DIR; no installed daemon, per-user zmx or real clipboard.

Test plan

RED: zig test src\App.zig --test-filter "cancelled sheet" (pinned target and link flags, hook not registered) -> FAIL (TestUnexpectedResult) at the assertion that the terminal child has focus after the sheet is torn down
GREEN: zig test src\App.zig --test-filter "cancelled sheet" (pinned flags, with the change) -> All 1 tests passed; --test-filter "teardown" -> All 12 tests passed
REGRESSION: zig test for TerminalKeys, TerminalVt, TerminalKeyEncoding, TerminalSelection, Clipboard, TerminalSurface and App roots (pinned flags) -> 10, 22, 39, 35, 4, 233 and 1013 tests passed, 0 failed (main after the F6/F10 change: App 1011)

Checklist

  • I have read the Contributing Guidelines
  • I have signed off my commits (git commit -s) per the DCO
  • Tests pass locally (make test) - macOS target, not run: Windows-only change; the Windows roots above were run instead
  • Code follows the existing style (make check) - macOS lint target, not run: Windows-only change in Zig
  • I added the test/contract before the implementation and observed the intended RED failure

After a sheet closed, the shell window itself could be left holding keyboard
focus. Terminal keys are recognized by the window they are sent to, so
Ctrl+Shift+W then reached the Worktrees accelerator instead of closing the
tab. ModalTeardown.dismiss, shared by every sheet and dialog, now reports the
reactivated owner and the shell restores the selected terminal's focus.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Signed-off-by: Colin Neilens <coneilen@microsoft.com>
@coneilen
coneilen force-pushed the coneilen-terminal-focus-restore branch from bb562dd to c03b16d Compare October 10, 2026 07:40
@coneilen

Copy link
Copy Markdown
Collaborator Author

Closing without merging. While addressing review I found the premise does not hold: the in-process 'repro' used the live fixture's shell window, which is a plain STATIC window (overviewTestApp), so the app's real WM_ACTIVATE/WM_SETFOCUS handlers (which call focusRestoredPane after a modal closes) never ran. Focus staying on the shell window after the stand-in sheet is an artifact of that fixture, not evidence of a production defect, and this hook would duplicate the existing restore policy. The Dev Box symptom (Ctrl+Shift+W not closing the tab after a cancelled Ctrl+Shift+N sheet and an Alt+Shift+D split) is therefore NOT root-caused or reproduced; it needs a trace on the Dev Box (GetFocus after each step), or a fixture that runs the real window procedure. The review findings on this PR (UpdateInstallDialog bypass, focus captured before the modal, stale owner/global hook) become moot if the PR is not pursued.

@coneilen coneilen closed this Oct 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant