Repository navigation
feat(useMenuPress): a pointer that went down elsewhere is the menu's opening press (module FR11) - #8507
feat(useMenuPress): a pointer that went down elsewhere is the menu's opening press (module FR11)#8507vjeux wants to merge 4 commits 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
meta-cla
Bot
added
the
CLA Signed
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: continueGesture follows module:DropdownMenu/useMenuPress FR11 for a coarse finger, but this pull request's diff against main still carries #8078 and its dependencies (#8076, #8298, #8446) and currently conflicts with main, so merging it would land all of them at once. Please land the stack in its stated order and rebase so the diff is the continuation commit alone, and add the real-browser handoff case that FR11's verification map asks for beside the jsdom suite. Also, useMenuSurfacePress, the entry ContextMenu mounts, returns continueGesture but cannot select the 'uikit' touch model, so a context menu that hands over its long-press finger on a touch screen would get the settle rule instead of the handoff.
[Automated review]
| }, | ||
| isTriggerClickFromPress, | ||
| cancel, | ||
| continueGesture, |
There was a problem hiding this comment.
useMenuSurfacePress returns this too (its return is DropdownMenuPressReturn without triggerProps), but its options have no touchModel, so positions never reach the machine through it (:849-852, :870-873). A ContextMenu that hands over its long-press finger on a touch screen would get the settle rule (a move lights a row at once, and a lift after 300 ms picks) instead of FR11's handoff. Consider letting that entry take or resolve the touch model, with a continuation case through it, or keeping continueGesture off it until ContextMenu adopts it.
| expect(trigger).toHaveAttribute('aria-expanded', 'true'); | ||
| }); | ||
|
|
||
| describe('a pointer that went down elsewhere is the opening press (continueGesture, module FR11)', () => { |
There was a problem hiding this comment.
The record's verification map for FR11 asks for this suite plus a real-browser handoff case, and no browser spec drives continueGesture yet. A Chromium touch case on the interaction harness, with a fixture that hands a held finger to an open menu, would cover the handoff from the first sample, a pick after it, and an early lift leaving the menu open.
|
At c6a1803:
|
There was a problem hiding this comment.
Changes requested: the continuation itself (continueGesture, the surface model's touch model and their tests) matches the menu press record's FR11, but this branch still carries #8078 and, through it, #8076, #8298 and #8446, so 23,151 of its 23,694 changed lines belong to pull requests that have not landed. Please land those first (#8076, #8298 and #8446, then #8078), update this branch so its diff is the six continuation files, and add an @astryxdesign/core Changeset for the change to useMenuPress.ts, which the Changeset coverage check will then ask for. The real-browser handoff case and the surface touch model requested earlier are both in place.
[Automated review]
…b-menu pull request; two stale comments go From the review: the two FR10 cases pass without this change, so they leave for meta#8220, beside its real-browser FR10 case. handoffPosition's comment no longer cites FR33, the continued pointer's, which meta#8507 carries; the touch-press spec loses a doc comment whose function is gone.
|
At 2b4bfff: an |
There was a problem hiding this comment.
Changes requested: the continuation itself (continueGesture, the trigger-less entry's touch model, their tests and the Changeset) matches the menu press record's FR11, but this branch still carries #8078 and, through it, #8076, #8298 and #8446, so all but 536 of its 23,037 changed lines belong to pull requests that have not landed, and the copy of #8446 here is already behind that pull request's own head. Please land those first (#8076, #8298 and #8446, then #8078) and update this branch so its diff is the seven continuation files; nothing else found here blocks it. The @astryxdesign/core Changeset, the real-browser handoff case and the surface touch model requested earlier are all in place.
[Automated review]
There was a problem hiding this comment.
Changes requested: the continuation itself (continueGesture, the trigger-less entry's touch model, their tests and the Changeset) matches the menu press record's FR11, but this branch still carries #8078 and, through it, #8076, #8298 and #8446, so all but 557 of its 22,577 changed lines belong to pull requests that have not landed, and the copy of #8446 here is already behind that pull request's own head. Please land those first (#8076, #8298 and #8446, then #8078) and update this branch so its diff is the seven continuation files; nothing else found here blocks it.
[Automated review]
|
Nothing in the continuation needs changing, per this review. Its own change is the seven continuation files: vjeux/astryx@013f3ea...7332541. The rest lands with #8076, #8298, #8446 and #8078, in that order. The #8446 copy here is the one #8076 carries (2931530). It moves when #8076 folds #8446's current head, and this line folds #8076 at its next push. |
There was a problem hiding this comment.
Changes requested on scope: the continuation itself (continueGesture, its tests, fixture and story) matches module:DropdownMenu/useMenuPress FR11 and needs no changes. The diff, though, carries #8076, #8298, #8446, #8627 and #8078, which are open on their own, and its copy is already behind them: #8076 has moved on and the head conflicts with main in packages/core/src/motion/. Once those land, rebase so the diff holds only the seven continuation files.
[Automated review]
There was a problem hiding this comment.
Changes requested on scope: the continuation itself (continueGesture, its tests, fixture and story) matches module:DropdownMenu/useMenuPress FR11 and needs no changes. The diff still carries #8076, #8298, #8446, #8627 and #8078, which are open on their own, and its copy of #8076 is already one commit behind that pull request. Once those land, update this branch so its diff holds only the seven continuation files.
[Automated review]
There was a problem hiding this comment.
Changes requested on scope: the continuation itself (continueGesture, its tests, fixture and story) matches module:DropdownMenu/useMenuPress FR11 and needs no changes. The diff still carries #8076, #8298, #8446, #8627 and #8078, which are open on their own, and its copy of #8076 is one commit behind that pull request, without its usePressTracker fix. Once those land, update this branch so its diff holds only the seven continuation files.
[Automated review]
There was a problem hiding this comment.
Changes requested on scope: the continuation itself (continueGesture, its tests, fixture and story) matches module:DropdownMenu/useMenuPress FR11, but the diff still carries #8446, #8076, #8360, #8298, #8627 and #8078, which are open on their own. Once those land, update this branch so its diff holds only the seven continuation files; that also leaves the scripts/touch-support-bundle.test.mjs failure, which comes from #8078's trigger half importing gestures/nativeScroll, with #8078. In .changeset/menu-press-continuation.md, replace “where the primary pointer is coarse” with the touch-model wording, since FR11 and the code apply the handoff under every primary pointer.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: the menu press model takes a pointer that went down elsewhere, such as a context menu's long press, as the menu's opening press (`module:DropdownMenu/useMenuPress` FR11), and its trigger-less entry takes the touch model, so where the primary pointer is coarse a finger handed to the menu picks only past UIMenu's handoff distance. Not part of the public hooks API. |
There was a problem hiding this comment.
This says the handoff applies “where the primary pointer is coarse”, but at this head a handed finger follows the caller's touch model on every device, as module:DropdownMenu/useMenuPress FR11 requires (“Under every primary pointer, a finger picks only after the handoff distance”), and the changelog prints [chore] entries under “Other Changes”. Suggested wording: “…so under the 'uikit' touch model (a finger, on every device: spec:AST-094 FR43) a finger handed to the menu picks only past UIMenu's handoff distance.”
… 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.
e3c7fa5 to
50b8257
Compare
github-actions
Bot
removed
the
needs:design-review
There was a problem hiding this comment.
Changes requested, on scope. The continuation itself (continueGesture, the trigger-less entry's touch model, its tests, fixture and story) matches the menu press record's FR11 and needs no change; its seven jsdom cases pass and fail without it.
- The diff still carries #8627 and #8078, which are open on their own and make up 2,634 of its 3,170 authored lines, so once they land, update this branch to hold only the seven continuation files.
isInScrollViewinpackages/core/src/hooks/menuTriggerPress.tswalks the ancestors again aftercreateNativeScrollProbe(...).isInsideScrollerand repeats the probe's overflow and fixed-ancestor tests, so use the probe's answer and drop the walk (this belongs to #8078).normalizePointerTypeinmenuTriggerPress.tscopies the one inuseMenuPress.ts, so export one frommenuPressGesture.tsand import it in both (also #8078).
Two small comment suggestions are inline and do not block.
[Automated review]
| function isInScrollView(control: Element): boolean { | ||
| if (!createNativeScrollProbe(control).isInsideScroller) { | ||
| return false; | ||
| } | ||
| 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 already tests for a scroller with room that is not beyond a position: fixed ancestor (packages/core/src/gestures/nativeScroll.ts, isBeyondFixed and isInsideScroller). Once it returns true, this second walk returns true too in every case but a computed overflow: overlay, so it is a second copy of the scroll-view rule that can drift. Use the probe's answer (extend the probe if a case is missing) and drop the walk.
| function normalizePointerType(type: string): MenuPressPointerType | null { | ||
| return type === 'mouse' || type === 'touch' || type === 'pen' ? type : null; | ||
| } |
There was a problem hiding this comment.
This is the same function as normalizePointerType in useMenuPress.ts. Export one from menuPressGesture.ts, which owns MenuPressPointerType, and import it in both.
| // 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: this comment joins two topics in one sentence and wraps mid-line. Suggested: "Cancel the finger's compatibility mouse events. Their mousedown would take focus back from the menu."
| const {touchModel: _touchModel, ...publicOptions} = | ||
| options as DropdownMenuPressOptions; | ||
| const press = useDropdownMenuPress({...publicOptions, touchModel: 'pointer'}); | ||
| // The public return leaves out the continuation; it is internal. |
There was a problem hiding this comment.
Non-blocking: this restates the next two lines; it can go.
… 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.
50b8257 to
c32ba6e
Compare
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested, on scope. The continuation itself (continueGesture, the trigger-less entry's touch model, its tests, fixture and story) matches the menu press record's FR11; its seven unit cases pass here and fail without it. The earlier notes on menuTriggerPress.ts (the second scroll-view walk and the copied normalizePointerType) are fixed.
- The diff still carries #8627 and #8078, which are open on their own and make up all but the 554-line continuation commit of its 3,252 changed lines, so once they land, update this branch to hold only the seven continuation files.
docs/specs/components/DropdownMenu/DropdownMenu.spec.mdline 350 still says "the FR8 gap pending conformance" while the same change removes that gap from FR8, so replace it with what the renamed press-open case proves (this belongs to #8078).
Three small comment suggestions are inline and do not block.
[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 now names the press-open case, and this change removes FR8's implementation gap, but the last column still says "The held-touch case asserts the current 500 ms hold, the FR8 gap pending conformance." Suggested: "A finger press in no scroll view that does not open the menu fails."
|
|
||
| /** | ||
| * Where a pointer is, for a finger's handoff distance. Positions reach the | ||
| * machine only under `touchModel: 'uikit'` or for a continued finger. Without |
There was a problem hiding this comment.
Non-blocking: positions depend on touchModel === 'uikit' alone (see the handoffPosition calls in attach), so a finger handed over under 'pointer' gets none. Suggested: "Only the 'uikit' touch model reports positions. Without them, the settle rule decides."
| const {touchModel: _touchModel, ...publicOptions} = | ||
| options as DropdownMenuPressOptions; | ||
| const press = useDropdownMenuPress({...publicOptions, touchModel: 'pointer'}); | ||
| // The public return leaves out the continuation; it is internal. |
There was a problem hiding this comment.
Non-blocking: this restates the next four lines; it can go.
| /** How long the opening press must last before its release may act. */ | ||
| import {MENU_PRESS_HANDOFF_PX as UIKIT_MENU_PRESS_HANDOFF_PX} from './menuPressValues'; | ||
|
|
||
| /** How long a mouse's opening press must last before its release may act. */ |
There was a problem hiding this comment.
Non-blocking: the settle rule still governs a finger under presentation="popover" and every caller of the public menuPressStep, not only a mouse. Suggested: "How long an opening press with no handoff must last before its release may act."
c32ba6e to
4747a72
Compare
There was a problem hiding this comment.
Changes requested. The continuation's core (continueGesture entering the open phase, the trigger-less entry's touch model, and a jsdom suite that fails without it) follows the menu press record's FR11, and the earlier notes on the FR8 verification row and the comments are fixed.
- The diff still carries #8627 and #8078, which are open on their own and make up all but 556 of its 3,245 changed lines, so once they land, update this branch to hold only the continuation files.
continueGestureapplies the 10 px handoff only undertouchModel: 'uikit', so under the default'pointer'a handed finger picks a row after a 4 px move (the test atuseMenuPress.test.tsxline 587 pins this), while FR11 says a handed finger picks only after the handoff distance under every primary pointer; run the handoff for every handed finger or pen and keep the settle rule for a mouse.- The test fixture hands the pointer over from a
useEffectonisOpen(menuPressContinuation.fixture.tsxlines 46-52) so the press model sees the menu's document, which is work caused by the menu element attaching, so do it from a callback ref on the menu element instead.
One comment suggestion is inline and does not block.
[Automated review]
| // The menu opened under this pointer: the gesture is the opening press. | ||
| // With the 'uikit' touch model, a finger's handoff counts from the first | ||
| // position the menu sees after it opened, as the trigger's opening | ||
| // finger's does. With the 'pointer' model, and for a mouse, the settle | ||
| // rule decides its release. |
There was a problem hiding this comment.
Under 'pointer' (the default of both internal entries) a handed finger gets no positions, so it follows the settle rule and picks after any move into the menu; the test at useMenuPress.test.tsx:587 pins a 4 px pick. The menu press record's FR11 says "Under every primary pointer, a finger picks only after the handoff distance", and the presentation="popover" way back covers only the released defaults the touch record lists, which a continuation does not have. Suggested: report positions for every continued finger or pen whatever the touch model, keep the settle rule for a handed mouse, and turn the 'pointer' test into the handoff assertion.
| // Hands the held pointer to the menu once the menu is in the document. | ||
| useEffect(() => { | ||
| if (isOpen && handOffRef.current != null) { | ||
| continueGesture(handOffRef.current); | ||
| handOffRef.current = null; | ||
| } | ||
| }, [isOpen, continueGesture]); |
There was a problem hiding this comment.
This Effect waits for the menu so the press model attaches its listeners to the menu node's document: work caused by the menu element attaching. Hand the pointer over from a callback ref on the menu element instead (merge it with menuRef, ignore null), and drop the Effect. The fixture stands in for the context menu that will adopt continueGesture, so it is the pattern that gets copied.
| 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: this line repeats on every internal type (also lines 67, 109, 124 and 210-211), and the @internal tag already says it. Suggested: /** @internal */ on each, and one sentence on MenuPressState only: "The handoff fields stay internal."
…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.
…opening press The internal press model gains continueGesture: a pointer that went down elsewhere (a context menu's long press, a trigger the hook does not own) continues into the menu that just opened under it, tracked as the opening press. With the 'uikit' touch model, a finger on every device picks only after the handoff distance from the first position the menu sees; otherwise, and for a mouse, the opening press follows the settle rule. A menu with no trigger takes the touch model through useMenuSurfacePress, so the finger it is handed keeps the handoff. No component hands a pointer over yet. - useMenuPress: continueGesture on the internal return, and the touch model on useMenuSurfacePress. - Tests: the hook's continuation cases, and a real-browser spec with its fixture and story (MenuPressContinuation). - A [feature] changeset.
4747a72 to
71a6e06
Compare
There was a problem hiding this comment.
Changes requested, on scope. The continuation itself (continueGesture, its jsdom suite, fixture and story) follows the menu press record's FR11: a handed finger or pen now picks only after the 10 px handoff under either touch model, and the fixture hands the pointer over from a callback ref, so both earlier findings are fixed.
Blocking:
- The diff still carries #8627 and #8078 (3,286 authored lines, of which this pull request's own continuation commit is 585), so landing it also lands their DropdownMenu and MoreMenu touch default; once they land, update the branch so it holds only the continuation files.
Non-blocking: the continueGesture JSDoc (useMenuPress.ts:225-232) still says the handoff needs the 'uikit' touch model, and the touch model on useMenuSurfacePress no longer changes anything, since the trigger-less case passes without it.
[Automated review]
| * here as the opening press. With the `'uikit'` touch model, a finger on | ||
| * every device picks only after UIMenu's handoff distance from the first | ||
| * position the menu sees. That matches a trigger's opening finger, and its | ||
| * lift before that acts on nothing and leaves the menu open. Otherwise, and | ||
| * for a mouse, the opening press follows the settle rule. Ignored while | ||
| * another gesture is live or the hook is disabled. A menu with no trigger | ||
| * (a context menu) passes the touch model to `useMenuSurfacePress`, so the | ||
| * finger it hands over keeps the handoff. |
There was a problem hiding this comment.
Non-blocking: this still says the handoff needs the 'uikit' touch model and that otherwise a finger follows the settle rule, but every handed finger or pen now gets the handoff (isContinuedFingerRef, set in continueGesture). It also tells a context menu to pass the touch model, which no longer changes anything. Suggested: "A finger or pen picks only after UIMenu's handoff distance from the first position the menu sees, under either touch model. Its lift before that acts on nothing and leaves the menu open. A mouse follows the settle rule."
Re-applied onto current main, stacked on #8078. Comments drop record ids for the source-comment rule.
User need
A context menu opened by a long press leaves the finger down, and on iOS that same finger slides onto a row and picks it. Our press model can only follow a press that began on its own trigger, so a context menu cannot hand it the finger that opened it.
What led to it:
module:DropdownMenu/useMenuPressFR11 asks for this handover; a reviewer asked to split it out of #8078.Problem and solution fit
continueGesturehands the press model a pointer the menu just opened under, tracked from there as the opening press. A handed finger or pen then follows UIMenu's rule under either touch model: it picks only after moving 10 px from the first position the menu sees, so a still lift picks nothing and the menu stays open. A handed mouse keeps the usual release rule. It is ignored while another gesture is live. No component calls it yet.Current specification
module:DropdownMenu/useMenuPressFR11 (current);spec:AST-094FR29, FR33 and FR43 (current). No record changes.Public delta and caller burden
None.
continueGestureis internal; the publicuseMenuPressreturns no continuation, and a test pins that.Design and alternatives
Evidence
useMenuPress.tsrestored and pass here.Scope
Testing
On the
a11y/Menu press continuationstory, with touch emulation:Automated: the continuation unit tests and
MenuPressContinuation(Chromium, WebKit).Local gates at 71a6e06: the menu units pass (1,564 of 1,564) and the core type check; without the continuation 7 of the hook's cases fail, and with positions only under
'uikit'the'pointer'handoff case fails. The full gates are running.Stack: #8627 → #8078 → this PR. Review only this PR's change: vjeux/astryx@4ad7e96...71a6e06