🪃 fix: Return Focus to Trigger When Dialogs Close - #120
Conversation
Inline @clickhouse/click-ui in vitest server deps and exclude .claude/** from test discovery. click-ui 0.9.1 ships ESM that imports .css files, which Node's loader cannot parse, so Vitest must process the package through Vite.
Every app dialog is a controlled Radix modal rendered without a Dialog.Trigger, so Radix's default onCloseAutoFocus prevents default and tries to focus a triggerRef that only DialogTrigger ever sets. With the ref null, focus restoration is suppressed and focus falls to document.body whenever a dialog closes. A new useReturnFocus hook captures document.activeElement during the render where open flips true (before Radix's FocusScope moves focus into the dialog) and returns an onCloseAutoFocus handler that prevents Radix's default and refocuses the captured element. It is wired into ImportYamlDialog, ConfirmSaveDialog, EditRoleDialog, CreateRoleDialog, EditGroupDialog, CreateGroupDialog, EditCapabilitiesDialog, and the shared FormDialog. ConfirmSaveDialog needs a fallback because a successful save clears the dirty state and unmounts the StickyActionBar that held the Save trigger. ConfigPage now passes a ref to its always-mounted root container (tabIndex -1) so focus has somewhere sensible to land when the captured element is gone. The hook also treats a captured document.body as nothing captured so the fallback applies in browsers that do not focus clicked elements. GrantManagementTab's hand-rolled requestAnimationFrame focus-return workaround covered the same Radix behavior and is migrated to the hook, with the clicked row passed as the fallback target so focus still returns to the row deterministically.
dustinhealy
changed the title
|
@codex review |
![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: 9cb4349cb2
ℹ️ 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".
Pointer activation does not always move focus to the grant table row, so useReturnFocus could capture whatever element was focused before (for example the search box) and restore that instead of the row, regressing the previous close handler which always refocused the edited row. Focusing the row in openEditor before opening makes the hook capture the row itself, so close restores it in every activation mode. SettingsDialog, UserDetailDialog, ResetBaseConfigDialog, ProfileValueModal, DeleteProfileValueModal and ProfileIndicator are controlled dialogs with no Radix trigger, so closing them still dropped focus to document.body. Wire them with the same useReturnFocus onCloseAutoFocus pattern and extend the focus tests to cover the new dialogs and the grant row scenario.
Frontend component tests belong in an adjacent __tests__ directory per the repo convention in AGENTS.md, and this file was added directly under src/components in this branch.
|
@codex re-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffde702cff
ℹ️ 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".
…s trigger unmounts A successful base reset clears the last database override, which unmounts the header Reset button before the dialog closes, so the reset dialog now receives the same page container fallback ConfirmSaveDialog already uses. The audit detail drawer's four raw Radix content branches had neither a trigger nor onCloseAutoFocus and now share one useReturnFocus wiring, and the hook test moves under __tests__ per the repo convention.
|
@codex re-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34867c109a
ℹ️ 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".
Codex round 3 flagged five dialogs whose captured opener either unmounts before close or never receives focus from pointer activation, so restore landed on document.body or an unrelated filter input. The settings dialog now falls back to the sidebar user menu trigger, the profile value and delete modals fall back to the cascade list container (their scope-selection and per-row buttons unmount on a successful save or removal), and the role and group edit dialogs fall back to their tab containers since a rename can drop the row out of the filtered list. The users page and audit log now mirror GrantManagementTab by focusing the activated row before opening, so the detail dialog and drawer capture the row instead of whatever was focused before, and the user rows also thread that row as the dialog fallback for the kebab menu path. A parametrized fallback test covers the new prop threading.
|
@codex re-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49ec02eb73
ℹ️ 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".
…ermalinks The row ref callbacks now delete entries when rows unmount so paginated audit and user browsing does not accumulate detached table rows. The users row gets the same focus-visible treatment as the grant and audit rows so restored keyboard focus is visible, and the audit drawer takes the tab container as a fallback for cold entryId permalinks where no opener was ever focused.
In browsers where pointer activation does not focus the Import button the hook captures document.body, which it rejects on close, so without a fallback focus stayed on the body. The import dialog now takes the same page container fallback the save and reset dialogs use.
|
@codex re-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69ccc5ebcb
ℹ️ 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 (captured && captured !== document.body && document.contains(captured)) { | ||
| captured.focus(); | ||
| return; |
There was a problem hiding this comment.
When a profile removal is confirmed, FieldProfilePopover starts removeMutation and closes the modal in the same callback (FieldProfilePopover.tsx:271-273), while saving disables the captured trash button (FieldProfilePopover.tsx:181-185). Calling focus() on that still-connected disabled button is a no-op, but this branch returns unconditionally, so the supplied fallbackRef is never focused and focus falls to the body. Only return when document.activeElement === captured after the attempt, or reject disabled/inert targets before trying the fallback.
Useful? React with 👍 / 👎.
1 participant
Summary
Closing any app dialog (Import YAML, save confirmation, role edit/create, group edit/create) dropped keyboard focus on
document.bodyinstead of returning it to the button that opened the dialog.Root cause: every dialog in the app is a controlled Radix modal rendered without a
Dialog.Trigger. Radix's defaultonCloseAutoFocuscallsevent.preventDefault()and then focusescontext.triggerRef, which onlyDialog.Triggerpopulates, so with controlled dialogs the ref is null, the default focus restore is suppressed, and nothing receives focus. This is a pre-existing keyboard accessibility bug surfaced by QA around the click-ui 0.9.1 bump, not a regression from the bump itself (@radix-ui/react-dialogis pinned to 1.1.15 via overrides on both click-ui versions).Fix: a new
useReturnFocushook captures the previously focused element on the open transition and restores it viaonCloseAutoFocus(click-uiDialog.Contentpasses the prop through to Radix). It is wired into all eight app dialogs.ConfirmSaveDialogadditionally accepts afallbackRefbecause a successful save unmounts its trigger (the sticky Save bar); focus then lands on the always-mounted page container. The hand-rolledrequestAnimationFramefocus workaround inGrantManagementTabis migrated to the same hook, preserving the focus-the-row semantics locked bye2e/grants.spec.ts.Change Type
Testing
11 new Vitest cases render the real click-ui
Dialog(no dialog mocking): hook tests cover close via Escape, close via the close button, and the fallback path when the trigger unmounts before close; a table-driven suite asserts focus return for all eight dialogs.Manually verified in the app: opening and closing each affected dialog returns focus to its trigger; after a successful config save, focus lands on the page container.
Behavioral proof: all 11 new dialog focus cases fail on the base branch (focus lands on document.body) and pass with this change, exercising the real click-ui Dialog with keyboard-driven open and close.
Test Configuration:
Checklist