Repository navigation
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.
|
github-actions
Bot
added
the
needs:spec-owner-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request can't be approved yet: its main changes — the 150 ms finger hold-open (300 ms in a scroller) with a 13 px drift allowance, the 10 px handoff before the finger that opened a menu can pick, and the new public continueGesture — are not stated in any approved spec, because module:DropdownMenu/useMenuPress FR9 and DEC-4 are still proposed and still describe a 500 ms delay and the settle rule. An engineering owner needs to decide that direction, together with the new draft spec:AST-094, the touchOpen DEC-8 and the adaptive-query change measured against Selector DEC-1; once the module record carries the proposed text, an engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so if they are an engineering owner. Independently of that decision, please address:
- Land the four dependencies first, or base this pull request on their branch, so its diff is this change's own ~1,000-line commit rather than 24,416 lines.
useMenuPress.doc.mjsdocsDensestill says a finger never triggersonDismiss, which is no longer true withtouchOpen: 'press', and it lists neithertouchOpennorcontinueGesture.spec:AST-094FR7 proposes amendingspec:AST-090, which does not exist in the repository.
[Automated review]
| * UIKit's hold-open delay for a pull-down button (`menu.holdOpenMs`). A tap | ||
| * released sooner opens the menu through the trigger's click. | ||
| */ | ||
| export const MENU_PRESS_LONG_PRESS_MS: number = menu.holdOpenMs; |
There was a problem hiding this comment.
The default hold-open is now menu.holdOpenMs (150 ms, 300 ms in a scroller), but useMenuPress.spec.md still lists the long-press delay as 500 ms (proposed) and FR9 still sends the opening finger through the settle rule. The module record needs the new values (FR9, DEC-4 and the constant row) for an engineering owner to approve before this default ships.
There was a problem hiding this comment.
Fixed at 309b4eb: useMenuPress.spec.md now carries the proposed values, pending owner review: the long-press row (150 ms, 300 ms inside a scrolling region, on every touch device), a 10 px handoff row, FR9’s handoff and cancel, and DEC-4’s mouse-only settle rule.
| * this call; a mouse follows the settle rule. Ignored while another gesture | ||
| * is live. | ||
| */ | ||
| continueGesture: (pointer: MenuPressContinuation) => void; |
There was a problem hiding this comment.
continueGesture and MenuPressContinuation are new public API on an exported hook, but nothing in the repository calls them and no spec record names them. Consider leaving them out until a consumer such as ContextMenu adopts them, or add them to the module record for an owner's decision.
There was a problem hiding this comment.
Kept and listed for the owner decision. continueGesture hands a menu a pointer that went down elsewhere (a context menu's long press), and the record's Public contract row now names it with MenuPressContinuation. It comes out if the owner prefers to wait for a consumer.
There was a problem hiding this comment.
Removed at fcecec5: continueGesture and MenuPressContinuation have no caller (spec:AST-002 DEC-1), and the record no longer names them.
| @@ -170,7 +183,8 @@ export const docsDense = { | |||
| 'options.onDismiss': 'MOUSE released outside: close. Finger: never called.', | |||
There was a problem hiding this comment.
With touchOpen: 'press' the hook does call onDismiss for a finger (when the browser takes the press before it reaches the menu), so Finger: never called. is now wrong. The dense block also misses touchOpen and continueGesture.
| follows. | ||
| - **FR7 — Exits animate as UIKit's do.** A touch presentation MUST play the | ||
| exit its UIKit counterpart plays. **Proposed; pending owner review:** this | ||
| departs from the draft `spec:AST-090` FR1 (exits off by default) and FR10 |
There was a problem hiding this comment.
spec:AST-090 does not exist on main or at this head, so this proposed amendment (and the one in Current-state impact) points at nothing an owner can rule on. Cite it once it lands, or drop it here.
There was a problem hiding this comment.
AST-094 is now the maintainer’s record in #8316 (one touch model on every touch device); the body cites it there.
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
3d6b786 to
6b1e606
Compare
There was a problem hiding this comment.
This pull request can't be approved yet: its main changes — the 150 ms finger hold-open (300 ms in a scroller), the 10 px handoff before the finger that opened a menu can pick, the cleared highlight after a cancelled press and the new public continueGesture — are not stated in any approved spec, because module:DropdownMenu/useMenuPress FR9 and DEC-4 are still proposed and still describe a 500 ms hold and the settle rule. An engineering owner needs to settle that direction, together with the draft spec:AST-094, touchOpen (DEC-8) and the adaptive sheet query measured against Selector DEC-1; once the module record carries the proposed text, an engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request, and the author, as an engineering owner, may do so. Independently of that decision, please address:
- Land the four dependencies first, or base this pull request on their branch, so its diff is this change's own ~1,060-line commit rather than 26,635 lines.
.changeset/menu-press-uimenu.mdshould name the old 500 mslongPressDelayMsdefault and how to keep it, and say that the hook adds 150 ms to a caller's own value inside a scroller.useMenuPress.doc.mjsdocsDensestill says a finger never triggersonDismiss, and it lists neithertouchOpennorcontinueGesture.spec:AST-094FR7 proposes amendingspec:AST-090, which does not exist in the repository.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feature] A finger on a DropdownMenu or MoreMenu trigger follows UIMenu: resting on the trigger opens the menu after 150 ms (UIKit's delay; 300 ms inside a scrolling region, which waits for the scroll view's content-touch delay first), and the finger that opened the menu picks a row only after it has moved 10 px, so a menu that appears under a resting finger never picks the row beneath it. A mouse keeps its settle rule. `useMenuPress` gains `continueGesture` for a pointer that went down elsewhere (a context menu's long press), and `longPressDelayMs` defaults to 150. |
There was a problem hiding this comment.
This changes a released default (longPressDelayMs 500 → 150 ms). Under AST-017 FR3 the changeset should name the old and new default and the option that restores the old one. Note too that longPressDelayMs: 500 opens at 650 ms inside a scroller, because the hook adds 150 ms to a caller's own value (useMenuPress.ts:812-821).
There was a problem hiding this comment.
Fixed at 309b4eb: the changeset names the old 500 ms default and the new 150 ms on every touch device, says longPressDelayMs: 500 keeps the old one, and states that inside a scrolling region the content-touch delay is added to any value, a caller’s own included.
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
|
Agreed: one implementation.
|
6b1e606 to
309b4eb
Compare
github-actions
Bot
added
the
needs:design-review
|
At f5ef17e:
|
meta#8078's module FR10 cases moved to the sub-menu pull request, so they leave this line too; the flyout's cancel case keeps this line's gesture-system finger. meta#8508's real-Chromium FR28 cases wait for this line's open animation to settle, as its own touch cases do; the blur case waits for the gesture host's fix.
|
On size: measured against its dependencies (#8076, #8298, #8446), this pull request's own change is 2,246 lines, and 1,241 of them are tests. The other 1,005 lines are one capability: the trigger opens as a finger presses, and that opening finger picks only past the handoff distance. It stays one pull request for two reasons:
The next push adds a compare link that shows this change alone. |
There was a problem hiding this comment.
Changes requested: the DropdownMenu and MoreMenu open on press, the 150 ms scroll-view wait and the 10 px handoff match the current touch spec (spec:AST-094 FR29 and FR25), so no spec decision is pending. The diff still carries #8076, #8298 and #8446, about 20,250 of its 22,498 authored lines, so please land them and restack so this pull request shows only the menu press. At 2,246 lines the menu press alone is still over the 2,000-line scope threshold, so the restacked head needs a per-row case for why its parts cannot be split, and the unused pressRef prop in the useMenuPress test harness, left from #8507, can go now.
[Automated review]
| touchModel?: 'pointer' | 'uikit'; | ||
| longPressDelayMs?: number; | ||
| /** Receives the hook's return, for calls the trigger does not make. */ | ||
| pressRef?: {current: DropdownMenuPressReturn | null}; |
There was a problem hiding this comment.
Removed here. #8507 keeps it, with the continuation cases that use it.
|
Done at 013f3ea:
|
There was a problem hiding this comment.
Changes requested: the DropdownMenu and MoreMenu open on press, the 150 ms scroll-view wait and the 10 px handoff match the current touch spec (spec:AST-094 FR29 and FR25), so no spec decision is pending. The diff still carries #8076, #8298 and #8446 (the last behind its open head), about 19,800 of its 22,031 authored lines, so please land them and restack so this pull request shows only the menu press. At 2,239 lines the menu press alone is still over the 2,000-line threshold, and the size table's case that the handoff cannot run without the open on press does not hold, since a finger held for the released delay reaches the same open phase: split the handoff out first, or explain why that step is unacceptable.
[Automated review]
|
Done: the handoff is now its own pull request, #8627 (1,094 lines, with its own real-browser proof), merged here as a dependency. This pull request's own change is 1,576 lines: vjeux/astryx@ffb2d2d...b656856.
|
There was a problem hiding this comment.
Changes requested: the menu press itself matches the current touch spec (spec:AST-094 FR29), so no spec decision is pending, but the diff still carries #8076, #8298, #8446 and #8627, about 21,300 of its 22,853 authored lines. Please land those (or wait for them to land) and restack so this pull request shows only its own 18 files and 1,576 lines.
[Automated review]
There was a problem hiding this comment.
Changes requested: the menu press itself follows the current touch spec (spec:AST-094 FR29), so no spec decision is pending, but the diff still carries #8076, #8298, #8446 and #8627, which are 17,553 of its 18,989 authored lines, and the older #8076 fold in it now conflicts with main in packages/core/src/motion/animator.ts and packages/core/src/motion/README.md. Please land those pull requests first (or wait for them to land) and restack, so this one shows only its own 18 files and 1,576 lines.
[Automated review]
There was a problem hiding this comment.
Changes requested: the trigger's open-on-press change matches the touch spec, but this pull request still carries the full contents of four other open pull requests.
- Land #8076 (with #8446), #8298 and #8627 first, or retarget this branch onto #8627, so the diff holds only the menu-press change.
- The #8076 copy carried here is not that pull request's current head, so merging from here would ship a different version of it.
[Automated review]
There was a problem hiding this comment.
Changes requested: the trigger's open on press follows spec:AST-094 FR29 where the primary pointer is coarse, but three things hold this pull request.
- The diff still carries the full contents of #8076, #8446, #8298 and #8627 (17,350 of its 18,810 authored lines), so land those first or retarget the branch so the diff holds only this change.
module:DropdownMenu/useMenuPressFR9 applies FR29 under every primary pointer (spec:AST-094FR43), yet the trigger's touch model follows(pointer: coarse)and the new fine-pointer cases inDropdownMenu.test.tsxpin the 500 ms wait, so key the model onpresentationalone and turn those cases into FR29 cases.test-nodefails main'sscripts/touch-support-bundle.test.mjsbecause the root and hooks entries now bundlegestures/nativeScroll.ts, so list #8671 in the stack and land it first, or keep the scroll probe off the public hook's import path.
[Automated review]
| onDismiss: closeMenu, | ||
| // Under a finger the trigger is UIMenu's pull-down button (spec:AST-094); | ||
| // `presentation="popover"` pins the pointer model, today's included. | ||
| touchModel: touchModes ? 'uikit' : 'pointer', |
There was a problem hiding this comment.
Where the primary pointer is fine, touchModes is false (utils/presentation.ts:102, over TOUCH_QUERY = '(pointer: coarse)'), so a finger on a touchscreen laptop keeps the 500 ms hold and the settle rule here. module:DropdownMenu/useMenuPress FR9 governs a finger's opening and handoff by spec:AST-094 FR29 "under every primary pointer (spec:AST-094 FR43)". menuTriggerPress.ts already applies 'uikit' to touch and pen gestures only, so the model can follow presentation alone.
| expect(trigger).toHaveAttribute('aria-expanded', 'true'); | ||
| }); | ||
|
|
||
| // spec:AST-094 FR19 and module:DropdownMenu/useMenuPress FR9: the touch |
There was a problem hiding this comment.
FR19 and module FR9 no longer say this: since spec:AST-094 FR43, FR19 reads "Recognizers attach for every pointer, with FR43's exceptions" and module FR9 applies FR29 "under every primary pointer". The fine-pointer cases below (3088, 3098, 3180) pin the wait FR29's verification row fails ("a fixed-bar press that waits").
|
|
||
| import {useCallback, useMemo, type RefObject} from 'react'; | ||
| import {currentGesture} from '../Layer/gestureCounter'; | ||
| import {createNativeScrollProbe} from '../gestures/nativeScroll'; |
There was a problem hiding this comment.
This import puts gestures/nativeScroll.ts (2,585 bytes minified) into Core's root and hooks entries, which fails main's scripts/touch-support-bundle.test.mjs in test-node (2/4). The public useMenuPress pays for it without using it: under the pointer model both hold detectors take the same delay and tolerance (:161-170). Either keep the probe off the public hook's import path, or list #8671 in the stack and land it first.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feature] Under a finger, a DropdownMenu or MoreMenu trigger now opens its menu as the finger presses, instead of after a 500 ms hold (`spec:AST-094` FR29). Inside a scroll view it opens once the 150 ms content-touch delay has passed with no scroll, so a scroll that starts on it still scrolls. To keep the previous behavior, a tap that opens at the lift and a 500 ms hold, pass `presentation="popover"`. A mouse is unchanged. |
There was a problem hiding this comment.
"Under a finger" holds at this head only where the primary pointer is coarse: on a touchscreen laptop or a tablet with a trackpad the finger still waits 500 ms. Once the trigger's touch model follows FR29 under every primary pointer, this sentence is true as written.
There was a problem hiding this comment.
Changes requested: the trigger's open on press now follows spec:AST-094 FR29 on every device, as FR43 and the useMenuPress record ask, but three things hold this pull request.
- The diff still carries the full contents of #8076, #8298, #8446 and #8627 (17,406 of its 18,861 authored lines), so land those first or retarget the branch so the diff holds only this change.
test-nodefails main'sscripts/touch-support-bundle.test.mjsbecause the root and hooks entries now bundlegestures/nativeScroll.ts, so put #8671, which updates that test, ahead of this pull request in the stack.- A pen now opens the menu on press on every device, and
spec:AST-017FR59 asks for fine-pointer evidence before and after, so please add a pen case and show the mouse and pen behavior before and after in the description.
[Automated review]
|
|
||
| import {useCallback, useMemo, type RefObject} from 'react'; | ||
| import {currentGesture} from '../Layer/gestureCounter'; | ||
| import {createNativeScrollProbe} from '../gestures/nativeScroll'; |
There was a problem hiding this comment.
This import puts gestures/nativeScroll.ts (2,585 bytes) into core's root and hooks entries, and CI's test-node (2/4) fails main's scripts/touch-support-bundle.test.mjs:110 on both. The public useMenuPress pays for it too, although under its pointer model both hold detectors take the same delay (:161-170). #8671 updates that test but is not in this stack; putting it ahead of this pull request would let CI pass.
| const isUikitFinger = | ||
| pointerType !== 'mouse' && | ||
| ctx.optionsRef.current.touchModel === 'uikit'; |
There was a problem hiding this comment.
Every non-mouse pointer takes the UIKit path here, so a pen on a fine-primary device (a laptop with a stylus) now opens on press and waits for the 10 px handoff instead of the 500 ms hold. spec:AST-017 FR59 asks for fine-pointer evidence before and after such a change, and the fine-pointer cases in DropdownMenu.test.tsx drive only touch events; a pen case under a fine-pointer stub would cover it.
There was a problem hiding this comment.
Changes requested: the trigger's open on press follows spec:AST-094 FR29 on every device, and the new pen cases supply the fine-pointer evidence, but the diff still carries the full contents of #8076, #8298, #8446 and #8627 (17,387 of its 18,890 authored lines). Land those, and #8671 as the stack line orders, then restack, or retarget this branch onto #8627, so the diff holds only the menu press; the red bundle test in test-node clears with #8671, whose updated test passes on this head.
[Automated review]
There was a problem hiding this comment.
Changes requested: the trigger's open on press follows spec:AST-094 FR29 on every device, but the diff still carries the full contents of #8076 (with #8446), #8298, #8360 and #8627, at least 27,956 of its 30,290 authored lines. Land those and #8671, then restack so the diff holds only the menu press, and name #8360 in the stack line until then. Also say "under a finger or a pen" in .changeset/menu-press-uimenu.md and the presentation docs of DropdownMenu and MoreMenu, since a pen's press now opens the menu too.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feature] Under a finger, on every device, a DropdownMenu or MoreMenu trigger now opens its menu as the finger presses, instead of after a 500 ms hold (`spec:AST-094` FR29, FR43). Inside a scroll view it opens once the 150 ms content-touch delay has passed with no scroll, so a scroll that starts on it still scrolls. To keep the previous behavior, a tap that opens at the lift and a 500 ms hold, pass `presentation="popover"`. A mouse is unchanged. |
There was a problem hiding this comment.
A pen's press now opens the menu too (packages/core/src/hooks/menuTriggerPress.ts:225-227 treats every non-mouse pointer alike, and DropdownMenu.test.tsx:3131 pins it), so this note should say "under a finger or a pen"; the presentation entries in DropdownMenu.doc.mjs and MoreMenu.doc.mjs (both languages) need the same words (spec:AST-017 FR3).
… moves (spec:AST-094 FR29, FR43) Under a finger or pen, on every device, a DropdownMenu or MoreMenu opened under a resting finger picked the row beneath it once the press had lasted 300 ms. The press machine's open phase now takes UIMenu's handoff: until the finger is 10 px, in a straight line, from the first position the open menu sees, nothing lights and a lift acts on nothing; then it slides and picks as usual. A mouse keeps the settle rule, and presentation="popover" pins the pointer model. - menuPressGesture: the handoff state and transitions, on internal types; the released menuPressStep keeps its public shapes. - useMenuPress: the internal useDropdownMenuPress entry takes a touch model; the public hook is unchanged. - menuPressValues: UIKit's handoff distance, copied from the constants table so a menu bundles the number, not the table. - DropdownMenu and MoreMenu run the touch model unless presentation is "popover"; docs and a changeset say so. - A real-browser spec (DropdownMenuTouchHandoff); core declares the interaction harness it imports.
There was a problem hiding this comment.
Changes requested. The menu press itself follows the current touch spec (spec:AST-094 FR29), and its unit and real-browser tests pass, but two things need fixing first:
- Land #8627 and restack this branch: the diff still carries #8627's handoff commit (1,111 of its 2,634 authored lines), which keeps it over the 2,000-line scope limit with a separable part.
- In
menuTriggerPress.ts,isInScrollView(lines 120-142) walks the ancestors again after askingcreateNativeScrollProbe(control).isInsideScroller, which already applies the same fixed-ancestor rule the press controller uses, so return the probe's answer alone (the existing tests still pass that way).
Two comments in the same file could also use a pass: lines 175-178 still say a held finger opens later inside a scroll view, which no longer matches the detectors, and lines 229-232 narrate the branch.
[Automated review]
| const doc = control.ownerDocument; | ||
| const view = doc.defaultView; | ||
| const root = doc.scrollingElement ?? doc.documentElement; | ||
| for ( | ||
| let node = control.parentElement; | ||
| node != null && node !== root && node !== doc.body; | ||
| node = node.parentElement | ||
| ) { | ||
| const style = view?.getComputedStyle(node); | ||
| const scrolls = | ||
| style != null && | ||
| ((/(auto|scroll)/.test(style.overflowY) && | ||
| node.scrollHeight > node.clientHeight) || | ||
| (/(auto|scroll)/.test(style.overflowX) && | ||
| node.scrollWidth > node.clientWidth)); | ||
| if (scrolls) { | ||
| return true; | ||
| } | ||
| if (style?.position === 'fixed') { | ||
| return false; | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
createNativeScrollProbe(control).isInsideScroller (line 117) already drops every scroller past a fixed ancestor (isBeyondFixed in gestures/nativeScroll.ts), and the press controller uses that answer for FR11's in-scroller delay. Once it says yes, this second walk can only disagree on overflow: overlay. Returning the probe's answer alone keeps one "inside a scroll view" rule; the existing hook, DropdownMenu and MoreMenu cases (fixed bar, sticky, scroll view) still pass that way.
| // a scrolling region, UIKit first holds the touch for the scroll view's | ||
| // content-touch delay (`scroll.delaysContentTouchesMs`). So a held finger | ||
| // opens the menu that much later, and the press picks its detector when it | ||
| // lands. |
There was a problem hiding this comment.
Non-blocking: "a held finger opens the menu that much later" no longer matches the detectors below. The pointer model waits the same pointerHoldMs everywhere, and the touch model waits 150 ms instead of a hold. Perhaps: "Inside a scroll view the touch model waits for the content-touch delay. A scroll can claim the touch first."
| // A trigger in a fixed bar opens as the finger presses, and the page | ||
| // holds still under it as a bar's does in UIKit. The finger's compatibility mouse events must not follow it | ||
| // onto the trigger: their mousedown would take focus back from the | ||
| // menu. |
There was a problem hiding this comment.
Non-blocking: the first sentence restates the branch, and line 230 runs to 116 characters. Perhaps: "Stop the finger's compatibility mouse events. Their mousedown would take focus back from the menu."
… carry positions The released menuPressStep hands its events to the internal transition, which starts the finger's handoff when a move carries x and y. The public gesture drops the handoff origin, so a caller whose events carried positions got no pick from the opening finger. menuPressStep now strips the positions first, and a test whose events carry them pins it. Three comments are reworded.
|
/approve-spec 4348b8a |
There was a problem hiding this comment.
Changes requested. The menu press follows the current touch spec (spec:AST-094 FR29), and its unit tests and real-browser touch specs pass, but two things need fixing first:
- Land #8627 and restack this branch: the diff still carries #8627's handoff commits (1,151 of its 2,718 authored lines), which keeps it over the 2,000-line scope limit with a separable part.
- In
docs/specs/components/DropdownMenu/DropdownMenu.spec.md, the FR8 verification row (line 350) still says the held-touch case asserts the 500 ms hold with the FR8 gap pending, which this change removes, and the "a tap on the trigger still opens through its click" case it cites now opens on the press, so update that sentence and rename the case.
Non-blocking: menuTriggerPress.ts lines 142-151 state the scroll-view wait twice, and DropdownMenu.tsx lines 1383-1384 repeat the pressTouchModel doc comment. The record edit already has an engineering owner's /approve-spec for this head; the restacked head will need it recorded again.
[Automated review]
| | FR5 | `DropdownMenu.test.tsx` press-model cases ("a finger that lands on one row and lifts on another acts on the second, once", "a mouse released outside closes the menu; a finger released outside leaves it open", "marks the menu as carrying the press model and owns touch scrolling"); `MenuPress.a11y.chromium.spec.ts`; the module record's own map | Finger and mouse release; scrolling menu | A row that acts on press-down, a mouse release outside that keeps the menu, or a missing press marker fails. | `audit:DropdownMenu/behavior` | | ||
| | FR6 | `DropdownMenu.test.tsx` focus-return cases ("restores focus to the trigger after keyboard dismissal", "returns focus to the trigger after pointer dismissal without a focus ring", "leaves focus on the control a press outside landed on") and the "focus return after a pointer pick" and "a trigger the caller renders" cases | Keyboard and pointer close; Button and custom trigger | A lost focus return, a ring after a pointer close, no ring after a keyboard close, or focus pulled off a control pressed outside fails. | `audit:DropdownMenu/accessibility` | | ||
| | FR7 | `DropdownMenu.test.tsx` menuMaxHeight cases | A cap above 300px; viewport bound | A menu clipped to 300px under a raised cap, or one that escapes the viewport, fails. Evidence gap: no test asserts that the touch sheet ignores the cap. | `audit:DropdownMenu/layout` | | ||
| | FR8 | `DropdownMenu.test.tsx` press-model trigger cases ("a mouse press on the trigger opens the menu and a drag-release acts on the row under it", "the opening release before the settle time acts on nothing and the menu stays", "pressing the trigger of an open menu closes it and does not reopen it in the same gesture", "under a coarse pointer a finger opens the menu as it presses the trigger (no scroll view), and a slide picks", "a tap on the trigger still opens through its click") | Mouse press-open; held finger; tap | A mouse that opens only on click, an opening release that acts early, or a re-open in the closing gesture fails. The held-touch case asserts the current 500 ms hold, the FR8 gap pending conformance. | `audit:DropdownMenu/behavior` | |
There was a problem hiding this comment.
This row still ends "The held-touch case asserts the current 500 ms hold, the FR8 gap pending conformance." The renamed case now asserts the open on press, and this change removes the FR8 gap. Suggested: "A finger press that waits for a hold, or a tap whose click toggles the menu its press opened, fails." The cited "a tap on the trigger still opens through its click" case (DropdownMenu.test.tsx:3307-3313) also opens on the pointerdown now; its name should say so.
| // The long-press detector of the system: the same hook ContextMenu spreads | ||
| // onto its trigger, driven here from the pointer stream this model already | ||
| // observes. Its touch handlers go unused; the hold duration, the movement | ||
| // tolerance and the timer are what matter, and they have one home. Inside | ||
| // a scroll view the touch model waits for the content-touch delay | ||
| // (`scroll.delaysContentTouchesMs`), so a scroll can claim the touch first. | ||
| // The press picks its detector when it lands. | ||
| // With `touchModel: 'uikit'`, only a trigger inside a scroll view waits: | ||
| // the content-touch delay, within the drift allowance. One in a fixed bar | ||
| // opens as the finger presses, so its detector never runs. |
There was a problem hiding this comment.
Non-blocking (INV29): this block states the scroll-view wait twice. Suggested: "The long-press detector ContextMenu uses, fed from this model's pointer stream. Only its timer, hold time and drift allowance are used. Under the touch model a trigger in a fixed bar opens on press. In a scroll view it waits for the content-touch delay, so a scroll can claim the touch."
| // Under a finger, on every device, the trigger acts as UIMenu's pull-down | ||
| // button. `presentation="popover"` keeps the pointer model. |
There was a problem hiding this comment.
Non-blocking (INV29): this repeats the pressTouchModel doc comment a few hundred lines up; it can go.
|
/approve-spec 1b27273 |
There was a problem hiding this comment.
Changes requested. The trigger's open on press follows the current touch spec (spec:AST-094 FR29), and its unit tests, real-browser touch specs and type check pass at this head, but one thing needs fixing first:
- Land #8627 and restack this branch: two of its three commits are #8627's handoff (1,151 of 2,713 authored lines), which keeps the diff past the 2,000-line limit with a separately reviewed change inside it.
Non-blocking: menuPressGesture.ts repeats the same @internal sentence at lines 39, 67, 109 and 124, and menuTriggerPress.ts lines 56 and 148-150 restate the drift-allowance comment. The record edit has an engineering owner's /approve-spec for this head, so the restacked head will need it recorded again.
[Automated review]
| return type === 'mouse' || type === 'touch' || type === 'pen' ? type : null; | ||
| } | ||
|
|
||
| /** @internal The machine's own shape, with the finger's handoff; not public contract. */ |
There was a problem hiding this comment.
Non-blocking (comments): this sentence repeats on lines 67, 109 and 124. Keep /** @internal */ on each and say once, at the released-shape divider, that the handoff fields are not public contract.
| */ | ||
| export const MENU_PRESS_TRIGGER_DRIFT_PX = MENU_PRESS_HOLD_OPEN_SLOP_PX; | ||
|
|
||
| /** A held finger on the trigger gets the same drift allowance under every primary pointer. */ |
There was a problem hiding this comment.
Non-blocking (comments): this restates MENU_PRESS_TRIGGER_DRIFT_PX's comment just above, and lines 148-150 say it a third time. Delete both; the constant's comment already gives the reason.
…resses (AST-094 FR29) Under a finger or pen, on every device, a DropdownMenu or MoreMenu trigger in a fixed bar opens the menu as the finger presses, as UIMenu's pull-down button does. Inside a scroll view it opens once the 150 ms content-touch delay has passed with no scroll, within a 13 px drift allowance. The opening finger then picks only after the handoff distance. A mouse is unchanged, and presentation="popover" keeps the pointer model. - menuTriggerPress.ts: the trigger half of the press model (the press-down open, the held finger, the scroll-view wait), reached only by menus with a trigger. - useMenuPress: the trigger-less callers inside core's own menus (sub-menus, ContextMenu, Selector's list and the sheet's action list) take the internal useMenuSurfacePress, so they do not bundle the trigger half. - menuPressValues: UIKit's hold-open slop and content-touch delay, copied from the tables with drift tests. - A real-browser spec (DropdownMenuTouchPress) and an accessibility case. - The touch-support bundle check names the eight imports that now carry the trigger half's scroll-view wait.
There was a problem hiding this comment.
Requesting changes. The menu press itself follows spec:AST-094 FR29: a finger or pen opens a DropdownMenu or MoreMenu as it presses (after the content-touch delay in a scroll view), the opening finger picks only past the 10 px handoff, and presentation="popover" keeps the released behavior.
Blocking:
- This branch still carries the open #8627 (its two commits are 1,151 of the 2,727 changed lines), so merging it would land that separately reviewed handoff change too and keeps the diff over the 2,000-line limit; land #8627 first and restack so this pull request carries only its own change (about 1,724 lines).
Optional: the @internal note "The machine's own shape, with the finger's handoff; not public contract" repeats five times in menuPressGesture.ts; saying it once at the released-shape divider is enough.
[Automated review]
| return type === 'mouse' || type === 'touch' || type === 'pen' ? type : null; | ||
| } | ||
|
|
||
| /** @internal The machine's own shape, with the finger's handoff; not public contract. */ |
There was a problem hiding this comment.
Non-blocking (comment style): this sentence repeats on the four other internal shapes and on menuPressTransition. Keep @internal on each and say once, at the released-shape divider: "The handoff positions and handoffFrom stay internal."
Re-applied onto current main, stacked on #8627.
User need
Under a finger, on a phone or a touchscreen laptop, a DropdownMenu or MoreMenu opens only when a tap lifts or after a 500 ms hold. On iOS it opens as the finger presses, so the same finger slides straight to a row.
What led to it: #8627's UIKit comparison (
spec:AST-094FR29); FR43 applies it on every device.Problem and solution fit
Under a finger, on every device, the trigger opens on press. In a scrolling page it first waits UIKit's 150 ms content-touch delay, so a flick still scrolls; drifting over 13 px in that time is a scroll, and nothing opens. The opening finger then picks only after #8627's 10 px handoff.
Current specification
spec:AST-094FR26–FR30 and FR43 (current);module:DropdownMenu/useMenuPress(current). Thecomponent:DropdownMenurecord catches up, text only: FR8's implementation gap closes, and its verification row names the renamed press case.Public delta and caller burden
presentation="popover"keeps the tap's lift and the 500 ms hold.useMenuPress: no new option or type; its 500 ms default stays (useMenuPress: unset, longPressDelayMs follows FR29 on every device (AST-094 FR25, FR43) #8531 changes it). Core's menus that never press a trigger (sub-menus, ContextMenu, Selector's list, the sheet's action list) take an internal surface hook without it. The nav, breadcrumb and tab menus keep the public hook, as a customer would. Only DropdownMenu passes the touch model's scroll probe, so the touch-support bundle check names DropdownMenu and MoreMenu.Design and alternatives
Evidence
iOS 26.2 simulator, Safari, a scrolling page: press "Actions", slide to "Duplicate". It opens at 156 ms; on main, at 500 ms. Recorded on an earlier head.
Real Chromium, fine primary pointer: a held pen opens at the press (14–15 ms), not after 500 ms; a mouse is unchanged.
Writing rubric results
R1 narration, R2 research, R4 rejected alternatives, R5 change references, R6 open questions: none. R3 evidence: FR8's verification row names the renamed press and tap cases and no longer cites the hold.
Scope
Testing
On the DropdownMenu "Default" story, with touch emulation:
With #8627's press model, DropdownMenu and MoreMenu restored at this head,
DropdownMenuTouchPressfails in Chromium: the press does not open the menu. It passes here, in Chromium and WebKit.Local gates at 4ad7e96: the menu, nav and picker units pass (2,321 of 2,321) and the touch-bundle test (8 of 8), which fails with the scroll probe imported statically; the core type check and strict lint pass. The full gates are running.
Stack: #8627 → this PR → #8531, #8507, #8508. Review only this PR's change: vjeux/astryx@6d0389c...4ad7e96