Skip to content

fix(accessibility): show focus indicators app-wide#229

Open
LarryHu0217 wants to merge 2 commits into
johannesjo:mainfrom
LarryHu0217:codex/global-focus-visible-210
Open

fix(accessibility): show focus indicators app-wide#229
LarryHu0217 wants to merge 2 commits into
johannesjo:mainfrom
LarryHu0217:codex/global-focus-visible-210

Conversation

@LarryHu0217

Copy link
Copy Markdown
Contributor

Summary

  • promote the existing interactive-control :focus-visible rule from dialogs to the full app
  • stop suppressing the focus outline on the new-task placeholder
  • add regression coverage for the global selector and placeholder behavior

Validation

  • npx vitest run src/lib/focus-visible-styles.test.ts
  • npm test (1573 passed, 23 skipped)
  • npm run check
  • npm run lint:dead
  • npm run lint:arch

Addresses the global focus-indicator item in #210.

@johannesjo

Copy link
Copy Markdown
Owner

Thanks for picking this up — item 3 of #210 is a real gap and the selector move itself is faithful (no drift, and the comment was updated rather than left stale). Two things need addressing before this can merge, both verified in Chrome rather than just reasoned about.

1. :is() gives the rule specificity (0,3,0) and silently defeats every existing component focus style

:is() takes the specificity of its most specific argument. Here that's [tabindex]:not([tabindex='-1']) = (0,2,0), plus :focus-visible(0,3,0). Every component focus rule in the codebase is a single class + pseudo-class = (0,2,0), so they all lose:

Rule Result
styles.css:1135-1145.prompt-textarea:focus, .input-field:focus, .btn-primary/.btn-secondary/.agent-btn/.project-select:focus-visible, deliberate outline: none + box-shadow ring outline: none defeated → double ring
styles.css:949 .projects-toggle:focus-visible (--border-focus, offset 1px) overridden → accent, offset 2px
styles.css:1890 .focus-mode-task-indicator:focus-visible (accent/white mix) overridden
arena-history.css:100, arena-shared.css:125 (offset 1px) overridden

I confirmed these are all top-level rules (brace depth 0 at line 1145), not theme-scoped, so nothing shields them.

Reproduced in Chrome with the exact selector from this diff:

.agent-btn        outline: solid 2px red  + box-shadow ring   ← "outline: none" lost
.projects-toggle  outline: solid 2px red                      ← its own blue outline lost
.prompt-textarea  outline: solid 2px red  + box-shadow ring   ← "outline: none" lost

The worst case is .prompt-textarea. Per spec, text fields match :focus-visible on mouse focus too — I verified this: under pointer modality <button> and [role=button] both report focusVisible: false, but <textarea> reports true and paints the outline. So the app's most-used control gains an unintended 2px accent ring on every single click, stacked on its existing box-shadow ring. PromptInput is a main-panel component, so this is new behavior, not an extension of the old dialog-scoped rule. Same for .review-plan-btn (btn-secondary) in TaskNotesBody. (.agent-btn is only used inside dialogs, so that one is pre-existing, not a regression.)

Suggested fix: change :is( to :where(. :where() contributes zero specificity, so the rule lands at (0,1,0) — bare controls still get the indicator, and every component rule above keeps winning. That's the idiomatic way to express "app-wide default that components may override." Verified in the same harness:

.agent-btn        outline: none, ring only          ✓ component rule wins
.projects-toggle  outline: solid 2px blue, offset 1px ✓ component rule wins
.prompt-textarea  outline: none, ring only          ✓ component rule wins
plain <button>    outline: solid 2px red            ✓ still gets the indicator
[role=button]     outline: solid 2px red            ✓ still gets the indicator

One-word change, resolves the whole table.

2. Deleting .new-task-placeholder:focus-visible { outline: none } reintroduces a double indicator on the arrow-key path

That rule wasn't incidental. git log -S traces it to bd29816 ("give DOM focus to placeholder buttons during keyboard navigation"), which added it in the same commit as registerFocusFn('placeholder:add-task', () => addTaskRef?.focus()).

focusPlaceholder() (store/focus.ts:190-196) sets store state and moves DOM focus. The store state drives focusedBorder/focusedColor/focusedBg (NewTaskPlaceholder.tsx:23-30) → accent dashed border, accent text, tinted background. Arrow-key spatial nav is keyboard-driven, so :focus-visible matches and the placeholder now shows the dashed accent border and a solid 2px accent outline on top of it.

The issue's premise ("tabbing to it shows nothing") is correct, but describes only the Tab path. There are two focus paths here; this fixes one and regresses the other.

Since bd29816 made DOM focus authoritative for this control, the simplest fix is to drop the store-driven focusedBorder/focusedColor/focusedBg styling entirely and let :focus-visible be the single indicator for both paths. Worth noting the new test asserts the outline: none is gone, so it currently locks this in.

Minor

  • src/lib/focus-visible-styles.test.ts — greps CSS text rather than exercising behavior. There's precedent (custom-theme-contrast.test.ts), so the pattern is fine here, but note it can't catch either issue above: it passes unchanged whether the selector is :is or :where, and whether or not the rule is overridden everywhere it applies. It also asserts only 3 of the 17 selectors despite the name claiming app-wide coverage. Might also fit better outside src/lib/, since its subject is src/styles.css.
  • arena-config.css:60,142, arena-results.css:331 — inputs with base outline: none and :focus { border-color } designs now also pick up the accent outline. Probably desirable for a11y, but worth eyeballing.

@LarryHu0217

Copy link
Copy Markdown
Contributor Author

Addressed both blockers in c1252c5:

  • Changed the app-wide selector from :is() to :where() so component focus rules retain precedence.
  • Removed the store-driven placeholder border/color/background state so DOM :focus-visible is the single indicator for both Tab and arrow-key focus paths.
  • Strengthened the regression test to cover all 17 selectors, enforce :where(), and reject competing placeholder focus styling; also moved it out of src/lib.

Validation: npm run check, npm run check:static, and npm test (94 files passed, 2 skipped; 1,573 tests passed, 23 skipped).

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