Repository navigation
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📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. 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 |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an owner decision: the draft spec:AST-094 adds touch-presentation requirements (FR1–FR23) that no current record states and amends current claims in theme-tokens (INV3), user-states (DR1) and AST-025 (FR14), so an engineering owner needs to approve, narrow or reject it by commenting /approve-spec <full head SHA> on this pull request (the author may do so, being an engineering owner). Separately from that decision, three changes are needed:
- Under Reduce Motion, an
animate()that moves an element while its opacity is animating ends at the on-screen opacity, or at 0 withreducedMotion: 'instant', instead of the fade's target (packages/core/src/motion/animator.tslines 879, 957 and 1067). - The description needs the
## Writing rubric resultsthat the knowledge-contracts architecture asks of a pull request that creates a record. - Against
mainthis pull request still carries its eight predecessors (11,461 authored lines); landing them first leaves its own 564 lines to review.
[Automated review]
| landsAt, | ||
| ); | ||
| } | ||
| next.opacity = opacity; |
There was a problem hiding this comment.
Under prefers-reduced-motion this always writes next.opacity, even when the caller gave no opacity target. In 'instant' mode it is null, and the commit loop (lines 1067–1068: segments.delete before the valueAt fallback) then resets opacity to its value from before a running fade, so a move during a fade-in leaves the element at opacity 0. In 'crossfade' mode the dip returns to the on-screen opacity from line 879 instead of the fade's target. Ending the dip at finalValue('opacity'), leaving opacity alone in 'instant' mode, and reading the fallback before deleting the segment would keep a running fade intact.
…line) as a dependency
…g running Landing a waiting Reduce Motion move on the node just detached rebuilt every channel the landing touched there, so an axis still moving (a coast on y while x waited in a dip) was baked onto the detached node again and promoted it, leaving an animation, timers and will-change behind. After the landing, the unmount now cancels what runs, clears the retire timers and demotes, before the target goes back to none. The test fails on the previous head.
…seudo-element support
The README's busy-main-thread paragraph and busyWindowProof's JSDoc said an undelivered frame is never a miss; when fewer than minJudgedFrames frames were delivered in the judged part, its span counts every frame. One clause in each says so.
There was a problem hiding this comment.
Changes are needed before this can merge. The current spec:AST-094, design:reduced-motion and react-component-runtime records on main cover the motion code here, so no spec decision is pending, and the animator's browser proofs pass in Chromium and WebKit. Against main, though, the branch still carries seven open predecessors (#8056, #8521, #8522, #8045, #8548, #8549 and #8446): 9,978 authored lines, of which this pull request's own are 732, so landing those first, or restacking onto them, is the next step.
- The deferred unmount in
animator.ts(lines 2276–2282) now lands a waiting Reduce Motion move on the detached node, and when another translate axis is still coasting that commit promotes the node again, sowill-change: translate, an animation and two timers stay on it after the unmount (thewill-changesurvives a later re-attach); cancelling what the landing started and callingdemote()before restoringtarget, asdispose()does, would fix it, and the test atanimator.test.tsline 1877 should await the end of the task before it asserts.
[Automated review]
| const current = target; | ||
| target = detached; | ||
| try { | ||
| landPending(false); | ||
| } finally { | ||
| target = current; | ||
| } |
There was a problem hiding this comment.
This lands the waiting move on the detached node, but landPending() commits, and rebuild() re-animates any channel whose other axis is still moving: with y coasting from a decay (momentum keeps moving under Reduce Motion) while x waits for its dip, the translate channel gets a new animation on the detached node, promote() writes will-change: translate there, and a retire and a demote timer start, all after the synchronous cleanup further down has run. Once target is null again, demote() writes nothing, so the inline will-change stays, also after a later re-attach of the same node. Cancelling running, clearing retireTimers and calling demote() in this finally, before restoring target (as dispose() does after its own landPending), would release them.
| await commitFrame(clock); | ||
| await advance(20); | ||
| animator.attach(null); | ||
| expect(element.style.willChange).toBe(''); |
There was a problem hiding this comment.
This reads willChange right after attach(null), before the deferred unmount's microtask runs, so it passes while the microtask then writes will-change: translate on this element and leaves two timers. Awaiting the end of the task first (await Promise.resolve()), then checking willChange and vi.getTimerCount(), would catch it.
…ose finds it The unmount kept the node of the first attach(null) in the task, so after attach(null), attach(next), attach(null) it landed a waiting Reduce Motion move on the node already replaced, and next stayed at its dip's opacity. A dispose right after attach(null) landed on no element, leaving the detached node at opacity 0. The latest detached node is now kept while its unmount is pending; the unmount and dispose both land there. Two tests fail on the previous head.
…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.
Inside a trace, waitForAnimation and watch.started() read the element through handles, which find no element rather than wait for one, so a wait on an element the page removed rejected with "no animation started on the element" instead of "the element is not in the document", unlike the untraced path. At its deadline, a traced wait with no element now rejects as missing. Traced twin of the missing-element self-test; it fails on the previous code. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
There was a problem hiding this comment.
Changes are needed before this can merge, for scope alone: the current spec:AST-094, design:reduced-motion and react-component-runtime records cover the code here, the animator's browser proofs pass in Chromium and WebKit, and the earlier unmount issue with a coasting axis is fixed. Against main, though, the branch still carries seven open predecessors (#8045, #8056, #8446, #8521, #8522, #8548 and #8549): 10,054 authored lines, of which only 648 sit in files this pull request alone changes. Landing those first, or restacking onto them, leaves only the hook, its test and the browser proofs.
[Automated review]
…rred unmount runs It read will-change right after attach(null), before the unmount's microtask, so it passed while that microtask could still promote the element and leave timers behind. It now awaits the microtask, then checks will-change and that no timer is left. It fails on meta#8521's earlier head (97436b2), before the unmount cleared what the landing rebuilt.
…pseudo-element support
There was a problem hiding this comment.
Changes are needed before this can merge, for scope alone: the current spec:AST-094, design:reduced-motion and react-component-runtime records cover the code here, and the animator's browser proofs pass in Chromium and WebKit on this head. Against main, the branch still carries seven open pull requests (#8045, #8056, #8446, #8521, #8522, #8548 and #8549): 10,057 authored lines, of which only 648 sit in files this pull request alone changes, and its copy of the interaction harness is already behind #8446 and #8548. Landing those first, or restacking onto them, leaves only the hook, its test and the browser proofs.
[Automated review]
There was a problem hiding this comment.
Changes are needed before this can merge, for scope alone: the current spec:AST-094, design:reduced-motion and react-component-runtime records cover the code here, and the animator's browser proofs pass in Chromium and WebKit on this head. Against main, the branch still carries six open pull requests (#8045, #8446, #8521, #8522, #8548 and #8549): 5,829 authored lines, of which 732 are this pull request's own. Landing those first, or restacking onto them, leaves only the hook, its test and the browser proofs.
[Automated review]
…tor's browser proofs The motion README's Files table is the union: main's rows, with meta#8663's resolveSnapPoint.ts and selectPage.ts, plus this branch's, in main's order. # Conflicts: # packages/core/src/motion/README.md
There was a problem hiding this comment.
Changes are needed before this can merge, for scope alone: the current spec:AST-094, design:reduced-motion and react-component-runtime records cover the hook and the browser proofs, and those proofs pass in Chromium and WebKit on the current branch. Against main, the branch still carries five open pull requests (#8045, #8446, #8548, #8549 and #8522): 5,376 authored lines, of which 736 are this pull request's own. Landing those first, or restacking onto them, leaves only the hook, its test and the browser proofs.
[Automated review]
Depends on #8056 and #8446 (on #8045, and their predecessors); review its own commits.
Why
Whether touch motion keeps moving while the page's JavaScript is busy can only be shown in real browser engines, and unit tests with a fake clock cannot catch a spring that freezes for a second on a phone. Components also need the animator without a React render on every frame.
What led to it
This comes from the work to make Astryx's touch components behave as native iOS ones do (sheets, menus, gestures and their motion).
spec:AST-094FR5 asks motion to start at its commit and play whole, delayed rather than skipped or flashing when the main thread or compositor is busy.Options and tradeoffs
Why this choice
The fixture page keeps the proofs fast enough for CI's interaction job, and the curve check inverts the curve at each frame and requires one time offset for all frames (in WebKit, only a small spread), which works in both engines.
What changes
useAnimator()returns a stable{ref, animator}, so a component drives motion with no re-render; unmounting settles its handles.__tests__/animator.interaction.browser.spec.tsruns in Chromium and WebKit and proves that a slow spring advances on every frame through a 1 s main-thread block, a spring that ends inside the block keeps showing its end state, no Layout or Paint runs while it animates, and a dialog's::backdropfollows the curve.How to check it
pnpm test:interaction animator.interaction.browser.spec.tsin Chromium, and in WebKit throughinternal/interaction-harness/scripts/webkit-container.mjs.Evidence
[chore].Scope