Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: the player’s approach fits the compositor and Reduce Motion rules, and the new landing test fails before the fix and passes after, but two things need to change first.
crossFade()times its midpoint and end from the call rather than from when its animations start (wheelMotion.ts:402-430), so with the main thread busy 120–160 ms after the call the rows swap from the still-visible origin to the fully opaque destination in one frame; re-arming those timers from the first animation’sstartTime, asplay()does, with a unit case for a late start, would fix it.- Against
mainthis pull request still carries the unmerged #8156 stack (28,540 of its 29,270 authored lines), so it needs #8156 and its predecessors to land and this branch to be updated before it can merge on its own.
[Automated review]
| motion.timers.push( | ||
| window.setTimeout(() => { | ||
| if (this.motion === motion) { | ||
| onMidpoint?.(); | ||
| if (!baked) { | ||
| this.render(to, true); | ||
| } | ||
| } | ||
| }, durationMs / 2), | ||
| window.setTimeout(() => { | ||
| if (this.motion !== motion) { | ||
| return; | ||
| } | ||
| this.finish(motion); | ||
| this.restingOffset = to; | ||
| this.render(to, true); | ||
| onEnd?.('arrived', to); | ||
| }, durationMs), | ||
| ); |
There was a problem hiding this comment.
These two timers are armed from the call, but the animations above start only when the browser next commits a frame. play() re-arms its timers from the first animation’s startTime once ready settles (shiftOrigin, lines 444-454); nothing does that here, so when the main thread stays busy after the call the end timer cancels the fade and the hold-then-step animations before the compositor reaches the midpoint. In Chromium, with the main thread busy 160 ms after the call, the rows went from the origin at opacity 0.50 to the destination at opacity 1 in one frame. Re-arming both timers from the first animation’s startTime, with a unit case whose mocked animate() resolves ready with a later startTime, would keep the landing a cross-fade.
There was a problem hiding this comment.
Fixed in ce4e3c1: the cross-fade's midpoint and end are now part of schedule(), and followStart re-arms them from the first animation's startTime through shiftOrigin, as play()'s end is; a timer that comes due while the animations still wait for the frame that starts them waits for that start. "Times the midpoint and the end from when its animations start, not from the call" holds the start 250 ms past the call with a mocked ready and a later startTime; without the fix the midpoint and the end ran before the fade began. In Chromium, with the main thread busy 160 or 260 ms right after a reduced-motion tap, the rows now land at opacity 0.00, at the fade's bottom (before the fix: 1.00, in one frame).
| easing?: string; | ||
| /** What fades: the drawings of the rows. */ | ||
| fade: ReadonlyArray<HTMLElement>; | ||
| /** Called just before the rows land, at the midpoint. */ |
There was a problem hiding this comment.
With the landing baked, the rows step on the compositor while onMidpoint still waits for a main-thread timer, so a busy main thread runs it after the rows have landed, and a late start runs it well before the midpoint. Consider saying it runs on a main-thread timer near the midpoint, here and in the crossFade comment at line 330.
There was a problem hiding this comment.
Fixed in ce4e3c1: onMidpoint and the crossFade comment say it runs on a main-thread timer near the midpoint, timed from when the fade starts on screen, and that a busy main thread can run it after the rows have landed on the compositor.
meta-cla
Bot
added
the
CLA Signed
# Conflicts: # packages/core/src/DateInput/wheelMotion.ts
There was a problem hiding this comment.
Changes requested, for scope only: the cross-fade timing fix holds up (the new late-start test fails on the previous player and passes now, and in Chromium the rows step at opacity 0 through 160 and 260 ms of main-thread work), and no further change to the player was found necessary. Against main this pull request still carries #8320, at an older head than its current one, and #8156: 28,180 of its 29,020 authored lines. Once those two land, updating this branch so it holds only wheelMotion.ts, its test and the changeset should clear it.
[Automated review]
# Conflicts: # packages/core/src/DateInput/wheelDrum.ts
There was a problem hiding this comment.
Changes requested, for scope only: the player itself (wheelMotion.ts, its test and the changeset) needs no change. Against main, this pull request still carries #8320, at an older head than its current one, and #8156, which together make up 28,289 of its 29,129 authored lines. Once those two land, updating this branch so it holds only the player's three files should clear it.
[Automated review]
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsDateInput (@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. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
…that Effect The isEnabled sync's silence covered recognizer listeners but not usePressTracker: a press-tracked control inside an element whose double tap a rerender turns off heard touchUpInside (and its un-highlight) from that layout Effect (react-component-runtime INV2). Silencing it would leave the control highlighted, so the tracker's callbacks that arrive during a sync run in a microtask just after it, in order: the press still ends and un-highlights, outside the Effect. Tests, failing before (the events arrived inside the Effect): a press waiting on an ancestor double tap, alone and with a single tap waiting on it, reports nothing from the Effect that turns the double tap off, then touchUpInside and the un-highlight just after. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
…ile it ran A page opened after startCompositorTrace and closed before stop() was still credited: its frames are in the browser-wide record, and neither check saw it. The record now watches for new pages from start to stop, in the page's own context and through the browser's target discovery in every context, and stop() throws if one opened. Negative controls: a page opened and closed mid-record, and one in another context, both make stop() refuse; alone, it records.
…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.
Changes requested. This branch still carries #8340, #8360 and #8156 (29,258 of its 30,158 authored lines), so once those land, please update it so the compare against main holds only the player, its test and the changeset, and refresh the description's dependency list, since #8320 is closed.
WheelMotionre-implements what the shared animator already does (keyframe baking, read-back, stop, start-following, end hold, Reduce Motion cross-fade), whicharchitecture:react-component-runtimeINV13 places in that one module: play the rows through the animator with the rotation channel the drum needs, or have an engineering owner amend INV13.- Release ticks run on main-thread timers that fire together after a long task (
wheelMotion.ts:489-506), a haptic outside UIKit's moment under AST-094 FR23: skip late ticks, apply the 40 ms interval when they fire, and add a case for it.
[Automated review]
| * motion.play(planRelease(motion.offset(), 900, layout).curve, {ticks: true}); | ||
| * ``` | ||
| */ | ||
| export class WheelMotion { |
There was a problem hiding this comment.
INV13: WheelMotion is a second compositor engine: it bakes curves into Web Animations, reads them back, stops them, follows their start, holds their end, promotes its rows and cross-fades on its own, while architecture:react-component-runtime INV13 has Core's touch behavior come from one shared set of modules, the animator among them, with a component's own touch logic as small glue over them. The animator has no X-axis rotation channel (motion/animator.ts:189-190), so conforming means adding the channel the drum's rotateX rows need and playing them through it, or having an engineering owner amend INV13.
| window.setTimeout(() => { | ||
| if (this.isPending(motion)) { | ||
| return; | ||
| } | ||
| this.lastTick = now(); | ||
| playHaptic('selection'); | ||
| }, delay), |
There was a problem hiding this comment.
AST-094 FR23: each release tick is its own timer and checks only isPending. After a long task that starts once the motion runs, every tick that came due fires back to back as the block ends; each navigator.vibrate call cancels the last, so one buzz plays after the rows have passed the band, outside UIKit's moment. Skip a tick that runs more than picker.selectionTick.minimumIntervalMs past its due time (against performance.now()), apply tickBetween()'s interval check here, and add a case that holds the timers past two tick times.
Depends on #8156 (on main) and #8360 (for
gestures/haptics). Review only this PR's change: vjeux/astryx@045f666...5fffebe (the player iswheelMotion.ts, its test and the changeset; the rest is #8156 and #8340's lines, which #8360 lacks).User need
On a phone, a picker wheel animated from the page's main thread freezes whenever the page is busy, then jumps: with the thread blocked for 700 ms, the next row reached the band only when the block ended. iOS keeps its picker moving through that. This player keeps the touch wheel (#8159) moving and landing on its row however busy the page is.
Problem and solution fit
What led to it: Astryx's UIKit-parity work (
spec:AST-094) asks phone motion to keep running through a busy main thread (FR5), as iOS's does. #8156 computes the wheel's curves; this plays them.DateInput/wheelMotion.tsowns a wheel's offset. A drag writes rows frame by frame; any other motion is baked into per-row Web Animations, so the compositor plays it to the end without the main thread. It reads back the offset on screen and stops a motion where it is. Under Reduce Motion the wheel cross-fades instead, a landing baked the same way.Current specification
spec:AST-094FR5, FR6, FR8;design:reduced-motionDR2.Public delta and caller burden
None: internal, unused until #8159.
Design and alternatives
requestAnimationFrameloop. Simplest, but it stops whenever the main thread does.Evidence
With the main thread busy 160 or 260 ms after a Reduce Motion tap, the rows land at opacity 0.00, not 1.00, in 6 of 6 Chromium runs each. #8162 holds the played motion to #8156's plan within 0.07 px in Chromium and 0.06 px in WebKit.
Scope
Testing
pnpm exec vitest run packages/core/src/DateInput/wheelMotion.test.ts. All pass. Three cases fail without their fixes: a late timer no longer moves the rows (the landing is baked), the fade's midpoint and end count from when it starts on screen, and a finger landing on a motion that hasn't started catches it at its start.Since the review on 0c17a38 (now 5fffebe): only the gesture foundation moved to #8360's and #8340's current heads; the player is unchanged.
Merge order: #8360 and #8156, then this PR, then #8159.