Skip to content

fix(accessibility): honor reduced motion#230

Open
LarryHu0217 wants to merge 2 commits into
johannesjo:mainfrom
LarryHu0217:codex/reduced-motion-211
Open

fix(accessibility): honor reduced motion#230
LarryHu0217 wants to merge 2 commits into
johannesjo:mainfrom
LarryHu0217:codex/reduced-motion-211

Conversation

@LarryHu0217

@LarryHu0217 LarryHu0217 commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • disable task enter/exit animations when the OS requests reduced motion
  • suppress status, Ask Code, and keybinding pulse animations in reduced-motion mode
  • preserve the status dot's busy sub-state with a static outer ring after its pulse is removed
  • move the main reduced-motion block to the end of the stylesheet and reserve !important for inline animations
  • avoid stale task-entry classes when reduced motion is already enabled
  • strengthen regression coverage so selector groups are paired with their declarations

Functional loading spinners remain animated because rotation is the loading signal in icon-only states; status dots now have a non-motion fallback.

Validation

  • npx vitest run src/lib/reduced-motion-styles.test.ts
  • npm test (1,573 passed, 23 skipped)
  • npm run check
  • npm run check:static

Addresses the reduced-motion item in #211.

@johannesjo

Copy link
Copy Markdown
Owner

Reviewed locally on codex/reduced-motion-211. The change is sound and I'd merge it — but there's one accessibility trade-off worth a decision before it lands, plus a convention suggestion that would let most of the !important go away.

The thing that could have broken, and doesn't

.task-removing / .task-item-removing are exit animations, and killing an exit animation is the classic way to strand nodes in the DOM — if teardown hangs off animationend, animation: none means it never fires. It doesn't here. Removal is timer-driven:

  • src/store/tasks.ts:597-606setTimeout(..., REMOVE_ANIMATION_MS)
  • src/store/terminals.ts:91 — same pattern

