Skip to content

feat(accessibility): add terminal screen reader mode - #245

Open
LarryHu0217 wants to merge 2 commits into
johannesjo:mainfrom
LarryHu0217:codex/terminal-screen-reader-211
Open

feat(accessibility): add terminal screen reader mode#245
LarryHu0217 wants to merge 2 commits into
johannesjo:mainfrom
LarryHu0217:codex/terminal-screen-reader-211

Conversation

@LarryHu0217

Copy link
Copy Markdown
Contributor

Summary

  • add an opt-in Terminal screen reader mode setting, disabled by default
  • persist and strictly validate the setting across app restarts
  • initialize new xterm instances with screenReaderMode and update open terminals when the setting changes

Validation

  • npm run check
  • npm run check:static
  • npm run test:security-rules
  • npm test (1,633 passed, 23 skipped)
  • npm run build:frontend

Implements the terminal screen-reader mode portion of #211.

@johannesjo johannesjo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Important — include the new preference in the autosave snapshot

I double-checked the current head and reproduced this against persistedSnapshot(): once the initial startup autosave has completed, changing only terminalScreenReaderMode leaves the snapshot unchanged, so setupAutosave() does not schedule its 1-second saveState().

Normal window close explicitly awaits saveState(), so clean restarts are covered. However, a crash or hard termination before another tracked/direct save can revert an enable or resurrect a previously enabled value.

saveState() already serializes the field at src/store/persistence.ts:186. Please add terminalScreenReaderMode: store.terminalScreenReaderMode beside terminalFont in src/store/autosave.ts, plus a regression test in src/store/autosave.test.ts asserting that toggling it changes persistedSnapshot().

The xterm constructor/live-update wiring and strict load validation otherwise check out, and CI/full tests pass.

@LarryHu0217

Copy link
Copy Markdown
Contributor Author

Addressed the autosave gap in 9c27792. persistedSnapshot() now tracks terminalScreenReaderMode beside terminalFont, and the regression test verifies that toggling the preference changes the snapshot.

Validation: 96 focused store tests and 1,634 full-suite tests passed; compile, typecheck, ESLint, Prettier, Knip, and dependency-cruiser passed.

@johannesjo

Copy link
Copy Markdown
Owner

Reviewed at 9c27792, rebased on current main (208a933). LGTM — merge as-is; everything below is a nit.

Verified

  • npm run check clean (compile + typecheck + eslint + prettier); full vitest suite: 1630 passed, 25 skipped, 0 failed.
  • Touchpoint parity. The new field hits exactly the same 11 sites as terminalFont — both type decls, core.ts default, saveState, LegacyPersistedState, loadState validation, persistedSnapshot, ui.ts setter, store.ts re-export (needed — SettingsDialog imports from ../store/store), TerminalView construction + effect, settings row. There's only one loadState assignment path and no settings-reset/export site, so nothing is missed.
  • The runtime toggle genuinely works. @xterm/xterm@6.1.0-beta.195 registers optionsService.onSpecificOptionChange('screenReaderMode', …) inside open(), and _handleScreenReaderModeOptionChange creates/disposes the AccessibilityManager, guarded on !this._accessibilityManager.value — so re-setting the same value is a no-op and open terminals update live.
  • No selection/click regression. .xterm-accessibility is an inset:0; z-index:10 overlay, but ships pointer-events: none, and terminal selection here goes through term.getSelection() / getSelectionPosition() rather than window.getSelection(). Bookmarks, copy and the selection-anchored markers are unaffected.
  • Effect ordering is sound. It reads store.terminalScreenReaderMode before the if (!term) return, so the dependency registers on the first run, and onMount (l.397) is registered ahead of the effects (l.1097+) with no early return before new Terminal, so term always exists by then.
  • Persistence shape matches the coordinatorControlHintDismissed precedent (|| undefined on save, strict typeof === 'boolean' on load), and the test volume matches the house pattern for a persisted boolean.

Nits

  1. src/store/autosave.test.ts:33 — the new case sits inside describe('autosave snapshot includes new-task-default fields'), but screen-reader mode isn't a new-task default. Rename the block (e.g. "autosave snapshot tracks persisted settings") or move the case out. The test itself earns its keep: it guards precisely the regression 9c27792 fixed, and persistence.test.ts can't catch it because it calls saveState() directly rather than going through the dirty check.

  2. src/store/ui.test.ts:202 — this asserts a one-line pass-through to setStore with no logic in between; the other blocks in that file all cover setters that actually do something (focus clearing, size clamping), and this one also forces a field onto MockStore just to exist. Dropping it loses no coverage (−13 lines). Optional.

  3. src/components/TerminalView.tsx:1111 — the effect deliberately omits markDirty() unlike its font/theme neighbours, which reads like an oversight at a glance. Worth a one-line why. It is correct as written: AccessibilityManager's constructor ends with _refreshRowsDimensions(); _refreshRows(), so toggling the setting on an idle terminal populates the current viewport immediately with no repaint needed.

  4. src/remote/AgentDetail.tsx:208 — the phone/remote client builds its own Terminal with no access to the desktop store, so that surface stays silent to assistive tech. Separate app, fair to defer, but worth noting Reduced-motion guards, opt-in terminal screen-reader mode, tooltip upgrade #211's problem statement isn't fully closed by this PR.

Behavioural caveat (not a code issue)

The accessibility tree mirrors only the viewport of the active buffer, and the live region cuts a batch off at 21 lines with a "too much output" message. Claude Code runs on the alternate buffer and redraws continuously, so announcements will be chatty during streaming. Arrow-key navigation still reaches scrollback via xterm's boundary-focus scrolling. That's inherent to the xterm approach the issue asked for — just calibrating expectations, nothing to change here.

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