Repository navigation
fix(core): a long press held through a busy main thread still reports, on the touch events' clock - #9455
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 |
|---|---|---|---|
| TableBody | 3.4 kB (0) · 3.3 kB CSS (0) | — | ✓ · ✓ · ✗ |
| TableFooter | 3.4 kB (0) · 3.3 kB CSS (0) | — | ✓ · ✓ · ✗ |
| TableHeader | 3.4 kB (0) · 3.3 kB CSS (0) | — | ✓ · ✓ · ✗ |
22 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| TableRow | 6.5 kB (0) · 4.0 kB CSS (0) | — | ✓ · ✓ · ✗ |
| richtext/RichTextEditorToolbar | 219.9 kB (+363 B) · 454 B CSS (0) | — | — · — · — |
| Markdown | 143.9 kB (+329 B) · 18.3 kB CSS (0) | — | — · — · — |
| Table | 83.2 kB (+327 B) · 16.0 kB CSS (0) | — | ✓ · ✓ · — |
| TableCell | 74.5 kB (+322 B) · 15.8 kB CSS (0) | — | ✓ · ✓ · ✓ |
| Pagination | 84.4 kB (+318 B) · 15.9 kB CSS (0) | — | — · — · — |
| BreadcrumbItem | 65.2 kB (+316 B) · 11.4 kB CSS (0) | — | — · — · — |
| TopNavMenu | 55.5 kB (+314 B) · 10.5 kB CSS (0) | — | — · — · — |
| TableHeaderCell | 74.1 kB (+310 B) · 15.5 kB CSS (0) | — | ✓ · ✓ · ✓ |
| DropdownMenu | 71.1 kB (+303 B) · 15.4 kB CSS (0) | — | — · — · — |
| ContextMenu · Storybook | 73.3 kB (+299 B) · 15.3 kB CSS (0) | 693 (0) | ✓ · ✓ · ✓ |
| lab/CommentThread | 225.1 kB (+299 B) · 2.4 kB CSS (0) | — | — · — · — |
| lab/Comments | 230.4 kB (+297 B) · 2.5 kB CSS (0) | — | — · — · — |
| MoreMenu | 71.4 kB (+295 B) · 15.4 kB CSS (0) | — | — · — · — |
| NavHeadingMenu | 14.6 kB (+295 B) · 3.4 kB CSS (0) | — | — · — · — |
| SideNavHeading | 56.1 kB (+293 B) · 10.5 kB CSS (0) | — | — · — · — |
| DropdownMenuSubMenu | 56.2 kB (+288 B) · 9.9 kB CSS (0) | — | — · — · — |
| TabMenu | 52.7 kB (+287 B) · 10.4 kB CSS (0) | — | — · — · — |
| TopNavHeading | 55.6 kB (+286 B) · 10.4 kB CSS (0) | — | — · — · — |
| Selector | 75.3 kB (+283 B) · 15.6 kB CSS (0) | — | — · — · — |
| PowerSearch | 180.8 kB (+275 B) · 20.5 kB CSS (0) | — | — · — · — |
| TableSelectionToolbar | 49.4 kB (0) · 11.5 kB CSS (0) | — | ✓ · ✓ · ✓ |
Package size: @astryxdesign/core 5.81 MB packed (+2.0 kB); no new dependencies.
API changes: core +0 · ~2 · −0
Committed API report behind this build: core 7 rows; run pnpm api:report.
2 API changes (advisory)
| Package | Where | Signature | Usage | |
|---|---|---|---|---|
| ~ | core |
. · Types · interface UseLongPressHandlers |
onTouchCancel: () => void → onTouchCancel: (e?: React.TouchEvent) => void |
import type {UseLongPressHandlers} from '@astryxdesign/core'; const x: UseLongPressHandlers = {onTouchCancel: …} |
| ~ | core |
. · Types · interface UseLongPressHandlers |
onTouchEnd: () => void → onTouchEnd: (e?: React.TouchEvent) => void |
import type {UseLongPressHandlers} from '@astryxdesign/core'; const x: UseLongPressHandlers = {onTouchEnd: …} |
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 event-clock deadline fixes the dropped long press on a busy page, but it measures a touch in a same-origin iframe or Picture-in-Picture window against the page's performance.now(). As a result, a tap of about 120 ms there opens ContextMenu, which the base never did.
- Read the page clock from the event's own window (
(e.view ?? window).performance, captured attouchstart) inbeginand in the late-timer check, and add a unit case whose clock starts later than the page's. - Share one event-clock deadline timer with
GestureArena(the one-time yield andeventTime's clock normalization) instead of keeping a second copy inuseLongPress, or say in the PR why the arena's timer cannot serve.
[Automated review]
| const handledLateBy = | ||
| startedAt == null ? 0 : performance.now() - startedAt; |
There was a problem hiding this comment.
startedAt is on the clock of the touch's own window, but performance.now() here is the page's. For a trigger rendered into a same-origin iframe or a Picture-in-Picture window (which family:overlay-dismissal FR8 supports), the difference between the two time origins reads as lateness, so remaining is 0. Combined with the late check at line 211, a tap of about 120 ms then opens ContextMenu. Capturing (e.view ?? window).performance at touchstart and using it here and in run fixes it.
| const run = (mayYield: boolean) => { | ||
| timerRef.current = null; | ||
| if (mayYield && deadline != null && performance.now() > deadline) { | ||
| timerRef.current = setTimeout(() => run(false), 0); | ||
| return; | ||
| } | ||
| onLongPressRef.current({x: from.x, y: from.y}); | ||
| }, delayMs); | ||
| fire(); | ||
| }; |
There was a problem hiding this comment.
This copies GestureArena.scheduleRealTimer's fire(mayYield) (gestures/GestureArena.ts:1092-1108), while the clock handling above already differs from GestureHost's eventTime. Could one shared event-clock deadline timer serve both, so a clock fix lands once?
|
Both points, at 0952cbc:
|
There was a problem hiding this comment.
Approving. Timing the long press as a deadline on the touch events' own clock makes a hold through a busy main thread open the menu late instead of never, and reading that clock from the touch's own window fixes the same-origin iframe and Picture-in-Picture tap regression in the previous revision. The new unit cases fail on the base hook and pass here, and Chromium reproduces both the busy-thread hold and the iframe tap.
[Automated review]
…, on the touch events' clock useLongPress timed a hold with a page timer set at touchstart and cleared by touchend. When the main thread was busy through the hold, the page ran the queued touchend before the overdue timer, and the long press was lost: a Table cell's ContextMenu opened nothing after an 800 ms hold. spec:AST-094 FR16 asks a gesture to keep the event clock: a blocked main thread delays the report and never the moment reported. - The hold is a deadline on the touch events' own timestamps (touchstart.timeStamp + delay). A move, a second finger, or a lift stamped at or past it reports the pending press first, at its start point, then acts. - The page timer waits only for what is left of the hold, so a touch the page handled late does not lengthen it. A timer that fires late yields once to queued input, so a lift stamped before the deadline still cancels the press: the gesture arena's rule (gestures/GestureArena.ts). - The clock is the touch's own window's, (e.view ?? window).performance, captured at touchstart: a same-origin iframe or a Picture-in-Picture window has its own time origin. Epoch stamps (jsdom) come onto it through its time origin, and the timer never waits past the delay. - onTouchEnd and onTouchCancel take the touch event, optionally; ContextMenu passes it. Tests: useLongPress's event-clock cases (six fail on main), among them a touch on a window clock whose origin is later than the page's, and a TableCellMenus browser case whose first long press is held through a 1,200 ms busy main thread (fails on main). A [fix] changeset.
0952cbc to
4372170
Compare
There was a problem hiding this comment.
Requesting changes. Timing the long press on the touch events' own clock fixes the dropped ContextMenu long press on a busy page: in Chromium an 800 ms hold through a 1,200 ms busy main thread now opens the menu, and a short tap through the same busy thread still opens nothing. One regression remains: timeOn converts a jsdom stamp by subtracting the real performance.timeOrigin, so in a test that freezes the clock at an earlier date (vi.setSystemTime) a 100 ms tap opens ContextMenu; measuring an epoch stamp's age against Date.now() and adding a frozen-clock case would fix it. Two comments in useLongPress.ts could also be tightened (non-blocking).
[Automated review]
| if (timeStamp == null || !Number.isFinite(timeStamp)) { | ||
| return undefined; | ||
| } | ||
| if (timeStamp > 1e12 && clock != null && clock.timeOrigin > 0) { |
There was a problem hiding this comment.
jsdom stamps events with Date.now(), but timeOrigin is the real origin. In a test that freezes the clock at an earlier date (vi.setSystemTime(new Date(2026, 2, 15))), this puts the touch months in the past: remaining is 0 and the press fires right after touchstart, so a 100 ms tap opens ContextMenu (the base never did). Measuring the stamp's age on the clock that made it avoids that: clock.now() - (Date.now() - timeStamp). A frozen-clock case would pin it.
| /** The part of a window's performance timeline a press reads. */ | ||
| type EventClock = Pick<Performance, 'now' | 'timeOrigin'>; | ||
|
|
||
| /** The clock of the window a touch came from. */ |
There was a problem hiding this comment.
Non-blocking: this restates the name clockOf; it can go.
| // The hold is a deadline on the touch events' own timestamps, read on the | ||
| // clock of the window the touch came from (an iframe or a Picture-in-Picture | ||
| // window has its own). A busy main thread can hold the timer back past the | ||
| // deadline; a touch stamped past it then reports the long press before it | ||
| // acts, so a busy page delays the report and never drops it. |
There was a problem hiding this comment.
Non-blocking: these two sentences run 31 and 36 words. A shorter form: "The hold is a deadline on the touch events' timestamps, on the clock of the touch's own window. A busy main thread can delay the timer past the deadline. A touch stamped after the deadline then reports the press before it acts."
jsdom stamps events with Date.now(). The hold converted such a stamp by subtracting the window's real time origin, so in a test that freezes the system time at an earlier date the touch landed months in the past, its timer fired at once, and a 100 ms tap opened ContextMenu. An epoch stamp's age is now read on Date.now(), the clock that made it, and placed on the window's clock: clock.now() - (Date.now() - timeStamp). Tests: an epoch stamp handled 300 ms late is due in 200 ms, and a tap under a frozen earlier system time opens nothing; both fail on the previous version.
|
At cab4f23: an epoch stamp is now placed by its age on |
There was a problem hiding this comment.
Requesting changes. Placing a jsdom stamp by its age fixes the frozen-earlier-date case, and both comment suggestions are in. One case remains: timeOn treats a stamp as a Date.now() stamp only above 1e12, so in a jsdom test that freezes the date at 0 and leaves performance unfaked, a 100 ms tap opens ContextMenu. Choosing the clock by distance, Math.abs(Date.now() - timeStamp) < Math.abs(clock.now() - timeStamp), plus a case for that setup, fixes it and keeps the existing tests green.
[Automated review]
| if (timeStamp == null || !Number.isFinite(timeStamp)) { | ||
| return undefined; | ||
| } | ||
| if (timeStamp > 1e12 && clock != null) { |
There was a problem hiding this comment.
jsdom stamps events with Date.now(), but a stamp counts as one only above 1e12. In a test that freezes the date at 0 (vi.setSystemTime(0)) and leaves performance unfaked (an explicit toFake list without it), the stamp is read as a performance time several seconds in the past: remaining is 0 and a 100 ms tap opens ContextMenu, which the base never did. Choosing the clock by distance keeps a tap a tap in that setup and keeps the PR's own cases green:
if (
clock != null &&
Math.abs(Date.now() - timeStamp) < Math.abs(clock.now() - timeStamp)
) {
return clock.now() - (Date.now() - timeStamp);
}| * An event's time on its window's clock. jsdom stamps events with | ||
| * `Date.now()`, so such a stamp's age is read on that clock: a test that | ||
| * freezes or moves the system time keeps the press's timing. |
There was a problem hiding this comment.
Non-blocking: this sentence runs 28 words, in the passive, with three topics. A shorter form: "jsdom stamps events with Date.now(). The hook reads the age of such a stamp on that clock. A frozen system time then keeps the hold's length."
…one by distance A stamp counted as a Date.now() stamp only above 1e12, so in a jsdom test that froze the date at 0 and left performance real, a small Date.now() stamp read as a window-clock time seconds in the past and a 100 ms tap opened ContextMenu. A stamp nearer Date.now() than the window's clock is now read as a Date.now() stamp, by its age. A stamp of 0 carries no time, so the hold falls back to the full delay on the page's timer. Tests: a tap under a date frozen at 0 with a real window clock, and a touch stamped 0; each fails on the previous version.
|
At 5153bfb: the clock is chosen by distance, as suggested, so a stamp nearer |
There was a problem hiding this comment.
Approving. Timing the long press on the touch events' own clock opens ContextMenu through a busy main thread, and choosing the Date.now() branch by distance fixes the earlier tap regression in tests that freeze the date near 0. One optional follow-up: the new date-frozen-at-0 test stamps its touch at exactly 0, so the zero-stamp guard decides it; advancing the frozen clock before the touch would make it cover the distance comparison too.
[Automated review]
| }); | ||
|
|
||
| it('keeps a tap a tap in a test that freezes the date at 0 and leaves the window clock real', () => { | ||
| vi.setSystemTime(0); |
There was a problem hiding this comment.
Non-blocking: Date.now() is exactly 0 here, so the touch is stamped 0 and the timeStamp <= 0 guard decides this case before the distance comparison. With timeStamp > 1e12 restored in timeOn, every case in this file still passes. Advancing the clock before the touch (for example vi.advanceTimersByTime(50)) would make this case need the distance test.
|
/approve-spec 5153bfb |
cixzhang
added
the
ci:run
User impact
On a busy page, a long press on a ContextMenu trigger can open nothing.
Found as a TableCellMenus case that failed only on a loaded machine.
Problem and solution fit
useLongPresstimed the hold with a page timer cleared bytouchend; with the main thread busy, the queuedtouchendruns first and the press is dropped. On main 83f5da1, Chromium, 3 runs each:touchendhandled aftertouchstart(stamped: 800 ms)touchstartExpected behavior and authority
spec:AST-094FR16 (current): a long press is a deadline on the input events' timestamps, "so a blocked main thread delays the report and never the moment reported", as the gesture system's long press already does.Smallest restoration
touchstart.timeStamp + delayon the touch's own window clock (an iframe or Picture-in-Picture window has its own origin): a move, second finger, lift or cancel stamped past it reports the press first.Date.now()than the window's clock) is placed by its age, so a frozen or zeroed system time keeps a tap a tap; a stamp of 0 gets the full delay.onTouchEndandonTouchCanceltake the event, optionally; ContextMenu passes it.Not here:
useMenuPress's trigger hold, the same exposure, moves with DropdownMenu's section. A window-aware deadline helper shared withGestureArenafollows once #8309, #8342, #8665 and #8763 land. Drafts editinguseLongPress.tstake this at their rebase; #9642 builds on it.Evidence
useLongPress.test.tsx: six event-clock cases fail on main's hook; the later-origin window, frozen-date, zeroed-date and zero-stamp cases fail on earlier versions of this PR; the yield and the cap each fail under mutation.TableCellMenus.a11y.browser.spec.ts: a long press held through a 1,200 ms busy main thread opens the menu, present and painted, at the finger: 3/3 fail on main in Chromium and WebKit; 10/10 pass here on the 2-core Chromium pin and in WebKit (--cpus=4,LP_NUM_THREADS=4).Scope
Testing
With touch emulation, open the TableRowExpansion "Gliding rows" story, run a 1 s busy loop right after touching a cell, and keep holding: the menu opens at the finger.
Local gates at 5153bfb: the static set (build, Storybook, typecheck, strict lint, provenance check, Prettier, changesets, knowledge, changeset coverage), test:ui and test:node all pass; the ContextMenu, TableCellMenus and TableRowExpansion accessibility specs pass; the busy-main-thread long press passes 10 of 10 on the 2-core Chromium profile at 1 worker and in the CI WebKit lane.