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📚 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 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 |
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
meta#8076's new head carries main 70ea859 with meta#8073's writer of the press attribute on main's press controller, and its README names both writers. utils/pressFeedback.ts keeps that controller with this line's own changes on it: the pointer, drag, scroll and visibility listeners through the shared input stream (meta#8356) and touchcancel only while the primary pointer is coarse (meta#8270); the key path stays at the window's bubble phase, so a Space press the page cancels paints nothing. usePressTracker stays in its own module, one tracker for a component's life (meta#8315), so meta#8073's rebuilt-tracker hook and its test are not taken. The harness files are meta#8076's.
…s, and the action-group tracker writes its own The header and writePress called it the one writer of the press attribute, but main's action-group tracker writes its groups' presses itself. They now say what holds, as meta#8076's gestures README does.
…he press attribute The release note said every press painter writes data-astryx-press through one internal writer, but the action-group tracker (hooks/actionGroupTracker.ts) writes its groups' presses itself, as the gestures README and utils/pressFeedback.ts already say. The note now says paintPress writes every press it tracks and the action-group tracker writes its own.
…llowance under every primary pointer The useMenuPress record's FR9 puts the trigger's held finger under spec:AST-094 FR29 under every primary pointer (FR43), so a finger on a touchscreen laptop may drift 13 pt before the open, as on a phone. The trigger read (pointer: coarse) as its press started and took the long press's 10 px where the pointer was fine. Three cases fail until it does not: a 12 pt drift opens under a fine pointer too, a (12, 12) settle opens there, and nothing reads a pointer query, the trigger's press included.
…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.
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).
Depends on #8627, #8360, #8298, #8671. Review only this PR's change: vjeux/astryx@b7c67d6...dcda3d0
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: the same UIKit comparison as #8627, on the iOS 26.2 simulator (
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);component:DropdownMenuFR8 (proposed).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). Menus without a trigger, including feat(core): the nav, breadcrumb and tab menus track a held pointer across their items #8791's nav, breadcrumb and tab menus, use the internaluseMenuSurfacePressand load no trigger or gesture code. The public hook keeps its trigger becausemodule:DropdownMenu/useMenuPressFR9 andspec:AST-094FR25 have it open as FR29 says.Design and alternatives
Evidence
iOS 26.2 simulator, Safari, a scrolling page: press "Actions", slide to "Duplicate" (opens at 156 ms; main: 500 ms):
Android Chromium 158 emulator, a page that doesn't scroll: the press opens it.
Fine pointers (
spec:AST-017FR59): a pen now opens the menu on press on every device, as a finger does, for cross-device consistency (spec:AST-094FR43;module:DropdownMenu/useMenuPressFR1); a mouse is unchanged. Real Chromium, fine primary pointer, Default story, time from press to open:Scope
Testing
On the DropdownMenu "Default" story, with touch emulation:
Automated: unit tests for each case, on a fine primary pointer too (three failed before #8627 keyed the press model on
presentation), a pen case in jsdom and in real Chromium (both failed on the previous head), andDropdownMenuTouchPress(Chromium, WebKit).Stack (merge in this order): #8360 (which carries #8076, #8073 and #8446), #8298 and #8627 (any order) → #8671 → this PR. Main's touch-bundle check fails here until #8671 makes it per component: the trigger's scroll-view wait is gesture code.
#8360 also changes
useMenuPress.ts. This PR moves #8360's trigger changes intomenuTriggerPress.ts: the long-press detector, FR29's 13 px drift allowance passed as each press starts on every device, and the shared click guard's window. #8360's own trigger tests pass on the moved code.