Skip to content

fix(ui): expose one reading-ordered value from AnimatedNumber - #54977

Closed
argszero wants to merge 1 commit into
anomalyco:devfrom
argszero:animated-number-a11y
Closed

fix(ui): expose one reading-ordered value from AnimatedNumber#54977
argszero wants to merge 1 commit into
anomalyco:devfrom
argszero:animated-number-a11y

Conversation

@argszero

@argszero argszero commented Oct 12, 2026 •
edited by opencode-agent Bot
Loading

Copy link
Copy Markdown
Contributor

Issue for this PR

Closes #54653

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

AnimatedNumber was unreadable to assistive technology in every state, for two independent reasons.

The component renders each digit in reverse DOM order and puts the visual order back with row-reverse. The
static digits therefore sat in the accessibility tree in reverse, so 12 was announced as 2 1. On top of
that, every digit carries a 30-cell odometer strip that is only meant to be visible while it animates; the strip
stayed in the tree too, so a 5 to 6 update exposed 0123456789 three times over.

The existing aria-label on the root did not replace those children — an aria-label on a generic element does
not hide its descendants — so it never silenced either of them.

The fix exposes the value once as .sr-only text and marks the visual layer — the reversed digits and the
strips — aria-hidden="true". Rendering, the row-reverse ordering and the animation itself are untouched, so
the sighted output is byte-for-byte the same.

How did you verify your code works?

packages/session-ui/component-tests/animated-number.spec.ts mounts the story in this PR at a pinned state and
asserts the accessibility snapshot is a single reading-ordered value.

  • Red first: with only the component change reverted, the snapshot is the reversed, tripled noise the issue
    reports. The assertion is a one-shot await loc.ariaSnapshot() compared to a string — deliberately not
    toMatchAriaSnapshot(), which retries until it passes and would have gone green on the broken tree as soon as
    the animation ended. The test waits on a deterministic precondition (data-animating="true") before reading.
  • Green after: one value, correct order, in both the animating and the settled state.

All 136 tests in the packages/session-ui component suite pass. tsgo -b exits 0 and oxlint reports 0 warnings
/ 0 errors on the changed files.

A story (packages/ui/src/components/animated-number.stories.tsx) renders the component so the states can be
explored by hand; it is also what the component test mounts, so the fixture the test measures is the one a
reviewer can click through.

Screenshots / recordings

No response.

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

Added by the opencode bot (AI-generated).

Related: #43643

@argszero
fix(ui): expose one reading-ordered value from AnimatedNumber
188d326 
AnimatedNumber renders each digit in reverse DOM order (`row-reverse`
puts the visual order back) and every digit carries a 30-cell odometer
strip that is only visible while it animates. The strip and the reversed
static digits stayed in the accessibility tree, so `12` was read as
`2 1`, and a `5` to `6` update exposed `0123456789` three times. The
`aria-label` on the root did not replace those children, so it never
hid them.

Expose the value once as `.sr-only` text and mark the visual layer
(digits and strip) `aria-hidden`, so assistive technology reads the
current, correctly ordered number in every motion state. Rendering,
`row-reverse` ordering and the animation itself are untouched.
@github-actions

github-actions Bot commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

The following comment was made by an LLM, it may be inaccurate:

opencode-agent[bot]
opencode-agent Bot reviewed Oct 12, 2026
View reviewed changes

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The fix works. I ran the new story and spec against the base component: 12 was read as 2 1, and during the 5→6 animation the snapshot was the 30-cell strip (0 1 2 3 4 5 6 7 8 9 ×3). With this PR, both states read as a single 12 / 6. Type-checks pass in packages/ui and packages/session-ui, and the session-ui component suite passes apart from one unrelated flaky test that passes alone.

The one consumer, tool-count-label.tsx, didn't depend on the old root aria-label, and .sr-only already exists in utilities.css. The public API is unchanged.

No blocking concerns. This doesn't change the reversed DOM order, so the RTL visual bug in #43643 remains.

Related: #43643

@Hona Hona mentioned this pull request Oct 15, 2026
Merged
@argszero

argszero commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

Superseded upstream — recording this so it is not read as abandoned.

#55617 landed on dev as e25b7abf89 and rewrote packages/ui/src/components/animated-number.tsx. Among its changes it applies the same fix this PR carries:

  • the root aria-label is gone,
  • the current value is exposed once as .sr-only text,
  • the digit layer that holds the reversed/rolling glyphs is marked aria-hidden="true".

So the defect this PR was opened for is fixed on dev, and the issue it closes (#54653) is already closed.

I also checked the test half before rebasing, because that was the part that could still have been unique: #55617 added regression coverage for the same contract in packages/session-ui/component-tests/tool-group.spec.ts — reads and shows a two-digit count once, in order, in ltr/rtl asserts the multi-digit reading, and rolls the count and exposes only its current value while rolling asserts that only the current value is exposed mid-roll. Those are exactly the two assertions this PR's spec makes, one level up through the tool-group count that wraps AnimatedNumber.

For completeness I did run this PR's spec against current dev: both cases pass, and both go red when either half of the mechanism is removed (dropping the .sr-only value reads ""; dropping aria-hidden reads 12 0 1 2 / 5 0 5). The coverage is real, but it now duplicates what #55617 already guards, and the underlying fix is no longer this PR's to land.

Leaving this open for a maintainer to close. The branch is untouched at 188d32682f and I am not pushing to it.

@argszero

argszero commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

Superseded upstream: #55617 (fix(session-ui): slide the tool group count when it changes) gave AnimatedNumber the same accessibility layer this PR adds — a single .sr-only text node carrying the value in reading order, with the digits and their odometer strips marked aria-hidden. The conflict is exactly that hunk, so there is no remaining production change here.

I also carried a Storybook story and a Playwright component test for the behaviour while rebasing. I am dropping them rather than shipping unverified: this environment cannot download a Playwright browser (15 minutes of playwright install chromium fetched 452 KB), and a test that has never run is not evidence. Closing as superseded.

@argszero

argszero commented Oct 18, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded — see the comment above.

@argszero argszero closed this Oct 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@opencode-agent opencode-agent[bot] opencode-agent[bot] left review comments

Assignees

No one assigned

Labels

contributor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug(ui): V2 AnimatedNumber exposes animation digits and reversed reading order to screen readers

1 participant

@argszero