Repository navigation
Conversation
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>
bb562dd to
c03b16d
Compare
|
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. |
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.onTerminalKeyRouterequiresworkspace.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:
ModalTeardown.dismiss) leavesGetFocus()on the shell window even though the app already hasWM_ACTIVATEandWM_SETFOCUShandlers that callfocusRestoredPane, 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.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.UpdateInstallDialogusesdismissWithwith its ownApiand is not covered.Appregisters the hook at startup.restoreFocusAfterModalputs 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 samefocusRestoredPaneas the existingWM_SETFOCUS/WM_ACTIVATEpolicy.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.pressFocusedsends 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,dismisscalls 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
ModalTeardown.dismiss.Appregisters 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.handleAction), and the close-tab command posted by the route is observed, then applied by callinghandleAction(.close_tab); neither WM_COMMAND is delivered through the shell window procedure in this fixture.USERPROFILE,APPDATA,LOCALAPPDATA,TEMP,TMPandGRAPHCODE_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
git commit -s) per the DCOmake test) - macOS target, not run: Windows-only change; the Windows roots above were run insteadmake check) - macOS lint target, not run: Windows-only change in Zig