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.
|
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.
This needs an engineering owner's decision before approval, and it also needs changes. Two public additions have no governing spec yet: update(options) on the handle createMenuPress returns, and deprecating DropdownMenuTriggerProps.onPointerDown/onContextMenu in favour of a press delivered through ref. An engineering owner should identify or provide the spec for them and record the decision by commenting /approve-spec <full head SHA>; the author can do this as an engineering owner. Independently:
spec:AST-017FR29c asks maintained packages to migrate in the deprecation patch, but DropdownMenu and the five item-only menus (BreadcrumbItem, NavHeadingMenu, TopNavMenu, SideNavHeading, TopNavHeading) stay onuseMenuPress({menuRef, …}), and the ref form has no replacement for theirpressOnItemsOnlyfilter.- ContextMenu's
listRefalso sits on theitemsbottom-sheet container, so after a drill-in the whole sheet becomes a press root and the Back button loses its pressed paint under a coarse pointer. - At 3,145 authored lines, landing the long-press detector extraction separately would keep this PR to the INV22 move, and the StrictMode factory test should observe every
destroyit claims.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] `createMenuPress(menu, options)` runs the menu press model on any element without React: the row under a release acts, the highlight follows a held pointer, a mouse press on `trigger` opens the menu, and a finger held on it opens it after the long-press delay. `destroy()` removes every listener it added (`spec:AST-107` FR6). `useMenuPress(menuRef, options)` is its React form: `triggerRef` names the trigger, and there is nothing to spread. The options form, `useMenuPress({menuRef, …})` with its `menuProps` and `triggerProps`, still works and is deprecated: migrate to `useMenuPress(menuRef, {triggerRef, …})` and drop the spreads. It is removed at the next minor. Core's own users of the options form, which move at that minor: DropdownMenu, for its trigger's props, and the menus that track only presses on their items (BreadcrumbItem, NavHeadingMenu, TopNavMenu, and the SideNav and TopNav heading menus). |
There was a problem hiding this comment.
spec:AST-017 FR29c: "Maintained Astryx packages and authoring surfaces migrate in the deprecation patch." This Changeset defers DropdownMenu and the five item-only menus to the next minor, and the ref form has no replacement for their pressOnItemsOnly filter, so a consumer menu that filters the spread handler has no working migration either.
| // menu open. | ||
| const menuPress = useMenuPress({ | ||
| menuRef: listRef, | ||
| useMenuPress(listRef, { |
There was a problem hiding this comment.
listRef is also the ref on the items bottom-sheet container (line 815), which never spread menuProps before. After a drill-in re-render the factory marks that container with data-astryx-menu-press, so pressFeedback.ts:375 stops painting a mouse or Space press on the sheet's Back button under a coarse pointer. A ref that only renderedMenu carries would keep the base behavior.
| // Every factory the hook created is destroyed: StrictMode's replayed | ||
| // effect destroyed the first before creating the second. | ||
| expect(destroys.every(d => d.mock.calls.length <= 1)).toBe(true); | ||
| expect(handles.length - 1 + 1).toBe(handles.length); |
There was a problem hiding this comment.
This assertion is always true, and the destroy spies are attached after render, so the factory StrictMode's replay destroys is never observed. Spying each handle as it is created (wrapping the mocked createMenuPress) and asserting that created equals destroyed after unmount would prove the test's name.
| /** End the gesture in flight with nothing acting (the menu closed). */ | ||
| cancel: () => void; | ||
| /** Take new options: callbacks, the trigger, the row selector and the rest. */ | ||
| update: (options: CreateMenuPressOptions) => void; |
There was a problem hiding this comment.
architecture:public-component-api INV22 and spec:AST-107 FR6 name only destroy() on a factory's handle, and no current record decides an update(options) member. An engineering owner should confirm this public shape, or the hook could re-create the factory when its options change.
…, on the touch events' clock useLongPress timed a hold with a page timer set at touchstart and cleared by touchend. When the main thread was busy through the hold, the page ran the queued touchend before the overdue timer, and the long press was lost: a Table cell's ContextMenu opened nothing after an 800 ms hold. spec:AST-094 FR16 asks a gesture to keep the event clock: a blocked main thread delays the report and never the moment reported. - The hold is a deadline on the touch events' own timestamps (touchstart.timeStamp + delay). A move, a second finger, or a lift stamped at or past it reports the pending press first, at its start point, then acts. - The page timer waits only for what is left of the hold, so a touch the page handled late does not lengthen it. A timer that fires late yields once to queued input, so a lift stamped before the deadline still cancels the press: the gesture arena's rule (gestures/GestureArena.ts). - The clock is the touch's own window's, (e.view ?? window).performance, captured at touchstart: a same-origin iframe or a Picture-in-Picture window has its own time origin. Epoch stamps (jsdom) come onto it through its time origin, and the timer never waits past the delay. - onTouchEnd and onTouchCancel take the touch event, optionally; ContextMenu passes it. Tests: useLongPress's event-clock cases (six fail on main), among them a touch on a window clock whose origin is later than the page's, and a TableCellMenus browser case whose first long press is held through a 1,200 ms busy main thread (fails on main). A [fix] changeset.
jsdom stamps events with Date.now(). The hold converted such a stamp by subtracting the window's real time origin, so in a test that freezes the system time at an earlier date the touch landed months in the past, its timer fired at once, and a 100 ms tap opened ContextMenu. An epoch stamp's age is now read on Date.now(), the clock that made it, and placed on the window's clock: clock.now() - (Date.now() - timeStamp). Tests: an epoch stamp handled 300 ms late is due in 200 ms, and a tap under a frozen earlier system time opens nothing; both fail on the previous version.
f0907e7 to
492c328
Compare
…one by distance A stamp counted as a Date.now() stamp only above 1e12, so in a jsdom test that froze the date at 0 and left performance real, a small Date.now() stamp read as a window-clock time seconds in the past and a 100 ms tap opened ContextMenu. A stamp nearer Date.now() than the window's clock is now read as a Date.now() stamp, by its age. A stamp of 0 carries no time, so the hold falls back to the full delay on the page's timer. Tests: a tap under a date frozen at 0 with a real window clock, and a touch stamped 0; each fails on the previous version.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision, and the pull request also needs changes. No current spec decides two public additions: update(options) on the handle createMenuPress returns, and deprecating DropdownMenuTriggerProps.onPointerDown/onContextMenu so the press reaches a renderTrigger control through ref; an engineering owner (the author is one) should identify or provide the governing spec and record the decision by commenting /approve-spec <full head SHA>. Independently:
spec:AST-017FR29c asks maintained packages to migrate in the deprecation patch, but DropdownMenu and the five item-only menus stay onuseMenuPress({menuRef, …}), and the ref form has no replacement for theirpressOnItemsOnlyfilter.- ContextMenu's
listRefalso sits on theitemsbottom-sheet container, so after a drill-in the whole sheet becomes a press root and Back loses its pressed paint under a coarse pointer. - At 3,050 authored lines, landing the long-press detector extraction separately would keep this to the INV22 move, and the StrictMode factory test should observe every
destroyit claims.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [feat] `createMenuPress(menu, options)` runs the menu press model on any element without React: the row under a release acts, the highlight follows a held pointer, a mouse press on `trigger` opens the menu, and a finger held on it opens it after the long-press delay. `destroy()` removes every listener it added (`spec:AST-107` FR6). `useMenuPress(menuRef, options)` is its React form: `triggerRef` names the trigger, and there is nothing to spread. The options form, `useMenuPress({menuRef, …})` with its `menuProps` and `triggerProps`, still works and is deprecated: migrate to `useMenuPress(menuRef, {triggerRef, …})` and drop the spreads. It is removed at the next minor. Core's own users of the options form, which move at that minor: DropdownMenu, for its trigger's props, and the menus that track only presses on their items (BreadcrumbItem, NavHeadingMenu, TopNavMenu, and the SideNav and TopNav heading menus). |
There was a problem hiding this comment.
spec:AST-017 FR29c: "Maintained Astryx packages and authoring surfaces migrate in the deprecation patch." This defers DropdownMenu and the five item-only menus to the next minor, and the ref form has no replacement for their pressOnItemsOnly filter, so a consumer menu that filters the same way has no working migration either.
| // menu open. | ||
| const menuPress = useMenuPress({ | ||
| menuRef: listRef, | ||
| useMenuPress(listRef, { |
There was a problem hiding this comment.
listRef is also the ref on the items bottom-sheet container (line 815), which never carried the menu marker before. After a drill-in the factory marks that container with data-astryx-menu-press, so pressFeedback.ts:335 stops painting a mouse or Space press on the sheet's Back button under a coarse pointer. A ref that only renderedMenu carries would keep the base behavior.
| // Every factory the hook created is destroyed: StrictMode's replayed | ||
| // effect destroyed the first before creating the second. | ||
| expect(destroys.every(d => d.mock.calls.length <= 1)).toBe(true); | ||
| expect(handles.length - 1 + 1).toBe(handles.length); |
There was a problem hiding this comment.
This assertion is always true, and the destroy spies are attached after render, so the factory StrictMode's replay destroys is never observed. Spying each handle as it is created (wrapping the mocked createMenuPress) and asserting created equals destroyed after unmount would prove the test's name.
| const options: UseMenuPressRefOptions = isRefForm | ||
| ? second | ||
| : (first as UseMenuPressOptions); | ||
| // The latest options, read by the press at event time, as they always were. |
There was a problem hiding this comment.
Non-blocking: "as they always were" narrates history. Suggest: // The press reads the latest options at event time.
…ith no React The long-press detection moves out of useLongPressDetector's refs and callbacks into an internal factory, longPressDetector: a hand start from a point, the touch handlers on the events' clock, the movement tolerance, and the options it reads at each start, move and fire. useLongPressDetector creates one detector for the component's life, hands it the committed render's options after each commit, cancels it on unmount, and returns its methods as the handlers. A press model that runs without React can then share the one detector. No behavior change: the existing useLongPress cases pass unchanged. A new case keeps a suspended transition's options away from the press. The detector's own cases run with no React. A [chore] changeset. The core API report also gains the row for the optional event that useLongPress's onTouchEnd and onTouchCancel take, which the change below this one adds.
492c328 to
4935463
Compare
|
At 4935463, rebuilt on #9642 (the long-press detector, split out as suggested):
|
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Approval waits on an engineering owner's decision, and two lines need fixing either way.
No current spec settles two record changes this pull request proposes: module:DropdownMenu/useMenuPress DEC-9 with OQ2, which keeps useMenuPress({menuRef, …}) undeprecated until its callers can migrate, although architecture:public-component-api INV22 says the old signature is deprecated; and component:DropdownMenu DEC-11, which keeps DropdownMenuTriggerProps.onPointerDown and onContextMenu until a ref path exists. An engineering owner records the decision by commenting /approve-spec <full head SHA>; the author may do so as an engineering owner.
Independently:
useMenuPress.doc.mjsstill tells builders thatcreateMenuPressreturnsupdate(lines 104 and 175), which the handle no longer has, and it drops the options form'smenuPropsandtriggerPropsreturn rows while that form stays supported.- DEC-11 names
cixzhangas its decider on 2026-10-31, but no record or comment shows that ruling, so it should read as proposed and pending owner review, as DEC-9 does.
Otherwise the factory, the ref form and the five migrated menus behave as on main.
[Automated review]
| type: '() => void', | ||
| description: | ||
| 'End the gesture in flight with nothing acting, for when the menu closes under it.', | ||
| 'End the gesture in flight with nothing acting, for when the menu closes under it. Nothing is spread: the press listens on the menu and its trigger itself and marks the menu root with data-astryx-menu-press. Without React, createMenuPress(menu, options) takes the same options with the trigger as an element (trigger) and returns {isTriggerClickFromPress, cancel, update, destroy}; destroy removes every listener it added.', |
There was a problem hiding this comment.
The handle has no update any more (createMenuPress.ts:145-159). This should read {isTriggerClickFromPress, cancel, destroy}; a builder following it gets a type error, or a TypeError in plain JavaScript.
| 'true = the trigger click belongs to the press that already opened / closed; ignore it.', | ||
| cancel: 'end the gesture w/o acting.', | ||
| cancel: | ||
| 'end the gesture w/o acting. Nothing to spread. No React: createMenuPress(menu, {trigger, …}) → {isTriggerClickFromPress, cancel, update, destroy}.', |
There was a problem hiding this comment.
Same here: createMenuPress(menu, {trigger, …}) → {isTriggerClickFromPress, cancel, destroy}.
|
|
||
| **Reference:** `component:DropdownMenu/DEC-11` | ||
|
|
||
| **Decider:** `cixzhang`, 2026-10-31 |
There was a problem hiding this comment.
No record or comment shows a 2026-10-31 ruling on this, and the description says DEC-11 waits on /approve-spec. Suggest ### DEC-11 (proposed) and **Decider:** proposed by vjeux, 2026-11-02; pending owner review, as DEC-9 is written, until an owner rules.
| // The ref form: createMenuPress on the element behind the ref. The factory | ||
| // keeps the options it is created with, so its callbacks call the latest | ||
| // ones, and a new factory replaces it when its menu, the menu's document, the | ||
| // trigger, a value option, or the set of given callbacks changes. |
There was a problem hiding this comment.
Non-blocking (INV29): one 40-word sentence on several topics. Suggest:
// The factory keeps its creation options. Its callbacks call the latest ones.
// A new menu, document, trigger, value option or callback set creates a new factory.| // the only touch way to open a surface at the press point. | ||
| // | ||
| // A caller can spread the touch handlers, or call `start`, `moveTo` and | ||
| // `cancel` from its own pointer events, as `useMenuPress` does. Both ways use |
There was a problem hiding this comment.
Non-blocking: useMenuPress no longer drives these handlers; the press uses longPressDetector directly, and useLongPressDetector now has no caller but useLongPress. Suggest updating this comment and the @position line above, and folding useLongPressDetector into useLongPress.
4935463 to
499c662
Compare
|
At 499c662:
The owner decision on DEC-9 with OQ2 and on DEC-11 waits on the spec owners. |
There was a problem hiding this comment.
Approval waits on an engineering owner's decision, and one fix is needed either way.
No current spec settles two record changes this pull request proposes: module:DropdownMenu/useMenuPress DEC-9 with OQ2, which keeps useMenuPress({menuRef, …}) undeprecated until its callers can migrate although architecture:public-component-api INV22 says the old signature is deprecated, and component:DropdownMenu DEC-11, which keeps DropdownMenuTriggerProps.onPointerDown and onContextMenu until a ref path exists. An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner.
Required either way:
- The ref form creates no press while its menu element is unmounted (
useMenuPress.ts:303-305), so a menu rendered only while open never opens on a trigger press-down, yetuseMenuPress.doc.mjsrecommends that form for any new menu, itstriggerRefrow and the changeset state no such condition, and line 27 says the hook hands the press the latest options although the factory keeps its creation options.
Otherwise the factory, the ref form and the five migrated menus behave as on main, and the earlier notes on update in the hook docs and on DEC-11's decider are resolved.
[Automated review]
| delayMs: options.longPressDelayMs ?? MENU_PRESS_LONG_PRESS_MS, | ||
| attached?.press.destroy(); | ||
| attachedRef.current = | ||
| menu == null |
There was a problem hiding this comment.
While menuRef.current is null this creates no press, so the element behind triggerRef gets no listener either: a menu rendered only while open never opens on a trigger press-down or a held finger, and isTriggerClickFromPress() stays false, so only the click toggles it. With the menu rendered only while open, a mouse pointerdown on the trigger calls onTriggerPress in the options form and never in this form. Please state this in the hook docs and the changeset (or attach the trigger without a menu).
| { | ||
| guidance: true, | ||
| description: | ||
| 'Prefer the ref form, useMenuPress(menuRef, {triggerRef, …}), for a new menu: it listens on the menu and the trigger itself, so nothing is spread. The options form, useMenuPress({menuRef, …}), returns menuProps and triggerProps to spread instead.', |
There was a problem hiding this comment.
This recommends the ref form for any new menu, but its press, trigger half included, exists only while the menu element is mounted (useMenuPress.ts:303-305). Suggest adding: "Its press, the trigger's included, exists only while the menu element is mounted; for a menu rendered only while open, use the options form." The dense guidance at line 224 needs the same note.
| name: 'menuRef', | ||
| type: 'RefObject<HTMLElement | null>', | ||
| description: | ||
| 'The menu or listbox root — the surface whose rows a press picks from. The hook creates one press for the element behind the ref, hands it the latest options at each commit, creates a new one when the ref moves to another node, and destroys it at unmount.', |
There was a problem hiding this comment.
The factory keeps its creation options; the hook calls the latest callbacks and creates a new press when a value option changes, which ends a press in flight. Suggest: "The hook creates a press for the element behind the ref after each commit, calls the latest callbacks, creates a new press when the menu, its document, the trigger, a value option or the set of callbacks changes, and destroys it at unmount."
| getOptions: () => options, | ||
| }); | ||
| const disconnect = core.connect(); | ||
| // The highlight and the rows' hover focus follow the last input of the |
There was a problem hiding this comment.
Non-blocking (comment style): one 40-word sentence. Suggest:
// The highlight follows the last input in the menu's document. Track the
// page's document, and the menu's own document when it differs, until destroy.| // `cancel` from its own pointer events, as `useMenuPress` does. Both ways use | ||
| // this hook's hold time, movement tolerance and timer, so a long press means | ||
| // the same thing on a context menu and on a menu trigger. | ||
| // `cancel` from its own pointer events. The menu press runs longPressDetector |
There was a problem hiding this comment.
Non-blocking (comment style): one 32-word sentence. Suggest:
// `cancel` from its own pointer events. The menu press runs longPressDetector
// directly. The hold time, tolerance and timer are the same, so a long press
// means the same on a context menu and on a menu trigger.createMenuPress.test.ts runs the factory with no React: the press model on plain elements, every listener gone after destroy (mid-press included), a trigger moved by destroy and create, and a menu in another document. useMenuPress.factory.test.tsx checks the ref form only delegates: one factory per element, the latest callbacks without a new factory, a new factory when a value option changes, the committed callbacks while a transition suspends, and every factory StrictMode creates destroyed once. They fail here: the factory and the ref form come in the next commit.
createMenuPress(menu, options) runs the press model on any element: the row under a release acts, the highlight follows a held pointer, a mouse press on the trigger opens the menu, and a finger held on it opens it after the long-press delay, on the shared long-press detector. It keeps the options it is created with, and destroy() removes every listener it added. useMenuPress(menuRef, options) is its React form, with nothing to spread. It calls the latest callbacks, reads only committed options, and creates a new factory when the menu, its document, the trigger, a value option or the set of given callbacks changes. ContextMenu, DropdownMenuSubMenu, MenuBottomSheetActionList, Selector and TabMenu use it. ContextMenu's press rides its menu element, which carries the press marker in its markup as before, never the items sheet's container, so the sheet's Back keeps its press paint. The options form is unchanged, and its doc keeps its returns. useLongPressDetector folds into useLongPress, since the menu press runs the detector directly. The core API report gains the factory, its types and the ref form. The DropdownMenu record proposes that the trigger props' press handlers leave only with a ref path, and the useMenuPress record that the options form is deprecated together with its callers' migration, both pending owner review.
499c662 to
fc210bc
Compare
|
At fc210bc:
|
There was a problem hiding this comment.
Approval waits on an engineering owner's decision on two proposed spec records; nothing else in the code needs to change.
module:DropdownMenu/useMenuPressDEC-9 (with OQ2) keepsuseMenuPress({menuRef, …})undeprecated beside the newuseMenuPress(menuRef, options)until the ref form can start a press only on an item.architecture:public-component-apiINV22 says the old signature is deprecated under INV9, and INV21 allows a second API for one job only while the first is being deprecated. An owner either approves that exception to INV22 or asks for the deprecation now, with every maintained caller migrated in this patch (spec:AST-017FR29c).component:DropdownMenuDEC-11 is a new decision no current record states: the trigger'sonPointerDown/onContextMenuare deprecated only together with a ref path and every caller's migration.
An engineering owner records the decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner. Earlier feedback is addressed: the ref form's docs and changeset now say its press, trigger included, needs a mounted menu. The factory, the ref form and the event-clock change otherwise match INV22, spec:AST-107 FR6 and spec:AST-094 FR16, and a busy-page long press now opens the menu in Chromium where the base drops it.
[Automated review]
Stacked on #9642 and #9455; this change is the last two commits (+2,257 −808, 976 of them tests; the factory mostly moves out of
useMenuPress).User need
A menu built without React can't use the menu press model.
Problem and solution fit
INV22 asks every public interaction hook to delegate to a public
create…(element, options)factory withdestroy();useMenuPressgains a ref-taking form that does. INV22 also deprecates the old signature (INV9), butspec:AST-017FR29c migrates every maintained caller in that patch, and the item-only menus first need item-only starts in the ref form.Current specification
architecture:public-component-apiINV22 decides the factory and the ref-taking signature (the new API report rows;useMenuPresswas already public, and its row gains an overload);spec:AST-107FR6./approve-spec: proposedmodule:DropdownMenu/useMenuPressDEC-9 (with OQ2) and proposedcomponent:DropdownMenuDEC-11: both deprecate only with their callers' migration.Public delta and caller burden
createMenuPress(menu, {trigger?, itemSelector, …})returns{isTriggerClickFromPress, cancel, destroy}. It keeps its options; to change them, destroy it and create another.useMenuPress(menuRef, {triggerRef?, …})returns{isTriggerClickFromPress, cancel}. It calls the latest callbacks and re-creates the factory when the menu, its document, the trigger, a value option or the given callbacks change.useMenuPress({menuRef, …})is unchanged. No caller has to move.Design and alternatives
useLongPressDetectorfolds intouseLongPress.data-astryx-menu-press-surfaceentry moves with the code, count unchanged.update(options)on the handle (no record decides it).Evidence
createMenuPress.test.tsruns the factory with no React;useMenuPress.factory.test.tsxchecks the ref form only delegates. Both fail at their first commit.Writing rubric results
R1 narration: none (present-tense rules). R2 research: none. R3 evidence: none changed. R4 rejected alternatives: none. R5 change references: none. R6 open questions: OQ2, unanswered, names its decision.
Scope
Testing
Local gates at fc210bc: all pass: static checks,
pnpm test:ui,pnpm test:node, 334 menu accessibility specs, and pinned and WebKit press specs.