Repository navigation
test(interaction-harness): name a double tap the page read as two taps - #10916
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 |
|---|---|---|---|
| Lightbox · Storybook | 123.7 kB (0) · 10.6 kB CSS (0) | 3,412 (0) | ✓ · ✓ · ✓ |
Package size: measured; main has no measurement of the merge base to compare yet.
API changes: unknown (base surface unavailable): richtext, theme-butter, theme-chocolate, theme-gothic, theme-matcha, theme-neutral, theme-stone, theme-y2k
Committed API report behind this build: cli 36 rows, core 36 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.
Requesting changes: the tap-interval check is well evidenced, and two documentation fixes in internal/interaction-harness remain.
- The README (lines 690-692), the
tapIntervalMsJSDoc intouch.ts(lines 178-180) and the title say any late touch-down fails the play, buttapIntervalFaultalso requires a page frame between the window's close and the touch-down, so state that condition in both places and narrow the title to match (for example, "a second tap the runner delays past the tap interval fails the play by name"). - The new block in
touch.tsis inserted betweenTouchLiftStallError's doc comment (lines 907-915) and its class, andCHROMIUM_TOUCH_TYPESbetweenTOUCH_TYPES's doc comment (lines 1385-1388) and that constant, so editor hovers now show no documentation forTouchLiftStallErrororTOUCH_TYPESand the pointer-events description onCHROMIUM_TOUCH_TYPES; move the insertions above those comments.
The /approve-spec comment does not settle these, since no spec record changes here and both items are documentation fixes. Optional: the self-test's frameAfterCloseMs > 0 assertion holds for every returned fault, so it cannot fail. CI has not run on this draft head yet.
[Automated review]
|
/approve-spec 82bb885 |
cixzhang
added
the
ci:run
A double tap's second touch-down must reach the page within UIKit's maximumIntervalBetweenTaps (350 ms) of the first lift. The gesture arena times that window from the lift's stamp on the real clock. On a starved runner a play can send the second touch-down late. In Chromium the page then sees the scripted stamps, but the window's timer fires before the touch arrives. Lightbox's double-tap busy proof saw a second touch-down arrive 419 ms after the lift's stamp, where the script spaces them 120 ms. The page read two single taps, no zoom started, and the proof failed with a bare "no animation started". A play now takes boundaries.tapIntervalMs, the component's interval between taps. Every driver judges it. A tap play reads, on the page's clock, each touch's stamp, when the page received it, and when the page drew each frame. In Chromium the page sees the scripted stamps, so the play arms a page-side record of its touch events and frames. In WebKit the arrival stamps and the probe's frames serve. The window closes at the lift's stamp plus the interval. tapIntervalFault names a touch-down received after the close only where the page drew a frame between the close and the touch-down: the main thread was free first, so the component's timer ran first. Without such a frame the component can still pair the taps, so it decides. A gap the script itself spaces past the interval is no fault. The play then throws TouchTapIntervalError with both spacings and the frame. Lightbox's double-tap plays pass UIKit's interval. Under a heavier load than CI's (two pinned CPUs, six workers, four busy loops), all 37 of 57 double taps with a frame after the close failed to zoom, and all 20 others zoomed, two of them received after the close with no frame between. Under CI's profile (one worker), all 120 double taps zoomed, and none was named. touch.taps.test.ts covers the frame gate, a main thread held across the close, arrival stamps, taps the script spaces apart, and a lift that leaves a finger down. A new self-test stalls the runner 450 ms after the first lift. On main the play passes, though the page read two taps; with the change it throws TouchTapIntervalError, naming the frame.
82bb885 to
9650421
Compare
vjeux
changed the title
|
/approve-spec 9650421 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is still a draft; marking it ready for review starts the run.
The tap-interval check is well evidenced: the new self-test fails with the base's touch.ts and passes with this change, and the README and tapIntervalMs docs now state the frame condition. The earlier documentation findings are resolved.
- Optional: in
harness.interaction.browser.spec.ts, thetoContain('The page drew a frame')assertion cannot fail once the error name matches, since everyTouchTapIntervalErrormessage includes that sentence; drop it, or add a case where the page's own main thread is held across the close and the play resolves.
[Automated review]
|
/approve-spec aa1422c |
There was a problem hiding this comment.
Requesting changes: the tap-interval check names a runner that splits a double tap, but it also blames the runner when the page's own main thread splits it.
- Miss: in headless Chromium, a double tap whose runner keeps its schedule, while the page runs a 400 ms task from about 50 ms after the first lift, rejects with
TouchTapIntervalErrorsaying "a dispatch stalled before the page, as on a starved runner" (3 of 3, and Lightbox's "With zoom" story does not zoom), because Chromium draws a frame after the long task before it dispatches the queued touch-down; name the runner only when no frame gap longer thanFRAME_STARVATION_MSspans the close, otherwise leave the pairing to the component or report that the page drew no frame across it, and add a browser self-test where the page holds its main thread across the close (touch.tslines 962-964, 994-997 and 941).
The same task cut to 250 ms, ending before the close, resolves and zooms, so the runner delivers the touch-down on time in that setup. That browser case would also replace the self-test's message assertion on line 598, which cannot fail once the name assertion passes.
[Automated review]
A page task that holds the main thread across the tap interval's close ends, in Chromium, with a frame before the touch-down it held. The tap check read that frame as proof that a dispatch stalled before the page, so it named a starved runner. The fault now keeps the frame gap across the close, and the error names the cause by it. Where the page kept drawing, within FRAME_STARVATION_MS, a dispatch stalled before the page, as on a starved runner. Where it drew no frame for longer, its main thread was held across the close, by a task of its own or a starved machine.
|
/approve-spec 758da14 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is still a draft; marking it ready for review starts the run.
The earlier finding is resolved: a page's own 400 ms task across the tap window's close is now named a held main thread (3 of 3 in headless Chromium), and the new browser self-test fails in Chromium against the previous touch.ts and passes in Chromium and WebKit with this change.
- Optional: in
touch.ts(lines 951-952), a 450 ms runner stall beside an unrelated 70 ms page task across the close is named a held main thread ("the touch-down waited for that main thread"), although the page drew 9 frames before the touch-down arrived and the same task with an on-time runner resolves; naming the held main thread only when the frame gap also spans the touch-down's due time would keep the runner named there. - Optional: in
harness.interaction.browser.spec.ts, the new helpers sit between the "A real navigation" comment (lines 184-185) andfixture, so that comment now describesDOUBLE_TAP; move the helpers above it.
[Automated review]
| fault.closeFrameGapMs > FRAME_STARVATION_MS | ||
| ? `The page drew no frame for ${fault.closeFrameGapMs.toFixed(0)} ms across that window's close, then a frame ${after} ms after it, before the touch-down. Its main thread was held across the close, by a task of its own or a starved machine. So the page read separate taps: the touch-down waited for that main thread.` |
There was a problem hiding this comment.
Non-blocking: this names the cause by the close-frame gap alone. In headless Chromium, a runner that stalls 450 ms after the first lift, while the page runs an unrelated 70 ms task from 290 to 360 ms, gets this branch 3 of 3 (gap 77 ms): "the touch-down waited for that main thread". The page then drew 9 frames over about 107 ms before the touch-down arrived, and the same task with an on-time runner resolves, since its touch-down lands at 117 ms, before the task. Suggest naming the held main thread only when the gap also spans the touch-down's due time (page.stamps[index] in Chromium, or page.stamps[lift] + scriptedGapMs), and naming the runner otherwise, with a unit case where frames resume well before the touch-down.
| // A real navigation (not setContent, whose document.open() erases the window | ||
| // listeners the input log installs). | ||
| /** A double tap: the second touch lands 120 ms after the first lift. */ |
There was a problem hiding this comment.
Non-blocking: this comment explains why fixture (line 239) navigates instead of calling setContent. The new helpers now sit between them, so it reads as a note on DOUBLE_TAP. Move the helpers above line 184.
|
/approve-spec 070cee3 |
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet, because the pull request is still a draft; marking it ready for review starts the run.
This update only merges main. The tap-interval self-tests still pass in Chromium and WebKit, and the Lightbox double-tap specs pass in Chromium.
- Optional, still open: in
touch.ts(lines 951-952), a runner stall beside an unrelated short page task across the close is named a held main thread; naming the held main thread only when the frame gap also spans the touch-down's due time would keep the runner named there. - Optional, still open: in
harness.interaction.browser.spec.ts, the new helpers sit between the "A real navigation" comment (lines 184-185) andfixture; move them above that comment.
[Automated review]
Maintainer impact
A double tap's second touch-down must reach the page within UIKit's 350 ms of the first lift. A starved runner can send it late. The page then reads two taps, and a proof waiting for the zoom fails with a bare "no animation started". Lightbox's busy proof saw one arrive 419 ms after the lift, where the script spaces them 120 ms.
Intended invariant
A play fails by name when the page read separate taps where the script taps once, and the error names the cause. It never fails a play the component paired.
Change
boundaries.tapIntervalMs, the component's tap interval, and every driver judges it. It reads each touch's stamp and receipt, and each frame, on the page's clock. Chromium's page records them. WebKit's arrival stamps and probe frames serve.TouchTapIntervalErrornames the cause by the frame gap across the close. Up toFRAME_STARVATION_MS, the page kept drawing, so a dispatch stalled before the page, as on a starved runner. Past it, the page's main thread was held across the close, by a task of its own or a starved machine. In Chromium, such a task ends with a frame before the touch-down it held.Evidence
touch.ts, a runner stalls 450 ms after the lift. The play passes, though the page read two taps (red arm 0d21481). On aa1422c, a page task across the close is named a starved runner, 3/3 in Chromium (red arm fde30f3).Scope
Testing
pnpm exec vitest run internal/interaction-harnesspnpm exec tsc --noEmit -p internal/interaction-harness/tsconfig.jsonpnpm lint:strictHeads: this head is 070cee3. It merges main b1566fd into 758da14, and the change's own diff is unchanged (patch-id 5fec85e25168). At 070cee3, at one worker, the 11 Lightbox and harness interaction specs pass 286 in Chromium, with 20 skipped. In WebKit, 268 pass and 38 are skipped. The harness units pass 313, and strict lint has 0 errors.
At 758da14, on aa1422c, the harness spec and the three Lightbox specs passed 225 in Chromium, with 15 skipped. In WebKit, 206 passed and 33 were skipped. The shader-cache case failed only because the container ran without its setup script. With that setup it passed too. The harness units passed 311.
The tap-interval self-tests, three cases at one worker, each run three times:
touch.ts, 3/3 in Chromium and WebKit (red arm 0d21481). At 758da14 it names the runner, with the close-frame gap at mostFRAME_STARVATION_MS.touch.taps.test.tscovers the frameless case.Units:
touch.taps.test.tscovers the frame gate, the close-frame gap and the cause each gap names. It also covers a frameless arrival, arrival stamps, spaced taps, and a lift that leaves a finger down.Mutants, each a not-for-landing child of 758da14:
FRAME_STARVATION_MS. The cause unit fails.A log-only instrument of the rule (1a62eff) logged every double tap's page times and whether the zoom ran: