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
Generated by PR Enrichment workflow | View current CI run |
![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
|
Posted by Cindy's agent on her behalf: closed and reopened only to restart this PR's CI on the current workflow after tonight's queue-wide cancel; nothing in the PR changed. |
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.
meta#8360's change on the internal touch foundation: one click guard shared by the menu press and the gesture host (its click listener stays once the menu press arms it, as its record has it, and lasts only while armed for the gesture host, FR24), the host as the one touch owner (finger scroll, page holds, attachments, the press tracker hook and a held control's kept touch, the long press's single source, presented hit testing on iOS WebKit), its cases in the gesture browser proof (Chromium and WebKit) and the README. It carries meta#8074, meta#8270, meta#8293, meta#8315, meta#8320 and meta#8356, and folds in meta#8358 (the long press's tolerance comment and an unused import). Rebased onto main after section 2 as one commit; the touch-action rule came with meta#8071. Where fingers come from their pointer events, a claimed finger's touchend is cancelled from its element (FR21), under a fine primary pointer wherever the browser sends Touch Events and inside an open shadow root too; at most eight such lifts wait for their touchend, and each goes once taken or after 500 ms, kept across the host's uninstall, which can run in the microtask after the pointerup, before the touchend that comes later in the same task (FR24). An armed press's page listeners go once the arena has decided its last touch, a tap that waits on a double tap included, and after its last detach the host holds no listener, note, frame or timer (FR24). The animator's busy proof is judged against the harness's compositor-only control (meta#8906), with a main-thread mutant as its negative, and the spring's curve is placed against the control's own clock, so the late frames a loaded runner sends fail nothing. With the host serving every pointer (meta#8071), this change's own pieces follow it: a press tracker takes a mouse press as a press, painting at once and braking no scroll (FR11); a finger's or a pen's hold that does something keeps text unselected under any primary pointer, while a mouse keeps its released behavior (FR41); and `perEventPointer`, from before FR43, changes nothing. These come from meta#8665, which keeps only the undecided rule for text selection during a mouse or pen drag. The hover recognizer and the host's hover routing land with hover's first user. Co-authored-by: Cindy Zhang <cindyxz@meta.com>
…finger landed On iOS WebKit the host resolves moving content at touch-down and asks the engine what is under the finger. When a busy main thread hands it the touch later, the content has moved on: the host now asks at the point where what was under the finger at the touch's timestamp is drawn, so a tap on a moving sheet's Done still clicks Done (spec:AST-094 FR11). A presented source reports its offset at a given time for this. The presented browser proof drives the sheet with Core's animator and a critically damped spring, up into place and down off the screen over the scrim: a tap on the drawn Done clicks it once, and a drag on the drawn sheet catches it where it is drawn, in both directions.
…the finger (FR11, FR19); INV13 names the shared input listener Where the engine hit-tests an element a compositor animation moves at the motion's start (iOS WebKit), a touch on that content goes to what was drawn under the finger at touch-down, and a tap there activates that element with one click while the browser's own click is cancelled or swallowed (FR11). While such content around a recognizer that claims moving touches is moving, the document may carry a non-passive touchmove listener that cancels only owned moves and comes off when the motion ends; while it is on, a native scroll waits for a free main thread (FR19). The gesture host, press feedback, page hold, finger-scroll watch and click guard hear document-wide input through one capture-phase listener per event type on the window (architecture:react-component-runtime INV13).
…view fixes A touch the host handles late now also leaves alone content that slid under the finger after it landed: the touch goes to what was beneath it then (spec:AST-094 FR11). The input-stream changeset says what moved: the gesture host's listeners, and the press feedback's scroll and visibility listeners, since its pointer listeners were already on the window's capture phase. The coarse-pointer gate reads TOUCH_QUERY from touchQueries.ts instead of spelling it again. The WebKit interaction job's scope covers every module the gesture host now bundles: hooks/isRtlElement and utils' flatTree, hapticSwitch, inputStream, clickGuard, selectionLock and coarsePointer. The scope test's example of a util outside it moves from clickGuard, which the gesture fixture now bundles, to focusReturn. The presented proof checks that the sheet moves on a spring (its speed changes along the animator's curve) and no longer compares the drawn box with the animator's offset: under a 3-CPU cap the two disagree by 3 to 27 px in about half the frames whatever reference is used, while the tap and drag cases go through the host's real path.
…e origin most frames agree with Under the CI lane's WebKit profile (four CPUs, one worker), "a spring keeps advancing every frame through a 1 s main-thread block" failed in two ways, each about once in a few hundred runs, and a failing run's frames show each cause: - The spring drifted 138.6 ms against the control. The control's clock took its origin from the first usable frame alone, reading that frame's direction from the next usable one: the frame sat at x 210 on its way out, the next usable one at x 210 after the turn, and the tie read as "back". The clock now lives in controlClock.ts and takes the origin most frames agree with. On that run's own frames it reads 6.5 ms (bound 12), and its unit case for that layout fails on the old reading (150.7 ms astray). - The spring "moved on 61 of the 62 frames the control moved". At the block's end it was in its tail, under two of the cast's 1 px steps a frame, where a frame delivered early shows no step and the next catches up two. The harness now reads such a tail at its own pace (busyWindowProof), and the proof judges the whole block: on that run's own frames the earlier proof fails it and this one passes. No bound changes.
… (architecture:public-component-api INV23) Main's lint rule now keeps every private data attribute in its owner module. This change had moved `data-astryx-haptic-switch` from the haptics module to utils/hapticSwitch.ts, so the click guard could skip the hidden switch without importing haptics (architecture:react-component-runtime INV13). The name now lives in privateAttributes/hapticSwitch.stylex.ts, modelled on segmentDisabled.stylex.ts: it exports markHapticSwitch, which the haptics module uses on its hidden switch, and isHapticSwitch, which the click guard reads. utils/hapticSwitch.ts and the deprecated gestures/hapticSwitch.ts re-export go, the haptics module's baseline entry goes with its raw name, and the WebKit interaction job's scope covers the new module. No behavior changes.
…y the gesture host and the selection lock The selection lock and the gesture host each kept a copy of two helpers: the iOS WebKit engine check, and the walk that asks whether a node is inside an element along the flat tree. The engine check now lives in utils/touchQueries.ts, the React-free half of touchPresentation.ts, as IOS_SUPPORTS_QUERY with one isIosWebKit(view) reader; touchPresentation.ts re-exports the query, so its export is unchanged. The walk is containsAlongFlatTree in utils/flatTree.ts, beside parentAcrossShadow. Unit cases cover both in their new homes. No behavior changes.
No Core or Lab component attaches a gesture recognizer yet, so the input-stream changeset's fix entry told consumers about a change they cannot meet. It now keeps `[fix]` for what ships, touch press feedback hearing scrolls and page hides on the window's capture phase, and a new `[chore] Internal:` changeset carries the recognizers' shared stream and its passive-unless-cancelled listeners.
…trl-wheel (spec:AST-094 FR43) meta#8676's reviewed change, rebuilt as one commit on meta#8360's head: a wheel gesture drives the recognizers that allow its scroll type as a contact moving with the content, a pan along its own axis once every scroller between it and the page is at its edge, and a ctrl-wheel drives a pinch. A screen-edge pan stays a finger's.
…ain input reads as a mouse wheel, and a pan can take a swipe as one step (spec:AST-094 FR43) The round-4 trackpad behaviors, as a draft for the owner to try each one: - wheelSegmenter.ts splits the wheel stream into gestures and reads a trackpad's lift: once a run of shrinking deltas has lost a fifth of its size, momentum began. A pan the host follows ends there at the fingers' travel, released at their last move with their speed, and the momentum after it never moves the pan. Fingers that stop release still, 180 ms (vertical) or 220 ms (horizontal) after their last wheel. - Input the deltas cannot place reads as a mouse wheel: a delta a hair off a whole pixel, a scroll with no legacy wheelDelta (an engine that sends none), a legacy wheelDelta that is not three times the delta, or a 120 tick over a 40 px delta. - A pan created with wheelStep takes a trackpad swipe as one step: a short flick once the swipe travels 24 px, a slow swipe's travel carried across pauses, its slow tail absorbed, and one undo back to rest. A mouse wheel's gesture and a pinch keep their 100 ms end. In Chromium the harness's wheels arrive as notches (wheelDeltaY -120 whatever the delta), which now read as a mouse wheel, so the two trackpad-only browser cases run in WebKit, whose driver sends a trackpad's precise deltas. The lift's release is stamped at the fingers' last move: stamped at the lift, three held wheels later, it read as a stop and lost the fling.
…speed until the splitter decides, so it never stalls before a lift (spec:AST-094 FR43) The splitter reads a lift only after four shrinking deltas, so a pan that held them stood still for that stretch (about 64 to 130 ms) and then let go at the fingers' speed: a stop, then a go. Through the undecided stretch the pan now moves on from where it last was at the fingers' last speed, never past what that speed projects for the time since. At the lift it keeps that travel and lets go there, still moving at that speed, and nothing more moves. When the fingers carry on, their moves absorb what the stretch showed past their travel or short of it, half a move at a time, so the pan never steps back and ends on their travel; fingers that stop without a lift end on their travel where it runs ahead.
…t, and never a mouse wheel (spec:AST-094 FR43, DEC-24) The merged record decides it: a trackpad's two-finger scroll is direct manipulation, so a pan follows it where its record allows it; a mouse wheel's notches are a scrolling control, so a pan takes none, and a scroll without the trackpad signature, Shift+wheel included, counts as a wheel's. - A pan's allowedScrollTypes takes only 'continuous', and none by default, as UIKit's allowedScrollTypesMask: the sheet, the pager and swipe rows opt in. The host never feeds a wheel's notches to a pan, whatever a caller passes. ScrollType keeps 'discrete' for the pinch, which takes a ctrl-wheel from a trackpad or a mouse. - A scroll with Shift held reads as a mouse wheel's. - wheelStep goes: DEC-24 rejects a discrete step per trackpad swipe for rows and paging, and nothing used it. - The splitter's verification streams: ten swipes that each start in the last one's momentum, ten whose momentum runs out, and a slow drag with 150 ms pauses count ten, ten and one gestures.
UIKit's paging values for touch presentations: the thresholds that pick the page a release settles on, the settle that carries it there and setContentOffset(animated:), measured on iOS 26.2 and replayed against the recorded frames to under half a pixel. Not exported from the package entry point.
A pager for touch presentations. A horizontal pan moves the pages one to one and rubber-bands at the ends. The release picks the page with UIKit's rule and settles on UIKit's paging curve on the compositor, and a finger can catch it mid-flight. Not exported from the package entry point. The pan and the catch attach for every pointer (spec:AST-094 FR19, FR43). A mouse drags the track as a finger does, its short fast drag paging as a finger's flick does, and a mouse that catches a settle locks no text selection (the selection lock reads the pointer kind alone). The pan follows a trackpad's scroll along its axis and never a mouse wheel's (DEC-24): notches, lines, a whole tick and an accelerated mouse's deltas stay the page's. A trackpad's fling releases where the fingers lift, at their speed, and a scroll that stops releases still, as the record's slow 60 px swipe does. A vertical wheel stays the page's. Under Reduce Motion a release from a mouse, a trackpad or a pen follows DR2 on a coarse primary pointer and DR3 otherwise. The trackpad browser cases run in WebKit: Chromium's CDP wheels read as a mouse's wheel. A trackpad swipe moves the track at most one page from the page it began on, across a loop too; a pager that does not loop keeps the rubber band of its first and last pages. A trackpad swipe that moved under a tenth of a page releases still, whatever its speed: a speed spike is not a flick, while a finger's and a mouse's flick keep their speed. A swipe that lands while the last swipe's settle runs begins from that settle's page, so back-to-back swipes page on (spec:AST-094 FR43).
Paging rides the existing `hasSnap`. Under a finger (a coarse primary
pointer), a `hasSnap` carousel with its buttons is a UIKit paging scroll
view whose pages are its children: the shared pager (`pages: 'children'`)
moves the track one child per swipe, each child resting at its start edge
as the native snap places it (padding kept, the last children where the
row ends), following the finger, rubber-banding, settling on UIKit's curve
and catchable. A mouse, a trackpad and the keyboard keep native snapping;
so does a finger without the buttons, the single-pointer way to page (WCAG
2.5.1).
What existing `hasSnap` carousels see under a finger: a fast swipe moves
one child instead of coasting across several, and the content no longer
scrolls natively there (no `scroll` events, `scrollLeft` 0);
`hasButtons={false}` keeps native scrolling. The buttons and the handle
keep their step (about a viewport, landing on a child); an arrow key moves
one child; with `hasLoop` a step past either end goes to the other, as it
does natively (no continuous loop), and a looping move's slides wait where
the pager goes (its own nearest-copy rule, so with two slides Next goes
forward). The edge fade follows the pager's page.
A snapping carousel with buttons holds its items in a track on every
pointer, so hydration never remounts them; a clipping scroller takes
`min-width: 0` to stay the carousel's width. The layers for the children
near the page on screen divide an ancestor's CSS zoom out, as the pager's
measure does.
Tests: a jsdom suite and a browser spec on `hasSnap` (full-width pages and
a card row: one card per swipe, the end clamp; padding's inset; the fade;
looping as a rewind; two looping slides; a busy main thread). Its holds
and storyboards take the motion at its first frame (the harness's
`armHold`), so a loaded runner cannot miss a short curve. Carousel joins
the touch-support bundle check's gesture users: its import carries the
pager's pan. Docs, stories (Paging, Paging (Loop), Paging (Cards), Paging
(In a Long Page)) and the changeset say the same.
vjeux
changed the title
8b21dbb to
b6ed037
Compare
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Approval waits on an engineering owner's decision: component:Carousel does not yet define touch paging, so an engineering owner needs to decide whether Carousel's own record takes paging on hasSnap with buttons (with hasButtons={false} as the only way back to native touch scrolling), and whether the spec:AST-094 FR11/FR19 and architecture:react-component-runtime INV13 sentences this branch adds should be approved here or land through #8360. They record that by commenting /approve-spec <full head SHA> on this pull request, or say what should change; the author may do so as an engineering owner. Whatever the decision, these need fixing:
- Land or retarget the four open predecessors (#8360, #8676, #8145, #8146) so the diff is the 2,077-line Carousel change rather than 27,803 lines.
- The take-over reads
scrollLeftafteroverflow: cliphas applied, so a carousel scrolled natively jumps back to its first child when the pager starts (useCarouselPaging.ts:168-175). - Under the pager a mouse wheel (Shift + wheel, tilt) moves nothing, contrary to
spec:AST-094FR43, while the Changeset, JSDoc and docs say a mouse, trackpad and keyboard keep native snapping. useCarouselPagingrebuildsusePager's option sync and attach instead of calling the shared hook, and the pager's paging motion is not a motion role (spec:AST-094FR4, in #8146).- 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.
By the time this layout effect runs, the same commit has applied overflow: clip (Carousel.tsx:765), so the scroller is no longer a scroll container and scrollLeft reads 0: nearestPage(0) returns the first child. Reproduced in Chromium on the With Snap story: scrolled natively to slide 3, then the primary pointer turned coarse, and slide 1 showed. Read the offset before the clip commits (or from the scroll listener useScrollOverflow already runs), and add a browser case that scrolls natively, enables the pager and expects the same child.
| padding != null && paddingStyles[padding], | ||
| hasSnap && styles.snap, | ||
| hasSnap && !isTouchPaging && styles.snap, | ||
| isTouchPaging && styles.pagingTouchScroller, |
There was a problem hiding this comment.
Under this clip a mouse wheel cannot move the carousel: handleWheel calls scrollBy on a box that cannot scroll, and the pager's pan follows only trackpad scrolls. Three Shift + wheel notches moved a fine-pointer carousel to slide 3 and left the coarse-pointer one on slide 1. spec:AST-094 FR43: “a mouse wheel scrolls what is under it.” Sending a mouse wheel's horizontal steps to the pager while it runs would fix it.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] Carousel: under a finger, a `hasSnap` carousel with its buttons pages like an iOS paging scroll view. Each swipe moves one child and settles on that child's start edge on UIKit's paging curve; the content follows the finger and rubber-bands at the ends, even while JavaScript is busy. A mouse, a trackpad and the keyboard keep native snapping, and so does a finger without the buttons, because the buttons are the single-pointer way to page. |
There was a problem hiding this comment.
“A mouse, a trackpad and the keyboard keep native snapping” holds only with a fine primary pointer: where the primary pointer is coarse, trackpad swipes and arrow keys page through the pager and a mouse wheel moves nothing. Say “with a fine primary pointer” here, in the hasSnap JSDoc and in Carousel.doc.mjs.
| const [pager] = useState<Pager>(() => | ||
| createPager({ | ||
| count: Math.max(1, count), | ||
| pages: 'children', | ||
| onIndexChange: () => pageStore.notify(), | ||
| onPagesChange: () => pageStore.notify(), | ||
| onSettle: () => promote(), | ||
| shouldBeginPan: () => { | ||
| promote(); | ||
| return true; | ||
| }, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
This builds the pager with createPager and then re-implements usePager's option sync (hooks/usePager.ts:1341-1343) and attach (:1347-1352) in two layout effects below. Two paths will drift. Giving usePager an attach toggle, a take-over offset and a subscribable page would let Carousel call it once.
On #8146. Review only this PR's change: vjeux/astryx@464de7d...b6ed037 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
Simulator and emulator clips (iOS 26.2; P9: Android 15, Chrome 124; P6: Chromium), Paging stories; real-device capture plan: #9335; whether merge needs real-device evidence is with the spec owner. 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
test:ui17,917 passed, the node project (2 failures incache.test.mjs, the same on feat(core): a pan takes wheel and trackpad scrolling, and a pinch a ctrl-wheel (spec:AST-094 FR43) #8676's head), changesets, Storybook build,pnpm a11y:audit -- --components core/Carousel(no new violations), check:repo, Prettier on the changed files (Carousel.doc.mjskeeps main's one-line entries, which Prettier reflows on main too); Chromium: the interaction suite 296 passed, 6 skipped; Carousel's spec ×5 on two pinned cores, four workers: 80 passed, and its busy-thread proof ×5 at one worker: 5 passed; WebKit in a 4-CPU container (one worker,LP_NUM_THREADS=4) ×10: 140 passed, 20 skipped.LP_NUM_THREADS=3) ×10: one worker 140 passed, 20 skipped; 16 workers 110 passed, 20 skipped, 30 failed, by assertion (this spec has no load report yet): the edge fade read still at the end (8), the loop's five-flick titles (7), the storyboards' frames up to 5.1 px off the monotonic path (10: cards 7, swipe 3), Reduce Motion's resting track (5).scripts/touch-support-bundle.test.mjs, the rule test(core): a touch component's import carries its gesture code, and nothing else does #8671 set: a component that gains a gesture adds its import), 8 passed; its two red cases ("is in no public import that uses no gesture", "is in the root entry only as much as its touch components import") named Carousel before.armHoldbefore the input, so the page holds the motion at its first frame. They failed at four workers on two pinned cores (17 of 80), as the hold sent after the motion started came late. Red first, with a 2 s delay injected before each read: the previous wait-then-hold failed 5 of 5 ("no animation started … none recorded"),armHoldpassed 5 of 5.pnpm exec vitest run packages/core/src/Carousel: 69 passed. 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; WebKit 14 passed, 2 skipped (screencast clock; injected touches don't scroll Linux WebKit natively). 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..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 55,306 → 94,194 B, CSS 10,741 → 10,762 B.Stack: #8360 → #8676 → #8145 → #8146, then this PR.