Repository navigation
refactor(core): useLongPress runs its long press through a detector with no React - #9642
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) | — | ✓ · ✓ · ✗ |
| lab/CommentThread | 252.4 kB (+34 B) · 2.4 kB CSS (0) | — | — · — · — |
| TableCell | 77.2 kB (+17 B) · 15.9 kB CSS (0) | — | ✓ · ✓ · ✓ |
| TableHeaderCell | 76.8 kB (+13 B) · 15.6 kB CSS (0) | — | ✓ · ✓ · ✓ |
| TableSelectionToolbar | 55.7 kB (0) · 11.8 kB CSS (0) | — | ✓ · ✓ · ✓ |
| DropdownMenu | 74.0 kB (−1 B) · 15.5 kB CSS (0) | — | — · — · — |
| TabMenu | 56.3 kB (−1 B) · 10.5 kB CSS (0) | — | — · — · — |
| ContextMenu | 76.0 kB (−4 B) · 15.4 kB CSS (0) | — | — · — · — |
| Table | 85.4 kB (−6 B) · 16.1 kB CSS (0) | — | ✓ · ✓ · — |
| BreadcrumbItem | 68.4 kB (−10 B) · 11.5 kB CSS (0) | — | — · — · — |
| lab/Comments | 257.5 kB (−10 B) · 2.5 kB CSS (0) | — | — · — · — |
| MoreMenu | 74.3 kB (−12 B) · 15.5 kB CSS (0) | — | — · — · — |
| NavHeadingMenu | 14.8 kB (−12 B) · 3.4 kB CSS (0) | — | — · — · — |
| Selector | 78.4 kB (−19 B) · 15.7 kB CSS (0) | — | — · — · — |
| TopNavHeading | 59.0 kB (−19 B) · 10.4 kB CSS (0) | — | — · — · — |
| Pagination | 87.3 kB (−24 B) · 16.1 kB CSS (0) | — | — · — · — |
| SideNavHeading | 59.6 kB (−29 B) · 10.6 kB CSS (0) | — | — · — · — |
| Markdown | 146.3 kB (−35 B) · 18.4 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditorToolbar | 223.9 kB (−39 B) · 454 B CSS (0) | — | — · — · — |
| TopNavMenu | 58.9 kB (−40 B) · 10.5 kB CSS (0) | — | — · — · — |
| DropdownMenuSubMenu | 56.5 kB (−49 B) · 10.0 kB CSS (0) | — | — · — · — |
| PowerSearch | 192.7 kB (−50 B) · 21.6 kB CSS (0) | — | — · — · — |
Package size: @astryxdesign/core 6.22 MB packed (+1.8 kB); no new dependencies.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Current run: CI concluded cancelled; parts of this report may be missing.
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 long-press timing fix works (a long press held through a busy main thread now opens the menu), but the new detector copies the gesture system's event-clock logic instead of sharing it.
longPressDetector.tsreimplementsGestureArena's late-timer yield andGestureHost'seventTimewith a different epoch-stamp rule; please share one window-aware event-clock helper between them, or explain in the pull request why the arena cannot serve this case.- The
@pr-a11y scopetoken inTableCellMenus.a11y.browser.spec.tsno longer matches the file that now holds the timing; please widen it to^packages/core/src/hooks/(useLongPress|longPressDetector).
[Automated review]
| // A late timer yields to queued input once, so a lift stamped before the | ||
| // deadline still cancels the press. The gesture arena does the same. | ||
| const run = (mayYield: boolean) => { | ||
| timer = null; | ||
| if ( | ||
| mayYield && | ||
| pressClock != null && | ||
| pressDeadline != null && | ||
| pressClock.now() > pressDeadline | ||
| ) { | ||
| timer = setTimeout(() => run(false), 0); | ||
| return; | ||
| } | ||
| fire(); | ||
| }; | ||
| timer = setTimeout(() => run(true), remaining); |
There was a problem hiding this comment.
This copies GestureArena's late-timer yield (gestures/GestureArena.ts:1093-1108), as the comment says. Please move the deadline timer into one window-aware helper under gestures/ that the arena and this detector both call, or say in the pull request why the arena cannot use it.
| function timeOn( | ||
| clock: EventClock | null, | ||
| timeStamp: number | undefined, | ||
| ): number | undefined { | ||
| // A stamp of 0 means the event carries no time, so the hold falls back to | ||
| // the full delay on the page's timer. | ||
| if (timeStamp == null || !Number.isFinite(timeStamp) || timeStamp <= 0) { | ||
| return undefined; | ||
| } | ||
| if ( | ||
| clock != null && | ||
| Math.abs(Date.now() - timeStamp) < Math.abs(clock.now() - timeStamp) | ||
| ) { | ||
| return clock.now() - (Date.now() - timeStamp); | ||
| } | ||
| return timeStamp; | ||
| } |
There was a problem hiding this comment.
GestureHost's eventTime (gestures/GestureHost.ts:388-398) reads the same stamps with a different rule: a stamp over 1e12, minus the page's timeOrigin. With one shared reader, the iframe-clock and frozen-date fixes would reach both paths.
| @@ -1,5 +1,5 @@ | |||
| // Copyright (c) Meta Platforms, Inc. and affiliates. | |||
| // @pr-a11y scope ^packages/core/src/Table/ ^packages/core/src/ContextMenu/ ^apps/storybook/stories/TableRowExpansion | |||
| // @pr-a11y scope ^packages/core/src/Table/ ^packages/core/src/ContextMenu/ ^packages/core/src/hooks/useLongPress ^apps/storybook/stories/TableRowExpansion | |||
There was a problem hiding this comment.
The timing now lives in hooks/longPressDetector.ts, which this token does not match. A change to the detector alone would skip this spec. Suggest ^packages/core/src/hooks/(useLongPress|longPressDetector).
| // Clear a stale timer first, then record the start point. The clear also | ||
| // forgets the start. |
There was a problem hiding this comment.
Non-blocking: this narrates cancel(); from = …, which the code already states. Suggest deleting it.
| let options = initial; | ||
| let timer: ReturnType<typeof setTimeout> | null = null; | ||
| let from: {x: number; y: number} | null = null; | ||
| // The pending press's deadline and clock, when it started from a touch. |
There was a problem hiding this comment.
Non-blocking: the names already say this. Suggest keeping only the non-obvious part: // Null for a press started by hand. It has no stamp.
| ); | ||
|
|
||
| useEffect(() => clear, [clear]); | ||
| // One detector for the component's life. |
There was a problem hiding this comment.
Non-blocking: this restates the useState initializer. Suggest deleting it.
8d2773d to
166e481
Compare
|
At 166e481:
|
There was a problem hiding this comment.
Recommending approval. The detector keeps useLongPress's behavior, and the stacked event-clock timing holds up: the new cases fail on main and pass on this branch, and in Chromium a long press held through a busy main thread opens the cell menu only here.
- Both earlier requests are addressed: the
@pr-a11yscope now coverslongPressDetector, and the description explains whyGestureArenaandGestureHostcannot time this press yet, since both read the page's clock and time origin rather than the touch's own window. - One window-aware event clock shared with the gesture arena and host stays the stated follow-up.
[Automated review]
166e481 to
fa92cbe
Compare
There was a problem hiding this comment.
Changes requested. The refactor keeps useLongPress's behavior: the existing cases pass unchanged, and a touch long press opens the cell menu the same way before and after, including on a busy main thread. Two things need a fix first:
- The new internal factory
longPressDetector(packages/core/src/hooks/longPressDetector.ts:125) is named outside thecreate…factory family thatdocs/architecture/react-component-runtime.mdINV13 sets for touch modules, so please rename itcreateLongPressDetector. - No test fails when the layout effect at
packages/core/src/hooks/useLongPress.ts:109-111stops callingdetector.setOptions(options)(deleting it leaves the hooks, ContextMenu, DropdownMenu, Table and gestures suites green), so please add auseLongPresscase that rerenders with new options and expects only the new ones to apply.
Non-blocking suggestions on the changeset wording and two comments are inline.
[Automated review]
| } | ||
|
|
||
| /** @internal A long-press detector with no React (see LongPressDetector). */ | ||
| export function longPressDetector( |
There was a problem hiding this comment.
Blocking: docs/architecture/react-component-runtime.md INV13 says touch modules outside the recognizer family are "built from create… factories", like createPressTracker, createActionGroupTracker and createPager. Suggest createLongPressDetector (no public factory has that name), with useLongPress.ts, the test file and the SYNC lines updated to match.
| useIsomorphicLayoutEffect(() => { | ||
| detector.setOptions(options); | ||
| }); |
There was a problem hiding this comment.
Blocking: this call is now the only way a later commit's options (ContextMenu's disabled, a new onLongPress) reach the press. With it deleted, all hooks, ContextMenu, DropdownMenu, Table and gestures tests still pass. A case like this one, inside a describe with fake timers, passes here and fails without the call:
it('uses the options of the latest commit', () => {
const first = vi.fn();
const latest = vi.fn();
const {result, rerender} = renderHook(
(props: {disabled: boolean; onLongPress: () => void}) => useLongPress(props),
{initialProps: {disabled: true, onLongPress: first}},
);
rerender({disabled: false, onLongPress: latest});
act(() => {
result.current.start({x: 0, y: 0});
vi.advanceTimersByTime(500);
});
expect(first).not.toHaveBeenCalled();
expect(latest).toHaveBeenCalledTimes(1);
});| // The detector reads the committed render's options, as the handlers' | ||
| // closures did: a render that React discards never reaches it. |
There was a problem hiding this comment.
Non-blocking: "as the handlers' closures did" describes code this change removes. Suggest:
// The detector reads only committed options. A render that React discards
// never reaches it.| // Null for a press started by hand. It has no stamp. | ||
| let deadline: number | null = null; | ||
| let clock: EventClock | null = null; | ||
| // The press in progress's tolerance, when its caller chose one at start. |
There was a problem hiding this comment.
Non-blocking: the name pressTolerance and its type already say this. Suggest deleting the comment.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: `useLongPress` runs its long-press detection through an internal detector with no React, so a press model that runs without React can share it. The hook and its handlers behave as before. Not exported from the package entry point. |
There was a problem hiding this comment.
Non-blocking: onLongPress used to come from a ref written during render. So while a transition that swaps it is suspended, the old hook called the pending callback, and this one calls the committed callback. Suggest adding: "onLongPress now also comes from the committed render."
…ith no React The long-press detection moves out of useLongPressDetector's refs and callbacks into an internal factory, longPressDetector: a hand start from a point, the touch handlers on the events' clock, the movement tolerance, and the options it reads at each start, move and fire. useLongPressDetector creates one detector for the component's life, hands it the committed render's options after each commit, cancels it on unmount, and returns its methods as the handlers. A press model that runs without React can then share the one detector. No behavior change: the existing useLongPress cases pass unchanged. A new case keeps a suspended transition's options away from the press. The detector's own cases run with no React. A [chore] changeset. The core API report also gains the row for the optional event that useLongPress's onTouchEnd and onTouchCancel take, which the change below this one adds.
fa92cbe to
0cb9e7b
Compare
There was a problem hiding this comment.
Recommending approval. The refactor keeps useLongPress's behavior: the existing cases also pass against the old hook, and a touch long press opens the same menu before and after in Chromium, including through a busy main thread.
Both earlier requests are addressed: the factory is now createLongPressDetector, and uses the options of the latest commit fails if the layout effect stops passing new options to the detector.
[Automated review]
cixzhang
added
the
ci:run
Rebased onto main now that #9455 is merged: its three commits leave the branch, and this pull request's own commit is unchanged (an identical range-diff against 166e481).
Maintainer impact
The long-press detection lives in
useLongPressDetector's refs and callbacks, so code without React cannot share it. The menu press factory needs it: a finger held on a menu trigger must time the hold exactly as ContextMenu does.Intended invariant
No behavior change.
useLongPressanduseLongPressDetectorkeep the same hold time, movement tolerance, timer, handlers and hand-drivenstart/moveTo/cancel, and #9455's timing on the touch events' own clock. The press reads only a committed render's options, as the handlers' closures did. Nothing new is exported.Change
createLongPressDetector(options)(thecreate…family ofarchitecture:react-component-runtimeINV13), holds the detection: a hand start, the touch handlers on the events' clock, the movement tolerance,cancel,isPendingandsetOptions.useLongPressDetectorcreates one detector per component, hands it the committed options in a layout effect (a case pins that a rerender's options reach the press), cancels it on unmount, and returns its methods as the handlers.LongPressTolerancemoves with the detector;useLongPress.tsre-exports it.onTouchEndandonTouchCanceltake an optional event.The detector keeps its own clock rather than the gesture arena's. The arena's deadline timer and
GestureHost'seventTimeread the page's clock and time origin, where this press reads the touch's own window (#9455). A window-aware helper shared with them would change the gestures' timing in a same-origin iframe or a Picture-in-Picture window, andGestureArena.tsandGestureHost.tsare open in #8309, #8342, #8665 and #8763. Sharing it is a follow-up once those land.Evidence
longPressDetector.tseach turn cases red inlongPressDetector.test.tsanduseLongPress.test.tsx(a second finger that never reports a due press, no late-timer yield, a full wait for a start handled late). A detector fed every render's options fails the new suspended-transition case, which passes on fix(core): a long press held through a busy main thread still reports, on the touch events' clock #9455's head.useLongPress.test.tsxcases pass unchanged, with the new case and eight detector cases.Scope
Testing
At 12a30c5 (this head, 0cb9e7b, splits one comment sentence): the long-press units (33 of 33) and strict lint; without the committed-options effect (NOT-FOR-LANDING arm fe2bcec), the latest-commit case fails. At 76c4e332de, which differs only in three comment lines: the hooks, gestures, DropdownMenu, ContextMenu and Table units (2,678 of 2,678) and the core type check. At fa92cbe, before the rename: the frozen install, the build and
api:report --check.Before the rebase, local gates at 166e481 (the same patch): build, typecheck, strict lint, the comment-provenance baseline, Storybook, Prettier, knowledge, changesets, coverage,
check:repo;pnpm test:ui(17,989) andpnpm test:node, all passing; the TableCellMenus, ContextMenu and TableRowExpansion accessibility specs in Chromium (29); TableCellMenus' touch cases ×10 on a 2-core pin and ×10 in WebKit (--cpus=4,CI=1).