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 change is waiting on a maintainer spec decision before it can be approved.
touchOpen="press" is a well-contained opt-in: the default behavior is unchanged, the press reuses the existing settle and drag-to-pick mechanics, the tentative open correctly withdraws when the browser takes the press for a scroll, and the new cases pass alongside the surrounding press-model suites. No code changes are being requested here.
What is missing is governing spec authority for the addition itself. The claims it rests on — proposed FR9 as amended and new proposed DEC-8 in module:DropdownMenu/useMenuPress — depend on the still-unanswered OQ1 ruling on whether press-to-open is the menu trigger model. Approval needs a maintainer to provide or identify the governing spec admitting finger press-open on a trigger that opts in because it sits outside any scrolling region, including the tentative open that closes again if the browser takes the press for a scroll before the finger reaches the menu.
Once that ruling exists, this review found no other blocker: the scope is one cohesive capability, existing callers and other menu surfaces keep today's behavior, and bottom-sheet presentation is unaffected.
[Automated review]
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression46 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 26 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
62aa0b9 to
6cd4056
Compare
There was a problem hiding this comment.
This change is waiting on a maintainer spec decision before it can be approved.
touchOpen="press" is a well-contained opt-in: the default behavior is unchanged, the press reuses the existing settle and drag-to-pick mechanics, the tentative open correctly withdraws when the browser takes the press for a scroll, and the new cases pass alongside the surrounding press-model suites in this review. No code changes are being requested here.
What is missing is governing spec authority for the addition itself. The claims it rests on — proposed FR9 as amended and new proposed DEC-8 in module:DropdownMenu/useMenuPress — depend on the still-unanswered OQ1 ruling on whether press-to-open is the menu trigger model. Approval needs a maintainer to provide or identify the governing spec admitting finger press-open on a trigger that opts in because it sits outside any scrolling region, including the tentative open that closes again if the browser takes the press for a scroll before the finger reaches the menu.
Once that ruling exists, this review found no other blocker: the scope is one cohesive capability, existing callers and other menu surfaces keep today's behavior, and bottom-sheet presentation is unaffected. This review reran the jsdom suites; the real-Chromium touch results recorded on the PR were not rerun here.
[Automated review]
|
/approve-spec 6cd4056 |
github-actions
Bot
removed
the
needs:spec-owner-review
6cd4056 to
3b6c68a
Compare
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 3b6c68a |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
This change is waiting on a maintainer spec decision; no code changes are requested. touchOpen="press" is opt-in, keeps today's tap behavior by default, and its new cases pass along with the surrounding menu press-model suites. But no current spec admits it yet: proposed DEC-8 and the amended FR9 in module:DropdownMenu/useMenuPress rest on the still-open OQ1 (whether press-to-open is the menu trigger model), and the passing spec-owner check approves those record edits only as written, which still mark them "proposed; pending owner review". Approval needs a maintainer to rule on OQ1 and record DEC-8 and the FR9 press clause as governing spec (a finger's press-down open, and the tentative open that closes again when the browser takes the press for a scroll, plus the touchOpen prop on DropdownMenu), or to identify an existing current claim that already admits them.
[Automated review]
3b6c68a to
eea5ea8
Compare
|
/approve-spec eea5ea8 |
There was a problem hiding this comment.
This change is waiting on a maintainer spec decision; no code changes are requested. touchOpen="press" is opt-in, keeps today's tap behavior by default, and its new tests pass alongside the existing menu press-model suites. No current spec admits it yet: proposed DEC-8 and the amended FR9 press clause in module:DropdownMenu/useMenuPress rest on the still-open OQ1, the record text approved for this revision still marks them "proposed; pending owner review", and the DropdownMenu component record is a draft. Approval needs a maintainer to rule on OQ1 for a finger's press-down open on an opted-in trigger (including the tentative open that closes again when the browser takes the press for a scroll, and the touchOpen prop) and record that ruling in the module record, or to identify an existing current claim that already admits it.
[Automated review]
eea5ea8 to
4345f00
Compare
|
/approve-spec 4345f00 |
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
This is waiting on a maintainer spec decision; no code change is requested. touchOpen="press" is opt-in and keeps today's tap as the default, and in real Chromium a finger's press opens the menu, a press-slide-lift picks a row once, and a scroll that starts on a panning trigger closes the menu again. Approval needs the module owner to rule on OQ1 in useMenuPress.spec.md, admitting or rejecting DEC-8 and the FR9 press clause (including the withdrawn open and drag-to-pick's touch-action condition), to record that ruling as current, and to approve the resulting head, because this rebased head has no spec-owner approval.
[Automated review]
…ess-down A finger opened a DropdownMenu on the tap's click or after the long-press delay; nothing let a trigger outside a scroller open on the press, as a mouse press does. touchOpen="press" (a useMenuPress option of the same name) opens the menu on the finger's press-down; the settle rule and FR1-FR4 continue from the press, so a slide onto a row picks it. The open is tentative until the finger enters the menu: if the browser takes the press first (pointercancel: a scroll, a pinch), the menu closes again, and no touchmove preventer is installed, so a scroll that starts on the trigger still scrolls. Rebased onto main after meta#7530 as one commit; review rounds are folded in unchanged.
…m, never none Two findings from the stack that builds on touchOpen. The changeset, the touchOpen JSDoc on DropdownMenu and useMenuPress, and both docs offered touch-action none or pinch-zoom for a trigger whose press should slide onto a row. none also blocks a page pinch-zoom that starts on the trigger; pinch-zoom keeps a one-finger pan from starting there, which is all the slide needs, and is what the TouchOpenPress story declares. They now recommend pinch-zoom and say why none is wrong. The dense docs said useMenuPress never calls onDismiss for a finger, but with touchOpen 'press' it does when the browser takes the finger's press for a scroll or a pinch before it reaches the menu; that line now says so, and the dense block names touchOpen. A Chromium case on a phone pins the story trigger's touch-action at pinch-zoom and pinches from it: the page zooms (scale 5); with the story set to none it fails, and a pinch from that trigger leaves the scale at 1.
96e7493 to
166c0ce
Compare
There was a problem hiding this comment.
This is waiting on a spec decision; no code change is needed for it. touchOpen="press" keeps today's tap as the default, and on this head a finger's press opens the menu, a press-slide-lift picks a row once, and a pan that starts on a panning trigger closes the menu again, in jsdom and in real Chromium. No current spec admits it yet: DEC-8 and the FR9 press clause in useMenuPress.spec.md, and the FR8 sentence in DropdownMenu.spec.md, are proposed. So an engineering owner needs to admit or reject the press-open, including that a press the browser cancels still fires onClick and onOpenChange(true) before closing and that drag-to-pick needs touch-action: pinch-zoom on the trigger, record that decision in the module record, and approve the resulting head by commenting /approve-spec <full head SHA> (the author may do so as an engineering owner). Either way, the MenuPressEffect JSDoc in menuPressGesture.ts should also say that a cancelled tentative open sets dismiss.
[Automated review]
…ss-open menuPressStep is exported, and with touchOpen 'press' a cancel before the finger reaches the menu settles with dismiss true, but the settle effect's JSDoc still said dismiss means a mouse released outside the menu. It now names both causes. Comment only.
There was a problem hiding this comment.
This is waiting on a spec decision; no code change is needed for it. touchOpen="press" keeps today's tap as the default, and on this head a finger's press opens the menu, a press-slide-lift picks a row, and a pan that starts on a panning trigger closes the menu again, in jsdom and in real Chromium. No current spec admits the press-open yet: DEC-8 and the FR9 press clause in useMenuPress.spec.md, and the FR8 sentence in DropdownMenu.spec.md, are still proposed, so an engineering owner needs to admit or reject it (or identify a current claim that already covers it), including that a press the browser cancels still fires onClick and onOpenChange(true) and moves focus to the trigger, and that drag-to-pick needs touch-action: pinch-zoom on the trigger. If it is admitted, record that decision as current in the module record and approve the resulting head by commenting /approve-spec <full head SHA> (the author may do so as an engineering owner).
[Automated review]
|
Posted by Cindy's agent on her behalf. Requested change before approval: align with the touch gestures spec instead of adding a prop.
The menu PRs stacked on this one inherit the same change. |
…esses (spec:AST-094 FR29)
Under DropdownMenu's touch modes, a finger on a trigger in a fixed bar opens
the menu as it presses, and inside a scroll view once the 150 ms
content-touch delay has passed with no scroll (a scroll that starts first
takes the touch; 13 px of drift is allowed before the open). The finger that
opened the menu picks a row only after moving 10 px (UIMenu's handoff), so a
menu that appears under a resting finger never picks the row beneath it.
DropdownMenu resolves its presentation through usePresentation: the touch
modes are the default under a finger, and presentation="popover" pins the
pointer behavior, a tap's click open, the 500 ms hold and the settle rule
(spec:AST-094 FR25).
useMenuPress gains touchModel ('pointer' by default, 'uikit' for this
trigger); longPressDelayMs keeps its released meaning and 500 ms default
under the pointer model. The menu table registers through registerMenuTable(),
called by the menu's touch module; nothing registers on import.
Carries, from meta#7240 (by its owner): the settle-rule JSDoc and the dense-docs
onDismiss line. Its touchOpen prop and tentative open are not taken.
|
Agreed: closing in favor of #8078. A finger now opens a menu as it presses on a trigger in a fixed bar, and after the 150 ms content-touch delay inside a scroll view, as the touch default per |
…esses (spec:AST-094 FR29)
Under DropdownMenu's touch modes, a finger on a trigger in a fixed bar opens
the menu as it presses, and inside a scroll view once the 150 ms
content-touch delay has passed with no scroll (a scroll that starts first
takes the touch; 13 px of drift is allowed before the open). The finger that
opened the menu picks a row only after moving 10 px (UIMenu's handoff), so a
menu that appears under a resting finger never picks the row beneath it.
DropdownMenu resolves its presentation through usePresentation: the touch
modes are the default under a finger, and presentation="popover" pins the
pointer behavior, a tap's click open, the 500 ms hold and the settle rule
(spec:AST-094 FR25).
useMenuPress gains touchModel ('pointer' by default, 'uikit' for this
trigger); longPressDelayMs keeps its released meaning and 500 ms default
under the pointer model. The menu table registers through registerMenuTable(),
called by the menu's touch module; nothing registers on import.
Carries, from meta#7240 (by its owner): the settle-rule JSDoc and the dense-docs
onDismiss line. Its touchOpen prop and tentative open are not taken.
User need
A finger opens a
DropdownMenuon the click after it lifts, or after a 500 ms hold: right for a trigger on a scroller. A trigger outside any scroller (a control in a fixed bar) could open on the press like an iOS pull-down button; instead it lags, and a finger cannot press and drag onto a row. Only the caller knows where the trigger sits.Problem and solution fit
touchOpen?: 'tap' | 'press'onDropdownMenu, passed to a newuseMenuPressoption of the same name. With'press'a finger's press-down opens the menu as a mouse press does; the settle rule and FR1–FR4 continue (a slide onto a row picks it; a tap leaves the menu open).pointercancel: a scroll, a pinch) the menu closes and the scroll proceeds.menuPressStepgains optionaltouchOpenandisTentative, andcancel()no longer steps the machine. The press ispreventDefaulted (no compatibilitymousedownstealing focus); the existing invoker hold keeps the lift from light-dismissing the menu.Current specification
module:DropdownMenu/useMenuPressis current for FR1–FR7; its trigger half (FR8, FR9, DEC-4) is proposed under OQ1. This PR amends proposed FR9 and adds proposed DEC-8, a concept row, and a line incomponent:DropdownMenuFR8 (draft). The one owner decision: admittingtouchOpen(DEC-8), on the OQ1 ruling.Public delta and caller burden
DropdownMenuanduseMenuPress:touchOpen, default'tap'(today's behavior); bottom-sheet presentation ignores it.menuPressStep: two optional fields.touch-action: pinch-zoom(nonewould also block page zoom).Design and alternatives
Rejected (DEC-8): holding the page still from the press, which blocks scrolls; a movement threshold of the model's own, which closes the menu on the drag to its rows. The browser decides what a scroll is (FR4).
Evidence
main, 4 pin unchanged paths.main(no open within 250 ms).Writing rubric results
R1 narration: none. R2 research: none. R3 evidence: FR9 cites DEC-4, DEC-8, tests. R4 rejected alternatives: one line in DEC-8. R5 change references: none. R6 open questions: OQ1 unanswered; one sentence ties DEC-8 to it.
Scope
Testing
Review round (e48a755): the
settleeffect's JSDoc inmenuPressGesture.tssaiddismissmeans a mouse released outside the menu; it now also names the press-open the browser takes for a scroll or pinch, which settles withdismisstrue. Comment only; typecheck,eslint(strict), themenuPressGestureanduseMenuPresssuites (66 tests) pass.Review round (rebased onto
mainc743ae0, 166c0ce): the changeset, bothtouchOpenJSDocs and both docs offeredtouch-action: nonefor a press-open trigger;nonealso blocks a page pinch-zoom that starts on it, so they now saypinch-zoom, as the TouchOpenPress story declares. The dense docs saidonDismissis never called for a finger; withtouchOpen: 'press'it is, when the browser takes the press for a scroll or pinch, and the dense block now namestouchOpen. A new Chromium case on a phone pins the story trigger atpinch-zoomand pinches from it: the page zooms (scale 5); set tonone, the case fails and the scale stays 1. DropdownMenu, ContextMenu, hooks, Selector and CLI suites: 1482 tests; menu press and menu hover focus Chromium specs: 24; typecheck,eslint(strict),check:knowledgeand the Storybook build pass.Rebased onto
maincab6e90, no conflicts or changes: main's newpr-a11yTopNav step runs a spec file the previous head predates. The record is unchanged: DEC-8 and the FR9 press clause stay proposed, because OQ1 asks the module owner to rule on the trigger model they rest on. DropdownMenu, ContextMenu,menuPressGesture,useMenuPressand doc-prop suites: 451 tests; typecheck,check:knowledge: clean.Rebased onto
mainb7860bc after feat(DropdownMenuSubMenu): header and footer around a sub-menu's rows #7530 merged, as one commit; theuseMenuPressrecord conflict keeps both verification rows (this FR9 press row beside feat(DropdownMenuSubMenu): header and footer around a sub-menu's rows #7530's ORD1a). No other change. DropdownMenu, ContextMenu,useMenuPress,menuPressGestureand doc-prop suites: 419 + 65 tests;MenuPressChromium spec: 16; typecheck,lint:strict,check:knowledge, Storybook build: clean.Review round 3: the
touchOpenJSDoc inuseMenuPress.tsnow carries the sametouch-action: none/pinch-zoomcondition as the changeset and the option docs (comment only; pre-commit lint and repo checks pass).Not run on WebKit here (it cannot launch on this machine).