Repository navigation
fix(staged): make Return press the default action in alert and form dialogs - #1064
Merged
Merged
Conversation
An alert dialog opens with focus on its own container, not on a button (bits-ui puts it there), and nothing listened for Return on it. So the "Scan still running" confirmation raised by the transfer dialog, and every other alert dialog, dropped the key while Escape still cancelled. The shared alert-dialog content now handles a plain Return itself: - shared/dialogDefaultAction.ts decides whether a keydown is a Return for the default button. It rejects modifiers, a held key repeating, IME commits, events another handler already took, and targets with their own use for Return (buttons, links, fields, editable content, widget roles), so a focused Cancel only cancels. It also finds the default button, or nothing when that button is disabled. - AlertDialog.Action carries its variant as data-variant. - AlertDialog.Content takes a defaultAction prop (action, cancel or none). Unset, it is the Action, unless the Action is destructive, in which case Return does nothing, as before. The button is clicked, so its disabled state and click handler apply unchanged. A handled Return is prevented and stopped, since views under a dialog listen on the window. Return now confirms the transfer scan confirmation, the PR and push failure retries, the doctor fix and update prompts, resume-on-launch and the move-tag confirmation, with no change at those call sites. The PR detection dialog, which has only Close, sets "cancel". Destructive and access-granting confirmations keep Return inert. Ten of them have a destructive Action and get that from the variant. The revoke confirmation and the stagedctl authorization dialog use the default variant, so they set "none" explicitly. The form dialogs and the rest of the audit's fix plan are left for a separate commit. Verification: - New tests pass: 51 in dialogDefaultAction.test.ts, alert-dialog-content.component.test.ts (real bits-ui chrome in jsdom) and ProjectTransferScanConfirm.component.test.ts (unmocked). - With those, the affected existing tests pass, 84 tests in 8 files: imeKeydown, dialog-content, ProjectTransferDialog, ControlAuthorizationDialog and ConnectedAppsPanel. - Detaching the handler fails 7 of the new tests, and dropping the destructive rule fails the destructive test. - svelte-check reports 0 errors and 0 warnings, tsc -p tsconfig.node.json exits 0, and prettier passes on the touched files. - Not run: the full vitest suite (the machine was under heavy load), and the behaviour in the real WKWebView. Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Fix plan steps 6 to 8 of the Return key audit. A form dialog answered Return only while a text field had focus. The transfer dialog has no field, so Return never reached it, and the move and note-transfer dialogs went dead after a click on a project row: WebKit gives a clicked button no focus and hands it to the dialog instead. Dialog.Content now handles a plain Return, through the helper the alert dialogs use (shared/dialogDefaultAction.ts) and no second detection: - The helper gains findReturnSubmitForm and findEnabledSubmitButton. They find a dialog's form and its default button the way the browser does for a Return in a field: the first submit button that belongs to the form. A form that holds a multiline editor is skipped. - A Return on the dialog, or on text in it, clicks that button, so its disabled state and the form's submit handler apply unchanged. Fields, buttons, links and editors keep their own Return, so a field still submits once and a focused Cancel only cancels. A handled Return is prevented and stopped. - A dialog with such a form takes the focus itself a frame after it opens, unless something inside already has it, so the key is heard from the start. A field the dialog focuses keeps the focus. - A defaultAction prop (submit or none) overrides the default. Return now starts a project transfer, and moves a branch or a note after a click on a row, with no change at those call sites. It also saves an action after a click on its select, checkbox or icon picker. Unchanged by design: - A caller that passes an open-autofocus handler keeps full control of the open focus (note, session, write-note and diagram dialogs, the command palette). - A dialog with no form is not focused and ignores Return: the action output, image, media and diagram viewers. - A form with a multiline editor is not focused and never submits on a plain Return, from the editor or anywhere else. NewSessionModal also sets defaultAction="none", so Cmd/Ctrl+Return stays its only submit. NewProjectForm moves onto the same path. Its window keydown listener is gone, which created the project for a Return on a focused Cancel or location tile, during an IME commit, and under any dialog over the splash screen. A form now owns the Name field and Create only, so the browser submits it from the Name field. The repo, subpath and branch fields stay outside it and never create the project, as before. The repo chips stay outside it too: they are buttons with no type, and inside the form the first chip became the button Return pressed. The fields' container takes focus after a click on one of its buttons and presses Create on Return, which keeps that working on the splash screen, where no dialog surrounds the form. TagPicker uses isImeKeydown, so Safari's Return that commits a composition no longer toggles the highlighted tag. Behaviour changes to know about: - On the splash screen, a Return with focus outside the form, after a click on the page background, no longer creates the project. - On an iPhone or iPad, where fields wait for a tap, a form dialog now opens with focus on the dialog rather than on the page behind. - Not changed, but newly reachable: in the Add Repository dialog the recent-repo chips are untyped buttons inside the form. A Return in its empty search field already pressed the first chip and added that repo. A Return on the dialog itself now does the same. Verification: - The six new and extended test files pass, 105 tests: dialog-content.component.test.ts and dialogDefaultAction.test.ts, NewProjectForm.component.test.ts, and the new TagPicker, ProjectTransferDialogReturn and NewSessionModalReturn component tests, the last two with the real dialog chrome. - With those, the affected existing tests pass: 658 tests in 37 files. - Removing each behaviour in turn fails the matching tests: the keydown handler, the container focus, its three guards, the multiline rule, the session dialog's opt-out, the IME check and the new-project form's wiring. - pnpm check passes: svelte-check reports 0 errors and 0 warnings and tsc -p tsconfig.node.json exits 0. Prettier passes on touched files. - Playwright WebKit 26 with real key presses, from a throwaway harness under /tmp: the dialog fixture, NewProjectForm outside a dialog and AddRepoModal behaved as described above. That run found the chip problem, which jsdom cannot show. - Not run: the full vitest suite (load average near 40), the Tauri WKWebView itself, iOS or iPadOS, and a real IME composition. The transfer, move-branch, note-transfer and action-edit dialogs were not driven in a real browser, only the shared content under them. Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
…orce push Applies three findings from the review of the Return key work. Add Repository added a repo nobody picked. Its recent-repo chips sit inside the dialog's form, ahead of Add, and had no type, so each was a submit button. Return presses a form's first submit button, so one Return picked the first chip's repo and submitted the form with it, while Add itself was still disabled. - The chips in RepoConfigForm and the option rows in RepoSearchInput, SubpathInput and BranchPicker now say type="button". - shared/dialogDefaultAction.ts only treats a button as a form's default button when it says type="submit". A button with no type is never pressed from the keyboard, and a form with no explicit submit button gets no Return handling. - Every form in the app was checked first. All ten already have exactly one explicit type="submit" button, so none needed fixing. The shared Button component defaults to type="button". No other button with no type sits in a form: IconPicker's are in a popover outside it. Return no longer retries a failed force push. Retry repeats the push as it was asked for, so after "Force Push Failed" it force pushes again, without hooks if the first try skipped them. The alert moves out of BranchCardPrButton into BranchPushErrorDialog, which sets defaultAction="none" while the failed push is a force push. Return still retries a plain push, and a click on Retry works either way. Return on a composite container is no longer the dialog's. The helper now also leaves alone a Return on, or inside, a listbox, menu, menubar, grid, treegrid, tree, radiogroup, tablist, dialog or alertdialog that sits inside the dialog. It looks from the target up to the element that is listening and stops there, since the dialog carries a dialog role itself. Nothing in the app reaches this today. Behaviour changes to know about: - In Add Repository, a click on a recent-repo chip now only picks the repo, as Cmd+1 to 9 and the New Project form do. Before, the click also submitted the form, which added the repo and closed the dialog in one press. That came from the chips being submit buttons, not from their click handler. - A Return in that dialog's empty search field now does nothing while no repo is selected. Left alone, as asked: ResumeOnLaunchDialog, and the alert dialogs' default of confirming on Return. Verification: - New tests: AddRepoModal.component.test.ts (6, real form, repo fields and dialog chrome) and BranchPushErrorDialog.component.test.ts (11, real alert dialog chrome). Extended: dialogDefaultAction.test.ts (57) and dialog-content.component.test.ts. - Those and every test file that imports a changed source file pass: 506 tests in 28 files. - Removing each change in turn fails the matching tests: the chip's type, the explicit-submit rule, both together (Return on the dialog then adds the first recent repo), the force-push rule, the new roles and the stop at the listening element. - pnpm check passes: svelte-check reports 0 errors and 0 warnings and tsc -p tsconfig.node.json exits 0. Prettier passes on touched files. - Not run: the full vitest suite (load average between 30 and 90) and any real browser or the Tauri WKWebView. The chip click and the Return in the search field were shown in jsdom only, the second by pressing the button the browser would. "vitest related" could not parse a Svelte file, so the affected files came from a script that walks imports. Signed-off-by: Matt Toohey <contact@matttoohey.com> Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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.
Summary
Return did nothing in the transfer dialog's "Scan still running" confirmation, and in several other dialogs it only worked while a text field had focus. This branch makes Return press the default button in alert dialogs and submit form dialogs from any focus inside them, and then closes three holes that opened up.
Alert dialogs press their default button on Return
An alert dialog opens with focus on its container (bits-ui puts it there), and nothing listened for Return. The shared
AlertDialog.Contentnow handles a plain Return via a newshared/dialogDefaultAction.tshelper, which rejects modifiers, key repeat, IME commits, already-handled events and targets with their own use for Return (buttons, links, fields, editable content, widget roles). The default is the Action button unless it is destructive, in which case Return stays inert.defaultAction(action|cancel|none) overrides this; the revoke confirmation and the stagedctl authorization dialog setnone, the PR detection dialog setscancel.Form dialogs submit from any focus inside them
Dialog.Contentnow finds its form's firsttype="submit"button and clicks it on a Return on the dialog or on text inside it, so the button's disabled state and the form's submit handler apply unchanged. A dialog with such a form takes focus a frame after opening unless something inside already has it. Forms holding a multiline editor are skipped, andNewSessionModalopts out withdefaultAction="none"so Cmd/Ctrl+Return stays its only submit.Return now starts a project transfer, and moves a branch or a note after a click on a row (WebKit gives a clicked button no focus).
NewProjectFormloses its window keydown listener, which created a project on Return from a focused Cancel, during an IME commit, or under any dialog over the splash screen.TagPickerusesisImeKeydownso a Safari composition commit no longer toggles a tag.Follow-up fixes from review
RepoConfigForm,RepoSearchInput,SubpathInputandBranchPickernow saytype="button", and the helper only treats explicittype="submit"buttons as a form's default. All ten forms in the app already have exactly one.BranchCardPrButtoninto a newBranchPushErrorDialogwhich setsdefaultAction="none"while the failed push is a force push. Plain pushes still retry on Return.Behaviour changes to know about
Testing
dialogDefaultAction.test.tsand an extended NewProjectForm test.pnpm check(svelte-check + tsc) and prettier pass. Pre-pushstaged-cihook passed.🤖 Generated with Claude Code