Repository navigation
DropdownMenu: under a finger, the opening finger picks only after UIMenu's handoff (AST-094 FR29) - #8627
DropdownMenu: under a finger, the opening finger picks only after UIMenu's handoff (AST-094 FR29)#8627vjeux wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsMoreMenu (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression55 of 504 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
meta-cla
Bot
added
the
CLA Signed
![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 menu handoff itself matches AST-094 FR29: the finger that opened the menu picks only after moving 10 px, and presentation="popover" keeps the released rule. But this branch also carries the still-open #8076, #8298 and #8446, so the diff is about 21,400 lines for a roughly 1,100-line change. Please land those pull requests first, or wait for them, and rebase onto main so this one carries only its handoff change; if the resolver ships in the same release, also drop "No component reads it yet; behavior is unchanged" from .changeset/presentation-resolver.md.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] An internal `usePresentation` / `readPresentation` resolves a component's `presentation` prop into the surface to render and whether its touch input modes apply, keeping an omitted value distinct from an explicit `'popover'` so a caller's choice always wins over a touch default. No component reads it yet; behavior is unchanged. |
There was a problem hiding this comment.
At this head DropdownMenu reads usePresentation (packages/core/src/DropdownMenu/DropdownMenu.tsx:1784) and its behaviour changes under a finger (.changeset/menu-press-handoff.md), so "No component reads it yet; behavior is unchanged" is no longer true when both ship in one release. Ship this resolver in an earlier release, or drop that sentence.
There was a problem hiding this comment.
Changes requested. The menu handoff itself matches AST-094 FR29: under a finger, the finger that opened the menu picks a row only after moving 10 px, and presentation="popover" keeps the released rule. But this branch still carries the open #8076, #8298 and #8446 (17,553 authored lines for a 1,094-line change), and 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, then rebase onto main so this one carries only the handoff change.
[Automated review]
There was a problem hiding this comment.
Changes requested: the DropdownMenu handoff itself matches AST-094 FR29 and FR43 (a finger or pen picks only after moving 10 px from where the open menu first sees it, and presentation="popover" keeps the released rule), and the latest commit cleanly drops the unused presentation resolver call. The diff still carries the open #8076, #8446 and #8298, which make up 16,256 of its 17,387 authored lines next to this change's 1,131. Please land those pull requests first and rebase onto main, or rebase the handoff commits alone onto main with the one @astryxdesign/interaction-harness dev dependency their browser spec needs, so the diff holds only the handoff change.
[Automated review]
… 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.
b38a16c to
503bc5b
Compare
github-actions
Bot
removed
the
needs:design-review
There was a problem hiding this comment.
This looks ready to approve: the DropdownMenu change matches AST-094 FR29 and FR43, so under a finger or pen the finger that opened the menu picks only after moving 10 px from where the open menu first sees it, while presentation="popover" keeps the released settle rule, and the branch now carries only this change. The handoff unit tests and the Chromium touch spec pass, and a mouse behaves exactly as before. One optional cleanup: the extra TOUCH_QUERY and COMPACT_QUERY answers in the adaptive-presentation test's media stub are not needed by this change and could be dropped, with the COMPACT_QUERY import.
[Automated review]
| matches: [ | ||
| COMPACT_TOUCH_PRESENTATION_QUERY, | ||
| TOUCH_QUERY, | ||
| COMPACT_QUERY, | ||
| ].includes(query), |
There was a problem hiding this comment.
Optional: nothing DropdownMenu renders at this head reads TOUCH_QUERY or COMPACT_QUERY, and this test passes with the original one-query stub, so this edit (and the COMPACT_QUERY import) could be dropped to keep the diff to the handoff.
… as a dependency meta#8627 is now one commit on main; meta#8360 carries meta#8076, meta#8073 and meta#8446. useMenuPress.ts: the trigger half lives in menuTriggerPress.ts, so it takes meta#8360's trigger changes there. The trigger's holds run on the long-press detector and pass FR29's 13 px drift allowance as each press starts, under every primary pointer (FR43). The hold duration comes from motion/uikit/longPress.ts. MENU_PRESS_TRIGGER_DRIFT_PX is re-exported beside MENU_PRESS_LONG_PRESS_MS. The stray-click window is the shared click guard's, from meta#8360.
… device (spec:AST-094 FR43) The public hook's unset default runs DropdownMenu's touch model for a finger on every device: the coarse-pointer read goes. A set longPressDelayMs keeps the released wait, so the two cases that show an untyped touchModel reaches nothing now set one. From the review: the changeset names the 10 px handoff and longPressDelayMs: 500 as the way back, and meta#8627's changeset no longer says the public hook is unchanged. The docs say "on every device".
…ency meta#8627 no longer resolves the touch modes: since the press keys on presentation, nothing below reads them. The grow and collapse here do (spec:AST-094 FR31: a finger's on a touch device), so this pull request now brings them: usePresentation's touch default for the surface, and the popover's touchModes prop.
meta#8089 now takes meta#8080, which takes meta#8078 (with meta#8627, meta#8360 and meta#8298) and meta#8186. The DropdownMenu spec's FR5 row takes meta#8080's list (FR26–FR32, FR34 and FR35) and its state names FR35 verified by this PR; FR28's cancel clear and FR34 stay pending on this line. Prettier re-pads the table.
meta#8089 now takes meta#8080, which takes meta#8078 (with meta#8627, meta#8360 and meta#8298) and meta#8186. DropdownMenuTouch.ts registers UIKit's two menu springs for the grow, from meta#8080, and keeps the springs table for the in-place sub-menu; its header says so. The usage text takes meta#8080's wording, meta#8089's popover sentence and this PR's in-place sub-menu sentence.
…wins over touch defaults (#8298) <!-- Template: maintenance.md (an internal resolver; no behavior change until a component reads it). --> Small batch of the merge plan: one commit on main holding only this change. Its paragraph for the gestures README waits for that README, which comes with #8076; the resolver's file header carries the same contract. ## Maintainer impact A component that behaves differently under a finger, such as a menu that tracks the finger or becomes a sheet, must still let a caller pin the desktop behavior. Today components read `presentation ?? 'popover'`, which cannot tell an omitted prop from an explicit `'popover'`, so a component that adopts a touch default would override a caller who asked for the popover. The menu, context menu and picker work that adopts touch modes builds on this resolver (#8104, #8627, #8080, #8125, #8201, #8159, #8162). ## Intended invariant A caller's explicit `presentation` always wins over the touch default (`architecture:public-component-api` INV16), and the surface equals `useAdaptivePresentation`'s for every value on every device, so no caller sees a change: | `presentation` | surface | touch input modes | | ------------------------------- | --------------------------------------------- | ----------------- | | `'popover'` | popover | never | | `'bottom-sheet'` | sheet | under a finger | | `'adaptive'` | sheet on a compact touch screen, else popover | under a finger | | omitted, `whenOmitted: 'touch'` | popover | under a finger | The modes follow the pointer, never the platform. The server and hydrating render never present touch modes, and no hint changes that (`spec:AST-094` FR13). The hook listens only to the queries a request reads (`architecture:react-component-runtime` INV5): none for a pinned `'popover'`, the pointer for `'bottom-sheet'` and the omitted `'touch'` default, and the pointer and compact size for `'adaptive'`. A touch-mode style is applied through `touchModes`, never nested under the iOS-only `@supports` gate. ## Change - `utils/presentation.ts`: `usePresentation` and `readPresentation` (and the pure `resolvePresentation`) return the surface to render and `touchModes`, whether the component's touch input modes apply; `usePresentation` subscribes to each media query only while the request reads it. Internal: no component reads it yet. - `hooks/useAdaptivePresentation.ts`: its header points a component that also switches input modes to `usePresentation`, whose surface is its own. ## Evidence - Failure before the change: an omitted `presentation` and an explicit `'popover'` resolve the same, so a touch default cannot tell them apart; a server render given a touch hint must still answer the popover with no touch modes, a case that failed first. On an iPhone, a resolver that seeded its server snapshot from the live window fails both the server-render case and the `hydrateRoot` case; listening to both queries for every value fails the subscription case. A resolver that gives `'adaptive'` the sheet without the coarse pointer, or reads compact by width only, fails the surface matrix; one that ignores the theme's `md` fails the themed case. - Success after the change: the unit matrix (iPhone, iPad, Android, desktop, Mac Safari, a fine-pointer narrow window, a phone in landscape, and the 767/768 and 499/500 compact edges, plus `'adaptive'` under a theme whose `md` is 1000) covers each row above, a live pointer change and the one-moment read; on an iPhone, the server render and the hydrating render answer the popover with no touch modes, and after hydration the phone's sheet with touch modes; each request holds only the subscriptions it reads. - Product/runtime behavior verified unchanged: every value renders `useAdaptivePresentation`'s surface on every device, and no component reads the resolver yet. ## Scope - [x] No intended public API, product behavior, visual direction, or policy change. - [x] Product changes discovered during the work were removed or split. - [x] Public text and artifacts contain no internal Meta context. ## Testing Local gates at 9f0595f, one commit on main d73908f holding only this change (4 files, +571/−1): - `pnpm test:ui` (core UI): 654 files, 16,459 tests passed. - `pnpm exec vitest run --project node`: 804 files passed; the only failures are main's own story-tree and release-cache reds, tracked in #8509 and #8520. - `tsc --project packages/core/tsconfig.json --noEmit`, strict ESLint and Prettier on every changed file: clean. - `pnpm check:repo`: clean.
Rebuilt onto main alone, as review suggested; the earlier stacked history is replaced.
User need
Under a finger, on a phone or a touchscreen laptop, a menu can pick a row nobody chose. A finger held on a DropdownMenu or MoreMenu trigger opens the menu under it, and once the press has lasted 300 ms, lifting picks whatever row is beneath. On iOS, the finger that opened a menu picks only after it moves.
What led to it: comparing our menus with UIKit's on the iOS 26.2 simulator (
spec:AST-094FR29); FR43 applies it on every device.Problem and solution fit
For a finger or pen, on every device, the opening press counts distance, not time. Until the finger is 10 px, in a straight line, from the first position the open menu sees, nothing lights up and lifting picks nothing, so the menu stays open. Then it slides and picks as usual. A mouse is unchanged. The trigger still opens after the 500 ms hold; #8078 adds opening on press.
Current specification
spec:AST-094FR29, FR43 and FR25 (current);module:DropdownMenu/useMenuPressFR9 (current).Public delta and caller burden
Under a finger, on every device, the opening finger moves 10 px before it picks;
presentation="popover"keeps today's behavior.useMenuPress: no new option or type.Design and alternatives
Chosen: 10 px in a straight line from that first position, which matches every capture.
Evidence
iOS 26.2 simulator, after a 1 pt first step: (0, 9) and (7, 7) picked nothing; (0, 10), (10, 0), (9, 9) and (10, 10) picked. Real Chromium and WebKit: a 9 px move onto "Edit", then a lift, picks nothing; without this change it picks "Edit".
Scope
Testing
On the DropdownMenu "Default" story, with touch emulation:
Automated: the handoff unit tests (a fine-pointer case among them, which failed before the press model keyed on
presentation) andDropdownMenuTouchHandoff(Chromium, WebKit), which fails without this change.Stack: this PR → #8078. Besides the handoff, the one other change is the
@astryxdesign/interaction-harnessdev dependency its browser spec imports.