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 1755 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📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsContextMenu (@astryxdesign/core) · View in Storybook
MoreMenu (@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.
|
| Component | JavaScript | CSS |
|---|---|---|
| core/MoreMenu | +2.0KB |
0B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.6KB | 34.6KB | 0B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
55 of 502 shot(s) changed.
A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.
| component | story | theme | mode | pixels |
|---|---|---|---|---|
| Selector | Indicator Space Bottom Sheet Narrow End RTL | neutral | light | 753,933 |
| Selector | Indicator Space Bottom Sheet Wide Start | neutral | light | 740,072 |
| RichTextEditor | Markdown parity: side by side | probe | dark | 558,840 |
| RichTextEditor | Markdown parity: side by side | probe | light | 540,756 |
| Chat | Full AI Chat | probe | light | 230,823 |
| Chat | Full AI Chat | probe | dark | 229,805 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Collapsible | Dividers | probe | dark | 66,445 |
| Collapsible | Dividers | probe | light | 66,445 |
| AppShell | Playground | neutral | dark | 65,198 |
| AppShell | Playground | neutral | light | 63,788 |
| Chat | Default | neutral | dark | 36,304 |
| Chat | Default | neutral | light | 35,767 |
| Chat | Empty State | probe | light | 23,531 |
| Chat | Empty State | probe | dark | 22,622 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | dark | 15,378 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | light | 14,107 |
| Chat | With Attachments | probe | light | 13,406 |
| Chat | With Attachments | probe | dark | 11,232 |
and 35 more.
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report
![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. |
…e its own pull request's; this one keeps the open on press The handoff now lands first in its own pull request. Here the trigger's cases keep only what the open on press changes: the still-lift case and the public hook's settle case live in the handoff's describe blocks, the pointer model's case keeps its opening times, the pinned popover's keeps its 500 ms open, and the changeset drops the handoff sentence.
…lowance under every primary pointer The useMenuPress record's FR9 governs the trigger's held finger by spec:AST-094 FR29 under every primary pointer (FR43). The trigger now hands the long press FR29's 13 px drift allowance whatever the primary pointer, so it no longer reads (pointer: coarse) as its press starts, and the menus drop their coarsePointer import. ContextMenu keeps the long press's own allowable movement. The changeset says the same.
Strict ESLint reported six unused bindings in files this pull request carries: the host's HAPTIC_SWITCH_ATTRIBUTE import, two unused imports in the pan's tests (scrollProbe, ScreenEdgePanGestureRecognizer), the swipe test's drift, the presented-hit-testing test's gestures, and a GestureHost test's before, which now asserts what it was read for: a second finger's moves leave the pan where it was.
…eal angular speed The release velocity of a rotation whose listener sets rotation = 0 on each change, at 2 degrees every 16 ms, read 4.363 rad/s against the 2.182 rad/s of the same stream without a re-base: the test asserted only more than 1.5. It now asserts the 2.18 rad/s (spec:AST-100 FR2), and fails.
The rotation's velocity tracker samples the raw unwrapped angle, which setting rotation does not move, so the setter's shift of the samples read each re-base as extra turn and doubled the release velocity of a listener that re-bases on every change. The setter no longer touches the tracker.
…mates while it runs The proof read the element's animations 230 ms in, after the dip had ended, and asserted only that every animated key is opacity, which an empty list passes: with the cross-fade removed from the plan (the move still landing), the proof still passed. It now reads the keyframe keys right after the call starts and asserts they include opacity and nothing else; the same mutant fails it. Chromium and WebKit pass. (F1's in-flight read from c648034.)
…roller, and its claimed moves are cancelled under pointer events Two comparisons stop at the shadow boundary, each failing a case: - once the host's listener on a recognizer's inner node has seen the finger, isFingerDownIn reads it as outside a light-DOM scroller around that root, so a scroll under it is not the finger's (FR20); - under inputMode 'pointer-events', a claimed pan on a shadow host cancels none of its finger's moves over the inner content, since the finger's target is the inner node and its touches reach the host retargeted (FR19).
…the flat tree isFingerDownIn and the pointer-events finger match compare with containsAlongFlatTree, as the scroll watch already does: a finger on content inside an open shadow root is down in the scroller around that root, and its touches, retargeted to the shadow host, match its pointer finger, so a claimed pan cancels its moves there (spec:AST-094 FR19, FR20).
…achments asks, and only then The host let the last attachment decide whether an element is an action group (useGestureRecognizer always passes the option), and never recomputed it at a detach: a plain attachment after the group's turned the group off, and a group's attachment released left a recognizer that never asked taking presses under a fine pointer. Fails until the host counts the attachments that ask.
…ents asks The host counts the attachments that pass actionGroup: true and keeps the element a group while one remains, recomputed at each detach. An attachment that passes false, as useGestureRecognizer does by default, no longer turns a group off, and a group's last asking attachment taking its leave ends the group.
… that content The presented hit test takes the document's elementFromPoint, which answers a shadow host for anything inside its open root, and compares with Node.contains, which stops at the boundary: a recognizer on a control inside a moving sheet's shadow root gets no finger, and the tap's click goes to the host. Fails until the hit test descends into open roots and compares along the flat tree.
Where a moving element is drawn away from its layout box, the host hit-tests what is drawn under the finger with the document's elementFromPoint, which answers a shadow host for anything inside its open root. The hit test now descends through each open root's own elementFromPoint and compares with the movers along the flat tree, so a recognizer on a control inside a moving sheet's shadow root gets its finger and the tap clicks that control.
…nt lets go The host's touch-action rules sit in an adopted sheet or a style element per document or shadow root, and were never removed, so a page whose last recognizer detached kept them, against spec:AST-094 FR24 (nothing touch-related remains after the last instance unmounts). Fails until a root's rules go with its last marked element.
…th its last marked element The touch-action writer records which elements it marked in each document or shadow root, and when the last one is unmarked (its last recognizer detached) it removes that root's adopted sheet or style element, so nothing touch-related remains after the last instance unmounts (spec:AST-094 FR24). A later mark writes the rules again; an element marked in another root first lets go of the old one.
… the inline style it reads - The motion registry, the host's engine gate and the README name the presented hit test as an engine workaround (spec:AST-013 FR6) with its review condition (re-measure on each iOS release; remove it once iOS WebKit hit-tests a compositor-moved element where it is drawn) and its regression fixture. - While a curve runs the animator holds the value it starts from inline, so presentation.ts and the README no longer say the inline style holds the end.
…core/internal/touch Core ships the internal entry @astryxdesign/core/internal/touch, which re-exports the gesture primitives for Astryx's own packages, and the Lab-import guard fails a Lab import of core outside core's exports (architecture:react-component-runtime INV13). The README and the barrel's header named internal workspace imports and said nothing here is exported; they now name that entry.
…ill needs the element, and the listeners that cancel The release note, the stream's header, the host's comment and the README said the stream is one passive listener per event type and that a document-capture stopPropagation no longer starves the recognizers. Such a stop never hid a listener on the same node, and some subscriptions cancel. They now say what changed: an app's window-capture stopPropagation, or a document-capture stopImmediatePropagation registered before core, no longer hides a touch; a recognizer that must claim still needs the touch to reach its element; the gesture host's touchend, click and contextmenu cancel while a claimer is attached and the page hold's touchmove while it holds; and the stream stays ahead of an app's window-capture listener only until a type's listener is re-added. The README's list of blocking touchmove listeners gains the claimed touch's target and the document while a claiming element moves.
…default it moves
The {stylexOptions} form of astryxStylex now passes StyleX
treeshakeCompensation: true unless the caller sets it. spec:AST-017 FR3
asks the release note to name the old and new default and the option
that restores the old one; it now does.
…er and for the two stops the input stream fixes - A still finger with sub-pixel jitter over a scroll view keeps its tap and scrolls nothing (spec:AST-094 FR18–FR22). - A tap is still recognized under an app's window-capture stopPropagation, and under a document-capture stopImmediatePropagation registered before core's: the recognizers hear touches on the window's capture phase. Chromium and WebKit pass.
…s once Where nothing that claims is attached, the host's touchend listener is passive and cannot cancel the engine's own click after a retargeted tap clicks what is drawn. The shared click guard swallows that late click whatever is attached; this case pins it, as asked on meta#8293.
This pull request sat on meta#8076 and carried its old copies of the harness meta#8045 and meta#8046 landed on main. meta#8360 carries meta#8076 with main's harness, so after the merge this pull request's own diff is the presentation resolver's commits. The one conflict, the gestures README, keeps both sides: this pull request's paragraph on the presentation prop ends the touch presentation section, and meta#8360's migration section follows it.
…esture host's subscriptions The gesture host subscribes no click: the one cancelling click subscriber is the click guard's, added at a window's first arm and kept. The release note, the stream's header and the gestures README listed click with the host's touchend and contextmenu; they now name the guard's own.
… 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.
…del on every device (spec:AST-094 FR43) Under FR43 the press model keys a finger on the touch model, which DropdownMenu takes from its presentation, not from the primary pointer. The continuation's doc comment, its code comment and two test names now say "touch model" where they said coarse or fine pointer. The interaction fixture runs the 'uikit' model, as DropdownMenu's default presentation does, instead of reading the coarse-pointer query. Behavior does not change: the spec drives a finger through a touch device, where the query read 'uikit' already.
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.”
Depends on #8078. Review only this PR's change: vjeux/astryx@dcda3d0...e3c7fa5
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. On a phone or tablet, the finger then follows UIMenu's rule: 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 mouse, or a finger on a laptop's touchscreen, 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 and FR33.Public delta and caller burden
None.
continueGestureis internal; the publicuseMenuPressreturns no continuation, and a test pins that.Design and alternatives
Evidence
Scope
Testing
On the
a11y/Menu press continuationstory, with touch emulation:Automated: the continuation unit tests and
MenuPressContinuation(Chromium, WebKit). Underspec:AST-094FR43 the comments, two test names and the story's fixture name the touch model where they named a coarse or fine pointer; behavior does not change. Main'stouch-support-bundletest fails here, and #8671 updates it.Stack (merge in this order): #8360, #8298 and #8627 (any order) → #8078 → this PR.