Repository navigation
fix(session-ui): slide the tool group count when it changes - #55617
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The approach looks right: reusing AnimatedNumber for the group count, drawing the strip digits with CSS so they drop out of the text, and rolling with transform instead of margin-top. The roll doesn't fire on first mount, and reduced motion is still respected. I couldn't run it in a browser here because Playwright's Chromium is missing system libraries, and the change is in the web UI rather than the TUI, so I have no screenshots. The session-ui unit tests (251) pass, and the typecheck is clean in session-ui and ui.
Public API: unchanged (internal UI components and CSS only).
One concern, inline: putting the count through AnimatedNumber means counts of 10 or more are now stored in reverse order in the page, so selecting and copying the count, or a screen reader, gives 21 instead of 12. Before this PR the count was plain text and read correctly.
Related: #54653
| {(count) => <span data-slot="context-tool-group-count">{count()}</span>} | ||
| {(count) => ( | ||
| <span data-slot="context-tool-group-count"> | ||
| <AnimatedNumber value={count()} /> |
There was a problem hiding this comment.
AnimatedNumber renders the digits reversed (digits() is .reverse()d) and flips them back only visually with row-reverse. Before this PR the count here was plain text "12". Now the static digits in the page read "21", so copying the header or a screen reader gives the wrong number for any group with 10 or more tools. The aria-label on the inner span doesn't help much, since it's a span with no role. This is the reading-order half of #54653. Could the digits be rendered in reading order (e.g. drop the reverse and row-reverse, keyed from the right), or the visual layer be aria-hidden with the number exposed once in order? The tool-group spec only checks a single digit ("4"), so a two-digit case would catch this.
There was a problem hiding this comment.
Fixed in 4ee175c.
- Digits render in reading order, keyed by place value.
.reverse()androw-reverseare gone, and the row is pinned todirection: ltr, which also fixes bug(ui): AnimatedNumber renders multi-digit numbers reversed in RTL locales #43643. - The number is the only text: one
.sr-onlynode. The visual layer isaria-hiddenand draws every digit withcontent: attr(data-digit). - At 37 tools, the accessibility tree has a single
"37"andtextContentis37. With selection forced on (the trigger isuser-select: none), the header copies asUsed 37 Write, Shell, Grep, Edit. tool-group.spec.tschecks a two-digit count in LTR and RTL and takes one accessibility snapshot mid-roll.
There was a problem hiding this comment.
Confirmed: 4ee175c fixes what I raised, and I have no remaining concerns on this thread.
- Reading order:
places()lists the digits most-significant first and keys each one by place value (animated-number.tsx). Withrow-reversereplaced bydirection: ltr, the DOM order and the visual order are both37, in RTL documents too. When the count goes from 9 to 10, the existing ones digit stays mounted and a tens digit is added on the left. Because the row isflex-endwithoverflow: hiddenwhile the width transitions, the new digit slides in from the left. - One readable value: the only text node is the
.sr-onlylabel, and that class exists globally inpackages/ui/src/styles/utilities.css. The static digits and the strip cells are now CSS::beforecontent inside anaria-hiddenwrapper, so the accessibility tree andtextContenteach get the number once. Removing thearia-labelfrom the roleless span is correct. - Other consumer:
tool-count-label.tsxalso usesAnimatedNumber, so it gets the same fix automatically.
I couldn't run the new tests. I ran tool-group.spec.ts against pr/head in a temporary worktree, but all 8 tests failed, including ones that existed before this PR. The cause is the environment, not the code: Chromium exits with code 127 because system libraries are missing in this sandbox. The mid-roll snapshot test reads once on purpose, so it is the one most worth watching for flakiness in CI. I removed the temporary worktree, and repo/ is unchanged.
75d4d16 to
4a39b7e
Compare
Render the Used N group count with AnimatedNumber again, keep the strip digits out of the number's text content, and roll them with transform instead of margin-top.
Expose the value as one sr-only text node and draw every digit with CSS in an aria-hidden layer, render digits in reading order keyed by place value, and pin the row to ltr so RTL documents keep numeric order. Fixes anomalyco#54653 Fixes anomalyco#43643
Keep a spare leading zero outside the clipped width so a new digit rolls in from zero, and keep a departing digit until the width has shrunk past it so it rolls to zero instead of vanishing. Digits no longer shrink while the width eases.
4a39b7e to
12a2e96
Compare
Roll the digit strip with top instead of transform. A composited roll lands on whole or half pixels, so the digit shifted 0.1-0.5px when it settled at 100-200% scale; drawn by the normal text path it rests where it rolled. Ease the width only when the digit count changes, so a late font swap or zoom never animates or clips it. A change while detached (a cached session timeline) settles at once instead of leaving the strip mid-track until it runs off its 30 cells, and an unchanged digit keeps the roll it is in instead of jumping to rest.
Fixes #54653
Fixes #43643
The "Used N" tool group count jumped to its new value. It rolls again with
AnimatedNumber, including when it gains or loses a digit. The number now reads correctly to screen readers, keeps numeric order in RTL, and settles on the pixel it rolled to.Before (9 → 10 → 9)
before.mp4
After
after.mp4
AnimatedNumberdev)377,3; while rolling also0123456789×3 per digit37, once, in every state3737aria-labelon a rolelessspan3773371pops in / vanishes and leaves a gap1rolls in from0/ rolls out to0The value is the only text: one
.sr-onlynode in reading order. The visual layer isaria-hiddenand draws every digit withcontent: attr(data-digit), so screen readers, copy andtextContentall get the number once.Digits render in reading order, keyed by place value, and the row is pinned to
direction: ltr.row-reverseis gone.A spare leading zero waits outside the clipped width, so a new digit rolls in from
0as the width grows. A departing digit stays until the width transition ends, so it rolls to0as it is clipped away. Digits no longer shrink while the width eases.The strip rolls with
topon a relatively positioned strip, so the rolling and resting digit are drawn by the same text renderer. A compositedtransformroll ended 0.1–0.5px off the text pixel grid and snapped as it settled. Below: 5 → 4 on a 150% display, frame by frame. Mounting at a new value does not animate; reduced motion skips the roll.Only a change in digit count eases the width, so a late web font swap or a zoom change never animates it.
A change while the number is detached settles at once. The session cache keeps up to 16 timelines alive off-page; before, each change there pushed the strip further along its 30 cells until a background tab came back with a blank digit. An unchanged digit keeps its in-flight roll instead of jumping to rest.
tool-group.spec.tscovers a two-digit count in LTR and RTL, one accessibility snapshot mid-roll, both digits rolling across 9 → 10 → 9, no animation on mount or font change, changes while detached, and a second change mid-roll. The last three fail on the previous commit.fix(ui): expose one reading-ordered value from AnimatedNumber #54977 fixes the screen-reader half with the same
sr-only/aria-hiddensplit; this also keeps the text content to one value and fixes RTL.