Repository navigation
chore(core): the animator plans Reduce Motion in its own module, shares a track per element and starts at the commit - #8056
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
the
needs:design-review
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
PR Analysis Report
View Storybook for this PR · View Sandbox for this PR 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 can't be approved yet: every code change here rests on the draft spec:AST-094, which has no owner decision and also proposes touch-presentation amendments to current records (theme-tokens INV3, user-states DR1/DR3, the buttons family, interaction modality and AST-025 FR14). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, or by saying which requirements must change; the author may do so as an engineering owner. Whatever that decision, a few changes are needed:
- Under Reduce Motion, a move that starts while an earlier fade is still running leaves the surface at its mid-fade opacity for good, or at 0 with
reducedMotion: 'instant'(animator.ts:879,:957); please end an untargeted fade at the opacity the surface is heading to and add a regression test. - Against
mainthis pull request carries the whole stack, 6,370 authored lines, so landing #8052 to #8055 first would leave it at its own 599. - A few headers point at files that are not in the tree yet, such as
__tests__/animator.compositor.spec.ts,sources.test.tsanduikit/switch.ts.
[Automated review]
| ): Partial<Record<MotionProperty, Segment | null>> => { | ||
| const next: Partial<Record<MotionProperty, Segment | null>> = {}; | ||
| const opacityNow = valueAt('opacity', time); | ||
| const opacityTo = targets.opacity ?? opacityNow; |
There was a problem hiding this comment.
Under Reduce Motion this ends a fade the call did not ask for at the opacity on screen. A move that starts while an earlier fade is still running (an entrance fade cut at 200 ms by a detent move) leaves the surface at 0.33 for good, and with reducedMotion: 'instant' the null opacity segment from line 957 makes animate and play store the pre-fade resting value, 0. Ending at finalValue('opacity') and leaving an untargeted opacity alone in 'instant' mode would fix both; a test that starts from a running fade would cover it.
| * @position Testing; the compositor itself is proven in | ||
| * __tests__/animator.compositor.spec.ts |
There was a problem hiding this comment.
__tests__/animator.compositor.spec.ts is not in this tree, so this header (and the SYNC entry in animator.ts:83) points at a proof that does not exist yet. Point it at the follow-up or drop it until the file lands.
…here it is Under the Reduce Motion plan a call's travel waits in a landing, with no segment, so handle.stop() stopped only the call's fade, and the fade's landing then wrote the travel at once while the element was still visible. The handle now stops the call's waiting travel too, before its fade, as animator.stop() does: a catch freezes the element where it is on screen (spec:AST-094 FR6). Tests: a stop mid-dip, and a release with one axis past an edge, keep the travel where it was.
|
Two animator fixes and a comment line, offered separately: 1. A Reduce Motion call naming the opacity a waiting dip returns to plays a second dip (fork branch 2. A call held before its first frame never starts (fork branch 3. One comment line (fork branch |
|
A promotion replaces a stylesheet's |
There was a problem hiding this comment.
This needs changes before it can be approved: the Reduce Motion planner follows design:reduced-motion and spec:AST-094, and the handle's stop no longer lands a dip in view, but two things remain.
- Under Reduce Motion, when a second call starts before the first call's travel has landed,
finished,stop()andset()act on the wrong call (a dismissal caught by a re-presentation reportsfinished === true, andstop()on a superseded handle leaves the element atx0 instead of 200), so please key each carried landing value to its call inadoptLandings,cancelLanding,landand the handle'sstop(animator.ts:1647-1660,:1766-1821,:1919-1925) and add regression tests. - At 4,116 authored lines, please move the start at the commit with promotion,
animatorFor,atRest,rebaseand the batched style read to their own pull request, with AST-094 FR5's Chromium and WebKit busy-main-thread proof, so that this one holds the Reduce Motion planner the line-count waiver describes.
[Automated review]
| } else if (landing.values.delete(property) && !landing.owner.settled) { | ||
| // Set or stopped before it landed: the call did not finish. | ||
| settle(landing.owner, false); |
There was a problem hiding this comment.
Under Reduce Motion this settles landing.owner, the call that carries the landing, even when the removed value belongs to a carried call. In the animator's test harness, animate({x: 100}) and animate({y: 50}) in one frame, then set({x: 40}), settle the second call false and, once the dip lands, the first true; on main the first settles false and the second true. Keying each landing value to its call and settling that call here would fix it.
| for (const [property, value] of landing.values) { | ||
| if (!properties.includes(property) && !plan.values.has(property)) { | ||
| plan.values.set(property, value); | ||
| } | ||
| } | ||
| carried.push(landing.owner, ...landing.carried); |
There was a problem hiding this comment.
The values merge without their owner, and every earlier call is listed as carried, including one whose property the new call takes over, so land later settles it true (:1920-1925). A dismissal animate({y: 800, opacity: 0}) re-presented 50 ms in with animate({y: 0, opacity: 1}) then reports finished === true under Reduce Motion, where main and the no-preference path report false. A carried call whose property the new call overrides should settle false here, as interrupt does without Reduce Motion.
| [...landings].some( | ||
| landing => | ||
| (landing.owner === owner || | ||
| landing.carried.includes(owner)) && | ||
| landing.values.has(p), |
There was a problem hiding this comment.
landing.values.has(p) is also true when the value is the newer call's own. After animate({x: 100}) and animate({x: 200}), the first handle's stop() deletes the second call's x from its landing, so the element stays at x 0 and the second handle settles false; without Reduce Motion, and on main, that stop does nothing and the element reaches 200. Checking that the value belongs to this call would keep a stale handle from stopping newer motion.
When a later call carried an earlier call's waiting move, the landing knew only its own call: setting or stopping the carried move settled the later call as interrupted (and the earlier one later as finished), an earlier handle's stop could freeze the later call's move on the same property, and a call whose property a later call took over still resolved finished. Each landed value now keeps the call whose move it is: setting or stopping it settles that call, a handle stops only its own values, and a move a later call takes over settles interrupted. Three tests fail on the previous head.
There was a problem hiding this comment.
This needs changes before it can be approved: the Reduce Motion planner follows design:reduced-motion and spec:AST-094, and overlapping Reduce Motion calls now settle on the call whose move it is, but two things remain.
- Under Reduce Motion, a move whose animations are paused before their first frame (a test harness's hold, a page pausing every animation) does not land on its own and leaves the element at opacity 0 with
finishedpending, because the call's start is read only in its first frame and atready(animator.ts:1412-1416,:1882-1891); please keep reading it while the animations are paused without a start, with tests for a release byfinish()and by a resume. - At 4,184 authored lines, please move the start at the commit with promotion,
animatorFor,atRest,rebaseand the batched style read to their own pull request, with AST-094 FR5's Chromium and WebKit busy-main-thread proof, so that this one holds the Reduce Motion planner the line-count waiver describes.
[Automated review]
| // or when its compositor starts them (Chromium). Read it in that first | ||
| // frame, ahead of anything that might pause or seek the animations, and | ||
| // when `ready` settles. | ||
| requestFrame?.(() => syncGroup(group)); |
There was a problem hiding this comment.
Besides ready, this is the only read of the call's start. When something pauses the call's animations before their first frame (a test harness's hold, a page pausing every animation), both see no start and nothing looks again, so the landing armed from handle.started (:1882-1891) never runs and the element stays at opacity 0 with finished pending. In Chromium with Reduce Motion and a coarse pointer, animate({x: 100}) paused in its own task and finished or resumed 30 ms later still shows opacity 0 and no translate 1 s later, where main lands it. Looking again each frame while the animations are paused without a start, or arming the landing from the call time when none arrives, with tests for a release by finish() and by a resume, would fix it.
There was a problem hiding this comment.
The held-start offer in #8056 (comment) fixes this: vjeux:uikit/f1-8056-held-start keeps reading the start each frame while the call's animations are paused or running without a start time, with two tests that fail without it.
| // entrance that starts visible, or an exit that ends visible, dips instead | ||
| // when it has travel or a waiting landing to carry. | ||
| const carries = travel.length > 0 || pending; | ||
| if (entering && !pending && (!carries || fadeFrom <= 1e-6)) { |
There was a problem hiding this comment.
With any landing waiting this branch is skipped, so a call that takes over every waiting value still dips: animate({y: 800, opacity: 0}) re-presented 50 ms in with animate({y: 0, opacity: 1}) fades from 0.85 to 0 and back to 1, where main fades straight back up from 0.87. Reporting which properties wait, and ignoring those this call sets, would keep a caught dismissal from blinking.
| ownWillChange = null; | ||
| return; | ||
| } | ||
| ownWillChange ??= element.style.willChange; |
There was a problem hiding this comment.
ownWillChange is only the inline value, so the promotion's inline will-change replaces a stylesheet's for the motion's length (BottomSheetPanel's stylesheet sets willChange: 'transform, opacity'), changing the layer's reasons at the motion's start and end. Merging the computed value, as the fix offered in the conversation does, would keep them.
…s straight back; promotion keeps the stylesheet's will-change - A call whose animations something paused before their first frame (a harness holding them, a page pausing every animation) never read a start, so a Reduce Motion move's landing never ran and the element stayed transparent. The call keeps looking each frame while its animations are paused without a start, until they resume (their start) or finish (its own clock). - Under Reduce Motion a call that sets a value still waiting to land takes it over (the earlier call settles interrupted), and once nothing waiting is left, a call naming its own opacity replaces the earlier fade back in or restore: re-presenting a sheet mid-dismissal fades straight back up from where it was instead of dipping. - Promotion merged only the element's inline will-change, so it replaced a stylesheet's will-change for the motion's length; without an inline value it now keeps the stylesheet's reasons beside its own and hands them back. Three tests fail on the previous head.
There was a problem hiding this comment.
This needs changes before it can be approved: the Reduce Motion planner follows design:reduced-motion and spec:AST-094, and a hold released by finish() now lands, but two things remain.
- Under Reduce Motion, a move whose animations are paused before their first frame and then resumed with
play()still never lands on its own in Chromium (opacity 0,finishedpending), because the poll atanimator.ts:1505-1516stops once the animation is running without a start; please keep reading until the call has a start, and make the test stand-in start a resumed animation a frame later, as Chromium does. - At 4,320 authored lines, please move the start at the commit with promotion,
animatorFor,atRest,rebaseand the batched style read to their own pull request, with AST-094 FR5's Chromium and WebKit busy-main-thread proof, so that this one holds the Reduce Motion planner the line-count waiver describes.
[Automated review]
| if ( | ||
| group.animations.some( | ||
| animation => | ||
| animation.playState === 'paused' && animation.startTime == null, |
There was a problem hiding this comment.
After play(), Chromium reports 'running' with startTime null until the next frame, so this condition is false on the poll's next look and it stops before the start exists; the ready promise subscribed in commitCall already settled at the pause. In Chromium 149 under Reduce Motion, a move paused in its own task and resumed 30 ms later stays at opacity 0 with finished pending (3 of 3 runs). Keep looking while any live animation of the call has no start, paused or pending, or subscribe to the new ready after a resume.
| async function commitFrame(at: number) { | ||
| for (const animation of recorded) { | ||
| if (animation.startTime == null && !animation.cancelled) { | ||
| animation.commit(at); |
There was a problem hiding this comment.
This commits a pending start before the frame callbacks run, so the poll in syncGroup sees a resumed animation's start in the same frame. Chromium sets it a frame later, which is why the 'resume' case at line 1501 passes here and fails in the browser. Starting a resumed animation one frame after its first frame callback would make that test catch it.
| // Travel lands only while the element is transparent (DR1, DR2): an | ||
| // entrance that starts visible, or an exit that ends visible, dips instead | ||
| // when it has travel or a waiting landing to carry. | ||
| const carries = travel.length > 0 || pending; |
There was a problem hiding this comment.
pending also counts a landing whose values a later call already took over (it stays in landings unless the call names opacity), so animate({x: 100}) then animate({x: 0}) 50 ms in dips to 0 and back where main fades straight up, and a re-presentation with crossfadeAs: 'entrance' after an exit with crossfadeAs: 'exit' dips too. Counting only landings with values would let both fade straight back.
…l retargets settles interrupted A call that named its own opacity and was carried by a later call naming opacity never reached that opacity, yet settled finished once its travel landed. It now settles interrupted when the later call takes its opacity over, as without Reduce Motion. The test fails on the previous head.
There was a problem hiding this comment.
This needs changes before it can be approved: the Reduce Motion planner follows design:reduced-motion and spec:AST-094, and the newest commit's settle fix is right, but two things remain.
- Under Reduce Motion, a move whose animations are paused before their first frame and then resumed with
play()can still stay at opacity 0 withfinishedpending in Chromium, because the poll atanimator.ts:1509-1516stops once the animation is running without a start; please keep reading until the call has a start (the held-start fix offered in the conversation does this), and make the test stand-in start a resumed animation a frame later, as Chromium does. - At 4,351 authored lines, please move the start at the commit with promotion,
animatorFor,atRest,rebaseand the batched style read to their own pull request, with AST-094 FR5's Chromium and WebKit busy-main-thread proof, so that this one holds the Reduce Motion planner the line-count waiver describes.
[Automated review]
The held-start poll stopped once the call's animations were no longer paused, but an animation resumed with play() stays pending without a start until a later frame (in Chromium, after the frame's callbacks), so a Reduce Motion move could stay invisible. The call now keeps looking each frame while any of its animations has no start and is not cancelled. The test's stand-in now gives a resumed animation its start a frame later, as Chromium does; it fails on the previous head.
There was a problem hiding this comment.
This needs changes before it can be approved: the Reduce Motion planner follows design:reduced-motion and spec:AST-094, and the held-start fix is in, but two things remain.
- At 4,363 authored lines, please move the start at the commit with promotion,
animatorFor,atRest,rebaseand the batched style read to their own pull request, with AST-094 FR5's Chromium and WebKit busy-main-thread proof, so that this one holds the Reduce Motion planner the line-count waiver describes. - Under Reduce Motion,
decaycannot be told that a touch release started it, so a spring back from past an edge lands at once on a fine primary pointer where DR2 asks for a cross-fade; please forwardtouchReleasethroughdecayand keep a move taken back mid-dip from blinking to opacity 0 (reducedMotionPlan.ts:162), each with a test.
[Automated review]
| @@ -1252,13 +2345,15 @@ export function createAnimator( | |||
| ? animator.animate(transitions, REDUCED_TRANSITION, {at: time}) | |||
There was a problem hiding this comment.
Under Reduce Motion this hands a spring back from past an edge to animate without touchRelease, and decay takes only at (line 313), so on a fine primary pointer a spring back that a touch drag's release started lands at once, where design:reduced-motion DR2 asks for the opacity-only cross-fade ("or when the release of a touch drag starts the transition"). Accepting touchRelease in decay's options and passing it here, with a test on a fine pointer, would conform; the changeset, the README, the header and coast.ts would then say the same.
| } | ||
| // A move still waiting to land (an earlier dip's) lands at this call's | ||
| // invisible moment, never while the element shows. | ||
| const pending = view.hasPendingLanding; |
There was a problem hiding this comment.
hasPendingLanding is also true for a landing whose values a later call already took over (takeOverLandings leaves the emptied landing in the set when the call names no opacity), so a take-back such as animate({x: 100}) then animate({x: 0}) 50 ms in dips from 0.5 to 0 and back, where main fades straight up from 0.5. Counting only landings that still hold values would let it fade straight back; an entrance by role caught mid-exit also opens its fade at 0 (line 152).
… an entrance catching an exit starts where it is
- decay accepts touchRelease and hands it to the spring back's transition,
so a spring back a touch drag's release starts cross-fades on a fine
pointer too (design:reduced-motion DR2); the README and coast.ts say so.
- A move taken back mid-dip (animate({x: 100}) then animate({x: 0})) left
an emptied landing that kept the dip going down to 0 and back; a dip
left with nothing to land now fades straight back in from where it is.
- An entrance by role caught mid-exit opened its fade at 0 and headed for
the exit's 0; it now starts from the opacity on screen and fades back to
what the exit would have restored, the exit's landing taken over as a
named opacity's is.
Three tests fail on the previous head.
There was a problem hiding this comment.
This needs changes before it can be approved: the Reduce Motion planner follows design:reduced-motion and spec:AST-094, and this head's fixes for decay's touch release, a mid-dip take-back and a caught exit by role are in and tested, but the pull request still carries about 1,370 added lines of separable work outside the line-count waiver for the planner.
- Please move the start at the commit with promotion,
animatorFor,atRest,rebaseand the batched style read to their own pull request, with AST-094 FR5's Chromium and WebKit busy-main-thread proof, so that this one holds the Reduce Motion planner the waiver describes. - Please also say in
.changeset/uikit-motion-animator.mdand the animator header that a touch-released spring back cross-fades on any pointer, and refresh the description's size and test count.
[Automated review]
| --- | ||
|
|
||
| [chore] Internal: a compositor animator for touch presentations. It plays UIKit's springs, and scroll coasts (built by a separate coast module, so a surface that never coasts does not carry them; `decay` plays a release's momentum, and under Reduce Motion cross-fades a spring back from past an edge) as Web Animations on translate, rotate, scale and opacity, so they keep running while JavaScript is busy, and reads values and velocities back from the curves for interruption, retargeting and Reduce Motion cross-fades. Not exported from the package entry point; no component uses it yet. | ||
| [chore] Internal: a compositor animator for touch presentations. It plays UIKit's springs, and scroll coasts (built by a separate coast module, so a surface that never coasts does not carry them; `decay` plays momentum, and sends a spring back from past an edge through the Reduce Motion policy: a cross-fade on a coarse pointer, at once elsewhere) as Web Animations on translate, rotate, scale and opacity, so they keep running while JavaScript is busy, and reads values and velocities back from the curves for interruption, retargeting and Reduce Motion cross-fades. Not exported from the package entry point; no component uses it yet. |
There was a problem hiding this comment.
With decay now taking touchRelease, a spring back from past an edge also cross-fades on a fine pointer when a touch drag's release starts it (design:reduced-motion DR2). "a cross-fade on a coarse pointer, at once elsewhere" leaves that out; "a cross-fade on a coarse pointer or after a touch release, at once elsewhere" matches the code, the README and coast.ts.
| * picks (a detent, a page), is a transition: it plays through the Reduce | ||
| * Motion policy above, a cross-fade on a coarse pointer and at once | ||
| * elsewhere (DR2, DR3). |
There was a problem hiding this comment.
The same here: decay's own docs at lines 300-312 say a spring back cross-fades on any pointer when touchRelease is passed, but this paragraph still says "a cross-fade on a coarse pointer and at once elsewhere". Adding "or after a touch release (touchRelease)" keeps the header in step.
…cross-fade for a spring back With decay taking touchRelease, a spring back from past an edge also cross-fades on a fine pointer when a touch drag's release starts it (design:reduced-motion DR2); the header and the changeset now say so, as decay's docs, the README and coast.ts do.
There was a problem hiding this comment.
This needs changes before it can be approved: the Reduce Motion planner follows design:reduced-motion and spec:AST-094, but the pull request still carries about 1,370 added lines of separable work that the line-count waiver for the planner does not cover.
- Please move the start at the commit with promotion,
animatorFor,atRest,rebaseand the batched style read to their own pull request, with AST-094 FR5’s Chromium and WebKit busy-main-thread proof, or explain why they cannot be split. - Please also refresh the description’s size, test count and “One capability is added” box, and its line saying a release past an edge lands at once on a fine pointer, which leaves out the touch-release cross-fade.
[Automated review]
| }; | ||
|
|
||
| // -- segment factories ------------------------------------------------------ | ||
| // -- starting from rest --------------------------------------------------- |
There was a problem hiding this comment.
This section (the start at the commit: StartGroup, handle.started, the held reads, through resolveGroup), with the promotion block above it, is separable from the Reduce Motion planner: the planner arms its landing from handle.started, which a call with an explicit start resolves at once. Please move it to its own pull request with AST-094 FR5’s busy-main-thread proof, or explain why it must land with the planner.
| // are checked where the group declares them too. | ||
| const bindings = /^const \{([^}]+)\} =\s*(?:\n\s*)?(\w+);/gm; | ||
| for (const [, names, group] of text.matchAll(bindings)) { | ||
| for (const [, names, group] of lines.join('\n').matchAll(bindings)) { |
There was a problem hiding this comment.
lines is text.split('\n') (line 61), so lines.join('\n') is the same string as text: this edit changes nothing and can go.
lines.join('\n') is the text it was split from; the edit changed nothing
and goes back to main's line.
There was a problem hiding this comment.
This needs changes before it can be approved: the Reduce Motion planner follows design:reduced-motion and spec:AST-094, but the pull request still carries 1,369 added lines of separable work that the line-count waiver for the planner does not cover.
- Please move the start at the commit with promotion,
animatorFor,atRest,rebaseand the batched style read to their own pull request, with AST-094 FR5's Chromium and WebKit busy-main-thread proof, or explain why they cannot be split. - Please also correct the description's checked "One capability is added" box, and its line saying a release past an edge lands at once on a fine pointer, which leaves out the touch-release cross-fade.
[Automated review]
|
The first offer above (a Reduce Motion call naming the opacity a waiting dip returns to plays a second dip) is refreshed onto this pull request's head: fork branch The defect is still live at 2b264d3, and a swipe row's release hits it: its move plays as a dip, then its state layer lands opacity 1 on the same element in the same task, and each entry but the outermost fades out twice (its release storyboard records two identical opacity curves for such an entry). The planner's join is unchanged from the first offer; the animator's side now follows this head's landing ownership, so the call it replaces is carried and each value keeps the call whose move it is. A call that names travel, or another opacity, plans as before. Three new tests, the planner's, mid dip and in the dip's own task, fail on 2b264d3 and pass with it; the motion and gestures suites pass (614 of 614). The same fix rides on #8076, the experimental touch layer that carries this pull request, so the swipe storyboard can be re-recorded there. |
|
The other two offers in that comment, against this pull request's head: will-change. The second fix's main point, keeping a stylesheet's DEC-11 comment line. Unchanged, refreshed onto this head (fork branch Both also ride on #8076's next update. |
Update (head 2b264d3): size is now 14 files, 3,961 lines added, 623 removed;
pnpm exec vitest run --project ui packages/core/src/motion: 14 files, 498 tests passed. The figures under Testing below are from an earlier head.#8055 landed (30a4a2b); review the commits above
main.play(curves)moved to #8521 and pseudo-element support to #8522.User need
Touch presentations need Reduce Motion per
design:reduced-motion, two components animating one row, and motion that survives a busy Mobile Safari.Problem and solution fit
reducedMotionPlan.ts, a pure planner the animator applies): nothing moves, scales or springs; on a coarse pointer or after a touch release a change becomes an opacity-only 200 ms cross-fade, otherwise it completes at once, and no option or theme value changes that. Momentum after a throw keeps moving (DEC-2); a release past an edge cross-fades back on a coarse pointer and lands at once on a fine one; a change at once leaves a running fade to finish.reducedMotionFade()serves motion beside the animator.animatorFor(element)shares one animator per element, so callers' axes compose in one track;atRest()waits out every curve;rebase(deltas)follows a layout shift, keeping springs.handle.startedreports it.set()promotes its channels (will-change) in the same task, until 500 ms after rest.Current specification
spec:AST-094FR5, FR6, FR8;design:reduced-motionDR1–DR4.Public delta and caller burden
None: internal and unused; the changeset is
[chore].Design and alternatives
{at}).Evidence
atRestacross retargets,rebase, the browser-set start, and promotion.Writing rubric results
No record text of its own: AST-094 is current on
main; R1–R6: none.Scope
Testing
pnpm exec vitest run --project ui packages/core/src/motion: 14 files, 484 tests passed.pnpm build,pnpm typecheck, ui and node suites,pnpm check:repo: clean.pnpm -F @astryxdesign/core typecheck, strict lint, prettier,pnpm check:changesets,pnpm check:repo(pre-commit): clean.Stack, in merge order: #8061, #8056, #8067, #8258, #8071, #8072, #8073, #8074, #8076, #8340; #8067 also follows #8505, #8348, #8045 and #8446. #8521 and #8522 stand on
main.