fix(a11y): reconcile message render (A1/A2/A11) - #45
Conversation
Stop wiping the message list on every render so unchanged rows keep their DOM nodes and keyboard focus. Route stream lifecycle announcements through one role=status element instead of rebuilding live regions with the transcript. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe chat adds English and Spanish status translations and a persistent hidden Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/dom/render.test.js (1)
268-290: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the stopped announcement and its suppression.
The A11 test covers the responding and complete transitions. It does not cover
stopGeneration, which announces "Response stopped" and setsstopAnnouncementPendingso the following streaming-to-idle transition stays silent. That suppression flag is the most intricate part of the new status logic, and a regression there produces a duplicate announcement that no test detects.Add a case that starts streaming, invokes the stop control, and then asserts that
#chatStatusholds "Response stopped" after the runtime settles to idle.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/dom/render.test.js` around lines 268 - 290, Add coverage to the A11 status-region test for stopGeneration: begin a streaming response, invoke the existing stop control, settle the runtime through idle, and assert that `#chatStatus` contains “Response stopped,” verifying the subsequent idle transition does not replace it with another announcement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/app.css`:
- Around line 623-634: Update the .visually-hidden rule by replacing the
deprecated clip declaration with clip-path: inset(50%), preserving the existing
visually hidden and assistive-technology behavior.
In `@public/app.js`:
- Around line 1299-1305: Update the user-message rendering around the row
construction to create the message bubble element separately and assign the
user-provided text via textContent, rather than interpolating text into
row.innerHTML. Preserve the existing conditional omission of the bubble when
text is empty and keep filesHtml rendering unchanged.
- Around line 1023-1030: Defer the second `#chatStatus` text assignment in
announceChatStatus with requestAnimationFrame after clearing the region,
preserving the existing forced layout read and message value. In
tests/dom/render.test.js lines 281 and 289, no direct assertion changes are
required because settle() already drains the timer-backed frame callback.
---
Nitpick comments:
In `@tests/dom/render.test.js`:
- Around line 268-290: Add coverage to the A11 status-region test for
stopGeneration: begin a streaming response, invoke the existing stop control,
settle the runtime through idle, and assert that `#chatStatus` contains “Response
stopped,” verifying the subsequent idle transition does not replace it with
another announcement.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ec777b3-344c-4432-812e-4be681b6c2fb
📒 Files selected for processing (6)
i18n/en.jsoni18n/es.jsonpublic/app.csspublic/app.jspublic/index.htmltests/dom/render.test.js
| function announceChatStatus(message) { | ||
| const el = document.getElementById('chatStatus'); | ||
| if (!el) return; | ||
| // Clear-then-set so repeated identical strings still announce. | ||
| el.textContent = ''; | ||
| void el.offsetWidth; | ||
| el.textContent = message; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the test settle() helper already flushes animation frames.
rg -n -C 8 -e 'function settle' -e 'const settle' tests/Repository: CodeSignal/learn_cosmo-chat
Length of output: 1219
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- announceChatStatus and call sites ---'
rg -n -C 12 'announceChatStatus|requestAnimationFrame|offsetWidth' public/app.js
printf '%s\n' '--- test harness and assertions ---'
sed -n '110,145p' tests/dom/harness.js
sed -n '245,300p' tests/dom/render.test.js
printf '%s\n' '--- rAF setup and test environment ---'
rg -n -C 8 'requestAnimationFrame|cancelAnimationFrame|pretendToBeVisual|jsdom|settle\(' tests package.jsonRepository: CodeSignal/learn_cosmo-chat
Length of output: 22398
🌐 Web query:
ARIA live region clear and reset same task repeated identical text requestAnimationFrame screen reader announcement forced layout accessibility tree
💡 Result:
When a web application updates an ARIA live region with text identical to its current content, assistive technologies (AT) often suppress the announcement because they detect no change in the accessibility tree [1][2][3]. To ensure a screen reader announces the same message repeatedly, you must force a DOM mutation [3][4]. The industry-standard pattern to achieve this is the clear-then-set approach [3][4][5]: 1. Clear the content of the live region (e.g., set innerText or textContent to an empty string) [6][3][7]. 2. Use requestAnimationFrame to defer setting the new (identical) text until the next browser paint [3][4]. This sequence forces the browser to register two distinct updates in the accessibility tree—first to an empty state, then to the target text—which triggers the screen reader to treat the second update as a new, reportable event [3][4]. Without the requestAnimationFrame delay, the browser may batch the two operations into a single update, resulting in no perceivable change for the AT [6][3]. Additionally, for reliable cross-browser behavior, developers often implement a short delay (typically 100ms to 150ms) using setTimeout after the update [8][3][9]. This prevents the AT from clipping or dropping the announcement if the region is updated too rapidly, allowing the screen reader sufficient time to begin speaking the previous message [8][3][9]. While aria-atomic="true" is recommended to ensure the entire content of the region is announced, it does not bypass the requirement for a visible DOM mutation to trigger the announcement [6][3][5].
Citations:
- 1: Clarify what browsers should do with text replaced with same text in a live region w3c/aria#671
- 2: https://lists.w3.org/Archives/Public/public-aria/2017Nov/0023.html
- 3: https://modern-framework-accessibility.com/react-nextjs-accessibility-patterns/react-hooks-for-accessibility/building-a-useannouncer-hook-for-live-regions/
- 4: fix(accessibility): ensure screen reader announcements update consistently clauderic/dnd-kit#1973
- 5: https://davis.libretexts.org/docs/components/live-announcer
- 6: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Guides/Live_regions
- 7: https://github.com/SableClient/Sable/blob/dev/src/app/utils/announce.ts
- 8: https://www.jamesjacobs.me/blog/deep-dive-understanding-live-regions-part-3/
- 9: bug(LiveAnnouncer): LiveAnnouncer does not announce subsequent calls to the same text angular/components#21507
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- harness setup before boot ---'
sed -n '1,125p' tests/dom/harness.js
printf '%s\n' '--- test configuration and scripts ---'
cat package.json
printf '%s\n' '--- all status-region references ---'
rg -n -C 6 'chatStatus|role="status"|aria-live' public testsRepository: CodeSignal/learn_cosmo-chat
Length of output: 10483
Defer the live-region text update with requestAnimationFrame. Clearing and resetting #chatStatus in one task can be batched by assistive technologies, so repeated messages may not be announced. The existing settle() helper already drains the timer-backed frame callback; no assertion changes are needed at tests/dom/render.test.js#L281 or tests/dom/render.test.js#L289.
📍 Affects 2 files
public/app.js#L1023-L1030(this comment)tests/dom/render.test.js#L281-L281tests/dom/render.test.js#L289-L289
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@public/app.js` around lines 1023 - 1030, Defer the second `#chatStatus` text
assignment in announceChatStatus with requestAnimationFrame after clearing the
region, preserving the existing forced layout read and message value. In
tests/dom/render.test.js lines 281 and 289, no direct assertion changes are
required because settle() already drains the timer-backed frame callback.
VoiceOver could not discover replies via the Headings or Articles rotor because messages were unlabelled divs inside a scrollable main. Mark each turn as an article with a visually hidden h2 so AT can jump to Cosmo's reply. Co-authored-by: Cursor <cursoragent@cursor.com>
Token ticks were replaceWith-ing Cosmo's article on every update, which kicked VoiceOver out of the text and left duplicate headings in the rotor. Keep the article/heading stable and update body/status/thoughts in place. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
public/app.js (1)
1145-1147: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse one assistant model per row per render.
messageRowSignaturesbuilds the full assistant model to compute the signatures, andrenderMessagesthen callsbuildAssistantModelagain for the patch or create path. Each build runssegmentAssistantParts,marked.parse,stripEmojisFromHtml, andrenderFilePart.renderMessagesruns on every streaming tick, so the transcript is markdown-parsed twice per message per tick, including for rows that are then skipped as unchanged.Return the model from the signature step and pass it to the patch/create path.
♻️ Sketch of the change
- const { structureSig, contentSig } = messageRowSignatures(msg, ctx); + const { structureSig, contentSig, model } = messageRowSignatures(msg, ctx); planned.push({ key: String(i), structureSig, contentSig, + model, msg,Then use
planned[i].modelin thepatchAssistantRowContentcall instead of rebuilding it.Also applies to: 1157-1167
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/app.js` around lines 1145 - 1147, Update messageRowSignatures to return the assistant model it builds alongside each row’s signatures, then have renderMessages pass planned[i].model into patchAssistantRowContent or the create path. Reuse that model for assistant rows during each render and remove the second buildAssistantModel call while preserving existing behavior for non-assistant rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@public/app.js`:
- Around line 1145-1147: Update messageRowSignatures to return the assistant
model it builds alongside each row’s signatures, then have renderMessages pass
planned[i].model into patchAssistantRowContent or the create path. Reuse that
model for assistant rows during each render and remove the second
buildAssistantModel call while preserving existing behavior for non-assistant
rows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7e87aaf-7a82-4830-93e5-63eb1d909754
📒 Files selected for processing (2)
public/app.jstests/dom/render.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/dom/render.test.js
Content ## headings were siblings of "Cosmo's reply" in the VoiceOver rotor. Demote markdown headings by two levels (#→h3, ##→h4) and keep the original visual weight via message__heading--N. Co-authored-by: Cursor <cursoragent@cursor.com>
A +2 shift made the common ## case into h4 and skipped h3. Shift by +1 with a floor of h3 so section titles nest directly under Cosmo's reply. Co-authored-by: Cursor <cursoragent@cursor.com>
Defer status announcements one frame, set user bubbles via textContent, replace deprecated clip with clip-path, and cover the stop announcement. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
messagesEl.innerHTMLon every render, so unchanged rows keep their DOM nodes and keyboard focus survives status ticks (A1, A2).role="status"region (#chatStatus) outside the message subtree for short lifecycle announcements only: "Cosmo is responding", "Response complete", "Response stopped" (A11). Streaming transcript text is never routed through a live region.<article>+ visually hidden turn headings, in-place streaming patches (no per-token article recreate), and markdown headings nested under the turn h2 (##→ h3).it.failscharacterization tests to plainit(acceptance), plus coverage for status announcements, article headings, streaming node stability, and heading demotion.Closes #41
Closes #42
Closes #43
Changes
renderMessageskeys rows by message index and splits structure vs content signatures. Unchanged chrome is left in place; streaming token ticks patch body/status/thoughts inside the existing Cosmo article so VoiceOver can keep reading. Session switches clear the list. The visual.message__ai-statusstage label remains for sighted users but no longer carriesaria-live.Each turn is an
<article>labelled by a visually hidden h2 ("Your message" / "Cosmo's reply") for the Headings/Articles rotor. Markdown headings inside the reply are shifted to start at h3 so they nest under the turn chrome instead of competing with it.Status announcements fire on streaming start/complete in the chat subscription, with stop generation suppressing the generic "complete" message in favor of "Response stopped".
A3 (composer
disabledmid-stream) is intentionally not in this PR — it depends on this landing and is tracked separately in #44.Test plan
npm test— 179 passednpm run build+A11Y_CI=1axe gate against local server — baseline passed (no shrink; axe cannot detect A1).message__ai-statushas noaria-liveduring a streamReference
a11y-audits/8-5-26/audit.md → A1, A2, A11
a11y-audits/8-5-26/resolution-plan.md → Wave 1 foundation PR