Repository navigation
fix(Carousel): under snap paging, the edge buttons follow the held row - #10359
Conversation
While snap paging holds the row, its scroll offset stays where the hold began and its items move. The edge buttons and fades read the scroll offset, so they changed only at the hand-back, about half a second after the row had visibly left an edge. attachSnapPaging takes an optional listener, still through attach. The track reports where the held row is shown on each frame of the hold, from the lead child's own animator, with the direction read at the takeover, so a frame reads no style or layout. Carousel turns each report into its edges and sets state only when an edge flips. After the hand-back it keeps the held edges for one frame, until the scroll measure reads the new offset, so no button flickers.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| TabList | 40.4 kB (+125 B) · 6.4 kB CSS (0) | — | — · — · — |
| Carousel · Storybook | 59.8 kB (+120 B) · 10.3 kB CSS (0) | 599 (0) | ✓ · ✓ · ✓ |
Package size: @astryxdesign/core 6.80 MB packed (+1.7 kB); no new dependencies.
API changes: core +0 · ~1 · −0
Committed API report behind this build: cli 19 rows, core 25 rows; run pnpm api:report.
1 API change (advisory)
| Package | Where | Signature | Usage | |
|---|---|---|---|---|
| ~ | core |
. · Hooks · hook useScrollOverflow |
hook useScrollOverflow: () => { overflowStart: boolean; overflowEnd: boolean; hasOverflow: boolean; scrollRef: (el: HTMLElement | null) => void } → hook useScrollOverflow: () => { scrollRef: (el: HTMLElement | null) => void; overflowStart: boolean; overflowEnd: boolean; hasOverflow: boolean } |
import {useScrollOverflow} from '@astryxdesign/core'; useScrollOverflow(…) |
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval needs an engineering owner's decision first, and two fixes are needed after it.
Decision needed: under snap paging, the edge fades and handleRef's canScrollPrev() / canScrollNext() now follow the held row the way the edge buttons do, but the Carousel record lists edge fades as uncontracted and does not cover the imperative handle. An engineering owner records approval by commenting /approve-spec <full head SHA> on this pull request (the author can do this as an engineering owner), or asks for them to keep reading the scroll offset.
Required fixes:
Carousel.tsx:477-484copiesuseScrollOverflow's edge tolerance and keeps its ownheldEdgesstate beside the shared hook, so extenduseScrollOverflowto take the held position and delete the copy.- Unmounting the Carousel or dropping
hasSnapmid-flick leaves the release animation frame running, because the cleanup atCarousel.tsx:486-490cancels it beforedetach()schedules a new one; cancel it afterdetach().
The fix itself holds up: the start button now returns one frame after the row leaves the start, and the new unit tests fail without the change. CI has not run on this draft yet.
[Automated review]
| // Reports where the held row is shown, once a frame, from the lead child's | ||
| // own animator. |
There was a problem hiding this comment.
Non-blocking: this comment narrates follow(). A name such as reportHeldRow says the same thing, and the no-layout reason already sits at lines 126-127.
Carousel kept its own copy of the scroll measure's tolerance and its own held-edge state beside useScrollOverflow. The hook now does both: an internal useShownScrollOverflow takes the position snap paging reports and measures the edges from it, and null measures the scroll offset again from the next frame. That frame ends with the container or the hook, so a detach mid-flick leaves none running. useScrollOverflow's own return is unchanged.
There was a problem hiding this comment.
Approval waits on one engineering-owner decision; the two fixes from the last review are in.
Decision needed: under snap paging, the edge fades and handleRef's canScrollPrev() / canScrollNext() now follow the held row the way the edge buttons do, but the Carousel record lists edge fades as uncontracted and does not cover the imperative handle. An engineering owner records approval by commenting /approve-spec <full head SHA> on this pull request (the author may do this as an engineering owner), or asks for them to keep reading the scroll offset until the hand-back.
The edge buttons now come back one frame after the row leaves an edge in both writing directions, and the new tests fail without the change. The held edges now live in the shared scroll-overflow hook, and its frame is released on detach and unmount. Two comments could be tightened (inline, non-blocking). CI has not run on this draft yet.
[Automated review]
| shownRef.current = null; | ||
| return; | ||
| } | ||
| // The reported edges stand until then, so no edge flickers. |
There was a problem hiding this comment.
Non-blocking (comment style): "then" has no referent at this line. Suggested wording:
// The reported edges stand until the next frame, when the handed-back
// offset is current. No edge flickers.
|
/approve-spec dbd183d |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, and it runs once the pull request leaves draft and a maintainer adds the ci:run label.
Under snap paging, the edge buttons and fades now come back one frame after the row leaves an edge in both writing directions, and the new tests fail without the change. An engineering owner approved the fades and canScrollPrev() / canScrollNext() following the held row for this head. Two comments could still be tightened (inline, non-blocking).
[Automated review]
cixzhang
added
the
ci:run
User impact
Under snap paging (
hasSnap={useSnapPaging()}), a flick from a Carousel's start left the start button hidden about half a second after the row had visibly moved. That is 29 frames in Chromium, 21–22 in WebKit, and 33–34 frames (557 ms) in Safari on the iOS 26.2 Simulator. The edge fades lagged too, and the end button did on a flick back from the end.Problem and solution fit
The buttons and fades read the row's scroll offset (
useScrollOverflow). While snap paging holds the row, the offset stays where the hold began and the items move by a translate. So the edges changed only when the settle handed the row back. Carousel's native modes follow the row on everyscrollevent, as a UIKit control fed byscrollViewDidScrollfollows its content.Expected behavior and authority
component:CarouselFR1: "An edge scroll button whose direction has run out is hidden." Under snap paging, a direction runs out or comes back with the row the person sees. FR8 still holds: Carousel imports no pager code, and calls only attach and detach on the value, plus its move of a held row. FR2's focus handoff still fires once per edge transition.Smallest restoration
attachSnapPagingtakes an optional listener, passed throughattach.useScrollOverflowmeasures the edges from those reports and sets state only when one flips. After the hand-back it keeps them one frame, until the scroll offset is current.useScrollOverflow's return is unchanged.Evidence
CarouselSnapPagingEdges.interaction.browser.spec.tsmeasures the start button coming back 29 frames after the row leaves the start in Chromium, and 21–22 in WebKit. The end button comes back 29–30 and 23–24 frames after the row leaves the end.SnapPaging.test.tsxcases that fail on main, and threeuseScrollOverflow.test.tsxcases for the reported position.Scope
Testing
CarouselSnapPagingEdges.interaction.browser.spec.ts, against Storybooks built from main and from this change, on the three-up snap paging story in paging mode with real touch flicks:--cpus=4,LP_NUM_THREADS=4), 3 runs each: 22/22/21 and 23/24/23 on main; 1 frame in every run with this change.SnapPaging.test.tsx, three new cases. A held row's reports enable and disable each button before the hand-back. The held edges stand for one frame after the hand-back. A held row reaching the focused button's edge hands focus to the scroll area once. All three fail on main's sources.useScrollOverflow.test.tsx, four new cases: the edges follow a reported position; they stand one frame after null, then read the scroll offset; a detach or an unmount leaves no frame running;useScrollOverflow's return keeps its four fields. The Carousel, SnapPaging, TabList and hook suites pass 183/183.pnpm test:ui): 896 files, 21,973 tests pass. Core typecheck: 0 errors. ESLint: no new warnings (Carousel.tsx's unusedfocusVarsis on main). Prettier is clean.merge-tree: clean against live main and feat(ContextMenu): a held finger grows the trigger on UIKit's lift spring #9816, chore(Layer): the outside-press watch can close on a pan, follow chosen pointer types, and judge a press in a closed shadow root #9814, feat(ContextMenu): where a coarse pointer's held finger opens the menu (presentation unset, or adaptive where it stays a popover), the same finger slides onto an item and its lift selects it #8133 and feat(ContextMenu): open from a held finger on iOS 26's timeline #8125. fix(core): make disabled Button focusable via aria-disabled per AST-085 #7906 conflicts with main only inDateTimeInputTouch.test.tsx, without this change. This change'sCarousel.tsxhunks are clear of fix(core): make disabled Button focusable via aria-disabled per AST-085 #7906's edge-button lines.Review notes
component:Carousel's verification map does not name this proof yet. Its FR1 row lists unit and accessibility-tree tests at rest. A records follow-up adds the new interaction spec and the held-row state to that row, so the owners' stamp on the record does not hold this fix.