Settings modal lacks focus trap — has `role="dialog"` and `aria-modal="true"` but no focus management, so Tab can move focus to the underlying page #168

New issue

Closed

opened 2026-10-03 17:30:29 +00:00 by Mark · 1 comment

Mark commented

2026-10-03 17:30:29 +00:00

Owner

Finding

src/components/workspace/SettingsModal.tsx:64-65 has the right ARIA attributes:

role="dialog"
aria-modal="true"

But there is no focus trap. A user opens the Settings modal, presses Tab repeatedly, and focus moves to elements behind the modal — the Navbar, the Workspace's tool buttons, etc. The visual overlay is shown; the keyboard focus is not contained.

A screen reader user opens the modal and the screen reader announces the modal; they Tab to a button that they can't see (it's behind the backdrop). A keyboard user with a modal open can't Tab to the close button because Tab is moving focus through hidden elements.

Where

src/components/workspace/SettingsModal.tsx:64-65 (the role and aria-modal). No focus management code anywhere.

Other modals in the codebase:

ExportDialog (src/components/workspace/ExportDialog.tsx): has role="dialog" aria-modal="true" (line 234) — same gap
NewFolderDialog: per #134, has duplicate-name detection; verify if it has focus trap
ThreadLibraryView (src/components/workspace/ThreadLibraryView.tsx): has role="dialog" aria-modal="true" (line 39) — same gap

Suggested fix shape

Use the standard focus-trap pattern (or a library like focus-trap-react):

On modal open: focus the first focusable element (or the modal container itself if the modal is the unit of focus).
On Tab from the last focusable element: wrap to the first.
On Shift+Tab from the first focusable element: wrap to the last.
On Escape: close the modal (the current implementation already has an X button; an Escape handler would be a small addition).
On modal close: return focus to the element that opened it.

A minimal implementation is ~30 lines per modal. A useFocusTrap custom hook (one file, ~50 lines) that the three modals all import keeps the implementation DRY.

Severity

High. Accessibility is broken for keyboard and screen reader users. The ARIA attributes claim a modal but the implementation doesn't back it up.

Discovered by

Gap analysis (2026-10-02) of modal accessibility.

All three modals in the codebase (SettingsModal, ExportDialog, ThreadLibraryView) have the same gap. One issue covers them.

## Finding `src/components/workspace/SettingsModal.tsx:64-65` has the right ARIA attributes: ```tsx role="dialog" aria-modal="true" ``` But there is no focus trap. A user opens the Settings modal, presses Tab repeatedly, and focus moves to elements **behind** the modal — the Navbar, the Workspace's tool buttons, etc. The visual overlay is shown; the keyboard focus is not contained. A screen reader user opens the modal and the screen reader announces the modal; they Tab to a button that they can't see (it's behind the backdrop). A keyboard user with a modal open can't Tab to the close button because Tab is moving focus through hidden elements. ## Where `src/components/workspace/SettingsModal.tsx:64-65` (the role and aria-modal). No focus management code anywhere. Other modals in the codebase: - `ExportDialog` (`src/components/workspace/ExportDialog.tsx`): has `role="dialog" aria-modal="true"` (line 234) — same gap - `NewFolderDialog`: per #134, has duplicate-name detection; verify if it has focus trap - `ThreadLibraryView` (`src/components/workspace/ThreadLibraryView.tsx`): has `role="dialog" aria-modal="true"` (line 39) — same gap ## Suggested fix shape Use the standard focus-trap pattern (or a library like `focus-trap-react`): 1. **On modal open**: focus the first focusable element (or the modal container itself if the modal is the unit of focus). 2. **On Tab from the last focusable element**: wrap to the first. 3. **On Shift+Tab from the first focusable element**: wrap to the last. 4. **On Escape**: close the modal (the current implementation already has an X button; an Escape handler would be a small addition). 5. **On modal close**: return focus to the element that opened it. A minimal implementation is ~30 lines per modal. A `useFocusTrap` custom hook (one file, ~50 lines) that the three modals all import keeps the implementation DRY. ## Severity High. Accessibility is broken for keyboard and screen reader users. The ARIA attributes claim a modal but the implementation doesn't back it up. ## Discovered by Gap analysis (2026-10-02) of modal accessibility. ## Related - All three modals in the codebase (SettingsModal, ExportDialog, ThreadLibraryView) have the same gap. One issue covers them.

Mark added the

feature-request

triage-approved

labels

2026-10-03 17:30:29 +00:00

Mark referenced this issue from a commit

2026-10-03 17:54:38 +00:00

fix(#168): trap focus and Escape inside Settings, Export, Thread Library modals

Mark commented

2026-10-03 17:54:56 +00:00

Author

Owner

Fixed in 7a95d7d (fix(#168): trap focus and Escape inside Settings, Export, Thread Library modals).

Added src/hooks/useFocusTrap.ts (~50 lines, no new dep) and wired it into all three modals: SettingsModal, ExportDialog, ThreadLibraryView. NewFolderDialog was already on Radix Dialog so it gets focus management free.

Tests: 11 hook unit tests + 2 integration tests per modal (Tab-wrap, Escape closes). 706/706 vitest passing, tsc clean, eslint clean.

Fixed in 7a95d7d (fix(#168): trap focus and Escape inside Settings, Export, Thread Library modals). Added `src/hooks/useFocusTrap.ts` (~50 lines, no new dep) and wired it into all three modals: SettingsModal, ExportDialog, ThreadLibraryView. NewFolderDialog was already on Radix Dialog so it gets focus management free. Tests: 11 hook unit tests + 2 integration tests per modal (Tab-wrap, Escape closes). 706/706 vitest passing, tsc clean, eslint clean.

Mark closed this issue

2026-10-03 17:54:58 +00:00

Mark referenced this issue from a commit

2026-10-04 07:15:07 +00:00

feat(#233): in-editor '+ Custom color' entry point for the Thread Color picker