Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
|
/approve-spec 578373e |
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
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]
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]
0d1fdb7 to
caf271c
Compare
|
/approve-spec caf271c |
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]
b168c85 to
037a530
Compare
|
/approve-spec 037a530 |
There was a problem hiding this comment.
Changes requested. The spec side is settled at this head: an engineering owner approved component:ContextMenu FR12 for it, and the growth, its 150 ms onset and both returns follow FR12 and FR5.
Required fix
- 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 onset this head times from the recognizer's landing.
In Chromium with phone emulation the trigger starts to grow at 150 ms and holds at 1.0735 while the menu is up. This pull request lands only when it and #9814 and #8125 below it are approved.
[Automated review]
|
/approve-spec 27de157 |
There was a problem hiding this comment.
Changes requested. The spec side is settled at this head: an engineering owner approved component:ContextMenu FR12 for it, and the hold's 400 ms and 735 ms moments, the growth from 150 ms and both returns follow FR12, FR5 and the touch-presentation spec.
Required fix
- 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 onset this head times from the recognizer's landing, and the description's Evidence line still calls it a run at this head.
#8133 does not contain this head yet, so the stack line holds again once it is restacked; this pull request lands only when it and #9814 and #8125 below it are approved.
[Automated review]
|
/approve-spec 63c7a6c |
There was a problem hiding this comment.
Changes requested. The spec side is settled at this head: an engineering owner approved component:ContextMenu FR12 for it, and the hold's 400 ms and 735 ms moments, the growth from 150 ms and both returns follow FR12, FR5 and the touch-presentation spec.
Required fixes
- Regression: since #10438 on
main, an animation hold sent from a test must name the motion it judges, soholdAnimations(page, trigger)atContextMenuLongPress.interaction.browser.spec.ts:548and theanimationStoryboardcall at:833fail core's typecheck and throw once this branch is merged withmain; mergemainand passjudges: {target: trigger, property: 'scale'}to both. - 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 description reports only an earlier head's run.
One non-blocking suggestion: the flatTree.ts file header's "shadowHostOf, a shadow root's host" restates the name and can go.
#8133 does not contain this head yet, so the stack line holds again once it is restacked; this pull request lands only when it and #9814 and #8125 below it are approved.
[Automated review]
A finger held on the trigger opens the menu on UIKit's timeline: the hold commits at 400 ms and the menu shows at 735 ms. One outside-press watch closes the menu for every pointer, and a touch closes it as it begins to pan. It carries this pull request's own diff from 6d7fceb, replayed on main after meta#9814's squash, and folds these 12 commits (the history of 6d7fceb has each one): - chore(motion): ContextMenu's touch timeline and press curve, measured on iOS 26.2 - test(ContextMenu): a held finger opens the menu on iOS 26's timeline, and a touch outside closes it as it pans - feat(ContextMenu): open from a held finger on iOS 26's timeline, with no page lock - docs(ContextMenu): FR11 cites AST-094 FR45 for the hold's measured moments - test(ContextMenu): the long-press a11y contract follows the touch model - docs(ContextMenu): the touch changeset and docs name their condition, not every touch device - test(Layer): the outside-press watch closes a layer at the press for a mouse or a pen - feat(Layer): the outside-press watch closes a layer at the press for a mouse or a pen - refactor(ContextMenu): one outside-press watch closes the menu for every pointer, and its mousedown Effect goes - docs(ContextMenu): split long file-header sentences - test(Indicator): the ContextMenu box-hold case pins the popover menu - test(ContextMenu): the long-press spec note keeps to 20 words
Under a finger, a block trigger grows from the onset, 150 ms after the finger lands, toward 1.0735 times its size on springs.contextMenuLift, and returns on springs.contextMenuDismiss. It carries this pull request's own diff from 63c7a6c, replayed on main after meta#9814's squash, and folds these 27 commits (the history of 63c7a6c has each one): - test(ContextMenu): a held finger grows the trigger on the theme's motion tokens, and the timeline's other proofs - feat(ContextMenu): a held finger grows the trigger on the theme's motion tokens - test(ContextMenu): a trigger gets its size back when its press feedback goes while the menu it opened is up - fix(ContextMenu): the press feedback attaches through useAttachment, and a departing feedback gives the trigger its size back - refactor(ContextMenu): the touch layer's recognizer carries its transition listener, and the press recognizer drops onPressStart - docs(ContextMenu): FR11's growth cites AST-094 FR45 - test(ContextMenu): the trigger returns on the theme's exit duration and easing when the menu closes - fix(ContextMenu): the trigger returns on the theme's exit duration and easing when the menu closes - docs(ContextMenu): FR11 keeps the measured basis for the hold's moments, and the tests that verify them - docs(ContextMenu): the block-trigger guidance says the trigger grows while the menu is up - test(ContextMenu): the growth starts 150 ms after the finger lands, wherever the press machine marks the press - fix(ContextMenu): the growth starts 150 ms after the finger lands, wherever the press machine marks the press - test(ContextMenu): the trigger's growth plays springs.contextMenuLift, and its returns springs.contextMenuDismiss - fix(ContextMenu): the trigger's growth plays springs.contextMenuLift, and its returns springs.contextMenuDismiss - docs(ContextMenu): FR11 names the block trigger, and the tests, story and comments say the trigger springs back - refactor(ContextMenu): the press feedback stops returning the pressable marker, and two comments say what the code does - test(ContextMenu): a right-click in a scaled container gives the anchor no scale of its own - fix(ContextMenu): the anchor cancels only the press feedback's own growth - fix(ContextMenu): the press feedback imports only its two springs - test(ContextMenu): a drift the recognizer still follows grows at onset - fix(ContextMenu): keep the growth's onset while the recognizer follows - test(ContextMenu): a finger grows at its own onset, marked or not - fix(ContextMenu): a finger arms the growth from its own landing - docs(ContextMenu): the growth starts 150 ms after the finger lands - refactor(ContextMenu): the recognizer times a finger's onset - refactor(ContextMenu): drop a comment that restates followsFingerRef - refactor(ContextMenu): the onset ref comment keeps only its constraint
main's hold now rejects a hold that names no judged motion. The curve and storyboard proofs judge the trigger's scale, so they say so.
A touch that reaches the page 90 ms after its timestamp waits for Chromium's timer hold, so the growth starts about 50 ms late, and its back-dated curve skips its first frames. The proof wants the frame before the growth at rest, the first grown frame no further along than the spring one frame in, and that frame within two frames of the onset.
Chromium holds every timer for about 100 ms after it handles a touchstart, so a touch that reached the page late grew late, and its back-dated curve skipped its first frames. The first frame at or after the onset now starts the spring from rest. The motion engine is built in the first frame after the finger lands, not in its touchstart handler. A lift, a move past the tolerance, a cancel and the menu's display stop the frame watch.
The proof read "the menu shows in the lift's task" as "within one frame of the lift". Merged with main, the menu's render in that task takes 17 ms in the WebKit CI container, so the time proxy failed while the menu still showed before any frame. A page flag now records whether the menu shows while the lift's listeners run.
Merged with main, the lazy-sheet bundle proof pins that a menu with an on-demand sheet keeps the animator on demand. ContextMenu's touch growth plays on the animator from the first touch, so the animator loads with the menu. The proof now expects that for ContextMenu, and `rideKeyboard` still loads only with the sheet.
The hold's timer only sets ContextMenu's state, so React renders the menu in its next scheduled task: 17 ms after the deadline in the WebKit CI container, past the proofs' 20 ms in 4 of 24 takes. The 735 ms proofs now also want the menu to show inside the timer task that decides it, and that task at the deadline counted from the touch. The page's stamps record the first show only.
The hold's timer opened the menu with a plain state update, which waits for React's next scheduled task: the menu showed 9 ms after the deadline in Chromium and 17 ms in the WebKit CI container. UIKit shows the menu at its moment. The open now commits with flushSync, in the timer task, as a lift's open commits in the lift's dispatch.
The menu's first show mounts its layer, a render of 6 ms in Chromium and 17 to 21 ms in the WebKit CI container inside the task that shows the menu at 735 ms. UIKit prepares a context menu as the interaction begins and only presents it at its moment. The proofs want the menu mounted, closed and out of the accessibility tree, at the commit; the same element shown at 735 ms or at the lift; and nothing left mounted by a move past the tolerance or a cancel.
A lazy context layer gains prepare(), which mounts its content without showing it, and releasePrepared(), which unmounts that content unless the layer has shown since. Both are internal to core's layer hook. At the hold's commit, ContextMenu prepares its layer, so the show at 735 ms or at the lift only reveals it. A press that ends with no menu releases it.
63c7a6c to
0c316d9
Compare
|
/approve-spec 0c316d9 |
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's growth clause for it, and this pull request is judged on its own changes above #8125.
Required fix
- Report the Chrome-with-Gboard Android emulator result for the head that lands, as
spec:AST-094Platform support (DEC-27) requires: the growth's first frame from rest at the onset, its curve, the early-lift return and the return as the menu closes, with animations at normal scale and removed, plus a slow-device profile; the only reported run is from the previous head, where it found the start skipping into the spring that this head's frame-timed onset fixes.
The earlier inline note on the stopOnsetRef comment is non-blocking. This pull request lands only when it and #8125 are approved.
[Automated review]
| const fingerOnsetRef = useRef< | ||
| ((landedAt: number, target: Element | null) => void) | null | ||
| >(null); | ||
| // Stops the frame watch of a growth still to come. |
There was a problem hiding this comment.
Non-blocking (INV29): this restates stopOnsetRef. Suggest deleting it, or naming the ref stopOnsetWatchRef.
Stack: #8125 → #9816 → #8133
Merge group: #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 735 ms.
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 entry carries core's motion engine eagerly: +36.8 kB min, +13.2 kB gzip across this pull request, as 10 components on main do, under Cindy's Nov 2 SIZE decision (touch code stays eager; the Slider precedent).
Design and alternatives
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
Frames, not timers, decide the onset: Chromium holds a page's timers for about 100 ms after it handles a touchstart.
Replayed onto main cd748d0 after #9814's squash: 8010e0f carries this pull request's earlier own diff (old head 63c7a6c; its message lists the folded commits), and the newer commits sit on top of it. It sits on #8125's b8b8d88, which now carries the menu's commit inside its deciding task and its commit-time mount; with those, this head's tree equals 0c316d9's plus one #8125 unit test, so its product files are the same. Red first: c1c3d01 (a touch 90 ms late, 3 of 3 red before the frame watch).
Touch to first grown frame, Chromium phone profile: 170–173 ms at normal speed, 164–182 ms at 4× CPU, and 166–167 ms with a 60 ms input lag (183 ms, at 1.0110, before the frame watch). Each first frame shows the spring's one-frame value, 1.0033, after an exact 1.000. A quick tap starts no animation and writes no style. The touch handlers' cost does not change: 1.50 / 1.85 ms at 1× (1.50 / 1.70 ms before) and 6.60 / 7.95 ms at 4× (6.70 / 8.30 ms before).
A lift between the commit and 735 ms shows the menu 2–3 ms into the lift's own dispatch in WebKit (17 ms before #8125's commit-time mount). The lift proof, 378537d, asserts that dispatch, not a time. Under Reduce Motion, the 735 ms proof also wants the show inside its deciding task (3aaf37f).
Size, ContextMenu's eager closure (min / gzip): main 324,701 / 103,750 B; with #8125 406,513 / 128,560 B; this head 443,299 / 141,779 B, of which
motion/animator.tsis 28,120 B.lazy-sheet-bundle.test.mjskeeps #8208's sheet andrideKeyboardon demand; ContextMenu's row now expects the animator eager, since its growth needs the animator at the touch.Local checks at this head, 3aaf37f: core typecheck; the ContextMenu, Layer and DropdownMenu units (1,103 passed);
ContextMenuLongPressin Chromium (21 passed, 1 skipped) and in WebKit under the CI profile (17 passed, 5 skipped); the 735 ms proofs ×24 in WebKit (72 of 72). At the stack's top, 4fe24f0: the ui suite (22,436 passed), test:node (17,361 passed) andCI=true pnpm lint(0 errors).Code Quality's Android run at 0c316d9 (Chrome with Gboard), whose product files this head carries: the growth animation's start time is 152–165 ms after the landing, and it starts from 1.000 (first frames 1.000, then 1.002–1.005), with no back-dated jump. Both returns take about 285–354 ms, and Reduce Motion shows no growth. At 63c7a6c, an earlier head, the animation was created about 238 ms after the landing with a back-dated start, so its first painted frame was already at 1.016–1.039 after an exact 1.000.