Skip to content

fix(frontend): add focus management to wallet modal - #1349

Merged
K1NGD4VID merged 1 commit into
LabsCrypt:mainfrom
zainabwahab-eth:fix/issue-1257-wallet-modal-focus
Aug 30, 2026
Merged

fix(frontend): add focus management to wallet modal#1349
K1NGD4VID merged 1 commit into
LabsCrypt:mainfrom
zainabwahab-eth:fix/issue-1257-wallet-modal-focus

Conversation

@zainabwahab-eth

Copy link
Copy Markdown
Contributor

Closes #1257

Summary

frontend/src/components/wallet/WalletModal.tsx implemented its own Escape handler and body-scroll lock but had no focus trap and no focus restoration. Tab walked straight out of the open dialog into the page behind it, and closing the modal dropped focus on document.body.

This PR migrates the modal to the repository's existing useModalDialog hook, the same one already used by TopUpModal, CancelConfirmModal, StreamDetailsModal, StreamCreationWizard and the navbar's MobileMenu. No new focus-trap mechanism was written.

Accessibility changes

  • useModalDialog({ onClose, isCloseDisabled: isConnecting }) replaces the bespoke useCallback + document.addEventListener("keydown", ...) Escape handler and the standalone body-scroll-lock effect.
  • dialogRef is attached to the inner .wallet-modal panel, matching how TopUpModal and CancelConfirmModal place it (role="dialog" stays on the backdrop).
  • Focus now moves into the dialog on open, Tab wraps from the last focusable back to the first, Shift+Tab wraps from the first back to the last, and closing restores focus to the element that opened the modal.
  • isCloseDisabled is wired to isConnecting, preserving the existing rule that the modal cannot be dismissed mid-connection. That already matched the disabled close button and the guarded backdrop click.

Wallet selection, connect/clearError calls, the Freighter detection poll, the install-link fallback, error banner, props, state and styling are untouched.

Files changed

  • frontend/src/components/wallet/WalletModal.tsx (+12 / -25)
  • frontend/src/components/wallet/WalletModal.test.tsx (new)

Tests run and actual results

New WalletModal.test.tsx covers the acceptance criteria directly, using @testing-library/user-event against a harness with a real trigger button:

  1. focus moves into the dialog on open
  2. Tab through a full cycle plus one, asserting focus never lands on the trigger or document.body, and wraps last -> first
  3. the same for Shift+Tab, wrapping first -> last
  4. Escape closes and focus returns to the trigger (after first asserting focus had actually left it)
  5. body scroll locks on open and releases on close
  6. Escape is ignored while status === "connecting"

All commands run from frontend/:

npx vitest run src/components/wallet/WalletModal.test.tsx src/hooks/useModalDialog.test.tsx src/__tests__/wallet-entry.test.tsx
    -> 3 files passed, 15 tests passed

npx vitest run
    -> 27 files passed, 239 tests passed

npm run lint
    -> 0 errors, 1 pre-existing warning in src/components/dashboard/ActivityHistory.tsx (unrelated)

npm run build
    -> Compiled successfully, TypeScript finished, 13/13 static pages generated

Verification that the tests catch the actual bug: with WalletModal.tsx stashed back to its pre-migration state, tests 1, 2 and 3 fail (3 failed | 3 passed). They pass with the migration applied.

Acceptance criteria

  • WalletModal uses useModalDialog
  • Bespoke Escape handling removed (the hook provides it, gated by isCloseDisabled)
  • Bespoke body-scroll locking removed (the hook provides it)
  • Tab cannot move focus outside the modal
  • Shift+Tab cannot move focus outside the modal
  • Closing the modal restores focus to the triggering element
  • Keyboard-navigation test added and passing
  • Existing useModalDialog and wallet-entry tests still pass; full frontend suite green (239 tests)
  • Lint and next build pass

Pre-existing issues found, not fixed here

Out of scope for this PR, recorded rather than touched:

  • npx tsc --noEmit reports 27 errors, all in existing test files (src/lib/api/streams.test.ts, src/hooks/useIncomingStreams.test.tsx, src/app/streams/create/__tests__/create-stream-content.test.tsx). They predate this branch; next build does not type-check those files, so CI is unaffected.
  • src/components/dashboard/ActivityHistory.tsx:26 emits a react-hooks/incompatible-library lint warning.

🤖 Generated with Claude Code

WalletModal rolled its own Escape handler and body-scroll lock but had
no focus trap and no focus restoration, so Tab walked straight out of
the open dialog into the page behind it and closing the modal dropped
focus on document.body. Keyboard and screen-reader users lost their
place.

Migrate it to the shared useModalDialog hook already used by TopUpModal,
CancelConfirmModal, StreamDetailsModal, StreamCreationWizard and the
navbar mobile menu: dialogRef goes on the .wallet-modal panel, and
isCloseDisabled is wired to isConnecting so Escape stays inert while a
connection is in flight, matching the disabled close button and the
guarded backdrop click. The bespoke Escape listener and scroll-lock
effect are removed as the hook covers both.

Wallet selection, connection, error handling, props and styling are
untouched.

Adds WalletModal.test.tsx covering focus entry, Tab and Shift+Tab
wrapping at both boundaries, focus restoration to the trigger on close,
scroll lock and release, and Escape being ignored while connecting.
The three focus tests fail against the pre-migration component.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@K1NGD4VID
K1NGD4VID merged commit da4ad7d into LabsCrypt:main Aug 30, 2026
10 checks passed
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.

[Audit] Wallet connection modal has no focus trap or focus restoration

2 participants