Repository navigation
fix(Carousel): under snap paging, scroll calls move through the paging - #10384
Conversation
|
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.
|
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| Carousel · Storybook | 67.0 kB (+51 B) · 10.8 kB CSS (0) | 605 (+6) | ✓ · ✓ · ✓ |
| TabList | 41.6 kB (+7 B) · 6.4 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.88 MB packed (+1.1 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 19 rows, core 25 rows; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
CI impact: 0 job-min per update · pool 0% · time to green −1.5 min · standard runners · 1 CI action or script file changed (not measured)
| Before | After | Change | |
|---|---|---|---|
| Job-minutes per PR update | 112.4 | 112.4 | 0 |
| Runner pool, runner-minutes a day | 58,103 | 58,103 | 0 (0%) |
| Average concurrent jobs | 40.3 | 40.3 | 0 |
| Time to green (median → this run) | 17.1 min | 15.6 min | −1.5 min |
| Runs a day | 517 | 517 | 0 |
Not measured (the jobs that use these keep their recent minutes above): .github/scripts/ci-interaction-webkit.test.mjs.
Assumptions: before is each job's average over the last 20 pull-request runs; after keeps that for unchanged jobs and uses this run's minutes for added or changed ones, weighted by how often each changed job ran in those runs; runs a day averages the seven whole days before today; time to green compares this run with the median of those runs that ran the same jobs.
Generated by PR Enrichment workflow | View full report
|
/approve-spec 2c2c426 |
github-actions
Bot
removed
the
needs:spec-owner-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes needed, plus one spec decision.
The FR8 rewrite is approved for this head, but one behavior still needs an engineering owner's decision before approval: under snap paging the edge fades now follow the held row, and component:Carousel leaves edge fades uncontracted, so an owner should confirm it and name the clause that records it.
Blocking:
- Each edge-button press now interleaves layout reads with style writes during the takeover (
SnapPaging.ts:204-209and:339, thenpagingStepinCarousel.tsx), which runs 6 layouts in one click task in Chromium against 0 on main; please do every read before the takeover's first write (architecture:react-component-runtimeINV18). - With
padding,scrollTo(index)targets the item's border edge rather than its snap stop (Carousel.tsx:590-595), so the row overshoots by the scroll padding and jumps back at the hand-back (16 px atpadding={4}); please target the nearest stop instead.
The red-first unit tests and the edge buttons following the held row look good.
[Automated review]
|
/approve-spec 105b193 |
There was a problem hiding this comment.
Changes needed, plus one spec decision.
The FR8 rewrite is approved for this head, and both earlier blocking findings are fixed: a press now does no layout in its click task, and a padded scrollTo(index) lands on its stop. One behavior still needs an engineering owner's decision before approval: under snap paging the edge fades now follow the held row, and component:Carousel lists edge fades as uncontracted. An engineering owner should accept or reject that and name the clause that records it; a record change is approved by commenting /approve-spec <full head SHA> on its head, which the author may do as an engineering owner.
Blocking:
- Miss: every edge-button press and
scrollNext,scrollPrevorscrollTocall now leaveswill-change: transformon the items the takeover layered (SnapPaging.ts:229-231), becausehandBackremoves it (:246) before the animator's demotion restores it as the element's own value, so six items keep their own compositing layer after one press and all nine after three (main does this only after a gesture the pager takes over, such as a touch flick); please leave the promotion to the animator, or restore the row's own value after the demotion, and assert in the press test that no item keepswill-changeafter the settle.
[Automated review]
There was a problem hiding this comment.
Changes needed.
An engineering owner approved the FR8 rewrite for this head, which also settles the edge fades following the held row, and the earlier layout and scrollTo(index) findings stay fixed. One defect remains before approval.
Blocking:
- Miss: every edge-button press and
scrollNext,scrollPrevorscrollTocall under snap paging leaveswill-change: transformon the items the takeover layered (SnapPaging.ts:229-231), becausehandBackremoves it (:246) before the animator's demotion writes it back as the element's own value, so six items keep their own compositing layer after one press and all nine after three; please leave the promotion to the animator and assert in the press test that no item keepswill-changeafter the settle.
[Automated review]
There was a problem hiding this comment.
Approved pending CI. CI has not run on this head yet: the pull request is a draft, and since its branch is in a fork, a maintainer adds the ci:run label once it is ready so CI runs.
The FR8 rewrite carries an engineering owner's approval for this spec content, and the defect left by the last review is fixed: no item keeps will-change after a press or a flick settles. A press still does no layout in its click task, and edge-button rests match main in every snap-paging story checked.
Landing this also lands the two commits of #10359, which this branch contains.
[Automated review]
|
/approve-spec abe44ef |
abe44ef to
3cb24f4
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 3cb24f4 |
github-actions
Bot
removed
the
needs:spec-owner-review
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 7be7158 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Approved pending CI. CI has not run on this head yet: the pull request is a draft, and since its branch is in a fork, a maintainer adds the ci:run label once it is ready so CI runs.
The Carousel record changes (FR8's rewrite and its held-row sentence, the edge-fades line, and the FR1/FR8 verification rows) have an engineering owner's /approve-spec for this head. Under snap paging, edge-button presses and scrollNext, scrollPrev and scrollTo now move through the paging and rest where they rested before, the press lays out nothing in its own task, and no item keeps a layer once the row rests. The new unit cases and the press browser tests fail on main and pass here; the iOS catch itself rests on your simulator trials, which Linux browsers cannot reproduce.
No blocking findings.
[Automated review]
At rest, Carousel's edge buttons and its handle's scroll calls scrolled the row with a native smooth scroll. A still touch on iOS stopped that scroll between two items, and nothing put the row back on a page. Under snap paging, the paging now takes the row over from rest, as a flick does, and plays the move on its own animator. A touch then catches the move as it catches a settle, and the release rests on a page.
Carousel's scroll calls now move the row through the snap paging value from rest too, not only a row the value holds. FR8 names those moves.
Under snap paging an edge button press read layout between its own style writes: 18 forced layouts in one click task in Chromium, 0 on main. Every read now comes before the takeover's first write: the target, the stops and the view width. The pager learns its page while the row is at rest, a child already in place gets no write, and a held row's edges come from where it is shown. Each child's animator is made at attach, so its stylesheet read runs in a later task. The press now lays out nothing in its own task, as on main. scrollTo(index) targeted the item's border edge. With scroll padding the row overshot and the hand-back's snap pulled it back 16 px. It now targets the item's snap stop.
The takeover wrote will-change: transform on the items near the view. Each item's animator recorded that as the item's own value, the hand-back removed it, and the animator's demotion wrote it back. So after an edge-button press, or a scroll call, the items kept a layer of their own. Each child's animator already gives it a layer while it moves and takes it away after the motion, so the takeover writes no will-change of its own now.
Under snap paging a press on the end button moves the row through the paging. The edges spec now proves that the start button comes back within two frames of the row leaving the start, while the paging still holds the row.
FR8 takes the spec owners' line: under snap paging, the edge buttons, the edge fades and the handle's canScrollPrev()/canScrollNext() follow the held row, one state. FR1's and FR8's verification rows name the real-browser proofs, and verified_by lists both snap paging specs.
Out of scope keeps the fades' look uncontracted and points their state to FR8, as the spec owners asked.
A change to Carousel's code runs its folder's interaction specs, and this pull request adds the snap paging buttons spec there, so the routing test's expected list names it too.
7be7158 to
307b5b3
Compare
|
/approve-spec 307b5b3 |
There was a problem hiding this comment.
Approved pending CI. CI has not run on this head yet: the pull request is a draft, and since its branch is in a fork, a maintainer adds the ci:run label once it is ready so CI runs.
The Carousel record changes (FR8's moves of the row from Carousel's own scroll calls and its held-row sentence, the edge-fades boundary line, and the FR1 and FR8 verification rows) carry an engineering owner's spec approval for this head, and the code matches them. Under snap paging, edge-button presses and handleRef scroll calls now move through the pager, rest where main rests, leave no layer behind once the row rests, and do no layout in the press's own task. The new unit and browser cases fail on main and pass here, and the WebKit routing test now expects the new buttons spec.
No blocking findings.
[Automated review]
cixzhang
added
the
ci:run
User impact
Under snap paging on iOS, a touch during an edge button's move leaves the row between items: 1–48 px off a page in 5 of 6 trials. UIKit settles a caught
setContentOffset(animated:)on a page.Problem and solution fit
At rest, the edge buttons and
handleRef's scroll calls ran a native smooth scroll, which a still touch in Safari stops between stops. Moved by the paging, the press is the pager's own move: a touch catches it as a settle (spec:AST-094FR16), and the release rests on a page.Expected behavior and authority
component:CarouselFR7: an edge button steps about one view, landing on the nearest stop. FR8, as the owners approved, names Carousel's "moves of the row from Carousel's own scroll calls", and takes their line: "Under snap paging, the edge buttons, the edge fades and the handle'scanScrollPrev()/canScrollNext()follow the held row, one state."Smallest restoration
moveSnapPagingtakes the row over from rest, as a flick does, and plays a held move'sgoTo. Carousel'sscrollByandscrollToIndexcall it; a wheel stays native.setContentOffset(animated:)curve (spec:AST-094FR45's gap).architecture:react-component-runtimeINV18), so its task lays out nothing. Each item's animator owns its layer.scrollTo(index)targets the item's snap stop inside the scroll padding, so the hand-back pulls nothing back.Evidence
Scope
Writing rubric results
component:CarouselFR8, FR1's and FR8's verification rows,verified_by: R1, R2, R4–R6 none; R3: each new row names its spec. Out of scope keeps the fades' look uncontracted; their state follows FR8.Testing
.github/scripts/ci-interaction-webkit.test.mjs) expects Carousel's folder specs; this PR adds the buttons spec to its list. Without the line it fails on this branch (three specs found, two expected); with it, 23/23.--cpus=4,LP_NUM_THREADS=4).SnapPaging.test.tsx:scrollNext,scrollPrevandscrollTo(index)reach the paging'smoveand never the row's nativescrollToorscrollBy. Both cases fail on main. "moves a scroll to an item onto its snap stop, inside the scroll padding": the previous head targets 200 px (the border edge), this change 184 px. The Carousel, SnapPaging and scroll-overflow suites pass 83/83.pnpm test:ui) at this head: 910 files, 22,252 tests pass. Core typecheck: 0 errors.pnpm check:knowledgepasses.CI=true pnpm lintpasses (0 errors). Prettier is clean on the changed files.Review notes
verified_by), Out of scope's edge-fades line, and the routing test's line.usePagerfollow-up that catches a program's move at touch-down.