Repository navigation
test(interaction): real touch input for browser interaction proofs - #8043
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📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size SummaryNo component packages changed. Accessibility AuditStatus: No accessibility violations detected. Visual Regression48 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 28 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.
Changes requested: the touch harness works (its Chromium self-test passes on this head), but the new pr-interaction job runs as its own routed CI lane that no join requires, which docs/specs/AST-030/spec.md FR11 rules out for new coverage. Please fold the job into an existing owner, or drop it from this PR until an FR10 amendment admits it, and make the profiled-flick velocity match its documentation.
- Require
pr-interactionfrom an existing join (astestrequirestest-native-swiftthroughci-test-join.mjs) and add its own workflow file andpnpm-lock.yamlto its scope pattern, so edits to the job or a lockfile-only Playwright change still run the proofs. - With
profile,flicknever reaches the documented releasevelocity, andreleaseVelocityreports the 100 ms window mean (750 px/s for a 1,500 px/s linear-profile flick); align the code or the docs and add a profiled case toscripts.test.ts. - Minor:
src/index.tsnames a/mathsubpath that does not exist yet, the job comment promises negative controls the self-test does not have, and the README's "to the microsecond" timestamps arrive in 0.1 ms steps.
[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.
pr-interaction routes itself with its own path pattern and no join requires it, so a failing proof leaves test and build green. docs/specs/AST-030/spec.md FR11 asks new coverage to join an existing owner and not create a separately routed lane or required context: require this job from an existing join (as test requires test-native-swift through ci-test-join.mjs), or drop it until an FR10 amendment admits it.
| if git fetch --quiet --no-tags --depth=1 origin "$BASE_SHA" \ | ||
| && git show "$BASE_SHA:.github/scripts/pr-scope-touches.sh" > "$helper"; then | ||
| scope=0 | ||
| bash "$helper" "$BASE_SHA" '^(internal/interaction-harness/|playwright\.interaction\.config\.ts$|packages/[^/]+/src/|apps/storybook/)' || scope=$? |
There was a problem hiding this comment.
This pattern leaves out the job's own steps (this workflow file) and pnpm-lock.yaml, so a PR that edits only this job, or changes Playwright's resolution only in the lockfile, skips the proofs. native-swift-scope lists its own script in its pattern for the same reason; consider adding both paths here.
| /** | ||
| * A throw: the finger moves with the given velocity profile and lifts while | ||
| * still moving (the lift lands a quarter frame after the last move, as on a | ||
| * device), so the release velocity is exactly `velocity`. |
There was a problem hiding this comment.
With profile, each segment is integrated at its midpoint (line 161), so the finger never reaches velocity: a 1,500 px/s flick over 80 ms at 120 Hz ends at 1,425 px/s with p => p and 1,353.7 px/s with p => p * p. Either run the last segment at full velocity or limit this promise to the constant profile, and add a profiled case to scripts.test.ts.
| } | ||
|
|
||
| /** | ||
| * The velocity (px/s) of finger `id` at its lift: the slope over the last |
There was a problem hiding this comment.
This returns the mean slope over the last 100 ms, not the lift velocity: for a 1,500 px/s linear-profile flick it reports 750 px/s (498.7 px/s for a quadratic profile). A spec that derives an expected detent from it would be off by half; report the final segment's slope or document it as a window mean.
| * @input Re-exports the harness modules | ||
| * @output The `@astryxdesign/interaction-harness` entry point for Playwright | ||
| * specs. Pure helpers are also reachable without Playwright through | ||
| * `@astryxdesign/interaction-harness/math` and `/scripts` (unit tests). |
There was a problem hiding this comment.
@astryxdesign/interaction-harness/math is not in this package's exports and no such module exists at this head; drop the reference until the PR that adds it.
| Untrusted DOM events are not input: they start no native scroll, claim no | ||
| gesture, and read `isTrusted: false`. Chromium input goes through CDP | ||
| `Input.dispatchTouchEvent` with an explicit timestamp per event: the page's | ||
| `event.timeStamp` is the scripted time to the microsecond, whatever the |
There was a problem hiding this comment.
In Chromium the page saw a scripted 8.571 ms spacing as 8.5 to 8.7 ms, in 0.1 ms steps, so "to the microsecond" overstates it; "to 0.1 ms" matches what the page receives.
There was a problem hiding this comment.
Approval recommended: the touch harness's Chromium self-test passes in this head's CI, test now requires pr-interaction as docs/specs/AST-030/spec.md FR7 and FR11 ask, the scope decision's fail-closed edges are pinned by tests, and moving webkit-container.mjs out brings the pull request to 1,938 authored lines. Before merging, please update the description, which still lists scripts/webkit-container.mjs and a WebKit run through it. For the stacked pull requests, keep in mind that merge-queue runs pass pr-interaction without running the proofs.
[Automated review]
…ion-harness-1-touch
…r durations Since meta#8427 every job that can run on a pull request carries a `timeout-minutes` of at most 15, and `ci-duration` waits on every one of them to report run times. `pr-interaction` gets the budget's 15 minutes, like the other proof jobs, and `interaction-scope` 5, like the other jobs that only classify; both join `ci-duration`'s needs. `ci-duration-guard.test.mjs` failed three ways on this branch merged with main (no timeout on either job, 26 jobs expected in the report against 24 listed) and passes now.
There was a problem hiding this comment.
Recommending approval: pr-interaction now gates the required test check as docs/specs/AST-030/spec.md FR7 and FR11 require, its scope decision fails closed with tests covering those edges, and the harness self-test passed in this head's CI. Before the stacked component proofs land, please widen the interaction scope to theme sources (packages/[^/]+/src/ does not match packages/themes/<theme>/src/) and to .nvmrc and .github/actions/setup/, since themes can override touch motion and those files choose the Node the proofs run on. Keep in mind that merge-queue runs still pass pr-interaction without running the proofs.
[Automated review]
| && git fetch --quiet --no-tags --depth=1 origin "$BASE_SHA" \ | ||
| && git show "$BASE_SHA:.github/scripts/pr-scope-touches.sh" > "$helper"; then | ||
| scope=0 | ||
| bash "$helper" "$BASE_SHA" '^(internal/interaction-harness/|internal/a11y-spec/|playwright\.interaction\.config\.ts$|packages/[^/]+/src/|apps/storybook/|\.github/workflows/ci\.yml$|package\.json$|pnpm-lock\.yaml$)' || scope=$? |
There was a problem hiding this comment.
This reaches packages/<package>/src/ but not theme sources: [^/]+ stops at one directory, so packages/themes/<theme>/src/ reads as out of scope. No proof renders a theme at this head, but Storybook renders stories in those themes and a theme can override touch springs, durations and curves (docs/specs/AST-094-touch-presentations/spec.md FR4), so once component proofs land a theme-only pull request would skip them. Consider adding packages/themes/[^/]+/src/ here and a case in ci-interaction.test.mjs.
…nds down while a trace runs, and records animations that start and end inside it) into the animator's React hook and browser proofs
…table main's pr-interaction job checks out this branch's head and runs playwright.interaction.config.ts, which meta#8043 added on main.
…to the Reduce Motion policy
…nimator's React hook and browser proofs
…heme motion roles
…ole-reading animator
…ole-reading animator
meta#8076's head carries current main with meta#8043's harness, the motion foundation with AST-094 FR4's role names, and the CI duration guard's fix. Conflicts, resolved: - The harness, the gesture system, the motion tables, MEASUREMENTS.md, the motion README and the generated role kinds take meta#8076's side. The harness keeps this branch's trace option that leaves out Chrome's own compositor animation update (its README row and self-test, which ChatLayout's rendering proof uses). - The keyboard's notification duration takes the motion foundation's way to keep it out of the theme roles: it stays `timing.keyboardNotificationMs`, which the timing table excludes from registration, instead of moving to `keyboard.notificationMs`. The keyboard model, the layout guide and their README read it there; a test still pins it, and the keyboard's spring, as no motion role. - motion.doc.mjs and the touch-motion-theme-roles changeset take meta#8076's text, and keep this branch's sentence that ChatLayout's Reduce Motion cross-fade reads a role, now named `timing.curves.move`. The motion role table regenerates unchanged from the merged tables (45 roles); the CLI docs golden that hashes motion.doc.mjs is regenerated.
meta#8176's head now carries meta#8076's (current main with meta#8043's harness, the motion foundation with AST-094 FR4's role names, and the CI duration guard's fix). Conflicts, resolved: - keyboardModel.ts and its test: the keyboard's notification duration is `timing.keyboardNotificationMs` again (the motion foundation keeps it out of the theme roles), beside this branch's Android keyboard table, bézier and `profile` option. - keyboard/README.md: keeps the Chromium section, and the constants line names both tables with the duration at `timing.keyboardNotificationMs`. - motion/README.md: meta#8076's table, with the rows for the keyboard's two domain files (`uikit/keyboard.ts`, `android/keyboard.ts`).
## 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`: - Hold the running document-timeline Web Animations (CSS ones included), each at its offset from the earliest start, and seek them; a second hold adds to the held set, and one the page cancels leaves it. - Read the presented transform and opacity, with the time actually presented (`presentedAt`, from the sampled element's own animations, just inside an end). - Finished, page-paused and scroll-driven animations are left alone. Release resumes only what was held, and resumes an endless animation rather than finishing it. - `waitForAnimation`, `waitForRest`. The page records every animation from its first frame or its creation, so `waitForAnimation` also reports a short one that ended before the first look. `watchAnimations` arms the record before an action. - `math.ts`: oracles that share no code with `packages/core`: `cubicBezier`, `timingFunction`, `springStep`, `projectDecay`, `rubberBand`, `parseTransform`, `compareCurve`. - The proofs' scope includes `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 - Failure before the change: no instrument reads a presented value at an exact time. Polling after the action misses a short entrance on a cold page ("waitForAnimation: no animation started on the element"). - Success after the change, in Chromium and WebKit: - A CSS transition matches its cubic-bezier within 0.05 px, including the sample at its end. - A spring baked into `linear()` matches the SwiftUI oracle within 0.15 px. - Staggered animations keep their offset, a sample is labelled from its own element's animations, and an endless animation resumes. - A 60 ms transition read 250 ms late, and a 50 ms script entrance on the page's first contact, are both reported. - Product/runtime behavior verified unchanged: no package source changes. ## Scope - [x] No intended public API, product behavior, visual direction, or policy change. - [x] Product changes discovered during the work were removed or split. - [x] Public text and artifacts contain no internal Meta context. ## Testing - `pnpm test:interaction --project chromium` (harness): 18 passed. - WebKit 26.5 through `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.mjs` passes. - Harness typecheck, eslint, prettier and the pre-commit repo checks: clean.
Maintainer impact
Touch components cannot be tested with the input they ship for. Playwright's
touchscreen.taponly taps, anddispatchEventtouches are untrusted: they never scroll natively or claim a gesture, and components can tell. Velocity-dependent behavior (a flick that dismisses, a release projected onto a detent) needs test-controlled timestamps, which no browser spec here has.Intended invariant
No product code changes. A spec that claims an interaction drives it through the engine's own input pipeline, with positions and timestamps stated in the test, and a failing proof fails the required
testcheck.Change
internal/interaction-harness(private, never published):scripts.ts: touch scripts as data: tap, drag, flick (it lifts at its scripted velocity, with or without a velocity profile), hold-then-drag, pinch, rotate, any path, a second finger;releaseVelocity(the last move's slope);toTouchEvents.touch.ts:touchDriver(page).play(script). Chromium: CDPInput.dispatchTouchEventwith a timestamp per event; a partial lift releases only the lifted finger. WebKit: its own trusted input command; its limits are in the README, withPARTIAL_LIFT_SKIPfor skips.story.ts: the phone profile, a Storybook server per spec file,openStory.playwright.interaction.config.ts:*.interaction.browser.spec.tsin Chromium and WebKit on a phone-sized touch screen;pnpm test:interaction.test-native-swift(AST-030 FR7, FR11):testrequirespr-interaction. A Linuxinteraction-scopejob decides withpr-scope-touches.sh, failing closed, whether the harness, its config, components, stories,internal/a11y-spec, the workflow,package.jsonorpnpm-lock.yamlchanged; only then do the proofs run (Chromium, the PR head's Storybook build). Otherwise, and in merge groups as withpr-a11y, the job passes without work.pr-interactionandinteraction-scopetake timeouts within main's 15-minute budget and joinci-duration's needs (ci: hold pull-request jobs to the 15-minute budget and report job durations #8427).Stack: #8044 (curves), #8348 (storyboards), #8045 (frame evidence), #8423 (BottomSheet exemplar) follow.
Evidence
testgreen; the join and routing tests fail on that shape.releaseVelocityread 750).Scope
Testing
pnpm test:interaction --project chromium: 4 passed. WebKit 26.5 (in the official Playwright image): 2 passed, 2 skipped (no native fling from injected WebKit touches; no remaining touches on a partial lift).pnpm exec vitest run .github/scripts internal/interaction-harnesswith main merged: 1,346 passed (ci-interaction,ci-test-joinandci-test-routingcases fail before the join change;ci-duration-guardfailed three ways before the timeouts).internal/interaction-harness/src/scripts.test.ts: 11 passed (4 fail before the profile change).pnpm build && pnpm typecheck, eslint, prettier and the pre-commit repo checks: clean.