Repository navigation
test(Slider): the busy-main-thread coast proof runs in WebKit on the paced touch play - #9694
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.
|
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| Slider · Storybook | 92.3 kB (0) · 8.3 kB CSS (0) | 2,333 (0) | ✓ · ✓ · ✓ |
Package size: no published package changed; no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 9 rows, core 3 rows; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | 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.
Changes requested: the busy proof's new step-counted rule needs one fix, and the rest of the change reads well.
- Under
settledWithinSteps, the still-run allowance ininternal/interaction-harness/src/busy.ts(lines 987-1000) takes the slower of the paces around a run, sobusy.webkittail.fixture.tsheld at 212 px for 82 ms (four 2 px steps from rest) and then resumed passes, whilesettledWithin: 4fails it as an 82.2 ms stall; capping the allowance atceil(step / max(paceBefore, catchUp / runFrames)) + 1fails that hold, keeps every existingbusy.test.tsandbusy.control.test.tscase passing, and the hold makes a good must-fail case.
The paced play's release reading matches Core's VelocityTracker, and running the Slider proof in WebKit is what AST-094 asks for. Two smaller points: the touchPacingFault and PlayOptions.pacing JSDoc and the README's engine-limits row leave out the 'throw' fault, and a harness test that compares readRelease with VelocityTracker would stop the copied filter from drifting. CI skipped this draft, so the WebKit interaction lane still needs to pass once it is marked ready.
[Automated review]
| const moving = [paceBefore, pxPerFrameAfter(k)].filter( | ||
| (value): value is number => value != null && value > 0, | ||
| ); | ||
| const travel = | ||
| moving.length === 0 | ||
| ? null | ||
| : Math.max( | ||
| Math.min(...moving), | ||
| runFrames > 0 ? catchUp / runFrames : 0, | ||
| ); | ||
| if (castStepPx != null && travel != null && travel < castStepPx) { | ||
| // The ratio is read to a millionth: a pace of 0.5 that sums to | ||
| // 0.4999… still allows two frames, not three. | ||
| return unchanged <= Math.ceil(castStepPx / travel - 1e-6); |
There was a problem hiding this comment.
travel takes the slower of the paces before and after the run, so a freeze that resumes a slow tail gets the tail's long allowance. On busy.webkittail.fixture.ts, holding 212 px from +115.4 ms to +197.6 ms (rest is 220 px) and then replaying the rest of the coast passes under settledWithinSteps: 2, while settledWithin: 4 (the same 4 px bound on this cast) fails it with "longest stall 82.2 ms". Capping the allowance at one frame past what the pace before the run allows fixes that hold and keeps all 50 busy.test.ts and 24 busy.control.test.ts cases passing:
const beforeCap =
Math.ceil(
castStepPx /
Math.max(paceBefore ?? 0, runFrames > 0 ? catchUp / runFrames : 0) -
1e-6,
) + 1;
return (
unchanged <= Math.min(Math.ceil(castStepPx / travel - 1e-6), beforeCap)
);There was a problem hiding this comment.
Done in #9545's 9304656, which this pull request now sits on: the still-run allowance is capped at one frame past what the pace into the run allows, as you suggest. Your hold (212 px from +115.4 to +197.6 ms, the coast replayed four frames late) is a must-fail case there; it passed before and fails with "longest stall 82.2 ms", as settledWithin: 4 does, and the other busy cases still pass. The Slider proof here re-ran against the cap: 10 of 10 in WebKit in the CI job's profile, 20 of 20 in Chromium.
cf530ed to
de951a8
Compare
|
The two smaller points: the |
There was a problem hiding this comment.
Changes requested: the still-run allowance fix works, and one scope item remains.
- This branch targets
mainbut carries #9545's seven harness commits, so it is 2,042 authored lines (over the 2,000-line limit) where its own Slider change is 20; land #9545 first, then rebase this branch onmainso it carries only the Slider spec commit.
The capped still-run allowance now fails a WebKit coast that stalls and then resumes a slow tail, and the fault docs name throw. Two small follow-ups: the README's still-run paragraph and the busyWindowProof JSDoc still describe the allowance without the new cap, and a Core-side unit test comparing readRelease with VelocityTracker would guard the copy from both sides, since Core may import the harness. CI skipped this draft, so the WebKit interaction lane still needs to pass once it is marked ready.
[Automated review]
| * nearest step. The run also has to read as a slowing | ||
| * curve, at most one frame longer than the component's run before it. | ||
| * Under `settledWithinSteps`, a run under one cast step a frame may instead | ||
| * last ceil(step ÷ travel) frames, whatever the run before it. The travel is |
There was a problem hiding this comment.
Non-blocking: this sentence, and README.md lines 228-230, still describe the allowance as ceil(step ÷ travel) frames "whatever the run before it". Since the cap at lines 1001-1011, a run may also last at most one frame more than the pace into it allows; worth saying here and in the README.
|
This pull request is stacked on #9545. A fork's pull request can't target #9545's branch, so it targets |
de951a8 to
57b4be0
Compare
|
#9545 is now split, so this pull request depends on #9545 and #9725, and its compare link shows only its own commit. The Core-side cross-check you suggested is in #9725: |
There was a problem hiding this comment.
Changes requested: one scope item remains, and the earlier documentation and drift-guard points are addressed.
- This branch targets
mainbut carries the harness commits of #9545 and #9725, so it is 2,181 authored lines (over the 2,000-line limit) where its own Slider change is 20; land #9545 and #9725 first, then rebase this branch onmainso it carries only the Slider spec commit.
The README and the busyWindowProof doc now state the still-run cap, and VelocityTracker.harness.test.ts catches a change to the copied filter's weight and sample rules. One small follow-up: its only rest case lifts 88 ms after the last move, so a harness stillness interval anywhere from 40 to 87 ms still passes, and a rest 41 ms after the last move (or a direct comparison of TOUCH_STILLNESS_MS with UIKIT_VELOCITY.stillness) would pin it. CI skipped this draft, so the WebKit interaction lane still needs to pass once it is marked ready.
[Automated review]
| [116, 50, 16], | ||
| [132, 50, 32], | ||
| ], | ||
| lift: 120, |
There was a problem hiding this comment.
Non-blocking: this is the only rest case, and it lifts 88 ms after the last move, so a harness TOUCH_STILLNESS_MS anywhere from 40 to 87 ms still passes all seven cases (the harness's own boundary tests read that constant, so they move with it). A rest that lifts 41 ms after the last move, or a direct expect(TOUCH_STILLNESS_MS).toBe(UIKIT_VELOCITY.stillness.value) beside the weight, would pin it.
There was a problem hiding this comment.
Done in #9939 on main: two gestures lift exactly at UIKIT_VELOCITY.stillness and 1 ms past it, and a check confirms that the tracker reads a speed at the first and none at the second. A harness copy with a 39 or 41 ms interval, or a >= bound, now fails one case.
57b4be0 to
464ea7f
Compare
There was a problem hiding this comment.
Changes requested: the new WebKit run of the Slider busy proof can fail on a loaded runner without naming the runner.
- Miss: the flick plays with
pacing: 'spacing'but noreleaseTolerance(SliderTouch.interaction.browser.spec.ts:1330), so when each WebKit dispatch outlasts the script's 8.3 ms interval the page reads about 245 px/s instead of 840 px/s, the coast is too short to judge, and the proof fails with "too little motion in this window to judge" instead of the named runner fault the description promises; pass areleaseTolerance(0.5 turned every such run intoTouchPacingError: … a dispatch stalled before the page) or narrow the stated invariant.
The earlier size item is resolved: the branch is now one 20-line commit on main. Note that CI's WebKit lane does not run Slider specs, so this proof's WebKit runs stay local; once the branch is ready, a maintainer adds the ci:run label so CI runs the Chromium proof.
[Automated review]
| // stalled dispatch. A page that read a resting finger, or no throw, | ||
| // fails the play and names whose fault it was. | ||
| await driver.play(flick(at, {velocity: {x: 900, y: 0}, durationMs: 60}), { | ||
| pacing: 'spacing', |
There was a problem hiding this comment.
With no releaseTolerance, a stretched WebKit flick (each dispatch slower than the 8.3 ms interval) reads about 245 px/s instead of 840 px/s and still passes the play. The short coast then fails the proof with "too little motion in this window to judge", which names neither the coast nor the runner. A tolerance turns it into a named TouchPacingError:
await driver.play(flick(at, {velocity: {x: 900, y: 0}, durationMs: 60}), {
pacing: 'spacing',
releaseTolerance: 0.5,
});There was a problem hiding this comment.
I left the tolerance out because a 0.5 bound fails healthy runs. In WebKit in the CI job's profile (4 CPUs, ten runs per arm, alternating), the play with your snippet failed 4 and then 5 of 10, each a TouchPacingError reading 269 to 2,227 px/s against the script's 910. Without it, 9 of 10 passed.
The one red there was a BusyWindowTooLateError: the coast was 63 ms in when the page first saw it, past its curve's end at 58 ms. The one red in 100 runs of a local probe gave the same numbers, and there the page had read a bunched 6,493 px/s release. The paced play sent each touch its interval after the previous one left, so touches WebKit took late reached the page 1 to 3 ms apart, and the coast hit the track's end. #9956 fixed that: each touch now also waits its interval, less 2 ms, after WebKit answers the previous one. In the same probe, the 25 slowest releases (271 to 449 px/s) coasted 478 to 519 ms and passed, so "too little motion" did not occur.
At c544be2, on main with #9956, the busy proof passed 40 of 40 in WebKit in the CI job's profile.
464ea7f to
c544be2
Compare
There was a problem hiding this comment.
Changes requested: the Slider's new WebKit busy proof can still fail on a slow runner without naming the runner.
- Miss: with
pacing: 'spacing'alone (SliderTouch.interaction.browser.spec.ts:1330), a runner whose WebKit dispatch takes about 22 ms delivers the flick at about 250 px/s instead of 840 px/s, and 2 of 22 runs failed with "too little motion in this window to judge", which names neither the coast nor the runner as the description's invariant promises.
Thanks for the tolerance data: it rules out a release tolerance, so either keep driver.play's result and put the lift's release (page and script px/s) in the final expect message and the evidence JSON, or narrow the invariant to say a stretched throw can end in "too little motion to judge". The rest reads well: removing the skip, the paced play and the step-counted settle give the WebKit busy proof that AST-094 asks for, and the Chromium proof still passes.
[Automated review]
| // when it arrives, so the play keeps the script's spacing after a | ||
| // stalled dispatch. A page that read a resting finger, or no throw, | ||
| // fails the play and names whose fault it was. | ||
| await driver.play(flick(at, {velocity: {x: 900, y: 0}, durationMs: 60}), { |
There was a problem hiding this comment.
On a runner whose WebKit dispatch takes about 22 ms, this flick reaches the page at about 250 px/s instead of 840 px/s, and 2 of 22 runs failed "too little motion in this window to judge", which names neither the coast nor the runner. Since a tolerance fails healthy runs, keeping the play's result and showing its release in the failure would name the slow throw:
const played = await driver.play(
flick(at, {velocity: {x: 900, y: 0}, durationMs: 60}),
{pacing: 'spacing'},
);
// ...
const release = played.at(-1)?.release;
expect(
proof.verdict.failures,
release == null
? proof.summary
: `${proof.summary}; the page read ${release.pagePxPerS.toFixed(0)} px/s of the script's ${release.scriptPxPerS.toFixed(0)} px/s`,
).toEqual([]);There was a problem hiding this comment.
Done in 7fa7b33, with your snippet's message. The play's result is kept, and the verdict, the annotation and the evidence JSON carry the lift's release.
A local probe that added 20 ms after each WebKit answer reproduced your case. 1 of 22 runs failed "too little motion in this window to judge" on both heads, and on this one the failure also reads "the page read 184 px/s of the script's 910 px/s". On a healthy runner, the busy proof passed 40 of 40 in WebKit in the CI job's profile.
…paced touch play The proof plays its flick with `pacing: 'spacing'`, so a stalled dispatch stretches one gap instead of bunching the throw. A page that read a resting finger, or no throw, fails the play and names whose fault it was. It bounds the settle in two of the cast's own steps, so WebKit's 2 px plateaus near rest are not stalls. Its verdict names the release the page read beside the script's, so a throw that a slow runner stretched names the runner. The engine guard that kept it to Chromium is gone.
c544be2 to
7fa7b33
Compare
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet.
The Slider's busy-main-thread coast proof now runs in WebKit, and the earlier concern is resolved: a red caused by a slow runner's stretched throw now ends with the release the page read beside the script's, in the failure message, the annotation and the evidence file.
Next step: once the pull request leaves draft, a maintainer adds the ci:run label so CI runs. CI's WebKit lane does not plan Slider specs, so the WebKit run of this proof stays local.
[Automated review]
cixzhang
added
the
ci:run
#9545, #9725 and #9956 are merged, so this pull request is one commit on
main.Maintainer impact
#8190 shipped the Slider's busy-main-thread coast proof in Chromium only. Before #9725 and #9956, WebKit's injected touches could bunch behind a stalled dispatch, and its JPEG cast moves in 2 px steps, so the proof could not judge the coast there. This runs it in WebKit too, on #9725's paced touch play, which #9956 paces from WebKit's answers, and #9545's cast-step settle.
Evidence
BusyWindowTooLateError. test(interaction-harness): a WebKit touch play that does not watch each step waits for each answer, so no gap reaches the page more than 2 ms short #9956 fixed the play's pacing; this test needs no change for it.releaseTolerance: 0.5): the tolerance failed 9 of 20 healthy runs in the CI job's profile, each asTouchPacingError, so the play has none.Intended invariant
The busy proof judges the Slider's coast through a blocked main thread in both engines. It fails only for the coast, or names the runner's fault.
Change
pacing: 'spacing', so a stalled dispatch stretches one gap. A page that read a resting finger, or no throw, fails the play and names whose fault it was.settledWithinSteps: 2: 2 px in Chromium's cast, 4 px in WebKit's.Scope
Testing
Local gates at 7fa7b33, on
main70d6b46, on a Storybook build of thatmain: typecheck,CI=true pnpm lint(no errors, and no warnings in the changed spec), Prettier, changeset coverage (nothing ships),pnpm review:pre(no findings), the busy proof ×40 in WebKit in the CI job's container profile (40 passed) and ×20 in Chromium on a 2-thread pin (one physical core; 20 passed), the Slider's interaction specs in WebKit in the same profile (19 passed, 4 behind engine guards) and in Chromium (23 passed), the Slider's Chromium a11y contracts (30 passed), and the full unit suites (test:ui20,435 passed;test:node16,529 passed).