REMOVE_ANIMATION_MS is 300 while the animations are 0.24s, so the timer already outlived the animation and was never coupled to it. A repo-wide search for animationend|transitionend|getAnimations|.animate( turns up exactly one handler — TilingLayout.tsx:266-269 — and it only strips a cosmetic class. Nothing leaks. Under reduced motion the panel simply sits for 300ms and then vanishes without fading.

One real trade-off: the status dot loses a distinction

I initially assumed the pulse was redundant encoding (color + ring + tooltip all carry the state anyway). That's not right, and it's the one thing here I'd want a deliberate call on.

getDotColor and getDotShadow (StatusDot.tsx:6-28) key off attention and return early; getDotTooltip (:30-41) likewise early-returns on attention. But isPulsing() (:49) is attention === 'active' || status === 'busy'. So once attention is anything other than active, the busy vs non-busy status changes nothing except the pulse.

Two collisions that are reachable and fully identical once the pulse is gone — same color, same ring, same tooltip:

  • attention='needs_input' + busy vs waiting. getTaskAttentionState:844-845 returns needs_input on hasQuestion regardless of activity, while getTaskDotStatus:882-885 returns busy on hasActive. An agent that's asked a question but is still inside the idle window satisfies both.
  • attention='error' + busy vs waiting. hasTaskAgentError and hasRunningTaskActivity read different agents, so one errored agent alongside one running agent hits both.

(There's a third, attention='review' + busy vs review — the two functions check needsReview and hasActive in opposite order. Color and ring are identical there, but the tooltip does differ, so it's the mild case.)

The important state survives: attention='active' maps to theme.accent, which nothing else uses, so "is this agent working" is still color-encoded at the top level. What's lost is the "…and it's also still churning" sub-state inside needs_input / error. And title is hover-only, so it's not a real substitute for a visual encoding.

I don't think this blocks the PR — shedding motion-encoded nuance is arguably what the user asked for by setting the preference. But it's worth being explicit about, and it interacts with the spinner decision below.

Consistency question: spinners vs. this dot

The PR notes functional spinners stay unchanged, which is defensible on its own — .inline-spinner (styles.css:1459-1468, spinnerSpin ... infinite) is a case where motion is the signal and removing it leaves nothing.

But that's the same argument that applies to .status-dot-pulse, which this PR does disable, and per the section above the pulse turns out to be load-bearing in two states. So the two calls pull in opposite directions: the highest-motion animation in the app (an unbounded rotation) is exempt as "functional", while a 1.9s opacity fade that carries unique state is not. Either resolution is fine — just worth one line of rationale in the PR so the next person doesn't have to re-derive it.

!important is load-bearing — but only because of where the block sits

It's genuinely required as written, and worth flagging so nobody deletes it on sight:

  • .askcode-loading-pulse / .keybinding-recording-pulse get their animation from an inline style attribute (AskCodeCard.tsx:155,168, HelpDialog.tsx:417). Only author-!important beats the style attribute.
  • The other five base rules live at lines 1274, 1291, 1306, 1325, 1440 — all after the media block at 985, at equal (0,1,0) specificity. @media adds no specificity, so source order would otherwise win and the override would silently no-op.

Suggestion: this file already has a convention that avoids the problem. The repo's four other reduced-motion blocks — arena-battle.css:121, arena-shared.css:232, arena-countdown.css:109, arena-results.css:346 — all sit at end-of-file and use plain animation: none with no !important, relying on source order. This block is at 985 of 2211. Moving it to the end of styles.css would match that convention and let the five class-based rules drop !important (the two inline-style ones still need it).

Nits

1. The new test is weaker than it looks. reduced-motion-styles.test.ts:36-39 asserts the seven selector substrings and animation: none !important independently over the block text — not that they're in the same rule. I ran the test's exact logic against a deliberately sabotaged stylesheet where the seven selectors got only transition: none and the animation: none !important sat on an unrelated selector: it still passes.

Two smaller things: css.indexOf(marker) pins to the first block, so one inserted above line 985 would silently redirect every assertion; and it only reads styles.css, so the four src/arena/*.css blocks aren't covered despite the PR describing this as the shared reduced-motion contract. A single regex pairing the selector list with the declaration would close most of the gap cheaply.

2. .task-appearing now sticks permanently under reduced motion. With animation: none no animationend fires, so TilingLayout.tsx:266-269 never strips the class and it stays on every panel. Harmless while the preference is on — but Chromium re-evaluates prefers-reduced-motion live, so toggling the OS setting off mid-session makes every mounted panel replay taskAppear at once. Gating class application on a matchMedia signal would avoid it. Edge case; mentioning it because it's cheap to sidestep.

3. keybinding-recording-pulse is applied unconditionally. HelpDialog.tsx:367 puts it on every <kbd>, but the animation is gated on recording() (:417, keyframes at :509). No bug — animation: none on a non-animating element is a no-op — but the name asserts a state that's usually false. keybinding-key would read truer.

Aside — pre-existing, not this PR

slideUp (styles.css:1471, translateY(12px) scale(0.97)) is the one remaining translate-based animation not covered here, which looks like a gap but isn't: it's attached only to .dialog-overlay > form and .dialog-overlay > .dialog-content (:1384-1386), while Dialog.tsx:87-127 renders .dialog-overlay > .dialog-panel > {props.children}. No <form> is ever a direct child of the overlay (NewTaskDialog.tsx:1034 is a grandchild), and .dialog-content appears nowhere in the repo outside that selector. Dead CSS — nothing to suppress.

Worth knowing because if it's ever revived by repointing at .dialog-panel, it's exactly the motion this PR exists to kill and will need adding to the list.

Checks run locally

npm run check (compile + typecheck + lint + format) clean; the new test passes. All seven selectors verified to resolve to real render sites.

@LarryHu0217

Copy link
Copy Markdown
Contributor Author

Addressed the review points in 18a214d:

  • Moved the reduced-motion block to the end of styles.css; only the two inline-animation overrides retain !important.
  • Kept the required status pulse suppression, but added a static outer ring so the busy sub-state remains distinct without motion. Functional spinners remain animated where rotation is the loading signal.
  • Gated task-entry class application on the current media preference and clear it on animationcancel.
  • Renamed the unconditional keybinding class to keybinding-key.
  • Strengthened the test to pair exact selector groups with their declarations, require the block at EOF, and cover the stale-class safeguards.

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