Repository navigation
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
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 578373e |
github-actions
Bot
removed
the
needs:spec-owner-review
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
![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 spec side is settled: an engineering owner approved ContextMenu FR11 at this head. Each item below still needs a fix before this can merge.
- The growth and its early-lift return read the generic
--duration-medium,--duration-fastand--ease-movetokens, whichspec:AST-094FR4 does not allow for touch motion ("MUST be a motion role named for its use"), so amend FR4 in this pull request or move the growth onto named motion roles. - If
isDisabledturns true while a finger-opened menu is up, the touch layer unmounts and disposes its animator without restoring the trigger's inlinescale, so the trigger stays about 7% larger after the menu closes (contextMenuPressFeedback.ts, the unmount cleanup). - ContextMenu now has two outside-press paths, the shared
watchOutsidePressfor touch and its ownmousedownEffect with a different shadow-root rule for everything else, so extend the shared watch to close at the press for a mouse and a pen and delete the Effect (architecture:react-component-runtimeINV16). useContextMenuPressFeedbackruns its own attach, replace and dispose lifecycle for its animator and press observer, so move both ontouseAttachment, which the same layer'suseGestureRecognizeralready uses.- The outside-press changeset says "No component passes these options yet", but ContextMenu passes all three at this head, so reword it.
- The diff against
mainis 4,013 authored lines because it carries #9814 and #8125, so land those first or base this branch on #8125 to leave this pull request's own 1,915 lines.
A few smaller comment and wording notes are inline.
[Automated review]
| useEffect( | ||
| () => () => { | ||
| animatorRef.current?.dispose(); | ||
| animatorRef.current = null; | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
If the touch layer unmounts while the menu it opened is up (for example isDisabled turns true), this disposes the animator but leaves the inline scale that hold() froze, and the later release() finds no handle. The trigger then stays about 7% larger. Remove the feedback's inline scale (or settle it to 1) here before disposing.
| : element.ownerDocument.defaultView?.getComputedStyle(element); | ||
| const read = (name: string) => style?.getPropertyValue(name).trim() ?? ''; | ||
| return ( | ||
| pressTiming(read(durationToken), read('--ease-move'), { |
There was a problem hiding this comment.
spec:AST-094 FR4 asks every duration and curve a touch presentation plays to be a motion role named for its use. These generic theme tokens are not roles, and FR11's approval does not amend FR4. Amend FR4 in this pull request, or register the growth's duration and curve as named roles that derive from the theme's knobs.
There was a problem hiding this comment.
Per the spec owners, #9706 amends AST-094 for non-gesture motion: tap-, key- and program-started motion plays the theme's duration and easing tokens (its new FR45), and only gesture-driven motion keeps measured springs. So this pull request adds no FR4 line; FR11 cites FR45 in the rebase after #9706 merges.
| // pressable surface, so a control inside the trigger can carry the mark. | ||
| // The machine lets a press go at a shorter travel than the recognizer's | ||
| // tolerance, so only the recognizer ends the growth (`end`). | ||
| useEffect(() => { |
There was a problem hiding this comment.
This Effect re-attaches by triggerKey, and the animator above has its own create and dispose path. useAttachment (hooks/useAttachment.ts) already owns attach, replace and release for the same trigger through useGestureRecognizer; attaching the observer and the animator there keeps one lifecycle (architecture:react-component-runtime INV16).
There was a problem hiding this comment.
Done in 0d1fdb7: the animator and the press observer each attach through useAttachment, and the touch layer no longer passes a trigger key, since the attachment follows the element.
| // Under the touch model, a touch outside the open menu closes it as it | ||
| // begins to pan, or at its lift. The page stays free to scroll. A press | ||
| // that opens the menu, or moves it, keeps it open at its lift. | ||
| watchTouchOutsideRef.current = () => { |
There was a problem hiding this comment.
With this watch, ContextMenu has two outside-press paths: this one for touch, and the mousedown Effect above for every other press, each with its own shadow-root rule. Extend watchOutsidePress to close at the press for a mouse and a pen, make this the only call, and delete the Effect (INV16).
There was a problem hiding this comment.
Moving mouse and pen onto the shared watch changes how every desktop context menu closes, so the owners split it into its own pull request with its own decision; this stack keeps main's path for them.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: the layer's outside-press watch can follow chosen pointer types, close as a press that starts outside begins to pan (on either axis, at core's pan threshold) or when the browser takes it, and judge a press inside a shadow root there. No component passes these options yet, so every watch keeps today's behavior. Not exported from the package entry point. |
There was a problem hiding this comment.
At this head ContextMenu passes pointerTypes, closesOnPan and root, so "No component passes these options yet, so every watch keeps today's behavior" is no longer true. Suggest: "ContextMenu's touch model passes them; every other watch keeps today's behavior."
| // How much the press feedback has scaled a block trigger from its center. | ||
| // Its bounding rect is the scaled box, and its offset size the unscaled one. |
There was a problem hiding this comment.
Non-blocking: triggerScale now sits between the cursor-anchor comment and openAtLocalPoint, which that comment describes. Move triggerScale above the comment.
| expect(onTouchEnd).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('makes the trigger a pressable surface of core’s one press machine, which starts and ends its growth', () => { |
There was a problem hiding this comment.
Non-blocking: this case only checks the pressable attribute. Drop "which starts and ends its growth" from the name, or assert the growth.
578373e to
0d1fdb7
Compare
|
/approve-spec 0d1fdb7 |
There was a problem hiding this comment.
Changes requested. The spec side is settled: an engineering owner approved ContextMenu FR11, and that approval still covers this head.
- The growth and its early-lift return play the generic
--duration-medium,--duration-fastand--ease-movetokens, whichspec:AST-094FR1 and FR4 do not yet allow for touch motion, so rebase after #9706 lands and cite its FR45 in FR11, or amend FR1 and FR4 here. ContextMenuTouchLayersubscribes to its recognizer's transitions from an Effect whose only dependency never changes, so add that listener in the recognizer factory passed touseGestureRecognizerand delete the Effect.ContextMenuPressRecognizergains anonPressStartoption that only a unit test passes, and its doc says the press feedback starts from it, so remove the option.- The diff against
mainis 4,132 authored lines because it carries #9814 and #8125, so land those first, or declare the stack with one identicalStack: #9814 → #8125 → #9816line in all three descriptions.
Thanks for fixing the stuck trigger scale and moving the press feedback onto useAttachment. The changeset and the story still say the trigger "springs back" after an early lift, which FR11 now times as a curve on the theme's tokens.
[Automated review]
|
|
||
| // The recognizer's transitions. Its target-action listeners never hear | ||
| // `failed`, so a press that ends with no menu springs back from here. | ||
| useEffect( |
There was a problem hiding this comment.
recognizer never changes (useGestureRecognizer makes it once), so this Effect follows no prop or state change. Add the transition listener where the recognizer is made, in the factory passed at line 87, and delete the Effect.
There was a problem hiding this comment.
Done in d9f0aca: the transition listener is added in the recognizer factory, and the Effect is gone.
| | 'numberOfTouchesRequired' | ||
| > { | ||
| /** A finger landed and may become a context menu. */ | ||
| onPressStart?: (recognizer: ContextMenuPressRecognizer) => void; |
There was a problem hiding this comment.
Nothing outside contextMenuPress.test.ts passes onPressStart. The growth follows the press machine, though the doc at line 56 says it starts from this option. Remove the option and that sentence.
There was a problem hiding this comment.
Removed in d9f0aca, with that sentence and its test.
0d1fdb7 to
caf271c
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec caf271c |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested for this stack: #8125 is ready, and #9814 and #9816 each need one fix. A pull request lands only when it and every pull request below it are approved.
#9814: changes requested
- Miss: the title and
.changeset/outside-press-pan-watch.mdsay the watch can now "judge a press inside a shadow root", but a press inside an open shadow root was already judged through the event's composed path and every new test uses a closed root, so please say "closed shadow root" in both.
#8125: approved
Approved pending CI: CI has not run on this head yet, and it lands after #9814.
#9816: changes requested
- Miss:
PressTimingandtimedCurveincontextMenuPressTiming.ts, andthemeTimingincontextMenuPressFeedback.ts, copyThemeTiming,timedCurveandthemeTimingOffromSegmentedControl/useSegmentedSelectionMotion.ts, so please move them into one shared motion module that both components call. - Non-blocking: ContextMenu FR11 at this head no longer cites the measurement behind the 400 ms commit and 735 ms display, or the tests that verify them, which #8125's FR11 did.
Whole stack
The combined change adds no public API and follows spec:AST-094 and ContextMenu FR11, but it passes only once the two fixes above land.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: the layer's outside-press watch can follow chosen pointer types, close as a press that starts outside begins to pan (on either axis, at core's pan threshold) or when the browser takes it, and judge a press inside a shadow root there. ContextMenu's touch model uses them; every other watch keeps today's behavior. Not exported from the package entry point. |
There was a problem hiding this comment.
Miss (claims match scope): a press inside an open shadow root was already judged as inside before this change, through the event's composed path, and every new outsidePress.test.ts case attaches a closed root. Suggested wording, here and in #9814's title: "…and judge a press inside a closed shadow root there."
| } | ||
|
|
||
| /** A curve from `from` to `to` on `timing`, for the compositor animator. */ | ||
| export function timedCurve( |
There was a problem hiding this comment.
Miss (one shared path): this timedCurve, PressTiming above, and themeTiming with its 1 ms linear stand-in in contextMenuPressFeedback.ts repeat timedCurve, ThemeTiming, themeTimingOf and INSTANT in SegmentedControl/useSegmentedSelectionMotion.ts. Move them into one motion module that both import, so a fix to token-timed motion reaches both components.
There was a problem hiding this comment.
| | FR8 | In the pointer menu ArrowDown on the last enabled row wraps to the first and ArrowUp on the first to the last; PageDown and PageUp page as in `component:DropdownMenu` FR12; a held key's auto-repeat does not activate; typeahead matches the row's label element alone. Arrow keys and typeahead skip disabled rows, shipped behavior that is not a requirement until `component:DropdownMenu` OQ1 is ruled. | Current source and tests | | ||
| | FR9 | While the Pointer menu surface is open, it participates in `family:overlay-dismissal` through the shared stack and provides descendant depth. One Escape press affects only the top-most present nested surface; ContextMenu closes only when it is that surface. | `family:overlay-dismissal` FR1–FR6; `ContextMenu.test.tsx` nested-layer case | | ||
| | FR10 | ContextMenu MUST be built on the public headless context menu (`spec:AST-107` FR8), with its invocation, open state, and cleanup the factory's and no behavior of its own beside the factory's. Implementation gap: no headless context menu ships; ContextMenu owns its invocation, open state, outside-press listener, and cleanup itself. | `spec:AST-107` FR8 | | ||
| | FR11 | Under a finger, the Trigger area grows during a hold toward 1.0735 times its size over the theme's `--duration-medium` with `--ease-move`, and it returns after a lift before the commit over `--duration-fast` with `--ease-move`. The theme's tempo reaches both through its duration tokens, and neither follows UIKit's ramp or spring. The hold commits at 400 ms and the menu displays at 735 ms, as UIKit's do. They are measured input, not motion, and stay internal (`spec:AST-094` FR4, FR45). | `spec:AST-094` FR45: a hold is not a gesture, so its motion plays the theme's motion tokens on every pointer. It departs from UIKit's measured press growth on iOS 26.2: a linear ramp, 0.16 and then 0.10 per second, chased by a spring (response 0.4 s, damping ratio 0.6) and shown 38 ms late | |
There was a problem hiding this comment.
Non-blocking: this basis no longer names the measurement behind the 400 ms commit and the 735 ms display that #8125's FR11 cited (motion/uikit/contextMenu.ts), and the FR11 verification row drops their tests ("opens at the lift for a finger lifted after the commit (400 ms)" and contextMenuPress.test.ts). Keeping both bases and both sets of tests leaves every FR11 claim traceable.
There was a problem hiding this comment.
Done in 037a530: FR11's basis names the measured 400 ms commit and 735 ms display again (motion/uikit/contextMenu.ts), and its verification row lists their tests beside the growth's.
b168c85 to
037a530
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 037a530 |
|
/approve-spec a72909a |
There was a problem hiding this comment.
Changes requested: one evidence item remains. The spec side is settled at this head: an engineering owner approved component:ContextMenu FR12, the hold growth, for this exact head, and the code and tests match FR12, FR5 and spec:AST-094.
Required fix
- Report in the description the Chrome-with-Gboard Android emulator result for the trigger's growth and both returns at this head, with animations at normal scale and removed plus a slow-device profile, as
spec:AST-094's platform support (DEC-27) requires; the description still says the curves await a run.
The branch also conflicts with main in packages/core/src/Layer/outsidePress.ts, a file from #9814; the restacked head will need its own /approve-spec.
This pull request lands only when it and every pull request below it in the stack are approved.
[Automated review]
`utils/nodeType.ts` now holds the cross-window shadow-root check, so `shadowHostOf` uses it and the flat tree has one shadow-root test.
Under a coarse pointer the menu now opens on the touch model's timeline by default. This case covers the box-side rule for the popover menu's long press, so it asks for `presentation="popover"` and keeps its 600 ms hold.
`contextMenuPressFeedback.ts` imported the whole `springs` table and registered it, which put every spring role row in every ContextMenu import. It now imports `CONTEXT_MENU_LIFT_SPRING` and `CONTEXT_MENU_DISMISS_SPRING`, which register their own roles when the module loads.
|
/approve-spec 2ceebe8 |
There was a problem hiding this comment.
Changes requested. The spec side is settled at this head: an engineering owner approved component:ContextMenu FR12.
Required fixes
- Miss: on Android the trigger starts to grow 243–254 ms after the touch (368–451 ms on a slowed device) instead of FR12's 150 ms, because
contextMenuPressFeedback.ts(lines 244–247) waits for the press machine's mark, whose timer counts from when its listener ran, so take #10260's fix below this pull request and report the emulator onset again. - Report in the description the Chrome-with-Gboard Android emulator result at the head that lands for the growth's curve, its early-lift return and its return as the menu closes, with animations at normal scale and removed plus a slow-device profile, as
spec:AST-094Platform support (DEC-27) requires. - Miss: a finger that drifts (8, 8) px within its first 150 ms still gets the menu at 735 ms but the trigger never grows, because
contextMenuPressFeedback.tsclears the pending onset when the press machine lets the press go at 10 px (line 262), and inside a scroll view the machine never marks such a press at all, so let only the recognizer, which keeps the finger to 11.5 px, cancel the onset and add unit cases for a drift outside and inside a scroll view.
This pull request lands only when it and #9814 and #8125 below it are approved.
[Automated review]
A finger that drifts 11 px within its first 150 ms is still a hold: the recognizer's tolerance is 11.5 px, and the menu opens at 735 ms. But the press machine lets the press go at 10 px, and the trigger never grows. This case fails until the next commit.
The press machine lets a press go at a shorter travel than the recognizer's tolerance. The press feedback cleared its pending onset when the machine's mark went off, so a drift between the two never grew. It now keeps the onset while the recognizer still follows the finger. The onset itself still checks that the recognizer follows at that moment.
|
/approve-spec 8af6d1d |
There was a problem hiding this comment.
Changes requested. The spec side is settled at this head: an engineering owner approved component:ContextMenu FR12.
Required fixes
- On Android the trigger starts to grow 243–254 ms after the touch (368–451 ms on a slowed device) instead of FR12's 150 ms, because
contextMenuPressFeedback.ts(lines 243–247) waits for the press machine's mark, whose timer counts from when its listener ran, so land #10260's fix below this pull request and report the emulator onset again. - Report in the description the Chrome-with-Gboard Android emulator result for the head that lands, covering the growth's curve, its return after an early lift and its return as the menu closes, with animations at normal scale and removed plus a slow-device profile, as
spec:AST-094Platform support (DEC-27) requires. - A finger that drifts 10–11.5 px inside a scroll view before the onset still gets the menu at 735 ms from a trigger that never grew, because only the press machine's mark schedules the onset and the machine never marks such a press (
contextMenuPressFeedback.tslines 262–265), so once the recognizer follows the finger, schedule the onset from its landing whether or not a mark comes, and add a unit case for that drift.
This pull request lands only when it and #9814 and #8125 below it are approved.
[Automated review]
| const landed = landedAt.current(); | ||
| const onsetAt = | ||
| landed == null | ||
| ? markedAt | ||
| : Math.max(markedAt, landed + PRESS_ONSET_MS); |
There was a problem hiding this comment.
Inside a scroll view markedAt comes from main's 150 ms press timer, which counts from when its listener ran (utils/pressFeedback.ts:372), so Math.max(markedAt, …) holds the growth to that late mark: the description's Android emulator run shows the first frame at 243–254 ms (368–451 ms slowed), not FR12's 150 ms. Taking #10260 below this pull request moves the mark to the touch's own time; then please report the emulator onset again.
| if (next) { | ||
| clearTimeout(onsetTimer); | ||
| markedAt = performance.now(); | ||
| growAtOnset(false); | ||
| } else if (!isFollowed.current()) { |
There was a problem hiding this comment.
This keeps the onset when a mark goes away, but inside a scroll view the press machine drops a press that travels past 10 px before its onset without ever marking it (utils/pressGesture.ts:215-219), so growAtOnset never runs while the recognizer keeps the finger to 11.5 px and opens the menu at 735 ms. Once the recognizer follows a finger, scheduling the onset from landedAt whether or not a mark comes covers both cases; a unit case with an (8, 8) px drift and no mark would pin it.
Inside a scroll view the press machine marks a press late, from its own 150 ms timer, and it never marks a press that travels past 10 px before then. The recognizer still follows a finger to 11.5 px and opens the menu at 735 ms. So a drift of (8, 8) px with no mark, and a finger whose mark comes at 240 ms, both still grow at 150 ms after the finger lands. With no landing time, a finger's onset still comes 150 ms after its touch's own time, not at once and not from a late listener. A finger whose touched node goes mid-press, or a touch the recognizer never takes, does not hold back a later pen's press. These cases fail until the next commit.
The press feedback armed the growth's onset only from the press machine's mark. Now a touch on the trigger arms it from the finger's landing, the touch's own time that the recognizer keeps, so the onset is 150 ms after the touch whether or not the machine marks it, and a later mark does not move it. Without a landing time it counts from the touch's own time, and from now only when that time is an epoch or future stamp. A pointer-only pen still waits for the mark; an iOS stylus touches like a finger. The end of the press, or of the menu it opened, clears the armed finger, and so does a touch the recognizer never takes. The onset still checks that the recognizer follows the finger at that moment, so a lift or a drift past 11.5 px never grows.
|
/approve-spec 754fb3d |
There was a problem hiding this comment.
Changes requested. The spec side is settled at this head: component:ContextMenu FR12 has engineering-owner approval.
Required fixes
- Report in the description the Chrome-with-Gboard Android emulator result for the head that lands, covering when the growth starts, its curve, its return after an early lift and its return as the menu closes, with animations at normal scale and removed plus a slow-device profile, as
spec:AST-094Platform support (DEC-27) requires; the reported run still shows the onset at 243–254 ms, from before the commit that moves it. - Regression:
.changeset/contextmenu-touch-timeline.mdstill says the trigger starts to grow “once core’s one press machine marks the press”, but a finger now arms the growth from its own landing, so say it starts to grow 150 ms after the finger lands.
The onset no longer waits for the press machine, and a drift inside a scroll view now grows the trigger. This pull request lands only when it and #9814 and #8125 below it are approved.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] ContextMenu opens from a held finger on iOS 26's measured timeline where the primary pointer is coarse. A block trigger whose menu opens at the finger starts to grow at 150 ms, once core's one press machine marks the press, toward its held size on UIKit's lift spring, played on the compositor, and the hold keeps UIKit's measured moments. At 400 ms the press takes the finger: its lift no longer sends a click, and the page cannot scroll under it. The menu opens at 735 ms if the finger is still down, or as soon as it lifts after 400 ms. A finger that lifts before 400 ms, or drifts more than 11.5 px on either axis before the menu opens, gets no menu, and the trigger springs back to its size. The page stays free to scroll while the menu is up: a touch outside the menu closes it as it begins to pan, or at its lift. The trigger returns to its size when the menu closes. A touch open lights no row: the menu takes the focus, and a row lights only under the finger, as in UIKit (keyboard and pointer opens still focus the first item). A second finger cancels the press before 400 ms; after it, the menu still opens. A fine pointer (a mouse) keeps the right-click menu, and a finger there keeps today's 500 ms long press. Pass `presentation="popover"` to keep today's menu: the menu at the pointer, opened by a finger held for 500 ms. |
There was a problem hiding this comment.
Regression: a finger now arms the growth from its own landing (contextMenuPressFeedback.ts lines 258–260 and 269–285), and the new unit cases grow it after a late mark and with no mark, so “once core’s one press machine marks the press” no longer holds. Suggested: “A block trigger whose menu opens at the finger starts to grow 150 ms after the finger lands, toward its held size on UIKit’s lift spring, played on the compositor, …”
| * the trigger from the onset, 150 ms after the finger lands. It grows once | ||
| * core's press machine marks the press, while the recognizer follows the | ||
| * finger. It springs back when the recognizer ends the press with no menu, and it |
There was a problem hiding this comment.
Non-blocking: this still says the growth waits for the press machine’s mark, which a finger no longer does. Suggested: “The press feedback grows the trigger from the onset, 150 ms after the finger lands, while the recognizer follows the finger.”
The touch timeline changeset and the touch layer's header said the growth waits for the press machine's mark. A finger now arms it from its own landing, so both say the trigger starts to grow 150 ms after the finger lands.
|
/approve-spec bb6c18b |
There was a problem hiding this comment.
Changes requested. The spec side is settled at this head: component:ContextMenu FR12 has engineering-owner approval, and the growth, its onset and both returns follow it.
Required fixes
- Miss: the press feedback's own
touchstartlistener (contextMenuPressFeedback.ts, lines 268–285) detects the finger's landing a second time although ContextMenu's recognizer already records it aspress.time, so have the recognizer report the landing and delete the listener,isFingerArmedRefand its retry. - Report the Chrome-with-Gboard Android emulator result for the head that lands, covering the onset, the curve, the early-lift return and the return as the menu closes, with animations at normal scale and removed plus a slow-device profile, as
spec:AST-094Platform support (DEC-27) requires; the reported run predates the landing fix.
This pull request lands only when it and #9814 and #8125 below it are approved.
[Automated review]
| const {surface, touchModes} = usePresentation(presentation, { | ||
| whenOmitted: 'touch', | ||
| }); |
There was a problem hiding this comment.
Miss: with presentation omitted this asks usePresentation for the touch query, and its snapshot calls window.matchMedia with no guard during render (utils/presentation.ts:112, :119). So a default ContextMenu, or a Table with row actions, throws TypeError: window.matchMedia is not a function in a test DOM without it: packages/cli/api/template/use-table-sortable-state-hook-usage.test.tsx fails 3 of 3 here and passes on the base. Return false and subscribe to nothing when typeof window.matchMedia !== 'function', as hooks/useMediaQuery.ts does, and add a test that renders a default ContextMenu with no matchMedia.
| // A touch on a nested trigger arms only that trigger. | ||
| const onTouchStart = (event: TouchEvent) => { | ||
| if ( | ||
| !isElement(event.target) || | ||
| pressMarkOwnerOf(event.target) !== element | ||
| ) { | ||
| return; | ||
| } | ||
| clearTimeout(onsetTimer); | ||
| isFingerArmedRef.current = true; | ||
| // The touch's own time, on the page clock. An epoch or future stamp, | ||
| // as in some DOM emulators, counts as now. | ||
| const now = performance.now(); | ||
| armedAt = | ||
| event.timeStamp > 0 && event.timeStamp <= now ? event.timeStamp : now; | ||
| growAtOnset(false); | ||
| }; | ||
| element.addEventListener('touchstart', onTouchStart, {passive: true}); |
There was a problem hiding this comment.
Miss (one shared path, architecture:react-component-runtime INV16): this listener detects the finger's landing a second time, with its own nested-trigger rule, although ContextMenu's recognizer already takes that touch and records it as press.time (contextMenuPress.ts:106-124), which this hook reads through landedAt. Have the recognizer report the landing (a callback from touchesBegan, wired in the touch layer's recognizer factory) and start the onset from it, then delete this listener, isFingerArmedRef and the 0 ms retry.
The press feedback listened for the trigger's own touchstart to time a finger's onset, with its own nested-trigger rule, an armed flag and a retry. ContextMenu's press recognizer already takes that touch first and records its landing. Now the recognizer reports each press it takes once, through `onPressBegan`, and the touch layer passes a finger's landing to the press feedback's new `begin`, which grows the innermost trigger at the onset, 150 ms after the landing. The listener, the armed flag and the 0 ms retry are gone. The retry served a finger whose mark came before the recognizer had it. A pen's mark comes after the gesture host took that pen from the same input, so a pen's press still grows from the press machine's mark, as before; a new case pins it through the real host and press machine. A mark while the recognizer follows a finger does not move that finger's onset.
|
/approve-spec fd6cf04 |
There was a problem hiding this comment.
Changes requested. The spec side is settled at this head: component:ContextMenu FR12 has engineering-owner approval, and timing a finger's onset from the recognizer's landing removes the second landing detector raised in the last review.
Required fixes
- The
Stack: #9814 → #8125 → #9816 → #8133line no longer holds because #8133 does not contain this head, so this pull request was reviewed on its own and its 4,893 authored lines carry no size exemption; merge this head into #8133 so the stack holds and this pull request is judged on its own increment. - Report the Chrome-with-Gboard Android emulator result for the head that lands, covering the onset, the curve, the early-lift return and the return as the menu closes, with animations at normal scale and removed plus a slow-device profile, as
spec:AST-094Platform support (DEC-27) requires; the reported run's 243–254 ms onset predates the recognizer-timed onset.
Non-blocking: a default ContextMenu now reads window.matchMedia during render through usePresentation (utils/presentation.ts), so a jsdom test without a stub throws; a guard like useMediaQuery's avoids that.
[Automated review]
| const isFollowedRef = useRef<() => boolean>(() => false); | ||
| // When the finger the recognizer follows landed, for the growth's onset. | ||
| const landedAtRef = useRef<() => number | null>(() => null); | ||
| // Whether the contact the recognizer follows is a finger. |
There was a problem hiding this comment.
Non-blocking (INV29): this restates followsFingerRef. Suggest deleting the comment.
Stack: #9814 → #8125 → #9816 → #8133
Merge group: #9814, #8125, #9816, #8133
Behavior module: ContextMenu's press feedback.
User need
A held finger should see the trigger grow from 150 ms, as UIKit's view does, before the menu at about 0.74 s. #8125 alone moves nothing then.
Problem and solution fit
springs.contextMenuLift.springs.contextMenuDismisswhen the hold ends with no menu or the menu closes, and holds while the menu is up.Current specification
Since #9915,
component:ContextMenuFR5 names the long-press presentation a gesture component: the lift playssprings.contextMenuLift, and the returnsprings.contextMenuDismiss(spec:AST-094FR7, FR45). FR12 gains the growth on those springs.design:reduced-motiongoverns.Public delta and caller burden
No API change. A block trigger grows about 7%; the docs ask for room around it. ContextMenu's import grows from 110,004 to 122,771 B gzip, mostly core's motion engine (Testing).
Design and alternatives
A per-frame animation stalls on a busy main thread; the springs play on the compositor. The anchor cancels only the trigger's own growth: the menu stays under the finger, and a right-click lands as on main. UIKit's linear ramp under its spring gives way to the lift spring.
Evidence
spec:AST-094Platform support:Scope
Writing rubric results
Over
component:ContextMenu: FR12 adds the growth on FR5's springs beside the hold's internal moments. Owner-gated.Testing
Android, Code Quality's Chrome-with-Gboard emulator at this head: the menu opens at the finger, an early lift and a pan open nothing, an outside press closes it for touch, mouse and pen, and a right-click at 0.5 scale opens the menu at the pointer, within 1 px. The growth is reported: its first frame above 1 lands at 243–254 ms at normal speed and 368–451 ms under a 4× CPU slowdown, past FR12's 150 ms onset, because main's press timer counts from its listener, not the touch (#10260 fixes it). It peaks near 1.0745 and does not grow with animations removed. Curves from a quiet host follow.
ContextMenu's press recognizer now reports each press it takes, at its landing, and the press feedback grows a finger's trigger 150 ms later, in this pull request: the onset no longer waits for the press machine's late mark inside a scroll view (#10260 still moves the machine's own highlight). A drift past the machine's 10 px but inside the recognizer's 11.5 px still grows, marked or not; a pen still grows from the machine's mark. Local checks at fd6cf04: core typecheck; the Layer, ContextMenu, Indicator, utils, motion, gestures and DropdownMenu units (3,933 passed);
ContextMenuLongPressinteraction in Chromium and WebKit (35 passed, 5 skipped); the three ContextMenu a11y specs (27 passed); eslint on the changed files; check:knowledge.Restacked for the chain landing on #8125's head (main 79cc4ac):
contextMenuPressFeedback.tsimportsCONTEXT_MENU_LIFT_SPRINGandCONTEXT_MENU_DISMISS_SPRINGdirectly, withoutregisterSpringsTable()(architecture:component-style-authoringINV6). Local checks at 2ceebe8: core typecheck; the Layer, ContextMenu, Indicator, utils, motion, gestures and DropdownMenu units (3,924 passed); eslint on the changed files; check:knowledge; check:changesets.On #8125 at c4f8964, over the merge worker's #9814 head (main 3731ec3).
springs.contextMenuLiftandsprings.contextMenuDismiss. FR12 and its verification row name the springs, beside FR5's, which main already carries. Red first: 10 of the 162 ContextMenu unit cases (the growth and return cases) fail before the change. The browser proofs compute the lift spring's curve: the compositor curve check, the storyboard (monotone to its peak, overshoot under 3% of the travel, settled at the held scale before the menu) and the busy-thread case (a 150 ms block over the spring's moving part).contextMenuPressTiming.tsgoes and this pull request never touchesmotion/themeTiming.ts(main's refactor(motion): core's shared theme-timing reader #10068 reader). That also drops theeasedProgress/timedCurveduplicate.ContextMenuTouchGrowth.test.tsxcases fail before the fix).pressableCoverage.test.tskeeps main's fix(CheckboxListItem): a row answers a finger the same wherever it lands #8212 press-arm pattern besidepaintsPress, and the growth calls main's chore(lab): Lab's animator hook, over Core's internal touch entry #9754createMotionEngine.Local checks at 4efcbc4. Later commits change the changeset, docs, comments, the story text and test titles, drop a never-read field, and make the anchor cancel only the trigger's own growth (a right-click inside a scaled container is placed as on main; red first, 1 of 163 ContextMenu units). At the stack's top, f913915: core typecheck and the ContextMenu units (171 passed). On its code before a one-helper move (8bb478c):
CI=true pnpm lint(0 errors), the ContextMenu, Layer, DropdownMenu, hooks and utils units (3,010 passed), check:knowledge, check:changesets, the ContextMenu interaction specs in Chromium (22 passed, 1 skipped) and WebKit (19 passed, 4 skipped), and the ContextMenu a11y specs in Chromium (24 passed). At 4efcbc4:CI=true pnpm lint(0 errors), core typecheck, the ContextMenu, Layer, DropdownMenu, motion, hooks, gestures, SegmentedControl and utils units (4,809 passed), check:knowledge, check:changesets, and the Chromium interaction specs for ContextMenu and Layer (34 passed, 1 skipped). On the previous base (main 9e559e9), the stack's top with this content (a4a1a46, apart from two comments) passed the full ui (21,276) and node (16,672) suites, the Chromium interaction specs for ContextMenu, Layer, DropdownMenu, Dialog and Drawer (60 passed, 1 skipped), the same specs in WebKit (51 passed, 10 skipped; WebKit's Linux build runs the trigger's scale on the main thread, so its busy-thread case defers to the iOS run), and their a11y contracts (330 passed). Size (INV6), gzip: ContextMenu 110,004 → 122,771 B, mostly core's shared motion engine; DropdownMenu unchanged (79,793 B); CSS unchanged.Code Quality's earlier Android emulator run at 653a5cd (Chrome; default and Remove animations), on the unsplit touch model: the commit lands at 399–460 ms, and the menu opens at 734–746 ms at the finger. A pan gives no menu, the menu stays open after the lift, a Button trigger grows, and nested triggers do not. Under Remove animations there is no growth, and the menu cross-fades over about 200 ms. It predates the springs, so the booked run at this head replaces it.