Skip to content

fix(admin): keep keyboard focus in the wizard after adding a token inline - #268

Merged
DavidLeuter merged 1 commit into
devfrom
hotfix/token-companion-keyboard-focus
Sep 29, 2026
Merged

DavidLeuter merged 1 commit into
devfrom
hotfix/token-companion-keyboard-focus

Conversation

@DavidLeuter

Copy link
Copy Markdown
Collaborator

Problem

In the create-project wizard, adding a token via keyboard only (Tab + Enter) through "Add GitHub token" left focus in a broken state: after saving, the next Tab landed on the page behind the wizard modal, and you had to click back in with the mouse.

Cause: on desktop (≥ 1280px) the token form opens in a companion panel that is portalled next to the wizard, outside the wizard's Tab trap. On save the focused submit button unmounts and focus drops to <body>. The phone layout (inline form) had the same focus drop.

Fix

In wizard/sources/AddSourceFlow.tsx:

  • CompanionModal now uses the shared useDialogFocus hook: focus moves in on open, Tab stays inside while it is open, and focus is restored on close.
  • CredentialSlot hands focus back to its trigger button whenever the form closes (save or cancel), on both desktop and phone.

This also covers "Add Atlassian credential" (Jira / Confluence), which uses the same slot.

Behaviour change: while the companion is open, Tab no longer leaves it for the wizard. Escape or the close button returns you.

Tests

  • Two new tests in CreateProjectWizard.test.tsx (phone inline + desktop companion, keyboard only). Both fail without the fix.
  • Full routine green: tsc -b, lint, prettier, build, 2958 unit tests.
  • Manually tested.

🤖 Generated with Claude Code

…line

The desktop token companion is portalled next to the wizard, outside the
wizard's Tab trap. After saving, the focused submit button unmounted and
focus dropped to <body>, so the next Tab landed on the page behind the
wizard. The phone inline form had the same drop.

The companion now uses useDialogFocus (focus in, Tab trap, restore), and
the credential slot hands focus back to its trigger whenever the form
closes. Applies to the GitHub token and Atlassian credential forms.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@kiranfin kiranfin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

The fix is small and well aimed. I found nothing that blocks the merge.

🟡 Worth a look

  1. Focus is restored by two mechanisms on desktop. useDialogFocus already refocuses the element that was active when the companion opened (its effect cleanup), and the new effect in CredentialSlot refocuses the trigger as well (src/features/admin/components/wizard/sources/AddSourceFlow.tsx:355-360). Both land on the same button today, so nothing is wrong. They can drift apart, though: if the trigger did not hold focus when the companion opened (a mouse click in Safari, for example), the hook restores focus to body and only the new effect saves the day. A short comment saying the effect is what covers the phone path and the no-focus case would help the next reader. Alternatively, keep the effect only for the phone branch.
  2. aria-modal="false" combined with a Tab trap. With the trap, a keyboard user cannot reach the wizard behind the companion without closing it first, which behaves like a modal. That is probably the intended behaviour here, but the attribute says the opposite. It is also worth a comment that two document-level Tab traps (the wizard Modal and the companion) coexist and only work because the wizard's handler does nothing while focus is outside it.

🟢 Nits / cleanup

  • The desktop test hardcodes "(min-width: 1280px)", duplicating the private DESKTOP_QUERY constant. If the breakpoint changes, the test silently falls back to the phone path and fails on findByRole("dialog"). Exporting the constant (or a comment pointing at it) would make that failure obvious.
  • The Tab-wrap assertion only checks that focus stays inside the companion. It would also pass if the trap did nothing while focus moved from Submit to another companion control. Asserting that focus lands on the first control (the close button) would pin the wrap behaviour.
  • No test covers closing the companion with Escape or Cancel and expecting focus back on the trigger. Only the save path is tested.
  • Edge case, no action needed: if the viewport crosses the 1280px breakpoint while the form is open, isDesktop flips and the form remounts in the other layout. The wasOpenRef effect does not fire in that case (isOpen is unchanged), so focus is not restored there.

Overall a focused, low-risk accessibility fix with good regression coverage for both the phone and the desktop path. Approve from my side; the two 🟡 items are comment-level and can be handled here or left as they are.

@DavidLeuter
DavidLeuter merged commit bc53deb into dev Sep 29, 2026
4 checks passed
@DavidLeuter
DavidLeuter deleted the hotfix/token-companion-keyboard-focus branch September 29, 2026 11:21
daniilperkin added a commit that referenced this pull request Sep 29, 2026
…y-escalation-preview-edit (#235)

David's second backmerge. `311` now carries dev's app-wide keyboard-shortcut
work (#276, #268, and the focus/shortcut fixes that followed) plus the
sidebar/modal touches that came with them — a **clean merge, no conflicts**:
none of those commits touch the buddy files #235 reworks, so our wiring,
the card's own dismissal call sites and the reset-site clears all stay as
they were after e0d1d31.

Contents: 26 files, +1,867/−83, dominated by the new
`features/shortcuts/` registry, its modal and their tests.

Nothing of ours was rewritten for this one — the resolution work of the
previous backmerge (their files + our wiring, the memo-safe row props, both
reset behaviours) already meets this base.

Gate on the merged tree — `npm run try` green on Node 22: format:check,
build (`tsc -b` included), lint; unit 344 files / 3321 tests; a11y 57 files
/ 78 tests. Buddy + a11y + the incoming shortcuts suite alone: 87 files /
307 tests, so #235's own suites still pass against their memoised rows and
the dismissal API.
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.

2 participants