Repository navigation
test(interaction): curve sampling - #8044
Conversation
|
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
PR Analysis Report
|
![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 curve-sampling and storyboard instruments work in Chromium, but the stacked diff and the CI wiring need fixes before this can land.
- Land #8043 first and rebase, so this pull request shows only its own commit; as it stands the diff also carries all of #8043, 2,953 authored lines in total.
- Feed
pr-interaction's result into an existing required join, astest-native-swiftfeedstest, because today a failing interaction proof leaves every required check green, whichdocs/specs/AST-030/spec.mdFR7 and FR11 do not allow. - Fix three harness defects:
pnpm storyboardsexits 1 with “No tests found” because no test is tagged@storyboard, the timer gate still runs callbacks cancelled withclearTimeoutorcancelAnimationFramewhile frozen, and an end sample reports the requested time although it is read 0.5 ms before the end.
[Automated review]
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
No required join reads this job's result: test needs test-ui, test-node, test-build, registry-contract and test-native-swift (line 887), and build needs build-storybook, build-sandbox and typecheck, so a failing interaction proof leaves every required check green. docs/specs/AST-030/spec.md FR7 and FR11 ask new coverage to feed an existing owner's join; folding this job into test the way test-native-swift is folded (its needs plus .github/scripts/ci-test-join.mjs, accepting skipped only out of scope and in merge groups) would close it.
| "test:a11y-contract": "playwright test --config playwright.config.ts --project chromium", | ||
| "test:docsite-browser": "playwright test --config apps/docsite/playwright.config.ts", | ||
| "test:interaction": "playwright test --config playwright.interaction.config.ts", | ||
| "storyboards": "playwright test --config playwright.interaction.config.ts --grep @storyboard", |
There was a problem hiding this comment.
No test carries @storyboard yet, so this exits 1 with Error: No tests found. Tagging the storyboard self-tests (test.describe('storyboards', {tag: '@storyboard'}, …)) or adding --pass-with-no-tests fixes it.
| const realInterval = window.setInterval.bind(window); | ||
| const realFrame = window.requestAnimationFrame.bind(window); | ||
| const hold = run => (gate.frozen ? gate.queue.push(run) : run()); | ||
| window.setTimeout = (fn, ms, ...args) => realTimeout(() => hold(() => (typeof fn === 'function' ? fn(...args) : 0)), ms); |
There was a problem hiding this comment.
Once the real timer fires while frozen, the wrapped callback sits in gate.queue, and the page's clearTimeout (not wrapped) can no longer cancel it, so thaw runs a callback the component cancelled. The same holds for cancelAnimationFrame and line 115, whose held callback also gets a stale timestamp because frozen is already false when the queue runs. Tracking queued entries by id in wrapped clearTimeout and cancelAnimationFrame would fix both; note too that frames(), waitForRest and touchStoryboard wait on this gated requestAnimationFrame, so they never resolve while frozen.
| animation.currentTime = Math.min(t, end - inset); | ||
| } | ||
| const style = getComputedStyle(element); | ||
| return {t, transform: style.transform, opacity: style.opacity}; |
There was a problem hiding this comment.
For a sample at or past the end this returns the requested t, but the value was read at end - 0.5 ms (line 178), so compareCurve checks the oracle half a millisecond late: a linear 300 ms move of 200 px reads 199.667 at t = 300, 0.333 px off against a 0.05 px tolerance. Returning the time actually applied (or a separate presentedAt) would keep samples and oracle aligned; the storyboard frame labels have the same issue.
There was a problem hiding this comment.
Changes requested: the curve-sampling and storyboard instruments pass in Chromium, but the CI wiring, the stacked diff and several instrument edge cases need fixes before this can land.
- Feed
pr-interactioninto an existing required join, astest-native-swiftfeedstest, because today a failing proof leaves every required check green, whichdocs/specs/AST-030/spec.mdFR7 and FR11 do not allow. - Land #8043 first and rebase, so this pull request shows only its own commits (1,687 lines instead of 3,158).
holdAnimationsseeks every animation in scope to the same local time, so a finishedfill: bothanimation replays in storyboards, staggered starts collapse, and a scroll-driven animation such asDateInput's wheel makes the seek throw.- The timer gate still runs callbacks cancelled with
clearTimeoutorcancelAnimationFramewhile frozen, an end sample is reported at the requested time although it is read 0.5 ms earlier, andpnpm storyboardsexits 1 because no test is tagged@storyboard.
[Automated review]
| # them, so a change to an instrument is proven by its negative controls | ||
| # before a component spec relies on it. Chromium only on these runners; | ||
| # see the harness README for running WebKit. | ||
| pr-interaction: |
There was a problem hiding this comment.
No required join reads this job: test needs check-scope, test-ui, test-node, test-build, registry-contract and test-native-swift (line 887), and build needs build-storybook, build-sandbox and typecheck (line 1264), so a failing proof leaves every required check green. docs/specs/AST-030/spec.md FR7 and FR11 ask new coverage to join an existing owner; feeding test as test-native-swift does (its needs plus .github/scripts/ci-test-join.mjs) would conform.
| "test:a11y-contract": "playwright test --config playwright.config.ts --project chromium", | ||
| "test:docsite-browser": "playwright test --config apps/docsite/playwright.config.ts", | ||
| "test:interaction": "playwright test --config playwright.interaction.config.ts", | ||
| "storyboards": "playwright test --config playwright.interaction.config.ts --grep @storyboard", |
There was a problem hiding this comment.
No test carries @storyboard yet, so this exits 1 with Error: No tests found. Tagging the storyboard self-tests (test.describe('storyboards', {tag: '@storyboard'}, …)) or adding --pass-with-no-tests would make it run.
| const realInterval = window.setInterval.bind(window); | ||
| const realFrame = window.requestAnimationFrame.bind(window); | ||
| const hold = run => (gate.frozen ? gate.queue.push(run) : run()); | ||
| window.setTimeout = (fn, ms, ...args) => realTimeout(() => hold(() => (typeof fn === 'function' ? fn(...args) : 0)), ms); |
There was a problem hiding this comment.
Once the real timer fires while frozen, the wrapped callback sits in gate.queue, and the page's clearTimeout (not wrapped) can no longer cancel it, so thaw runs a callback the component cancelled. The same holds for cancelAnimationFrame with line 115, whose queued callback also receives the stale frame time because frozen is already false when the queue runs.
| (node.classList.length | ||
| ? `.${[...node.classList].slice(0, 2).join('.')}` | ||
| : ''); | ||
| const all = document.getAnimations().filter(animation => { |
There was a problem hiding this comment.
This holds every animation document.getAnimations() returns, not only running ones, and the seeks then give each the same local time: a finished fill: both or forwards animation replays from its start (a fade-in that ended before the transition is filmed at opacity 0, 0.33 and 0.67), a scroll-driven animation (DateInput's wheel uses animation-timeline: view(y)) makes the seek throw The provided double value is non-finite, and animations started at different times lose their offset. Leaving finished and non-document-timeline animations untouched and seeking each by its offset from the hold would keep the frames to states the page presents.
| } | ||
| const {a, b, c, d, e, f} = matrix; | ||
| return { | ||
| t, |
There was a problem hiding this comment.
For a sample at or past the end this returns the requested t, but the value was read at end - 0.5 ms (line 252), so compareCurve checks the oracle half a millisecond late: a linear 300 ms, 200 px move fails a 0.05 px tolerance at t = 300 (199.667 against 200). Returning the time actually applied would fix the comparison and the storyboard labels.
There was a problem hiding this comment.
Changes requested: the curve-sampling instruments need no spec decision and the reverse() hold and playback-rate fixes work, but waitForAnimation and watch.started() still report a run going backward at the wrong time. describe places a backward run from the moment of the look and adds the forward delay (internal/interaction-harness/src/curves.ts:840-865), so a reverse()d move with a 300 ms delay is reported about 300 ms late, a short one that ended before the look gets a to after its own finishedAt, and one the page cancels when it ends is reported as starting there. Anchoring a backward run on startTime minus endTime / |playbackRate|, in describe and in the recorder's saved start, and taking from and to from endDelay and endTime - delay fixes it, with a self-test for each case.
- The red
testcheck comes frombuild-theme.icons-specifier.test.mjs, which this pull request does not touch; a re-run should clear it.
[Automated review]
| const rate = a.playbackRate; | ||
| const startedAt = | ||
| rate < 0 && a.currentTime != null | ||
| ? performance.timeOrigin + | ||
| Number(document.timeline.currentTime ?? performance.now()) - | ||
| (Number(timing?.endTime ?? 0) - Number(a.currentTime)) / | ||
| Math.abs(rate) | ||
| : performance.timeOrigin + | ||
| Number(a.startTime ?? recordedStart ?? Number.NaN); | ||
| const name = | ||
| 'transitionProperty' in a | ||
| ? (a as unknown as {transitionProperty: string}) | ||
| .transitionProperty | ||
| : 'animationName' in a | ||
| ? (a as unknown as {animationName: string}).animationName | ||
| : null; | ||
| return { | ||
| kind: a.constructor.name, | ||
| name, | ||
| // Through the playback rate's speed: at rate 2 a curve runs in half | ||
| // its duration. | ||
| from: startedAt + Number(timing?.delay ?? 0) / Math.abs(rate || 1), | ||
| to: startedAt + curveEnd(a) / Math.abs(rate || 1), |
There was a problem hiding this comment.
A backward run is placed from the moment of the look, with the forward delay added. A reverse()d 1,000 ms move with a 300 ms delay is reported about 300 ms late; a 60 ms one that ended before the look gets a from about 200 ms after it started and a to past its own finishedAt; one the page cancels when it ends falls back to the recorder's start, which startTime put at its end. Anchoring a backward run on Number(a.startTime) - endTime / Math.abs(rate) (here and in the recorder's saved start at lines 71-72 and 102-104), and taking from and to from endDelay and endTime - delay, fixes it.
Chromium gives a run going backward a startTime where its current time would reach 0, a whole run after it began. waitForAnimation and watch.started() now anchor it at startTime minus endTime over the speed, in describe and in the recorder's saved start (so one the page cancels when it ends keeps its real start), and take from/to from the end delay and endTime minus the delay, which play first and last going backward. A self-test covers a delayed reverse(), one that ended before the look, and one cancelled when it ends; it failed by 296 ms before (Chromium, 27/27 and 5x repeat pass).
There was a problem hiding this comment.
Changes requested: the curve-sampling instruments need no spec decision and the fix that dates a backward run from when it began works, but at 2,043 authored lines this pull request is over the 2,000-line scope limit, and the animation-state waits (the recorder, waitForAnimation, watchAnimations and waitForRest, with their animation state self-tests) can land on their own. Please land the waits in their own pull request first and curve sampling on top, or explain in the description why the two cannot land apart, and fix two readings with a self-test each:
finishedAtread before the finish event is delivered (right afterfinish()) is a whole run late for areverse()d run and 500 ms late at playback rate 2 (internal/interaction-harness/src/curves.ts:937-946).minProgressMscounts a backward run'sendDelayas progress (curves.ts:920-928).
[Automated review]
| const finishedAt = (e: (typeof recorded)[number]) => | ||
| e.finishedAt ?? | ||
| (e.animation.playState === 'finished' && e.animation.startTime != null | ||
| ? Number(e.animation.startTime) + | ||
| Number( | ||
| ( | ||
| e.animation.effect as KeyframeEffect | null | ||
| )?.getComputedTiming().endTime ?? 0, | ||
| ) | ||
| : null); |
There was a problem hiding this comment.
Read before the finish event is delivered (as right after a script's finish()), this fallback is late for anything but a forward run at rate 1: a reverse()d 1,000 ms run reported finishedAt 1,000 ms after it finished, and a forward run at playbackRate 2 reported it 500 ms late (Chromium, 6 of 6 each). Computing it the way playStart does, startTime going backward and startTime + endTime / rate forward, with a self-test that calls finish() and reads at once, would fix it.
| const progressed = running.filter(a => { | ||
| const timing = a.effect?.getComputedTiming(); | ||
| const time = Number(a.currentTime ?? 0); | ||
| const local = | ||
| a.playbackRate < 0 | ||
| ? Number(timing?.endTime ?? 0) - time | ||
| : time - Number(timing?.delay ?? 0); | ||
| return local / Math.abs(a.playbackRate || 1) >= minProgress; | ||
| }); |
There was a problem hiding this comment.
Going backward, endTime - currentTime counts the endDelay, which plays first and presents nothing: a reverse()d 1,000 ms run with a 300 ms endDelay resolved watch.started({minProgressMs: 100}) 105 to 126 ms in, with translate still none. Subtracting endDelay here, as the forward branch subtracts delay, would match the JSDoc's "after any delay".
| const remember = (animation: Animation) => { | ||
| // A running animation is recorded while still pending: on a cold page the | ||
| // next frame can come only after a short animation has finished. | ||
| if (seen.has(animation) || animation.playState !== 'running') { |
There was a problem hiding this comment.
Minor: each animation object is remembered once, so a watch taken after a fade has run under an earlier watch rejects with "no animation started on the element" when the page plays the same object again. Recording a replay as a new run, or noting in the watchAnimations JSDoc that a watch reports only objects first seen after it, would close it.
…e, and a backward end delay is not progress waitForAnimation's fallback finish time (read before the finish event is delivered, right after finish()) now goes through the playback rate's speed, and for a run going backward is its startTime; it was a whole run late for a reverse()d run and 500 ms late at rate 2. minProgressMs no longer counts a backward run's end delay, which plays first and presents nothing. Self-tests for both; the finish case failed by 1,000 ms before (Chromium, 27/27 and 5x repeat pass).
There was a problem hiding this comment.
Changes requested: the curve-sampling instruments need no spec decision and the two backward-run fixes work, but at 2,085 authored lines this pull request is over the 2,000-line scope limit, and the animation-state waits can land on their own. Please land the recorder, waitForAnimation, watchAnimations and waitForRest with their animation state self-tests in their own pull request first and curve sampling on top, or explain in the description why the two cannot land apart, and fix one more backward-run reading with a self-test:
waitForRestreports rest during areverse()d run's end delay, before its curve plays: a 600 ms run with a 500 msendDelayresolved 49 ms in and then moved from 100 px back to 0 (internal/interaction-harness/src/curves.ts:1063-1070).
[Automated review]
| (a.playState === 'running' && | ||
| Number(a.currentTime ?? 0) < curveEnd(a)), |
There was a problem hiding this comment.
Going backward, a run starts at its end time, which includes its endDelay, so through the whole end delay its current time sits above curveEnd (delay plus active duration) and it counts as still. A reversed 600 ms run with a 500 ms endDelay was reported at rest 49 ms in and then moved from 100 px back to 0. Counting a backward run as moving while its current time is above its delay, with a self-test, would match the README's "waitForRest waits the hold out".
…d curve A run going backward plays its end delay first, then its curve down to the start delay; waitForRest counted it as done once its current time passed curveEnd, so it resolved inside the end delay (47 ms into a 600 ms run with a 500 ms end delay) and the element then moved. It now waits until the current time reaches the start delay. Self-test added (Chromium 27/27, 5x repeat).
There was a problem hiding this comment.
Changes requested: the curve-sampling instruments need no spec decision and the waitForRest fix for a reversed run's end delay works, but at 2,097 authored lines this pull request is over the 2,000-line scope limit, and the animation-state waits can land on their own. Please land the recorder, waitForAnimation, watchAnimations and waitForRest with their animation state self-tests in their own pull request first and curve sampling on top, or explain in the description why the two cannot land apart.
- Optional: a watch misses an animation object the page plays again (
internal/interaction-harness/src/curves.ts:80), and the timeout message says "none running" while a run sits inside its delay (curves.ts:968-974).
[Automated review]
| if (Date.now() > deadline) { | ||
| throw new Error( | ||
| 'waitForAnimation: no animation started on the element, none running ' + | ||
| 'and none recorded since the last wait. If the action is the ' + | ||
| "page's first contact with the harness, take " + | ||
| 'watchAnimations(locator) before it.', | ||
| ); |
There was a problem hiding this comment.
Minor: this message also fires while an animation is running but has not yet presented minProgressMs, as inside its delay: waitForAnimation(box, {timeout: 300}) threw "none running" while a fade inside a 1,000 ms delay was running at 283 ms. Naming a running animation that is short of minProgressMs here would keep the message true.
|
The backward self-test moved to #8505 with the waits, so the fix will be offered there. Offered as On scope (2,097 lines, 936 of them self-tests): the animation-state waits and curve sampling read one model of a run: its start on the document timeline, its direction and playback rate, and where a held end begins ( |
…ll request This pull request keeps curve sampling (hold, seek, sample, release, the curve end); the recorder, waitForAnimation, watchAnimations and waitForRest, with their self-tests, land separately on top. The holding self-tests that waited with them now poll the page for a running or idle animation. Chromium: 19 tests, 5x repeat, pass.
There was a problem hiding this comment.
Recommended for approval: the curve-sampling instruments need no spec decision, and with the animation-state waits moved to #8505 this pull request is 1,369 authored lines. Before landing, please update the description, which becomes the squash commit's body: it still lists waitForAnimation, waitForRest, watchAnimations and math.ts with their evidence, and test counts from before the split. Optionally, leave an animation the page froze with playbackRate = 0 out of holdAnimations (internal/interaction-harness/src/curves.ts:210): today it is seeked as if it ran at rate 1 (curves.ts:328, :483), and releaseAnimations({finish: true}) then throws and leaves the other held animations paused.
[Automated review]
| const target = | ||
| (animation.effect as KeyframeEffect | null)?.target ?? null; | ||
| return ( | ||
| animation.playState === 'running' && |
There was a problem hiding this comment.
Optional: Chromium and WebKit report an animation the page froze with playbackRate = 0 as running, so it is held here and then seeked as if at rate 1 (playbackRate || 1 at lines 328 and 483): a move frozen at 73.3 px samples 20 px at t = 100 and stays there after release. finish() also throws for it at line 371, leaving the other held animations paused. Skipping animation.playbackRate !== 0 here, as for animations the page paused, with a self-test, would close it.
# Conflicts: # internal/interaction-harness/README.md # internal/interaction-harness/src/curves.ts # internal/interaction-harness/src/harness.interaction.browser.spec.ts
…te waits) into the storyboards # Conflicts: # internal/interaction-harness/README.md
…frame evidence
…recorder stand-down # Conflicts: # internal/interaction-harness/src/curves.ts
…ded) as a dependency
Maintainer impact
Animation timing isn't verifiable today. Specs wait a fixed time and read one transform (the BottomSheet drag spec waits 40 ms, then checks > 5 px), so a curve can change shape, duration or starting velocity without a red test. Matching a measured reference curve needs the presented value at exact times and an oracle independent of the implementation.
Intended invariant
No product change. A curve claim is checked against the presented value at fixed times, never against wall-clock frames.
Change
curves.ts:presentedAt, from the sampled element's own animations, just inside an end).waitForAnimation,waitForRest. The page records every animation from its first frame or its creation, sowaitForAnimationalso reports a short one that ended before the first look.watchAnimationsarms the record before an action.math.ts: oracles that share no code withpackages/core:cubicBezier,timingFunction,springStep,projectDecay,rubberBand,parseTransform,compareCurve.packages/themes/*/src/.Second of four stacked pull requests, on #8043 (touch input). Storyboards (#8348) and frame evidence (#8045) follow. Until #8043 merges, review this pull request's own commits.
Evidence
linear()matches the SwiftUI oracle within 0.15 px.Scope
Testing
pnpm test:interaction --project chromium(harness): 18 passed.node internal/interaction-harness/scripts/webkit-container.mjs: 16 passed, 2 skipped (native fling; partial lift).pnpm exec vitest run internal/interaction-harness: 23 passed;.github/scripts/ci-interaction.test.mjspasses.