Repository navigation
test(interaction-harness): a hold the page arms itself takes a short animation on its first frame - #9230
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.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This looks ready to merge: armHold is a contained addition to the internal harness, and holdAnimations keeps its hold body unchanged. A few small follow-ups would make it stronger:
- In the new armed-hold test, read
hold.held()after a wait longer than the 120 ms run, so the test proves the page took the hold itself. - Define
__harnessHoldin the same evaluate that installs__harnessCurveEnd, soholdAnimationsreaches the page no later than before. - Say in
armHold's JSDoc that an unscoped hold still pauses the busy-window control, and listarmHoldin thecurves.tsheader.
[Automated review]
| {duration: 120, easing: 'linear'}, | ||
| ), | ||
| ); | ||
| const held = await hold.held(); |
There was a problem hiding this comment.
Reading held() right after the action lets a hold taken at read time pass on an idle runner. A wait longer than the run, such as await page.waitForTimeout(200);, before this line makes the test prove the page held the 120 ms run itself.
| */ | ||
| async function installHold(page: Page): Promise<void> { | ||
| await installCurveEnd(page); | ||
| await page.evaluate(() => { |
There was a problem hiding this comment.
This extra page.evaluate runs before every holdAnimations hold, so callers that still hold after an action reach the page one round trip later than before. Defining __harnessHold in the evaluate that installs __harnessCurveEnd keeps the base's number of round trips.
0bfda6b to
d658a05
Compare
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
There was a problem hiding this comment.
This looks ready to merge: armHold is a contained addition to the internal harness, holdAnimations keeps its hold body and round trips, and the changed holding, curve-sampling and storyboard self-tests pass in both Chromium and WebKit. One optional follow-up: animationStoryboard's JSDoc still says it holds every running animation and must be called right after the start, so say there that held replaces that hold and that scope is ignored when held is passed.
[Automated review]
| options: AnimationStoryboardOptions, | ||
| ): Promise<{storyboard: Storyboard; held: HeldAnimation[]}> { | ||
| const held = await holdAnimations(page, options.scope); | ||
| const held = options.held |
There was a problem hiding this comment.
With held passed, this skips the hold and ignores options.scope, while the JSDoc above still says the function holds every running animation and should be called right after the start. A sentence there saying held replaces the hold and scope would keep a caller from passing both.
…wn clock The bottom placement's two change-of-mind proofs turned the field with a scripted tap, or a focus, sent once the first motion had run a while. A busy runner delivered it late, near or past the gel's peak, and there the proofs checked only part of the claim and recorded a "reduced" annotation instead of "keeps going, then turns once". Each proof now arms the turn in the page before the input that starts the first motion: on the first frame, or 4 ms timer, at which the trailing end's new motion has run a set time on its own clock (120 ms of the activation; the cancel's first frame), the page clicks the close button or focuses the field. The first inputs start in the page too, since a scripted tap's dispatch can keep the page from looking at the motion for hundreds of milliseconds as it starts. The turn's own motion is held by an armed hold that takes only animations starting after it (armHold's newOnly), once they have run as long as the turn may start at the latest, so no round trip races a short turn. The reduced branch is gone: every run checks the whole claim, including that the turn started at least two frames before the peak. Depends on meta#9230, which adds armHold's newOnly to the interaction harness.
…animation on its first frame `holdAnimations` reaches the page after a round trip, and on a loaded runner a short animation can end before it arrives: component proofs that hold a 200 ms press release miss it on a two-core Chromium pin, and main's own "a second hold" self-test fails that way in WebKit at 16 workers on 3 CPUs and on a two-core pin with CPU-bound processes on the same cores. `await armHold(page, scope)` arms the page before the action; it holds every running animation under the scope as soon as one of them runs (or `count` of them, for parts that start apart; with `newOnly`, only animations that start after the arm, so a turn that starts mid-motion is held while the motion goes on), and `held()` after the action returns what it held. It holds through the same in-page body as `holdAnimations`, installed in the evaluate that installs the curve end so a hold costs no extra round trip, and into the same held set, so `seekAnimations`, `sampleCurve` and `releaseAnimations` read it unchanged, and a later hold adds to it. It looks on every frame callback and every 4 ms between them: on a cold page WebKit can deliver no frame callback for about 70 ms while it first composites a newly animated layer, while its timers run and its animations advance. The busy-window control never counts (an unscoped hold still pauses it, as it takes every running animation), and `held()` rejects when nothing under the scope ran before the timeout. `animationStoryboard` films what an armed hold took (`held`) instead of holding after the action. The holding, curve-sampling and storyboard self-tests that hold a short animation an action starts arm their holds. Three that hold after a wait or a watch, or hold two runners started 100 ms apart (which an armed hold would take one at a time), run their animations for 6 to 10 s instead of 0.6 to 1 s at the same speed. The two-runner test also makes its second runner once the first has started, so a loaded engine cannot give both one first frame, and seeks 150 ms past the second start, however late that came. The staggered-starts filmstrip sets its second start 83 ms after the first on the document timeline rather than with a timer, and the page holds all three runs once the second move runs (`count: 3`). Three new self-tests cover the armed hold: a 120 ms run held on its first frames, a turn held mid-motion with `newOnly` (both read only after the run would have ended), and a rejection when nothing starts. Red first: component proofs that hold a 200 ms press release fail on a two-core Chromium pin when they hold after the lift and pass with armed holds; on main, a second hold fails in WebKit at 16 workers on 3 CPUs, and the holding describe in Chromium under the load above; with the armed holds they pass there.
d658a05 to
9ebf996
Compare
cixzhang
added
the
ci:run
There was a problem hiding this comment.
This looks ready to merge: armHold is a contained addition to the internal harness, holdAnimations keeps its hold body and round trips, and the changed curve-sampling, holding and storyboard self-tests pass in both Chromium and WebKit. One optional follow-up: when held() times out it always says no animation ran, even when some ran but fewer than count (or none reached minProgressMs), and a second held() says the hold was never armed; saying how many ran and that the hold was already read would make those failures easier to debug.
[Automated review]
| done = true; | ||
| reject( | ||
| new Error( | ||
| `armHold: no animation under the scope ran within ${wait} ms`, |
There was a problem hiding this comment.
This message is also what a hold gets when some animations ran but fewer than count, or none reached minProgressMs: with count: 2 and one animation running it still says none ran. Saying how many ran against count would point at the real cause.
Status at 9ebf996 (rebased onto main eae4b9e): every run below has finished at this head. WebKit runs use the CI runner's render-thread count (
LP_NUM_THREADSset to the container's CPUs); WebKit reds an earlier version of this description listed came from containers without it and are withdrawn.Maintainer impact
A proof that holds an animation to seek it, sample it or film it (
holdAnimations,sampleCurve,animationStoryboard) sends the hold from the test after the action that starts the animation. The hold reaches the page one round trip later, and on a loaded runner a short animation can end before it arrives: the hold comes back empty, and the proof fails on a correct build. Component proofs that hold a 200 ms press release fail this way on a two-core Chromium pin, and main's own "a second hold adds to the held set, and releasing resumes both" fails this way in WebKit at 16 workers on 3 CPUs and in Chromium with CPU-bound processes on the pinned cores.Intended invariant
A held animation is held before it can end, however busy the runner: the hold is taken on the page itself, as soon as the animation runs, and joins the same held set
holdAnimationsbuilds, soseekAnimations,sampleCurve,releaseAnimationsandanimationStoryboardread it unchanged (README, "Curve sampling").Change
armHold(page, scope)arms the page before the action and resolves once it is armed;held()after the action returns what the page held. The page holds every running animation under the scope (the whole document without one) as soon as one of them runs, or as soon ascountof them run, for parts that start apart. WithnewOnly, only animations that start after the arm trigger and join the hold, so a turn that starts mid-motion is held while the motion already running goes on. It holds through the same in-page hold body asholdAnimations, which is now installed in the evaluate that installs the curve end, soholdAnimationstakes no extra round trip. The busy-window control never triggers a hold, but an unscoped hold, which takes every running animation, pauses it too.held()rejects when nothing under the scope ran within the timeout.animationStoryboardtakesheld, what an armed hold took, instead of holding after the action.linear(), a sample at an end, a sample labelled per channel, a pseudo-element's animation, a percentage translate, a second hold, a held transition the page retargets, finishing an endless animation, the animation filmstrip, the endless filmstrip, and the timer-gate filmstrip (whose gate is now frozen before the component's timer is set, so the timer cannot fire before the freeze).count: 3).newOnlywhile a 10 s motion goes on (both read only after the run would have ended); and a rejection when nothing starts.Prior art: open pull requests whose own diffs touch these four files were checked for the hold, the held set and the storyboard's options; none changes the hold. #9163, #8554 and #8446 edit the README's
curves.tsrow that this change also edits (a one-line rebase for whichever lands second), and #8446 and #8554 change the recorder incurves.ts. #8549 addscastStoryboardbesideanimationStoryboard. #8911 adds a self-test that holds a short animation after the action, which an armed hold would steady. #9185, #9215, #9233, #9235, #9244 and the recorder change for CSS runs that start and finish between frames, merged since, changed other parts of the harness; this head is rebased onto them and merges cleanly.Evidence
held(),holdAnimationsoranimationStoryboardread in the 16 holding, curve-sampling and storyboard tests that hold a short run (the late read a starved runner gives) passes 16 of 16 with this change in both Chromium and WebKit, and fails 16 of 16 on main in both. The timer-gate filmstrip with its read 200 ms after the component's 40 ms timer was set: 5 of 5 here, red on main ("no running animation to hold"). The two-runner offsets test with a 100 ms busy block instead of its timer wait: 5 of 5 here, 5 of 5 red on main (start gap 0). These probes come from an independent run against an earlier head of this change.CI=1, one worker) ×10 main passes these describes (240 of 240).Scope
Testing
pnpm exec playwright test --config playwright.interaction.config.ts --project chromium internal/interaction-harness/src/harness.interaction.browser.spec.ts --grep "curve sampling|holding|storyboards"CI=1 node internal/interaction-harness/scripts/webkit-container.mjs --config playwright.interaction.config.ts --workers=1 --repeat-each=10 internal/interaction-harness/src/harness.interaction.browser.spec.ts --grep "curve sampling|holding|storyboards", with the container capped at 4 CPUs andLP_NUM_THREADS=4Local gates at 9ebf996: Prettier, ESLint, the harness and core typechecks (core's interaction specs included) pass, and the harness and CI-script node tests pass (117 of 117).