Repository navigation
Conversation
A component that animates a pseudo-element (animatorFor(element, {pseudoElement: '::before'})) drew its motion where sampleCurve could not read it: it labelled samples by the element's own animations only and read the element's computed style. sampleCurve(locator, times, {pseudoElement}) now reads that pseudo-element's presented values, labelled by its own animations (those whose effect targets the element with that pseudo-element). Without the option nothing changes.
waitForRest already waits a pseudo-element's animation out: its read of getAnimations({subtree: true}) includes the element's pseudo-elements in Chromium and WebKit. A self-test now guards that.
Red first: on main's curves.ts the new self-test samples the box itself (opacity 1 at t 0, where the veil's is 0) in both engines; with the option it matches the veil's linear fade to 0.005 and its 40 px move to 0.05 px. The README describes the option in the curve-sampling paragraph, outside the module table.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting one small change before this lands; the sampleCurve pseudo-element option and its self-tests look correct, and sampling without the option is unchanged.
The JSDoc at internal/interaction-harness/src/curves.ts:592 cites animatorFor(element, {pseudoElement}), but AnimatorOptions on main has no pseudoElement option yet (it arrives with #8242), so please point to element.animate(keyframes, {pseudoElement}) as the self-test does, or drop the parenthetical until that option lands.
[Automated review]
| * `pseudoElement` samples the element's `::before` or `::after` instead: | ||
| * its presented values, labelled by its own animations (those whose effect | ||
| * targets the element with that pseudo-element), as a component that animates | ||
| * a pseudo-element (`animatorFor(element, {pseudoElement})`) draws it. |
There was a problem hiding this comment.
animatorFor(element, {pseudoElement}) doesn't exist on main: AnimatorOptions in packages/core/src/motion/animator.ts has only initial, now and requestFrame, and the pseudoElement option arrives with #8242. Until then this line could name the call that exists, as the new self-test does:
* a pseudo-element (`element.animate(keyframes, {pseudoElement})`) draws it.… call that exists on main
astracat found that the note cited `animatorFor(element, {pseudoElement})`, an animator option that arrives with meta#8242 and isn't on main. It now names `element.animate(keyframes, {pseudoElement})`, the call the self-test uses.
There was a problem hiding this comment.
Recommending approval: the new sampleCurve pseudoElement option reads a ::before or ::after timed by its own animations, sampling without the option is unchanged, and the option's self-test fails without the change and passes with it. The JSDoc at internal/interaction-harness/src/curves.ts:592 now names element.animate(keyframes, {pseudoElement}), which resolves the change requested on the previous revision, so nothing further is needed from you.
[Automated review]
…inst the busy control "A block that starts during a just-started transition" and "a block never starts on an animation that is not on the compositor yet" still judged their compositor animation against the display-rate bound alone. On a loaded runner that bound fails a component that moved on every frame. The second one failed this way on a gate run of meta#8911 (WebKit, 8 workers: "43/43 moved, longest stall 52 ms"), and in 3 of 16 Chromium runs at 16 workers ("31/31 moved, longest stall 169 ms"). Both now start the busy control before the cast and judge against what the compositor presented in the same window. Both pass 32 of 32 per engine at 16 workers. Their intent is unchanged: a block in the frame the transition has run 16 ms, and a block never on a pending animation, each still fails if the compositor's motion stalls longer than the control's.
Review only this PR's change: vjeux/astryx@90da4a7...93b46e7
Why
A component that animates a
::beforeor::after(animatorFor(element, {pseudoElement})) draws its motion wheresampleCurvecannot read it:sampleCurvereads the element's computed style, labelled by the element's own animations. Lightbox's chrome-fade proofs (#8242) sample a pseudo-element's fade three times.What led to it
The option lives only in #8242's line, on an older harness. The foundation re-fold doesn't carry it, so #8242's re-fold cannot take it without resolving the harness README by hand.
Options and tradeoffs
Why this choice
sampleCurve(locator, times, {pseudoElement: '::before'})reads that pseudo-element's presented values, labelled by its own animations: those whose effect targets the element with that pseudo-element. Without the option nothing changes.waitForRestalready waits a pseudo-element's animation out, since its subtree read includes the element's pseudo-elements, and a self-test now guards that. The README describes the option in the curve-sampling paragraph rather than the module table, which the re-fold re-pads.How to check it
pnpm test:interaction internal/interaction-harness/src/harness.interaction.browser.spec.ts --grep "::before animation": 2 pass in each engine.Evidence
curves.tsthe new self-test samples the box itself (opacity 1 at t 0, where the veil's is 0) in both engines; with the option it matches the veil's linear fade to 0.005 and its 40 px move to 0.05 px.pnpm check:repoand the node project (796 files) pass.