Repository navigation
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.
|
e092dd8 to
a7aadca
Compare
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsCarousel (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included.
|
| Component | JavaScript | CSS |
|---|---|---|
| core/Carousel | +32.5KB |
+21B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.6KB | 34.6KB | +9B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
55 of 502 shot(s) changed.
A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.
| component | story | theme | mode | pixels |
|---|---|---|---|---|
| Selector | Indicator Space Bottom Sheet Narrow End RTL | neutral | light | 753,933 |
| Selector | Indicator Space Bottom Sheet Wide Start | neutral | light | 740,072 |
| RichTextEditor | Markdown parity: side by side | probe | dark | 558,840 |
| RichTextEditor | Markdown parity: side by side | probe | light | 540,756 |
| Chat | Full AI Chat | probe | light | 230,823 |
| Chat | Full AI Chat | probe | dark | 229,805 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Collapsible | Dividers | probe | dark | 66,445 |
| Collapsible | Dividers | probe | light | 66,445 |
| AppShell | Playground | neutral | dark | 65,198 |
| AppShell | Playground | neutral | light | 63,788 |
| Chat | Default | neutral | dark | 36,304 |
| Chat | Default | neutral | light | 35,767 |
| Chat | Empty State | probe | light | 23,531 |
| Chat | Empty State | probe | dark | 22,622 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | dark | 15,378 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | light | 14,107 |
| Chat | With Attachments | probe | light | 13,406 |
| Chat | With Attachments | probe | dark | 11,232 |
and 35 more.
Generated by PR Enrichment workflow | Storybook | Sandbox | 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.
This needs an engineering owner's decision before it can be approved. No current spec covers the new hasPaging prop, the touch pager that replaces native scrolling (draft spec:AST-094 itself says scrolling stays native) or the 0.3 s slide kept under Reduce Motion, and the new adaptive bottom-sheet threshold changes the width-and-pointer query that component:Selector DEC-1 keeps. An engineering owner records the decision, or the amendments it needs, by commenting /approve-spec <full head SHA>; the author may do so as an engineering owner. Whatever that decision is, please also:
- land or retarget the six predecessor PRs so this diff is the 1,269-line Carousel change rather than 29,083 lines;
- run the interaction proofs through Core's existing test lane instead of the separately routed
pr-interactionjob (spec:AST-030FR11); - get
lintpassing (ESLint runs out of heap) and fix the Paging stories' text contrast that failspr-a11y; - keep a single-pointer alternative to swiping when
hasPagingis used withhasButtons={false}.
[Automated review]
| * `hasSnap`, `hasEdgeFade` and `padding` do not apply. | ||
| * @default false | ||
| */ | ||
| hasPaging?: boolean; |
There was a problem hiding this comment.
hasPaging already names PageUp/PageDown focus paging in the exported useListFocus options (useListFocus.ts:95). architecture:public-component-api INV2 says names carry the same meaning across the public surface. Keeping this name is part of the owner decision the open question asks for.
There was a problem hiding this comment.
Left to the owner decision the description asks for (the public prop name).
| pagingTouchScroller: { | ||
| overflowX: 'clip', | ||
| overflowY: 'clip', | ||
| }, |
There was a problem hiding this comment.
On a coarse pointer this clip removes native scrolling, so the custom pan is the only touch path. With hasButtons={false} the edge buttons are never rendered (line 770), which leaves no single-pointer alternative to the path-based swipe (WCAG 2.2 SC 2.5.1, the spec:AST-020 FR1 baseline). Keep the buttons whenever touch paging is active, or fall back to native per-page snapping when hasButtons is false.
There was a problem hiding this comment.
Fixed in 226a949: with hasButtons={false} the carousel snaps natively on iOS too, so the browser's own scrolling pages it and no author gesture is the only way. Listed in the description as proposed, pending owner review. Test: "without the buttons it pages natively" (failed before).
| return undefined; | ||
| } | ||
| const gap = Number.parseFloat(getComputedStyle(track).columnGap) || 0; | ||
| pager.setOptions({gap}); |
There was a problem hiding this comment.
The pager gets the gap only here, at attach. This effect's dependencies omit gap, and usePager's measure() re-reads only the width. After a gap change, place() uses the live gap while the pager keeps the old stride, so page n rests n × the difference off. Re-send pager.setOptions({gap}) when the gap changes.
| useIsomorphicLayoutEffect(() => { | ||
| pager.setOptions({count: Math.max(1, count), loop: loops}); | ||
| if (isEnabled) { | ||
| place(); | ||
| } | ||
| }, [pager, count, loops, isEnabled]); |
There was a problem hiding this comment.
This Effect names no external system (architecture:react-component-runtime INV3). When count shrinks below the current page it sets page through the pager's clamp (setOptions → goTo → onIndexChange → setPage). Derive the clamped page during render, and add the comment naming the pager it synchronizes.
There was a problem hiding this comment.
Fixed in 226a949: the page is the pager's, read through useSyncExternalStore and clamped during render; the count Effect names the pager it synchronizes and reports nothing. Test: "a smaller count clamps the page shown, and growing back keeps the pager and the controls in step" (failed before).
| useEffect(() => { | ||
| if (index != null && index !== pager.index) { | ||
| pager.goTo(index, {reason: 'controlled'}); | ||
| } | ||
| }, [pager, index]); |
There was a problem hiding this comment.
A controlled index change calls the consumer's onIndexChange from this Effect (goTo → report). INV2 of architecture:react-component-runtime says the callback fires from the handler that caused the change, never from an Effect reacting to the resulting state. Skip the echo for reason: 'controlled', or name the external system this synchronizes.
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
This job routes itself (pr-scope-touches.sh with its own path list) and is not part of the required test join. spec:AST-030 FR11: "New coverage inside an existing surface MUST join those owners and MUST NOT create a separately routed lane or required context." Run these proofs under Core's existing test owner, or split the lane with an owner-approved AST-030 amendment.
There was a problem hiding this comment.
Not this PR's code: the pr-interaction job comes from the interaction-harness foundation below this commit, and is routed to its owners.
| export const COMPACT_TOUCH_QUERY = | ||
| '(width < 744px) and (pointer: coarse), (height < 500px) and (pointer: coarse)'; |
There was a problem hiding this comment.
This query becomes presentation="adaptive"'s for five components. It adds a height test to the released (max-width: 768px) and (pointer: coarse), while component:Selector DEC-1 says adaptive "keeps its width-and-pointer query". Phones in landscape now get the sheet, and 744–768 px portrait tablets get the popover. This waits on the owner decision in the open question.
There was a problem hiding this comment.
Not this PR's code: the adaptive query belongs to the touch-presentation change below this commit, and is routed to its owner.
| {title: 'Pack', body: 'A checklist for every trip.', color: '#5b8c5a'}, | ||
| {title: 'Go', body: 'Tickets and directions offline.', color: '#c46a3a'}, |
There was a problem hiding this comment.
pr-a11y fails on the three Paging stories with a serious color-contrast violation, two nodes each. The white 14 px body text measures 3.93:1 on #5b8c5a and 3.84:1 on #c46a3a, under WCAG's 4.5:1. Darken these two fills, or use a token pair that reaches 4.5:1.
There was a problem hiding this comment.
Fixed in 7fad86a: the Paging stories' fills are darker (white text reaches 4.5:1), and pr-a11y passes.
a7aadca to
7fad86a
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec covers the new hasPaging prop, the touch pager that replaces native scrolling, or the 0.3 s slide kept under Reduce Motion, and the draft spec:AST-094 it relies on is unapproved (its FR8 asks for a cross-fade where the code keeps the slide). An engineering owner records that decision, or the amendments it needs, by commenting /approve-spec <full head SHA>; the author may do so as an engineering owner. Whatever is decided, please also:
- land or retarget the six predecessor PRs so this diff is the 1,269-line Carousel change;
- move the adaptive bottom-sheet query change, which contradicts
component:SelectorDEC-1, into its own PR with an amendment to that decision; - run the interaction proofs under Core's existing test owner instead of the separately routed
pr-interactionjob (spec:AST-030FR11); - get the required
lintcheck passing (ESLint runs out of heap); - keep a single-pointer alternative to swiping when
hasPagingis used withhasButtons={false}.
[Automated review]
| const token = ++settleToken; | ||
| const handle = animator.play( | ||
| {x: toTrackCurve(curve, sign())}, | ||
| {reducedMotion: 'keep'}, |
There was a problem hiding this comment.
This keeps every pager move under Reduce Motion, including button, key, handleRef and focus moves that no finger drives. Draft spec:AST-094 FR8 in this same PR says such a move "fades through" and that only motion following a finger keeps moving. Whichever way the owner decides, make the code and FR8 agree, for example the animator's default 'crossfade' for goTo and 'keep' only for the settle after a drag.
There was a problem hiding this comment.
Left to the owner decision listed in the description (Reduce Motion keeps paging motion, as UIKit does; AST-094 FR8 follows the decision).
7fad86a to
fcf974c
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec covers the new hasPaging prop, the iOS pager that replaces native scrolling, the 0.3 s slide it keeps under Reduce Motion, or its theme-tunable paging motion, and the draft spec:AST-094 they rely on is unapproved (its FR4 and FR8 also disagree with the code). An engineering owner records that decision, or the changes it needs, by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Whatever is decided, please also:
- land or retarget the seven predecessor PRs so this diff is the 1,375-line Carousel change;
- move the adaptive bottom-sheet query, the role-keyed theme motion API with its
platformcondition, and the experimental subpaths into their own PRs with the amendments they need (component:SelectorDEC-1,architecture:theme-authoring-contract,spec:AST-017FR21); - run the interaction proofs under Core's existing test owner instead of the separately routed
pr-interactionjob (spec:AST-030FR11); - keep a single-pointer way to page when
hasPagingis combined withhasButtons={false}, and keep the pager's gap in sync with the track.
[Automated review]
| * is everything else. Pair `ios` with `pointer: 'coarse'` for a touch | ||
| * profile, such as touch motion tuned per platform. | ||
| */ | ||
| platform?: 'ios' | 'other'; |
There was a problem hiding this comment.
This adds a platform axis to theme adaptation conditions, compiled to @supports (-webkit-touch-callout: none) in generateThemeRules.ts. architecture:theme-authoring-contract says “An adaptation change preserves the fixed condition vocabulary”, and the record is not amended here. Please move the role-keyed motion API and this condition to their own PR with that amendment.
There was a problem hiding this comment.
Not this PR's code: the platform adaptation condition belongs to the theme foundation below this commit, and is routed to its owner.
fcf974c to
226a949
Compare
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec covers the new hasPaging prop or the iOS pager that replaces native scrolling, the draft spec:AST-094 they rely on is unapproved, and the pager's 0.3 s slide and travelling settle under Reduce Motion contradict the current design:reduced-motion record (an opacity-only fade of 200 ms or less) unless that record is amended. An engineering owner records the decision, or the changes it needs, by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Whatever is decided, please also:
- land or retarget the seven predecessor PRs so this diff is the 1,501-line Carousel change;
- build the adaptive bottom-sheet query from the theme's compact definition (
spec:AST-012FR9: below themdpoint or under 500 px tall) instead of 744 px, in its own PR; - cap the animator's Reduce Motion cross-fade at 200 ms, and move the theme motion roles with their
platformcondition and the experimental subpaths into their own PRs with the amendments they need; - run the interaction proofs under Core's existing test owner instead of the separately routed
pr-interactionjob (spec:AST-030FR11), and addcomponent:CarouselFR2 and FR3 browser cases forhasPaging.
[Automated review]
| return; | ||
| } | ||
| follow( | ||
| animator.play({x: toTrackCurve(curve, sign())}, {reducedMotion: 'keep'}), |
There was a problem hiding this comment.
Under Reduce Motion this keeps the travel of the settle and of goTo (reducedMotion: 'keep', also at line 453). Current design:reduced-motion DR2 asks for an opacity-only fade of 200 ms or less on a coarse pointer and after a touch drag's release, DR4 rules out keeping a longer animation, and its DEC-1 rejected a per-component keep. Either an engineering owner amends that record for paging (the open question asks), or the settle and the moves conform.
| * Cross-Fade Transitions on (opacity CABasicAnimation 0.5 s, | ||
| * (0.42, 0, 0.58, 1)), Oct 23 2026 | ||
| */ | ||
| reducedMotionCrossFadeMs: 500, |
There was a problem hiding this comment.
This 500 ms is the default substituted fade for every animator move under Reduce Motion (animator.ts:1420). No core component plays it yet (the pager uses 'keep'), but current design:reduced-motion DR2 caps a touch fade at 200 ms, opacity only, and DR4 rules out a theme value that lengthens it, which defineTheme.ts:262 documents. Cap it at 200 ms and refuse longer theme values, or amend the record.
| export const TOUCH_QUERY = '(pointer: coarse)'; | ||
|
|
||
| /** The viewport is compact: layout behaviours. */ | ||
| export const COMPACT_QUERY = '(width < 744px), (height < 500px)'; |
There was a problem hiding this comment.
Current spec:AST-012 FR9 defines compact as a width below the effective md point (768) or a height below 500, and says the web touch presentation resolver uses the theme's one definition, with no numbers of its own; main exports it as compactDefinition. With 744 here, a 744–767 px coarse-pointer screen (an iPad mini in portrait) opens the popover where FR9 gives the sheet, and a theme that moves md is ignored.
| // pager's swipe (WCAG 2.5.1), and the browser's own scrolling needs none. | ||
| const trackRef = useRef<HTMLDivElement>(null); | ||
| const {idiom} = useTouchPresentation(); | ||
| const isTouchPaging = hasPaging && idiom === 'ios' && hasButtons; |
There was a problem hiding this comment.
hasButtons={false} now also turns the iOS pager off. architecture:public-component-api INV3 (one prop, one concept) and INV16 (a presentation value keeps the fine-pointer input mode) bear on this coupling, and draft FR9 and DEC-2 reject native snapping on iOS; it is part of the owner decision the open question asks for.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
No spec:AST-090 exists at this head or on main, so the departure recorded here, and repeated under Current-state impact, cannot be weighed. Cite the record where it exists, or drop the reference until it lands.
|
Agreed: one implementation.
|
226a949 to
03cb63d
Compare
github-actions
Bot
added
the
needs:design-review
…llowance under every primary pointer The useMenuPress record's FR9 puts the trigger's held finger under spec:AST-094 FR29 under every primary pointer (FR43), so a finger on a touchscreen laptop may drift 13 pt before the open, as on a phone. The trigger read (pointer: coarse) as its press started and took the long press's 10 px where the pointer was fine. Three cases fail until it does not: a 12 pt drift opens under a fine pointer too, a (12, 12) settle opens there, and nothing reads a pointer query, the trigger's press included.
…lowance under every primary pointer The useMenuPress record's FR9 governs the trigger's held finger by spec:AST-094 FR29 under every primary pointer (FR43). The trigger now hands the long press FR29's 13 px drift allowance whatever the primary pointer, so it no longer reads (pointer: coarse) as its press starts, and the menus drop their coarsePointer import. ContextMenu keeps the long press's own allowable movement. The changeset says the same.
Strict ESLint reported six unused bindings in files this pull request carries: the host's HAPTIC_SWITCH_ATTRIBUTE import, two unused imports in the pan's tests (scrollProbe, ScreenEdgePanGestureRecognizer), the swipe test's drift, the presented-hit-testing test's gestures, and a GestureHost test's before, which now asserts what it was read for: a second finger's moves leave the pan where it was.
…eal angular speed The release velocity of a rotation whose listener sets rotation = 0 on each change, at 2 degrees every 16 ms, read 4.363 rad/s against the 2.182 rad/s of the same stream without a re-base: the test asserted only more than 1.5. It now asserts the 2.18 rad/s (spec:AST-100 FR2), and fails.
The rotation's velocity tracker samples the raw unwrapped angle, which setting rotation does not move, so the setter's shift of the samples read each re-base as extra turn and doubled the release velocity of a listener that re-bases on every change. The setter no longer touches the tracker.
…mates while it runs The proof read the element's animations 230 ms in, after the dip had ended, and asserted only that every animated key is opacity, which an empty list passes: with the cross-fade removed from the plan (the move still landing), the proof still passed. It now reads the keyframe keys right after the call starts and asserts they include opacity and nothing else; the same mutant fails it. Chromium and WebKit pass. (F1's in-flight read from c648034.)
…roller, and its claimed moves are cancelled under pointer events Two comparisons stop at the shadow boundary, each failing a case: - once the host's listener on a recognizer's inner node has seen the finger, isFingerDownIn reads it as outside a light-DOM scroller around that root, so a scroll under it is not the finger's (FR20); - under inputMode 'pointer-events', a claimed pan on a shadow host cancels none of its finger's moves over the inner content, since the finger's target is the inner node and its touches reach the host retargeted (FR19).
…the flat tree isFingerDownIn and the pointer-events finger match compare with containsAlongFlatTree, as the scroll watch already does: a finger on content inside an open shadow root is down in the scroller around that root, and its touches, retargeted to the shadow host, match its pointer finger, so a claimed pan cancels its moves there (spec:AST-094 FR19, FR20).
…achments asks, and only then The host let the last attachment decide whether an element is an action group (useGestureRecognizer always passes the option), and never recomputed it at a detach: a plain attachment after the group's turned the group off, and a group's attachment released left a recognizer that never asked taking presses under a fine pointer. Fails until the host counts the attachments that ask.
…ents asks The host counts the attachments that pass actionGroup: true and keeps the element a group while one remains, recomputed at each detach. An attachment that passes false, as useGestureRecognizer does by default, no longer turns a group off, and a group's last asking attachment taking its leave ends the group.
… that content The presented hit test takes the document's elementFromPoint, which answers a shadow host for anything inside its open root, and compares with Node.contains, which stops at the boundary: a recognizer on a control inside a moving sheet's shadow root gets no finger, and the tap's click goes to the host. Fails until the hit test descends into open roots and compares along the flat tree.
Where a moving element is drawn away from its layout box, the host hit-tests what is drawn under the finger with the document's elementFromPoint, which answers a shadow host for anything inside its open root. The hit test now descends through each open root's own elementFromPoint and compares with the movers along the flat tree, so a recognizer on a control inside a moving sheet's shadow root gets its finger and the tap clicks that control.
…nt lets go The host's touch-action rules sit in an adopted sheet or a style element per document or shadow root, and were never removed, so a page whose last recognizer detached kept them, against spec:AST-094 FR24 (nothing touch-related remains after the last instance unmounts). Fails until a root's rules go with its last marked element.
…th its last marked element The touch-action writer records which elements it marked in each document or shadow root, and when the last one is unmarked (its last recognizer detached) it removes that root's adopted sheet or style element, so nothing touch-related remains after the last instance unmounts (spec:AST-094 FR24). A later mark writes the rules again; an element marked in another root first lets go of the old one.
… the inline style it reads - The motion registry, the host's engine gate and the README name the presented hit test as an engine workaround (spec:AST-013 FR6) with its review condition (re-measure on each iOS release; remove it once iOS WebKit hit-tests a compositor-moved element where it is drawn) and its regression fixture. - While a curve runs the animator holds the value it starts from inline, so presentation.ts and the README no longer say the inline style holds the end.
…core/internal/touch Core ships the internal entry @astryxdesign/core/internal/touch, which re-exports the gesture primitives for Astryx's own packages, and the Lab-import guard fails a Lab import of core outside core's exports (architecture:react-component-runtime INV13). The README and the barrel's header named internal workspace imports and said nothing here is exported; they now name that entry.
…ill needs the element, and the listeners that cancel The release note, the stream's header, the host's comment and the README said the stream is one passive listener per event type and that a document-capture stopPropagation no longer starves the recognizers. Such a stop never hid a listener on the same node, and some subscriptions cancel. They now say what changed: an app's window-capture stopPropagation, or a document-capture stopImmediatePropagation registered before core, no longer hides a touch; a recognizer that must claim still needs the touch to reach its element; the gesture host's touchend, click and contextmenu cancel while a claimer is attached and the page hold's touchmove while it holds; and the stream stays ahead of an app's window-capture listener only until a type's listener is re-added. The README's list of blocking touchmove listeners gains the claimed touch's target and the document while a claiming element moves.
…default it moves
The {stylexOptions} form of astryxStylex now passes StyleX
treeshakeCompensation: true unless the caller sets it. spec:AST-017 FR3
asks the release note to name the old and new default and the option
that restores the old one; it now does.
…er and for the two stops the input stream fixes - A still finger with sub-pixel jitter over a scroll view keeps its tap and scrolls nothing (spec:AST-094 FR18–FR22). - A tap is still recognized under an app's window-capture stopPropagation, and under a document-capture stopImmediatePropagation registered before core's: the recognizers hear touches on the window's capture phase. Chromium and WebKit pass.
…s once Where nothing that claims is attached, the host's touchend listener is passive and cannot cancel the engine's own click after a retargeted tap clicks what is drawn. The shared click guard swallows that late click whatever is attached; this case pins it, as asked on meta#8293.
…esture host's subscriptions The gesture host subscribes no click: the one cancelling click subscriber is the click guard's, added at a window's first arm and kept. The release note, the stream's header and the gestures README listed click with the host's touchend and contextmenu; they now name the guard's own.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: component:Carousel leaves snapping uncontracted, so an engineering owner needs to decide whether Carousel's own record defines touch paging on hasSnap with buttons (with no presentation value) and hasButtons={false} as its way back to native touch scrolling, or what should change, and record that by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Independently of that decision, these need fixing:
- Land or retarget the 13 open predecessor pull requests and merge
main, so the diff is the 2,127-line Carousel change rather than 34,244 lines. - Under the touch pager a trackpad or mouse wheel moves nothing (
spec:AST-094FR43,spec:AST-017FR59), although the Changeset says these inputs keep native snapping. - The take-over reads
scrollLeftafteroverflow: cliphas applied, so the carousel returns to its first child (useCarouselPaging.ts:168-175). - The pager plays its paging duration, curve and settle as literals instead of motion roles (
spec:AST-094FR4), andscripts/touch-support-bundle.test.mjsfails at this head. - Add browser cases for
component:CarouselFR2 and FR3 on the pager path, and real-device evidence for the iOS Safari and Android Chrome claims.
[Automated review]
| return undefined; | ||
| } | ||
| // Take over from native scrolling on the child it showed. | ||
| const nativeOffset = Math.abs(viewport.scrollLeft); |
There was a problem hiding this comment.
This reads scrollLeft after the render that enabled the pager has applied overflow: clip (Carousel.tsx:757). A clipped box is not a scroll container, so the read is 0 and nearestPage(0) returns the carousel to its first child after a swipe before hydration, on a pointer change, or when hasButtons or hasSnap turns on at runtime. Recording the native offset before the clip commits, or applying the clip after this read, keeps the child the person was on.
| padding != null && paddingStyles[padding], | ||
| hasSnap && styles.snap, | ||
| hasSnap && !isTouchPaging && styles.snap, | ||
| isTouchPaging && styles.pagingTouchScroller, |
There was a problem hiding this comment.
Under this clip nothing maps wheel input to the pager, and handleWheel (:469-484) calls scrollBy on a box that cannot scroll, so wherever the primary pointer is coarse a trackpad swipe or Shift + wheel moves nothing. spec:AST-094 FR43 says a pan also takes wheel and trackpad scrolling, and the Changeset promises these inputs native snapping.
| durationMs: paging.animatedScroll.durationMs, | ||
| curve: paging.animatedScroll.curve, |
There was a problem hiding this comment.
spec:AST-094 FR4: “A component MUST read touch motion through these roles and MUST NOT play a literal.” This duration and curve, and the integrator the settle plays at :857, come straight from motion/uikit/paging.ts, which is never registered through registerMotionTable; registering the paging table as internal roles (DEC-5) puts them where FR4's role tests and the theme's tempo apply.
| import {mergeProps, rtlStyles} from '../utils'; | ||
| import {useTouchPresentation} from '../utils/touchPresentation'; | ||
| import {reducedMotionFade} from '../motion/reducedMotion'; | ||
| import {useCarouselPaging} from './useCarouselPaging'; |
There was a problem hiding this comment.
With this static import, a bundle of packages/core/src/index.ts takes bytes from packages/core/src/gestures/, so the “is absent from the root entry” case in scripts/touch-support-bundle.test.mjs fails at this head, as the description says. Landing or retargeting onto #8671, or changing that test here with its reason, keeps the suite green.
…e it overflows (#8155) ## User impact A keyboard user who tabs onto a Carousel's scroll area lands on an element with no role and no name, so a screen reader announces nothing. The same unnamed stop appears even when everything fits. The edge buttons hand focus here when their direction runs out (component:Carousel FR2). ## Evidence Chromium, keys captioned: Tab rings the › button, a second Tab focuses the scroll area, → → → ← scroll it. Its ring is the browser's default outline.  - Before: the binding's `scroller-keyboard` state failed `carousel.scroller.role-and-name`, recorded as a known failure. After: it passes 8/8 with no known failures, and axe reports 0 violations on every Carousel story. - New unit tests fail without the change: the named group, the tab stop following overflow, and the tab stop following slides added, removed or grown with the box unchanged. Another pins the translated default name. - In Chromium and WebKit, before the re-measure, a fixed-width carousel's tab stop went stale as its content changed, and Scroll right stayed disabled after a slide was added at the end; both follow now. ## Problem and solution fit - The scroll area is `role="group"`, named like the carousel (the caller's `aria-label`, else the translated "Carousel"), so landing on it says where the user is. - It is a tab stop (`tabIndex={0}`) only while its content overflows; while it fits it is `tabIndex={-1}`, which keeps the edge buttons' programmatic hand-off (FR2) working. - The overflow is measured again when the content changes inside an unchanged box: `useScrollOverflow` now observes each child's box, children added or removed, and the container's class and style (`spec:AST-025` FR5). The edge buttons and fades, and TabList's fades, follow the same measure. ## Expected behavior and authority `spec:AST-025` FR12: a viewport that takes keyboard scrolling joins the tab order only while it overflows, with a role and an accessible name; as #9088 states it, a `group` named by the component that owns it, the caller's label, else its translated default. `component:Carousel` FR2 names it the hand-off target, out of the tab order when the content fits. ## Smallest restoration - Three attributes on the scroll area; `useScrollOverflow` also observes the content, with its API unchanged. - `Carousel.doc.mjs` and the Carousel record's review table describe the new behavior; the binding's known failure is removed. - Tab entry does not delegate to a first link or button (FR12's optional path); that needs #8111's shared delegation. ## Writing rubric results R1 narration: a projection row called the gap pending; it now states the behavior. R2–R6: none. ## Scope - [x] One defect is restored. - [x] Separable contradictory or unsettled tagalongs were removed or split. - [x] No new public API, visual representation, default, or interaction model is hidden under the bug-fix label. - [x] Public text and artifacts contain no internal Meta context. ## Testing Local gates at a2184a1: - `pnpm exec vitest run packages/core/src/Carousel packages/core/src/hooks/useScrollOverflow.test.tsx packages/core/src/TabList`: 5 files, 158 tests passed. - `pnpm exec playwright test packages/core/src/Carousel/__tests__/Carousel.a11y.chromium.spec.ts`: 8 passed. - `pnpm a11y:audit -- --components core/Carousel`: 0 violations, gate passed. - `pnpm test:ui`: 16,462 passed. The node project: 14,563 passed; its only failures are `packages/cli/foundation/release/cache.test.mjs` (fixed by #8520) and `apps/storybook/.storybook/story-tree.test.ts` (#8509), unrelated to this change. - Root build and typecheck, `CI=true pnpm lint`, `pnpm check:knowledge`, `pnpm check:changesets` and its coverage, the Storybook build, the interaction config (136 passed), `pnpm check:repo`, Prettier: clean. - Size: 10 files, 321 lines added, 43 removed. Sibling: #7906 and #8153 also edit Carousel.tsx; whichever lands second rebases.
On #8146. Review only this PR's change: vjeux/astryx@bb3b76b...8b21dbb Sibling: #7906 also edits Carousel's edge buttons.
User need
A flick carries a snapping Carousel across several items. Now, under a finger, a
hasSnapcarousel with buttons pages one child per swipe. No new prop.Evidence
Recorded on the iOS 26.2 simulator (P9: the Android 15 emulator, Chrome 124; P6: Chromium), Paging stories; a real-device check is the release gate. Red ring: the finger.
hasLoop: Prev on the firstpaddingkeeps its insetProblem and solution fit
handleRefandhasLoopkeep their native steps; the edge fade follows the page.What existing
hasSnapcarousels seeWith the buttons, a fast swipe moves one child instead of coasting, and nothing scrolls natively (no
scrollevents);hasButtons={false}keeps native scrolling, as the changeset says.Current specification
Carousel.spec.md(current) contracts the edge buttons (FR1–FR4, kept);spec:AST-094FR13, FR19, FR24;design:reduced-motion. Nopresentationprop: touch paging is Carousel's own touch-only behavior (public-component-api INV16's carve-out). Proposed for Carousel's record, pending owner review:Public delta and caller burden
No new API:
hasSnapgains the touch paging above.Design and alternatives
hasPaging(earlier heads) was dropped forhasSnap.Scope
Testing
scripts/touch-support-bundle.test.mjs"is absent from the root entry" lists the gesture modules the pager brings, because Carousel, a root export, imports its touch layer statically (architecture:react-component-runtimeINV13); the per-component form of that test is test(core): a touch component's import carries its gesture code, and nothing else does #8671.pnpm exec vitest run packages/core/src/Carousel: 65 passed, the paging suite moved tohasSnapwith laid-out slides. New: "cards: an arrow moves one card, the buttons about a viewport onto a card, and the last cards share the end", "focus moving into a card already on screen leaves the track where it is", "the edge fade follows the page: the end at the first, the start at the last", "with hasLoop, a step past either end goes to the other one, as it does natively", "without the buttons it snaps natively, so a single pointer can still page (WCAG 2.5.1)".pnpm test:interaction packages/core/src/Carousel: Chromium 16 passed (twice); WebKit 14 passed, 2 skipped (screencast clock; injected touches don't scroll Linux WebKit natively), three runs. New: "cards: a flick moves one card, and the last cards rest where the row ends", "padding keeps its inset: a flick lands the next child at the padded start edge", "the edge fade follows the page", "with hasLoop, a swipe past the last page rubber-bands back, and Next there returns to the first". This run caught the clipping scroller growing to its content's width (aclipbox is not a scroll container, so its automatic minimum width is its content's); it now takesmin-width: 0.pnpm a11y:audit -- --components core/Carousel: 0 new violations.CI=true pnpm lint(0 errors), the internal-module guard,pnpm check:repo, Prettier,pnpm check:changesets, and the CLI's docs tests (docs graph, prop docs, doc types and quality, topic refs, experimental isolation; 85): clean..github/scripts/lib/component-size.js, built package, gzip), chore(core): a pager that pages like a UIKit paging scroll view #8146 against this head: JavaScript 51,721 → 84,691 B, CSS 10,431 → 10,452 B.Stack: #8145 → #8146 (on #8360), then this PR.