Repository navigation
chore(core): the animator takes start: 'next-frame' for a call made in an input event's own task - #9261
chore(core): the animator takes start: 'next-frame' for a call made in an input event's own task#9261vjeux wants to merge 14 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:spec-owner-review
c4466e3 to
695ec9b
Compare
github-actions
Bot
removed
the
needs:spec-owner-review
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.
Changes requested for one small fix. start: 'next-frame' is set on whatever start group beginGroup returns, so a call with an explicit at that joins an earlier call still waiting for its start switches that earlier call to the next-frame rule, although the option's doc says it is ignored with at (animator.ts:247; set at :3201 and :3333). Guard the flag with at == null for both animate and play and add a unit case for the join. Separately, the description quotes spec:AST-094 FR6 for the start rule, but FR6 is "Motion is interruptible"; architecture:react-component-runtime INV13 (an internal chosen start) with FR5 is the governing text.
[Automated review]
| if (group != null && animateOptions.start === 'next-frame') { | ||
| group.nextFrame = true; | ||
| } |
There was a problem hiding this comment.
With an explicit at no later than a call from rest still waiting on these channels, beginGroup returns that earlier call's group (line 1797), so this marks a call that never asked for 'next-frame'. The option's doc says it is ignored with at (line 247). Suggest group != null && animateOptions.at == null && animateOptions.start === 'next-frame', the same guard for playOptions at line 3333, and a unit case where a {at, start: 'next-frame'} call joins a waiting call.
There was a problem hiding this comment.
Fixed in f2795a9: the next-frame flag is set only when the call has no at, in both animate and play. A new unit case has a {at, start: 'next-frame'} call join a waiting call and leave it on the browser's start after a block; it was red before the guard.
695ec9b to
f2795a9
Compare
|
On the description: fixed. It now cites |
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about three spec edits this branch carries from #8360: the new tap rule in spec:AST-094 FR11, the document-level non-passive touchmove listener in FR19, and the single window capture-phase listener in architecture:react-component-runtime INV13. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, which the author may do if they are an engineering owner. Two changes are needed either way:
- Rebase onto
mainso this pull request holds only the animator commit, which applies cleanly there with its animator tests passing; at this head the diff is about 26,000 lines from five other open pull requests and adds six public APIs the description says it does not. - Have
usePanGestureanduseDragToSnapPointscalluseAttachmentinstead of copying its attach-and-release lifecycle.
The at guard on start: 'next-frame' resolves the earlier finding.
[Automated review]
| }); | ||
| // One pan for the node behind `ref` with this axis: re-created when either | ||
| // changes, after a commit that replaced the node. | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
This Effect and the unmount one below re-implement useAttachment (packages/core/src/gestures/useAttachment.ts:68), which useGestureRecognizer and usePressTracker already call. useAttachment(ref, element => { …create the pan…; return () => { next.isForwarding = false; next.gesture.destroy(); }; }, axis) keeps one shared lifecycle.
| // creates the drag again through the factory and does nothing more | ||
| // (spec:AST-107 FR6). Destroying the old drag leaves the element where it | ||
| // is, a settle in flight included, and the new one starts there. | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
Same lifecycle as useAttachment (packages/core/src/gestures/useAttachment.ts:68): attach on the node behind ref, re-create on a key change, destroy at unmount. Calling it with axis, the serialized points and flickThreshold as the key, and storing the snap in a ref for settleTo, removes the copy.
f2795a9 to
980f0d1
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about three spec edits this branch carries from #8360: the new tap rule in spec:AST-094 FR11, the document-level blocking touchmove listener in FR19, and the single window capture-phase listener in architecture:react-component-runtime INV13; an engineering owner records it by commenting /approve-spec <full head SHA> on this pull request, which the author may do if they are an engineering owner. Two changes are needed either way:
- Rebase onto
mainso this pull request holds only the animator commit, which applies cleanly there with its animator tests passing; at this head the diff is about 26,000 lines from five other open pull requests. - Have
usePanGesture,useDragToSnapPointsanduseFingerScrollcalluseAttachmentinstead of copying its attach-and-release lifecycle.
[Automated review]
980f0d1 to
0aa861f
Compare
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested: the start: 'next-frame' option looks right and its three new cases fail without it, but this branch still carries #8676, #8653, #8655 and #8658, so the diff is about 9,300 authored lines instead of the option's 192. Please rebase onto main once that stack merges, and fix these two issues in the pull requests they come from:
usePanGesturere-implements the attach and release lifecycle thatuseAttachmentowns, whichuseDragToSnapPointsalready calls; callinguseAttachmentkeyed onaxiskeeps one lifecycle.- In both animator playing-card templates, a second Discard press during the first discard deals two cards (the ace goes straight to the queen); the pan templates' one-throw-at-a-time guard, with a double-press test, fixes it.
[Automated review]
| }); | ||
| // One pan for the node behind `ref` with this axis: re-created when either | ||
| // changes, after a commit that replaced the node. | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
This Effect and the unmount one below re-implement useAttachment (packages/core/src/hooks/useAttachment.ts), which useDragToSnapPoints already calls. useAttachment(ref, element => { let forwarding = true; const pan = createPanGesture(element, {axis, onPan: event => { if (forwarding) onPanRef.current(event); }}); return () => { forwarding = false; pan.destroy(); }; }, axis) keeps the silent unmount and leaves one lifecycle to fix.
|
|
||
| // The button moves the card off on the state-change motion: a planned | ||
| // move, which Reduce Motion turns into a fade where the card is. | ||
| const discardWithButton = async () => { |
There was a problem hiding this comment.
A second Discard press while this animate runs takes over its channels, which resolves it early, so both presses reach dealNext and the deck skips a card (ace straight to queen). The pan template's throwing guard closes the same race; a double-press case in playingCardsTemplates.test.tsx would keep it closed.
| } | ||
| // The button moves the card off on the state-change motion: a planned | ||
| // move, which Reduce Motion turns into a fade where the card is. | ||
| async function discardWithButton() { |
There was a problem hiding this comment.
Same race as the React example: two quick presses run dealNext twice, and the live region announces "K♥ discarded" for a king never shown. An isDiscarding flag cleared in dealNext fixes it.
The public animator (spec:AST-107 FR3, FR6): createAnimator(element) over the internal animator, with animate, decay, set, get, getVelocity, stop and destroy, from the package root; and useAnimator(ref), which takes the element's ref like the gesture hooks, so a gesture hook can share the element, and returns the animator. After every commit the hook follows the element behind the ref, through the attach lifecycle the gesture hooks share (useAttachment). The internal factory is renamed createMotionAnimator, along with its callers in the Slider's and the Switch's touch controllers and the pager's tests, so no internal factory shares a public factory's name. Docs, a changeset, and playing-cards templates in React and plain JavaScript, with their registrations. In both templates the card is an image named by its rank and suit, renamed on each deal; one card leaves at a time (a Discard press while a card leaves deals no second card), a leaving card takes no press, and a press on a card fading in moves only its position; and a drag the browser cancels springs back. The plain-JavaScript example mounts its deck from a callback ref on its wrapper, whose cleanup takes it down. The core API report gains the animator's rows. When the element behind useAnimator's ref unmounts, or a node replaces it, the animator's values return to rest, so a remount or a new node starts from the element's resting values. AST-107 records the owner's decision on four edge behaviors of the public animator and pan gesture (DEC-4): FR1 and FR6 state that a cancelled pan reports zero velocity, how the pan factory's destroy and the hook's unmount report, and that an opacity decay stops at 0 or 1; FR3 states the remount.
…s (spec:AST-107 FR1, FR6)
The public pan: `createPanGesture(element, {axis?, onPan}) → {destroy}` and
`usePanGesture(ref, {axis?, onPan})` over it, reporting
`{state, translation, velocity}` as a drag begins, changes, ends or is
cancelled, under a finger, a mouse and a pen; the translation leaves out the
travel the drag needed to begin, so an element that follows it does not
jump. Docs pages for both, with a playing-card example in plain JavaScript
and in React, the route lock and the sandbox's routes.
…ce and text colors Both playing-card pan blocks painted the card white and left its text color to the page, so in a dark theme the card's face read light on white. They now use the theme's surface background and primary text color. A case per block failed before.
usePanGesture kept its own node, axis and pan bookkeeping across three layout effects. It now attaches through useAttachment, Core's one owner of a node's attachment: one pan for the node behind ref with this axis, released when the node or the axis changes and at unmount. Its release still silences the pan first, so an effect never calls onPan. No behavior changes; the hook's cases pass as before.
… sentences Main's no-comment-provenance rule refuses a record or claim citation in a new source comment, and source comments use short, one-topic sentences. The pan factory's and the hook's headers and the package entry's gesture line cited the record they follow; the record's hooks path and the core architecture records still link those files. The factory's header and two test comments are split into short sentences. Comments only.
In both playing-card blocks, a drag that began during the 250 ms throw wrote the card's transition, which cancels the throw's transitions. They then end in transitioncancel, so the next card was never dealt: after a fast release the card stayed invisible and Discard did nothing. A drag that outlived the throw, or one a Discard interrupted, then moved the next card by its whole travel, a jump, and a fast release discarded a card the person never touched. The blocks now ignore a drag that meets a throw, to its end, and follow the next one. Cases per block, for a drag that begins during a throw and one a throw interrupts, failed before.
Both pan pages said a scroll that starts on the element moves it when no axis is set. That holds for a finger only: the pan takes no trackpad scroll, so a two-finger trackpad scroll over it still scrolls the page. They now say so. The factory test's header uses short sentences.
… types The new public exports, createPanGesture, usePanGesture and the PanGesture, PanGestureEvent, PanGestureOptions and PanGestureState types, get their rows in the core API report. Only these rows are added; rows that other changes left stale stay as they are.
…ack ref The example mounted its deck from an Effect after the table and button attached. A callback ref on the wrapper now owns that, as the animator's plain-JavaScript example does, and its cleanup takes the deck down. A case checks that the deck mounts with the wrapper and comes down, pan included, when it unmounts.
The comment restated the export below it. Comments only.
Both pan examples named their card "Top card" with an aria-label on a div with no role, which a screen reader may drop or read in place of the rank. The card is now an image named by its rank and suit, as the animator's examples name theirs, and each discard is announced by that name. A case per example failed before.
The header ran to 15 lines. The claim at 10 pt and the touch-action fact move into the factory's JSDoc, and the header keeps its input, output, position and SYNC lines. Comments only.
…element on snap points
`createDragToSnapPoints(element, {axis, snapPoints, flickThreshold?,
onSettle}) → {settleTo, destroy}` and `useDragToSnapPoints(ref, …)` over
it (spec:AST-107 FR2, FR6): a drag moves the element along its axis
under every pointer, and the release settles it where Core's one release
policy says (the nearest point; the next one past the flick speed, 1008
px/s by default; back to where the drag started when the release moves
back toward it), on the settle spring from the release velocity. Under
Reduce Motion the release cross-fades after a touch drag or on a coarse
primary pointer, and lands at once elsewhere. Docs pages and
playing-card examples for both, the route lock and the sandbox's routes,
and the core API report's rows for both. The plain-JavaScript example
mounts its card from a callback ref on its wrapper, whose cleanup takes
it down.
A caller handling an input event (a key, a click, a pointer, a touch) runs in that event's own task, never in a frame callback, so the first frame whose callbacks run after its call is the one that commits the call's animations, however long the task ran. The animator cannot tell that from a call a page's own frame callback makes before a busy task, whose browser start is right, so the caller says so with the internal option `start: 'next-frame'`. A call from rest made with it starts at that frame, and a later listener of the same event that holds the main thread delays the motion instead of letting it skip into its curve. - Chromium tells the page the start a frame or more after the compositor takes it. Such a call starts at its first frame, as a call whose first frame comes within LATE_START_MS already does, and also when an unrelated task delayed that frame. - WebKit dates the start at its last frame time before the task. The task's end is read at the call's first read, not at its first microtask, which runs before a later listener's block, so the dated start moves past the block. The first read stands for the task's end, so a first frame that merely comes late (more than LATE_START_MS after the call, with no block) moves the dated start to that frame too. Nothing was painted before it, so the motion waits for that slow frame instead of skipping into its curve. A call without the option, a call with an explicit start, and a call that continues motion keep the rule they had. Internal only: the public animator does not take it.
0aa861f to
bb0ea84
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 about four requirements this branch adds to spec:AST-107, which the record on main does not state: a cancelled pan reports zero velocity (FR1), a remounted element starts its animator at rest (FR3), createPanGesture's destroy() mid-drag reports cancelled once while usePanGesture's unmount reports nothing, and an opacity decay stops at 0 or 1 (FR6), recorded as DEC-4. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. The start: 'next-frame' option itself looks right, and its three new cases fail without it.
Two changes are needed either way:
- Rebase onto
mainonce #8653, #8655 and #8658 merge, so this pull request holds only the animator commit (about 200 of its 7,000 authored lines); the branch also conflicts withmaininscripts/touch-support-bundle.test.mjs. - In both animator playing-card templates, pressing Discard while dragging leaves the drag live, so the next
pointermovecuts the discard short and the same drag moves the newly dealt card; end the drag when a discard starts, as the pan templates do, with a mid-drag test.
[Automated review]
| const onPointerMove = (event: PointerEvent<HTMLDivElement>) => { | ||
| const current = drag.current; | ||
| if (current == null) { | ||
| return; | ||
| } | ||
| const x = event.clientX - current.start.x; | ||
| const y = event.clientY - current.start.y; | ||
| current.samples.push({t: event.timeStamp, x, y}); | ||
| // The card follows the finger, turning a little with the drag. | ||
| animator.set({x, y, rotate: x / 18}); | ||
| }; |
There was a problem hiding this comment.
While a Discard runs, this still writes x for the drag in progress. That takes the discard's channel over, so its animate resolves early, dealNext runs, and the next card jumps under the pointer and follows the same drag. Clearing drag.current when a discard starts (in discard and discardWithButton) ends the old drag, as the pan template's ignoringDrag does; a mid-drag case like pan-playing-card.test.tsx's would keep it closed.
| function onMove(event: PointerEvent) { | ||
| if (origin == null) { | ||
| return; | ||
| } | ||
| const x = event.clientX - origin.x; | ||
| const y = event.clientY - origin.y; | ||
| samples.push({t: event.timeStamp, x, y}); | ||
| animator.set({x, y, rotate: x / 18}); | ||
| } |
There was a problem hiding this comment.
Same as the React block: a Discard during a drag is cut short by the next move here, and the dealt card follows the old drag. Setting origin = null when a discard starts fixes it.
|
|
||
| Rejected: a context menu reachable only through the `<ContextMenu>` wrapper. | ||
|
|
||
| ### DEC-4 — Edge behaviors of the public animator and pan gesture |
There was a problem hiding this comment.
DEC-4 and the new sentences in FR1, FR3 and FR6 are requirements the record on main does not state; they need an engineering owner's decision (/approve-spec <full head SHA>) before this record change is settled.
| ): void { | ||
| const {axis, onPan} = options; | ||
| const onPanRef = useRef(onPan); | ||
| // The latest listener, for the next event. |
There was a problem hiding this comment.
Non-blocking: this comment restates the next line; consider deleting it.
| ): UseDragToSnapPointsResult { | ||
| const {axis, snapPoints, flickThreshold} = options; | ||
| const onSettleRef = useRef(options.onSettle); | ||
| // The drag calls the latest onSettle. |
There was a problem hiding this comment.
Non-blocking: this comment restates the next line; consider deleting it.
| // The snap points as a value, so a new array with the same points keeps | ||
| // the drag (an empty list stays empty). | ||
| const points = JSON.stringify(snapPoints); | ||
| // A new axis, snap points or flick speed creates the drag again. |
There was a problem hiding this comment.
Non-blocking: this comment restates the useAttachment key below; consider deleting it.
|
The AST-107 requirements this review names (DEC-4 and the new FR1, FR3 and FR6 sentences) come from the stack under this branch, in #8653's record change, which the spec owner has already ruled on. This pull request's own change is the animator's internal This branch rebases onto |
Depends on #8658 (on #8655 and #8653). This pull request's own commit: vjeux/astryx@27fd769...bb0ea84
Status: draft, rebased onto #8658's current head. Already passed locally at this head: typecheck, lint (strict), the unit tests of both test projects, and the animator's interaction proofs in Chromium (pinned to two cores at their default of one worker, every case). The WebKit proofs passed on the previous base and have not run again on this one.
User impact
A touch component that starts a motion from rest when someone presses a key, clicks, or taps should play that motion from its start, even if another listener of the same event holds the page. Today, when a later listener holds the main thread, the first frame after the block shows the motion about 100 ms into its curve, and the next frame continues from there: the start is skipped. A swipe row closed by Escape while a page's own keydown handler runs for 800 ms is one example.
No component on main uses this animator yet. Its first caller is the swipe row rebuild, #8690, stacked above this pull request with the shared drag path's internal options between them, and it merges in that chain's order.
Problem and solution fit
LATE_START_MSof the call.Expected behavior and authority
spec:AST-094FR5: touch motion keeps advancing at display rate through a main-thread block.architecture:react-component-runtimeINV13 lists: a call from rest starts at the frame that commits it, so a block before that frame delays the motion instead of skipping into its curve.Smallest restoration
The animator's internal
startoption gains a third value beside'commit'and'presented':start: 'next-frame'.LATE_START_MSalready does.LATE_START_MSafter the call, with no block.A call without the option, a call with an explicit
at, and a call that continues motion keep the rule they had. The option is ignored withat, as the otherstartvalues are: anatcan join an earlier waiting call's start, and that call keeps its own rule.Why an option and not the event: the start policy already lives in
start, so readers find the reason at the call site beside the other start rules, and no Event object has to travel through a component's settle path. The caller states the one fact it knows: it runs in its input event's own task.The option is internal: the public animator does not take it, so the public API does not change.
Evidence
atwins;animator.test.ts. The three that start at the next frame are red without this change. The sixth checks that an explicitatjoining an earlier waiting call leaves that call's rule alone.Scope
Testing
pnpm exec vitest run packages/core/src/motion/animator.test.tspackages/core/src/motion/__tests__/animator.interaction.browser.spec.ts).