Repository navigation
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
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: against main this pull request carries 23,228 authored changed lines, and all but its own 67-line useMenuPress commit come from the dependency stack it names (#8076, #8298, #8446, #8078), so it cannot be reviewed or merged as the one change it proposes. Please land those pull requests first, then merge main, so the diff here is only the useMenuPress change. That change itself follows spec:AST-094 FR25 and FR29: with longPressDelayMs unset, a finger opens the menu on press under a coarse primary pointer, and longPressDelayMs: 500 keeps the released hold.
[Automated 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. 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 502 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 |
There was a problem hiding this comment.
Changes requested: against main this pull request carries 22,561 authored changed lines, and all but its own 67-line useMenuPress commit come from the dependency stack it names (#8076, #8298, #8446, #8078), so it cannot be reviewed or merged as the one change it proposes. Please land those pull requests first and then merge main, so the diff here is only the useMenuPress change, which itself follows spec:AST-094 FR25 and FR29. In that commit, please also bring the hook's other descriptions up to date: the file header in useMenuPress.ts and the triggerProps, onTriggerPress and usage text in useMenuPress.doc.mjs still describe only a finger held for the delay, while with longPressDelayMs unset a finger on a touch screen now opens the menu as it presses.
[Automated review]
| * - A mouse press on the trigger opens the menu at once. A finger held for | ||
| * `longPressDelayMs` opens it and the opening press follows the settle | ||
| * rule (see menuPressGesture.ts). DropdownMenu's own entry | ||
| * (`useDropdownMenuPress`, touch model `'uikit'`, `spec:AST-094` FR29) | ||
| * instead opens it as a finger presses on a trigger in a fixed bar, and | ||
| * inside a scroll view once the 150 ms content-touch delay has passed with | ||
| * no scroll; the opening finger then travels UIMenu's handoff distance | ||
| * before it picks. |
There was a problem hiding this comment.
This header now misdescribes the public hook: with this commit, useMenuPress with longPressDelayMs unset takes the same 'uikit' path as useDropdownMenuPress where the primary pointer is coarse (lines 402-409), so a finger opens the menu as it presses and picks only after the handoff. Worth saying so here, and in the onTriggerPress JSDoc (line 201) and the triggerProps comment (line 274).
| description: | ||
| 'How long a finger must rest on the trigger before the menu opens under it.', | ||
| default: '500', | ||
| "How long a finger must rest on the trigger before the menu opens under it. Unset, a finger on a touch screen (a coarse primary pointer) opens the menu as it presses, or after 150 ms inside a scroll view, and a fine primary pointer waits 500 ms. Set, it is that wait everywhere. A tap released sooner opens the menu through the trigger's click.", |
There was a problem hiding this comment.
This entry now states the press-open default, but options.onTriggerPress (line 52), triggerProps (line 102), the usage text (line 119) and the dense entries (lines 157, 164, 179, 186) still describe only a finger held for the delay. One clause in each, such as "with longPressDelayMs unset, a finger on a touch screen opens the menu as it presses (after 150 ms in a scroll view)", keeps the consumer docs consistent.
There was a problem hiding this comment.
Changes requested: against main this pull request carries 22,094 authored changed lines, and all but its own 67-line useMenuPress change come from the dependency stack it names (#8076, #8298, #8446, #8078), so it cannot be reviewed or merged as the one change it proposes. Please land those pull requests first and then merge main, so the diff here is only the useMenuPress change, which itself follows spec:AST-094 FR25 and FR29. In that change, please also bring the hook's other descriptions up to date: the options.onTriggerPress, triggerProps and usage text in useMenuPress.doc.mjs, and the matching JSDoc and file header in useMenuPress.ts, still describe only a finger held for the delay, while with longPressDelayMs unset a finger on a touch screen now opens the menu as it presses.
[Automated review]
|
Done at a8ca433. This pull request's own change is vjeux/astryx@8014a27...a8ca433.
|
…an isEnabled sync) into the arena fixes This PR keeps usePressTracker in its own module, so meta#8309's deliver queue (a press tracker's callbacks wait for a microtask while a gesture hook's isEnabled sync runs) moves into usePressTracker.ts, and the sync depth both hooks read moves into useAttachment.ts (syncingGestures, isSyncingGestures). Gestures, utils and hooks 1432/1432; tsc clean.
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.
There was a problem hiding this comment.
Changes requested: against main this pull request carries 30,414 authored changed lines, and all but its own 154-line useMenuPress change come from the open dependency stack (#8446, #8076, #8360, #8298, #8627, #8078), so it cannot be reviewed or merged as the one change it proposes. Please land those pull requests first and then merge main, so the diff here is only the useMenuPress change, which itself follows spec:AST-094 FR25, FR29 and FR43. In that change, please drop "and a fine primary pointer waits 500 ms" from the longPressDelayMs description in useMenuPress.doc.mjs, since with no delay set nothing waits 500 ms on any device, and update the title and description, which still say a laptop's touchscreen keeps the old wait.
[Automated review]
| description: | ||
| 'How long a finger must rest on the trigger before the menu opens under it.', | ||
| default: '500', | ||
| "How long a finger must rest on the trigger before the menu opens under it. Unset, a finger on every device opens the menu as it presses, or after 150 ms inside a scroll view, and a fine primary pointer waits 500 ms. Set, it is that wait everywhere. A tap released sooner opens the menu through the trigger's click.", |
There was a problem hiding this comment.
With longPressDelayMs unset the hook always passes 'uikit' (useMenuPress.ts:360-361), so nothing waits 500 ms here: a finger or pen opens on the press, or after 150 ms in a scroll view (menuTriggerPress.ts:193-197, :225-241), and a mouse opens at once (menuPressGesture.ts:241-248). The clause "and a fine primary pointer waits 500 ms" contradicts the first half of this sentence, the JSDoc (useMenuPress.ts:190-194) and module:DropdownMenu/useMenuPress's long-press delay row; dropping it keeps the docs true.
… any device The longPressDelayMs description still said a fine primary pointer waits 500 ms when it is unset. Unset, the hook runs the 'uikit' model: a finger or pen opens on the press, or after 150 ms in a scroll view, and a mouse opens at once (spec:AST-094 FR25, FR29, FR43). The clause goes, as the JSDoc and the module record already say.
There was a problem hiding this comment.
Changes requested: against main this pull request still carries 30,414 authored changed lines, and all but its own 154-line useMenuPress change come from the open dependency stack (#8446, #8076, #8360, #8298, #8627, #8078), so it cannot be reviewed or merged as the one change it proposes. Please land those pull requests first and then merge main, so the diff here is only the useMenuPress change, which follows spec:AST-094 FR25, FR29 and FR43 and whose option documentation now matches the code. When you do, please also update the title and description, which still say the new default applies only where the primary pointer is coarse and leave #8360 out of the merge order.
[Automated review]
vjeux
changed the title
Depends on #8078. Review only this PR's change: vjeux/astryx@dcda3d0...c9e4fc2
User need
An app that builds its own menu on the public
useMenuPresshook still makes a finger wait 500 ms to open it, while DropdownMenu opens as the finger presses. Two menus in one app then feel different under the same finger.What led to it:
spec:AST-094FR25 says an unset delay follows the platform's rule (FR29), while a delay the caller sets is kept everywhere; a reviewer asked to split this out of #8078.Problem and solution fit
When
longPressDelayMsis not set, the hook now runs DropdownMenu's touch model for a finger on every device (spec:AST-094FR43): the menu opens as the finger presses (in a scroll view, after the 150 ms content-touch delay), and the opening finger picks only after moving 10 px. A set delay keeps its value, and a mouse is unchanged.Current specification
spec:AST-094FR25, FR29 and FR43 (current);module:DropdownMenu/useMenuPress(current).Public delta and caller burden
No new option or type. A caller that sets no
longPressDelayMsgets the new behavior on every device;longPressDelayMs: 500keeps the previous hold. The changeset names the 10 px handoff and that way back, and #8627's changeset no longer says the public hook is unchanged.Design and alternatives
Scope
Testing
pnpm vitest run packages/core/src/hooks/useMenuPress.test.tsx:touchModelan untyped caller passes still reaches nothing.The
longPressDelayMsdescription inuseMenuPress.doc.mjsno longer says a fine primary pointer waits 500 ms with no delay set.Also run: build, typecheck, lint, and the menu, hook and gesture suites. Main's
touch-support-bundletest fails here until #8671 makes it per component; #8671's check passes on this head.Stack (merge in this order): #8360, #8298 and #8627 (any order) → #8078 → this PR.