Repository navigation
fix(DropdownMenu): on a phone or a tablet, a tap while the menu enters is ignored, as on iOS - #8458
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
needs:spec-owner-review
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The tap-during-growth fix itself follows spec:AST-094 FR35, but approval waits on an engineering owner settling spec direction this branch carries from its dependencies: where DropdownMenu and MoreMenu sit on a phone (over their trigger here, which spec:AST-094 OQ9 still leaves as an open owner decision, so it needs a governing spec), architecture:layer-runtime INV13 with its Exit copy section, and component:DropdownMenu FR21. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Independently of that decision:
- Rebase onto main and resolve the
useMenuPress.spec.mdconflict by keeping main's current FR10, FR11, AR5 and DEC-8. - Land the 13 dependency pull requests first, so this one carries only its own 338-line commit rather than 41,237 authored lines.
[Automated review]
| | FR7 | While a press is tracked in a menu that scrolls, a pointer resting within the edge zone of the menu's top or bottom MUST scroll the menu toward that edge and re-read the row under the pointer as the rows move. Where the primary pointer is coarse, a finger or pen follows `spec:AST-094` FR30 instead; where it is fine, a finger or pen keeps this rule (`spec:AST-094` FR19). | `useMenuPress.test.tsx` autoscroll case | verify | | ||
| | FR8 | On a mouse a DropdownMenu trigger MUST open its menu on press-down (button 0, no Control), and a drag from the trigger into the menu that releases over an enabled row MUST act on it. The opening gesture's release MUST act only if the pointer entered the menu or the press lasted the settle time; otherwise nothing acts and the menu stays open. Pressing the trigger of an open menu MUST close it and MUST NOT reopen it in the same gesture. | Proposed DEC-4; `DropdownMenu.test.tsx` press model suite, `menuPressGesture.test.ts` trigger states | proposed; pending owner review | | ||
| | FR9 | Where the primary pointer is coarse, opening a menu from its trigger under a finger and the opening finger's handoff are governed by `spec:AST-094` FR29. Where it is fine, a finger keeps the released behavior (`spec:AST-094` FR19): a tap on the trigger opens the menu through its click, and a press held on the trigger for the long-press delay opens the menu with the finger down, prevents the default of the finger's later `touchmove`s until the gesture ends, and continues under FR1–FR4. | `spec:AST-094` FR29 | as `spec:AST-094` states | | ||
| | FR11 | A pointer that went down elsewhere (a context menu's long press) and is handed to the menu as it opens MUST be the opening press: a finger picks only after UIMenu's handoff distance, counted from the first position the menu sees after it opened (as `spec:AST-094` FR29 counts it), whatever the touch model, and its lift before that acts on nothing and leaves the menu open; a mouse follows FR8's settle rule. | Proposed; `useMenuPress.test.tsx` continuation suite | proposed; pending owner review | |
There was a problem hiding this comment.
Main now carries a current FR11 under DEC-8 (#8451): “Where the primary pointer is coarse, a finger picks only after the handoff distance, counted … as spec:AST-094 FR33 counts it”. This proposed row drops that condition, counts the handoff as FR29 does, and makes this file conflict with main in three hunks. Rebasing onto main's FR10, FR11, AR5 and DEC-8 resolves it; continueGesture, which has no caller at this head, would then need to meet the current FR11.
There was a problem hiding this comment.
Resolved by the dependency merge in this push: the module record is main's again (#8089 now carries FR10, FR11, AR5 and DEC-8 as main has them), so this file matches main and no longer conflicts. continueGesture meets the current FR11: where the primary pointer is coarse a handed-off finger picks only after the handoff distance, counted from the first position the menu sees; main's cases for it pass at this head.
There was a problem hiding this comment.
The tap-during-growth fix itself follows spec:AST-094 FR35, but approval waits on an engineering owner settling spec direction this branch carries from its dependencies: where DropdownMenu and MoreMenu sit on a compact touch screen (over their trigger here, which spec:AST-094 OQ9 still leaves as an open owner decision, so it needs a governing spec), architecture:layer-runtime INV13 with its Exit copy section, and component:DropdownMenu FR21. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner. Independently of that decision:
- Land the eight dependency pull requests first and rebase, so this one carries only its own 338-line commit rather than 34,904 authored lines.
- Add a pinch during the menu's growth to the FR35 browser cases, which FR35's verification row lists.
[Automated review]
| await expect(story.trigger).toHaveAttribute('aria-expanded', 'false'); | ||
| }); | ||
|
|
||
| test('a pan that starts while the menu grows starts nothing: no scroll takes it, and the menu stays (spec:AST-094 FR35)', async ({ |
There was a problem hiding this comment.
spec:AST-094's verification row for FR34 and FR35 also lists a pinch during the menu's growth (a blocked pinch fails). The only pinch at this head is the generic hold's in presentationHold.interaction.browser.spec.ts, over a held popover and dialog; a pinch case beside this one would cover the claim on the menu itself.
There was a problem hiding this comment.
The tap-during-growth fix itself follows spec:AST-094 FR35, but approval waits on an engineering owner settling spec direction this branch carries from its dependencies: where DropdownMenu and MoreMenu sit on a compact touch screen (over their trigger here, which spec:AST-094 OQ9 still leaves as an open owner decision, so it needs a governing spec), architecture:layer-runtime INV13 with its Exit copy section, and component:DropdownMenu FR21. An engineering owner records that decision by commenting /approve-spec with the full head SHA on this pull request, and the author may do so as an engineering owner. Independently of that decision:
- Resolve the merge conflict with main in three
internal/interaction-harnessfiles (main changed them in #8505), so the pull request can merge and CI can run. - Land the eight dependency pull requests first and rebase, so this one carries only its own 338-line commit rather than 33,166 authored lines.
- Add a pinch during the menu's growth to the FR35 browser cases, which FR35's verification row lists.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: one presentation hold for every layer (`Layer/presentationHold`). While a layer presents, a touch, a press or a click anywhere reaches nothing (keys and assistive technology still act, and a pinch still zooms the page); while it dismisses, the same, and the layer's content is inert. The gesture host holds the layer's touches through its touch gates, and `isPresentationShield` lets a hit test see past a hold's shield. Not exported from the package entry point; no component uses it yet. |
There was a problem hiding this comment.
At this head DropdownMenu uses the hold (packages/core/src/DropdownMenu/menuSurfaceMotion.ts:289), and .changeset/dropdownmenu-presentation-hold.md announces that fix, so released together the changelog would say both. "no component uses it yet" holds only if #8342 releases first; otherwise drop the clause.
There was a problem hiding this comment.
The tap-during-growth fix itself follows spec:AST-094 FR35, but approval waits on an engineering owner settling spec direction this branch carries from its dependencies: where DropdownMenu and MoreMenu sit on a compact touch screen (over their trigger here, which spec:AST-094 OQ9 still leaves as an open owner decision, so it needs a governing spec), architecture:layer-runtime INV13 with its Exit copy section, and component:DropdownMenu FR21. An engineering owner records that decision by commenting /approve-spec with the full head SHA on this pull request, and the author may do so as an engineering owner. Independently of that decision:
- Land the dependency pull requests first and rebase, so this one carries only its own 340-line change rather than 32,310 authored lines.
- Add a pinch during the menu's growth to the FR35 browser cases, which FR35's verification row lists.
- Drop "no component uses it yet" from
.changeset/layer-input-hold.mdunless #8342 releases first, and update the layer-runtime record's Current gaps, which still say no exit copy exists.
[Automated review]
| await expect(story.trigger).toHaveAttribute('aria-expanded', 'false'); | ||
| }); | ||
|
|
||
| test('a pan that starts while the menu grows starts nothing: no scroll takes it, and the menu stays (spec:AST-094 FR35)', async ({ |
There was a problem hiding this comment.
spec:AST-094's verification row for FR34 and FR35 also lists a pinch during the menu's growth (a blocked pinch fails). The only pinch at this head is the generic hold's in inputHold.interaction.browser.spec.ts, over a held popover and dialog; a pinch case beside this one would cover the claim on the menu itself.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [chore] Internal: one input hold for every layer (`Layer/inputHold`). While a layer presents, a touch, a press or a click anywhere reaches nothing (keys and assistive technology still act, and a pinch still zooms the page); while it dismisses, the same, and the layer's content is inert. The gesture host holds the layer's touches through its touch gates, and `isInputShield` lets a hit test see past a hold's shield. Not exported from the package entry point; no component uses it yet. |
There was a problem hiding this comment.
At this head DropdownMenu uses the hold (packages/core/src/DropdownMenu/menuSurfaceMotion.ts:289), and .changeset/dropdownmenu-presentation-hold.md announces that fix, so released together the changelog would say both. "no component uses it yet" holds only if #8342 releases first; otherwise drop the clause.
There was a problem hiding this comment.
The tap-during-growth fix itself follows spec:AST-094 FR35, but approval waits on an engineering owner settling spec direction this branch carries from its dependencies: where DropdownMenu and MoreMenu sit on a compact touch screen (over their trigger here, which spec:AST-094 OQ9 still leaves as an open owner decision), architecture:layer-runtime INV13 with its Exit copy section, and component:DropdownMenu FR21. An engineering owner records that decision by commenting /approve-spec with the full head SHA on this pull request, and the author may do so as an engineering owner. Independently of that decision:
- Land the dependency pull requests first and rebase, so this one carries only its own 340-line change rather than 33,141 authored lines at older versions of those branches, whose newer commits already fix the exit copy's form ownership and the FR29 wording in the DropdownMenu usage doc.
- Keep
pan-xfor a screen-edge pan from two edges and claim only inside the edge band, as the current FR18 and DEC-13 require, where the gesture foundation lands. - Add a pinch during the menu's growth to the FR35 browser cases, which FR35's verification row lists.
- Drop “no component uses it yet” from
.changeset/layer-input-hold.mdunless #8342 releases first, and update the layer-runtime record's Current gaps, which still say no exit copy exists.
[Automated review]
| // Out of every form and radio group: a cloned checked radio with the live | ||
| // group's name would take its selection, and a named field would submit. | ||
| for (const node of copy.querySelectorAll('[name], [form]')) { | ||
| node.removeAttribute('name'); | ||
| node.removeAttribute('form'); | ||
| } |
There was a problem hiding this comment.
Removing name and form leaves each cloned control owned by its nearest ancestor <form>, and the copy is inserted right after the layer (:237), so in a layer rendered inside a form the copy's controls join that form while the close plays. INV13 and this file's header say the copy has no form owner. #8186's branch has since fixed this (every cloned control gets form=""); a rebase onto it brings the fix.
| anatomy, | ||
| description: | ||
| 'A dropdown menu that displays a list of actionable items in a popup triggered by a button. Use to present action options as a next step in a process, or to offer contextual actions without cluttering the interface. Like the menus of macOS and iOS, the row under the pointer when it is released is the row that acts, the highlight follows a held mouse or finger, a mouse opens it on press and can drag straight into it, and a finger held on the trigger opens it with the finger down.', | ||
| 'A dropdown menu that displays a list of actionable items in a popup triggered by a button. Use to present action options as a next step in a process, or to offer contextual actions without cluttering the interface. Like the menus of macOS and iOS, the row under the pointer when it is released is the row that acts, the highlight follows a held mouse or finger, a mouse opens it on press and can drag straight into it, and a finger held on the trigger opens it with the finger down. Under a finger it behaves like UIMenu on iOS: a finger resting on the trigger for 150 ms opens the menu, the finger that opened it picks a row only once it has moved 10 px (so the row that appears under it is never picked by accident), the menu grows out of its trigger and collapses back into it on UIKit\u2019s springs, and a pan that starts outside it dismisses it as the page scrolls. On a phone the popover menu sits over its trigger as UIMenu does: centered on it, top-aligned with it (bottom-aligned when the trigger is in the lower half of the viewport), 8 px from the viewport\u2019s sides.', |
There was a problem hiding this comment.
“A finger resting on the trigger for 150 ms opens the menu” contradicts spec:AST-094 FR29: a trigger in a fixed bar opens as the finger presses, and only a trigger inside a scroll view waits for the 150 ms content-touch delay, as this file's presentation row already says. #8089's branch has since corrected the sentence.
| override get nativeTouchBehavior(): NativeTouchBehavior { | ||
| if (this.edges.length === 1) { | ||
| return ALL_NATIVE_TOUCH; | ||
| } | ||
| const horizontal = this.edges.some( | ||
| edge => edge !== 'top' && edge !== 'bottom', | ||
| ); | ||
| const vertical = this.edges.some( | ||
| edge => edge === 'top' || edge === 'bottom', | ||
| ); | ||
| return { | ||
| panX: !horizontal, | ||
| panY: !vertical, | ||
| pinchZoom: true, | ||
| doubleTapZoom: false, | ||
| }; |
There was a problem hiding this comment.
With two horizontal edges this reports panX: false, so the element's derived touch-action drops pan-x and a horizontal scroll that starts outside the edge band is blocked. The current spec:AST-094 FR18 says a screen-edge pan claims pan-x only for a touch that lands in the band, and DEC-13 rejects claiming pan-x across the whole element.
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| LayerProvider | 64.5 kB (0) · 9.4 kB CSS (0) | — | ✓ · ✓ · ✗ |
| DropdownMenu | 🔴 80.2 kB (+2.3 kB) · 16.4 kB CSS (0) | — | ✓ · ✓ · ✓ |
| MoreMenu | 🔴 80.4 kB (+2.3 kB) · 16.4 kB CSS (0) | — | — · — · — |
36 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| TableHeaderCell | 83.9 kB (+73 B) · 16.5 kB CSS (0) | — | — · — · — |
| Pagination | 93.3 kB (+68 B) · 16.6 kB CSS (0) | — | — · — · — |
| Table | 92.5 kB (+68 B) · 16.9 kB CSS (0) | — | — · — · — |
| TabMenu | 58.3 kB (+68 B) · 9.8 kB CSS (0) | — | — · — · — |
| TopNavHeading | 61.1 kB (+59 B) · 9.8 kB CSS (0) | — | — · — · — |
| DropdownMenuSubMenu | 59.7 kB (+58 B) · 9.3 kB CSS (0) | — | ✓ · ✓ · ✓ |
| NavHeadingMenu | 15.5 kB (+57 B) · 3.5 kB CSS (0) | — | — · — · — |
| TableCell | 84.3 kB (+57 B) · 16.7 kB CSS (0) | — | — · — · — |
| BreadcrumbItem | 71.5 kB (+54 B) · 11.3 kB CSS (0) | — | — · — · — |
| Markdown | 153.8 kB (+53 B) · 19.2 kB CSS (0) | — | — · — · — |
| SideNavHeading | 61.7 kB (+52 B) · 10.0 kB CSS (0) | — | — · — · — |
| Selector | 83.2 kB (+50 B) · 16.2 kB CSS (0) | — | — · — · — |
| TopNavMenu | 61.1 kB (+50 B) · 9.9 kB CSS (0) | — | — · — · — |
| ContextMenu | 83.1 kB (+46 B) · 16.3 kB CSS (0) | — | — · — · — |
| PowerSearch | 260.2 kB (+13 B) · 22.6 kB CSS (0) | — | — · — · — |
| Drawer | 41.4 kB (+4 B) · 8.2 kB CSS (0) | — | — · — · — |
| AlertDialog | 90.2 kB (0) · 13.1 kB CSS (0) | — | — · — · — |
| AppShell | 76.3 kB (0) · 12.2 kB CSS (0) | — | — · — · — |
| CommandPalette | 58.7 kB (0) · 12.3 kB CSS (0) | — | — · — · — |
| ContextMenuItem | 25.9 kB (0) · 6.1 kB CSS (0) | — | — · — · — |
| Dialog | 22.3 kB (0) · 6.7 kB CSS (0) | — | — · — · — |
| DropdownMenuCheckboxItem | 21.0 kB (0) · 6.4 kB CSS (0) | — | ✓ · ✓ · ✓ |
| DropdownMenuDivider | 5.2 kB (0) · 548 B CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuGroup | 6.3 kB (0) · 3.4 kB CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuItem | 20.3 kB (0) · 6.0 kB CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuRadioGroup | 6.3 kB (0) · 3.3 kB CSS (0) | — | ✓ · ✓ · — |
| DropdownMenuRadioItem | 21.1 kB (0) · 6.4 kB CSS (0) | — | ✓ · ✓ · — |
| MobileNav | 56.7 kB (0) · 9.3 kB CSS (0) | — | — · — · — |
| SideNav | 76.3 kB (0) · 10.1 kB CSS (0) | — | — · — · — |
| Tokenizer | 74.8 kB (0) · 17.1 kB CSS (0) | — | — · — · — |
| TopNav | 58.3 kB (0) · 9.6 kB CSS (0) | — | — · — · — |
| layer | — | — | — · ✓ · — |
| Layer | — | — | ✓ · ✓ · — |
| lab/Comments | 273.6 kB (new) · 2.5 kB CSS (new) | — | — · — · — |
| lab/CommentThread | 268.5 kB (new) · 2.4 kB CSS (new) | — | — · — · — |
| richtext/RichTextEditorToolbar | 233.8 kB (new) · 454 B CSS (new) | — | — · — · — |
Package size: measured; main has no measurement of the merge base to compare yet.
API changes: cli +8 · ~1 · −0 (against the merge base's committed report)
Committed API report behind this build: cli 9 rows, core 4 rows; run pnpm api:report.
9 API changes (advisory)
| Package | Where | Signature | Usage | |
|---|---|---|---|---|
| + | cli |
./api · Types · type SearchResultEntry |
deprecated?: ComponentDeprecation | undefined |
import type {SearchResultEntry} from '@astryxdesign/cli/api'; const x: SearchResultEntry = {deprecated: …} |
| + | cli |
./authoring · Types · interface ComponentDeprecation |
interface ComponentDeprecation |
import type {ComponentDeprecation} from '@astryxdesign/cli/authoring' |
| + | cli |
./authoring · Types · interface MultiComponentDoc |
deprecated?: ComponentDeprecation |
import type {MultiComponentDoc} from '@astryxdesign/cli/authoring'; const x: MultiComponentDoc = {deprecated: …} |
| + | cli |
./authoring · Types · interface SingleComponentDoc |
deprecated?: ComponentDeprecation |
import type {SingleComponentDoc} from '@astryxdesign/cli/authoring'; const x: SingleComponentDoc = {deprecated: …} |
| + | cli |
./doc · Types · interface ComponentBaseDoc |
deprecated?: ComponentDeprecation |
import type {ComponentBaseDoc} from '@astryxdesign/cli/doc'; const x: ComponentBaseDoc = {deprecated: …} |
| + | cli |
./doc · Types · interface SubComponentDoc |
deprecated?: ComponentDeprecation |
import type {SubComponentDoc} from '@astryxdesign/cli/doc'; const x: SubComponentDoc = {deprecated: …} |
| + | cli |
./doc · Also exported from an earlier entry point · ComponentDeprecation |
ComponentDeprecation (./authoring) |
import {ComponentDeprecation} from '@astryxdesign/cli/doc' |
| + | cli |
./template · Also exported from an earlier entry point · ComponentDeprecation |
ComponentDeprecation (./authoring) |
import {ComponentDeprecation} from '@astryxdesign/cli/template' |
| ~ | cli |
./api · Functions · function search |
function search: (query: string, options?: { cwd?: string | undefined; type?: "template" | "component" | "hook" | "theme" | "doc" | undefined; limit?: number | undefined }) => Promise<SearchResponse> → function search: (query: string, options?: { cwd?: string | undefined; type?: "template" | "component" | "doc" | "theme" | "hook" | undefined; limit?: number | undefined }) => Promise<SearchResponse> |
import {search} from '@astryxdesign/cli/api'; search(…) |
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | View full report
There was a problem hiding this comment.
The tap-during-growth fix itself follows spec:AST-094 FR35, but approval waits on an engineering owner settling spec direction this branch carries from its dependencies: where DropdownMenu and MoreMenu sit on a compact touch screen (over their trigger here, which spec:AST-094 OQ9 still leaves as an open owner decision), architecture:layer-runtime INV13 with its Exit copy section, and component:DropdownMenu FR21. An engineering owner records that decision by commenting /approve-spec with the full head SHA on this pull request, and the author may do so as an engineering owner. Independently of that decision:
- Land the dependency pull requests first and rebase, so this one carries only its own two commits rather than 33,806 authored lines at older versions of those branches, whose newer commits already fix the exit copy's form ownership, the FR29 wording in the DropdownMenu usage doc, the two-edge screen-edge pan and the layer-runtime record's Current gaps.
- Route Lab's access to core's motion and gesture modules through
@astryxdesign/core/internal/touch, asarchitecture:react-component-runtimeINV13 now states, or drop that plumbing until Lab needs it. - Leave the action-group and
perEventPointerfine-pointer paths in the gesture host to their own pull request, sincespec:AST-094FR19 does not allow them yet. - Add a pinch during the menu's growth to the FR35 browser cases, which FR35's verification row lists, and drop “no component uses it yet” from
.changeset/layer-input-hold.mdunless #8342 releases first.
[Automated review]
| const CORE_INTERNAL_SPECIFIER = | ||
| /^@astryxdesign\/core\/((?:motion|gestures)(?:\/[\w.-]+)*|utils\/touchPresentation)$/; |
There was a problem hiding this comment.
architecture:react-component-runtime INV13, current on main, says Core and Lab reach the shared touch modules through @astryxdesign/core/internal/touch, limited by lint to Core and Lab. This specifier makes @astryxdesign/core/motion/*, gestures and utils/touchPresentation Lab's route instead, paths core's exports do not list, so a published Lab build importing them would fail in a consumer's bundler. No Lab file imports them yet; dropping this plumbing until Lab needs a module, then using ./internal/touch, would match the record.
| const needsGroupListener = | ||
| registration.actionGroup && | ||
| !this.isPrimaryPointerCoarse() && | ||
| (registration.recognizers.some(r => r.isEnabled) || | ||
| (registration.consumer != null && | ||
| registration.consumer.isEnabled?.() !== false)); | ||
| if (needsGroupListener && registration.groupDownListener == null) { | ||
| const listener: EventListener = event => | ||
| this.beginGroupPress(event as PointerEvent, registration); | ||
| registration.groupDownListener = listener; | ||
| element.addEventListener('pointerdown', listener, {passive: true}); |
There was a problem hiding this comment.
Under a fine primary pointer this registers a pointerdown listener and serves the group's recognizers, while spec:AST-094 FR19 says “Where the primary pointer is fine, no recognizer attaches and no touch listener is registered”, and family:action-groups, which the comment cites, does not exist on main. Nothing at this head passes actionGroup: true; this path belongs with #8072 and the amendment it waits on.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about spec edits this branch adds or carries: component:DropdownMenu FR22 limits the entrance hold to a coarse primary pointer, which spec:AST-094 FR35 and its coarse-primary exception (Dialog, Drawer and MobileNav only) do not allow for DropdownMenu, and, from #9504, module:DropdownMenu/useMenuPress DEC-9, component:DropdownMenu DEC-11 with the FR9 rewrite, and the spec:AST-055 FR2 addition. 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), or those edits settle in their own pull requests first.
Whichever way that goes, these need changes:
- The branch carries 5,746 authored lines, 5,168 of them from #9910 and #9504, and #9504's head does not contain #9910's head, so point #9910's branch at the commit #9504 carries (or rebase #9504 onto #9910), or land both first and rebase.
- The hold still skips a finger where the primary pointer is fine (
packages/core/src/Layer/holdEntranceInput.ts:61) while the title says "under a finger", so hold whenever a finger or a pen opened the menu, or, if the owner approves the limit, add DropdownMenu to AST-094's coarse-primary exception and retitle the pull request to "on a phone or a tablet".
The hold now starts in the layer's show callback, holds on a phone profile in Chromium, and leaves a mouse unchanged; CI skipped its jobs on this draft head.
[Automated review]
| | FR17 | Arrow keys and typeahead skip disabled rows, including checkbox and radio rows. Shipped behavior, not a requirement until OQ1 is ruled. | Current source and tests | | ||
| | FR20 | With `header` or `footer` on a sub-menu (`DropdownMenuSubMenu`), its surface is a modal dialog named by its row (AR7), holding the header, the sub-menu's `role="menu"` (rows only), then the footer. It sits in a host `DropdownMenu` renders beside its menu, never inside a menu; under a root with no such host (`ContextMenu`) the sub-menu shows its rows alone. On a fine pointer the dialog is the flyout; on a compact touch display, the drilled-in view, which replaces the root's menu and regions until popped. A region keeps its own keys and presses. The rows are one Tab stop: Tab and Shift+Tab walk the header's controls, the rows, then the footer's, and wrap; from a deeper flyout, that flyout closes and the walk goes on from the rows. An open lands as for any sub-menu (FR11, FR12), not on a region. Escape in the dialog, outside an IME composition and unless a control there handled it, returns focus to the row and leaves the root open. A pointer leaving the flyout keeps it open until focus leaves, and it closes with the flyout holding its row. Without either prop nothing changes. | Current behavior; FR15; AR5 | | ||
| | FR21 | Under a finger, in the touch modes (`presentation` not pinned to `'popover'`), the pointer menu MUST grow out of the Trigger button on the theme's entrance duration and easing tokens and collapse back into it on the theme's exit duration and easing (`spec:AST-094` FR7, FR31, FR45), and MUST close as a pan that starts outside it begins (FR32); a pinned `'popover'` keeps the layer's own entrance. A pan dismissal is the menu's own close: focus that was in the menu returns to the Trigger button, whose `aria-expanded` turns false. A mouse keeps the layer's own entrance and exit. Implementation gap: the menu keeps the layer's own entrance and exit under a finger too, and has no pan-outside dismissal of its own. | `spec:AST-094` FR31, FR32; `family:overlay-dismissal` AV6 | | ||
| | FR22 | Under a finger, in the touch modes, where the primary pointer is coarse (a phone or a tablet), a tap or a drag that lands while the pointer menu enters MUST reach nothing: it closes nothing, scrolls nothing, moves no focus and clicks nothing (`spec:AST-094` FR35). The finger that opened the menu still picks the row it lifts on (FR8). Keys and assistive technology stay live. A mouse, a pinned `'popover'` and a finger where the primary pointer is fine (a touchscreen laptop) hold nothing, as for `component:Drawer`. | `spec:AST-094` FR35; `architecture:layer-runtime` | |
There was a problem hiding this comment.
The coarse-primary limit here is new record text: spec:AST-094's exception for a coarse primary pointer names only Dialog, Drawer and MobileNav, and FR35 holds "an anchored menu growing out of its trigger" under any finger. It needs an engineering owner's approval and an AST-094 amendment naming DropdownMenu, or the hold should also cover a finger on a touchscreen laptop.
| * Waits until the open menu has finished entering. Under a finger, input that | ||
| * lands during the entrance is held, as UIMenu takes none until it is shown. | ||
| */ | ||
| async function presented(page: Page) { |
There was a problem hiding this comment.
Non-blocking: presented() went in under collectActivations's JSDoc (line 50), so that comment now sits above this one and collectActivations has none. Move it back above collectActivations.
033d5f9 to
0aa4d9d
Compare
vjeux
changed the title
There was a problem hiding this comment.
This stack is reviewed as one: #9910 → #9504 → #8458. A pull request lands only when it and every pull request below it are approved.
#9910
Approved pending CI: CI has not run on this draft head yet; it runs once the pull request is ready and a maintainer adds the ci:run label. The internal followsDocument option has no caller of its own, and the tests show every current caller, useAnimator included, keeps its attachment across a document move.
#9504
Waiting on an engineering owner's decision; no other blocking finding.
- Approval needs an engineering owner to approve the spec edits at this pull request's current head:
module:DropdownMenu/useMenuPressDEC-9 (with its Migration line),component:DropdownMenuDEC-11 with the FR9 rewrite, and thespec:AST-055FR2 addition; DEC-9 is the only record forcreateMenuPress'supdate, the threehandle…forwards,MenuPressDownEvent,startBehavior, and the rule that a finger's press-down a page handler stops or cancels starts no press in Core's menus.
#8458
Waiting on an engineering owner's decision; no other blocking finding.
- Approval needs an engineering owner to approve
component:DropdownMenuFR22 and thespec:AST-094edit that adds DropdownMenu to the coarse-primary exception, so a finger on a touchscreen laptop is not held while the menu enters.
One non-blocking comment suggestion is inline.
Whole stack
No blocking finding across the stack: the net API is createMenuPress and its three types, nothing is deprecated, and no desktop behavior changes. An engineering owner records each decision by commenting /approve-spec <full head SHA> on that pull request, and the author may do so as an engineering owner.
[Automated review]
| // Under a finger, in the touch modes, a tap or a drag that lands while the | ||
| // menu enters reaches nothing, where the primary pointer is coarse. UIMenu, | ||
| // too, takes no input until it is shown. Keys and assistive technology stay | ||
| // live. The hold starts as the layer shows, and ends with the entrance, a | ||
| // close or an unmount. |
There was a problem hiding this comment.
Non-blocking: the last sentence restates the code below. A shorter comment that keeps only the reason:
// Under a finger, where the primary pointer is coarse, a tap or a drag during
// the entrance reaches nothing, as in UIMenu. Keys and assistive technology
// stay live.…s is ignored, as on iOS While the menu enters under a finger, in the touch modes, the layer runtime's entrance hold covers the viewport, so a tap or a drag that lands then closes nothing, scrolls nothing and moves no focus. The hold starts as the layer shows, and runs where the primary pointer is coarse, as for Dialog, Drawer and MobileNav. The press model's hit test sees through the hold's shield, so the finger that opened the menu still picks a row.
0aa4d9d to
789fc14
Compare
|
/approve-spec 789fc14 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. It runs once the pull request is marked ready for review and a maintainer adds the ci:run label.
The entrance hold restores spec:AST-094 FR35 for DropdownMenu on phones and tablets: a tap during the menu's entrance no longer closes it or reaches the page, the finger that opened the menu still picks a row, and a mouse is unchanged. An engineering owner approved the new component:DropdownMenu FR22 and the AST-094 coarse-primary edit for this head.
[Automated review]
|
/approve-spec 789fc14 |
cixzhang
added
the
ci:run
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about the record text at this head. The merge of main left the fix itself unchanged, but the earlier /approve-spec names the previous commit, so it does not cover this head.
- To settle:
component:DropdownMenuFR22 (with its verification row and relationship bullet), which holds a tap or a drag during the menu's entrance only where the primary pointer is coarse, and thespec:AST-094Current-state impact sentence that adds DropdownMenu to the coarse-primary exception. - 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.
The hold restores spec:AST-094 FR35 on phones and tablets: a tap during the menu's entrance no longer closes it or reaches the page, the finger that opened the menu still picks a row, a mouse is unchanged, and CI is green on this head.
[Automated review]
| // The entrance hold starts as the layer shows. Its start is set below, once | ||
| // the list ref exists, and its release is kept for the close. |
There was a problem hiding this comment.
Non-blocking: the first sentence and the last clause restate the code. A shorter comment that keeps only the constraint:
// The list ref comes from useListFocus below, so the hold's start is set there.| | FR17 | A disabled menu row MUST be reached by the keyboard and stay inert (DEC-11): in DropdownMenu, its sub-menus, MoreMenu, ContextMenu and NavHeadingMenu, ArrowDown, ArrowUp, Home, End and type-ahead visit a disabled row as they visit any other row, the row takes focus and exposes `aria-disabled="true"`, and Enter, Space or a click on it does nothing and leaves the menu open. Opening a menu from the keyboard still lands on an enabled row: the first, or the last after ArrowUp on the trigger (FR12, AR4). Implementation gap: the arrow keys and type-ahead skip disabled rows today. | DEC-11; WAI-ARIA Menu pattern | | ||
| | FR20 | With `header` or `footer` on a sub-menu (`DropdownMenuSubMenu`), its surface is a modal dialog named by its row (AR7), holding the header, the sub-menu's `role="menu"` (rows only), then the footer. It sits in a host `DropdownMenu` renders beside its menu, never inside a menu; under a root with no such host (`ContextMenu`) the sub-menu shows its rows alone. On a fine pointer the dialog is the flyout; on a compact touch display, the drilled-in view, which replaces the root's menu and regions until popped. A region keeps its own keys and presses. The rows are one Tab stop: Tab and Shift+Tab walk the header's controls, the rows, then the footer's, and wrap; from a deeper flyout, that flyout closes and the walk goes on from the rows. An open lands as for any sub-menu (FR11, FR12), not on a region. Escape in the dialog, outside an IME composition and unless a control there handled it, returns focus to the row and leaves the root open. A pointer leaving the flyout keeps it open until focus leaves, and it closes with the flyout holding its row. Without either prop nothing changes. | Current behavior; FR15; AR5 | | ||
| | FR21 | Under a finger, in the touch modes (`presentation` not pinned to `'popover'`), the pointer menu MUST grow out of the Trigger button on the theme's entrance duration and easing tokens and collapse back into it on the theme's exit duration and easing (`spec:AST-094` FR7, FR31, FR45), and MUST close as a pan that starts outside it begins (FR32); a pinned `'popover'` keeps the layer's own entrance. A pan dismissal is the menu's own close: focus that was in the menu returns to the Trigger button, whose `aria-expanded` turns false. A mouse keeps the layer's own entrance and exit. Implementation gap: the menu keeps the layer's own entrance and exit under a finger too, and has no pan-outside dismissal of its own. | `spec:AST-094` FR31, FR32; `family:overlay-dismissal` AV6 | | ||
| | FR22 | Under a finger, in the touch modes, where the primary pointer is coarse (a phone or a tablet), a tap or a drag that lands while the pointer menu enters MUST reach nothing: it closes nothing, scrolls nothing, moves no focus and clicks nothing (`spec:AST-094` FR35). The finger that opened the menu still picks the row it lifts on (FR8). Keys and assistive technology stay live. A mouse, a pinned `'popover'` and a finger where the primary pointer is fine (a touchscreen laptop) hold nothing, as for `component:Drawer`. | `spec:AST-094` FR35; `architecture:layer-runtime` | |
There was a problem hiding this comment.
Non-blocking: "A mouse … hold nothing" leaves out the rule the spec:AST-094 edit in this pull request now applies to DropdownMenu: after a finger, the first press of a mouse that has not moved since lands on the entering menu's shield and does nothing. Saying so here would keep the two records in step.
|
/approve-spec 7986187 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Approved. The entrance hold restores spec:AST-094 FR35 for DropdownMenu on phones and tablets: a tap during the menu's entrance no longer closes it or reaches the page, the finger that opened the menu still picks a row, and a mouse, a touchscreen-laptop finger and presentation="popover" are unchanged. component:DropdownMenu FR22 and the AST-094 coarse-primary sentence are approved for this head, and CI is green.
Non-blocking suggestions:
- The comment above
holdEntranceRef(DropdownMenu.tsx:961-962) could keep only its constraint: "The list ref comes from useListFocus below, so the hold's start is set there." - FR22 could add the shared rule it now inherits from the AST-094 sentence: after a finger, the first press of a mouse that has not moved lands on the entering menu's shield and does nothing.
[Automated review]
| // The entrance hold starts as the layer shows. Its start is set below, once | ||
| // the list ref exists, and its release is kept for the close. |
There was a problem hiding this comment.
Non-blocking: the first sentence and the last clause restate the code. Keep only the constraint, for example: "The list ref comes from useListFocus below, so the hold's start is set there."
| | FR17 | A disabled menu row MUST be reached by the keyboard and stay inert (DEC-11): in DropdownMenu, its sub-menus, MoreMenu, ContextMenu and NavHeadingMenu, ArrowDown, ArrowUp, Home, End and type-ahead visit a disabled row as they visit any other row, the row takes focus and exposes `aria-disabled="true"`, and Enter, Space or a click on it does nothing and leaves the menu open. Opening a menu from the keyboard still lands on an enabled row: the first, or the last after ArrowUp on the trigger (FR12, AR4). Implementation gap: the arrow keys and type-ahead skip disabled rows today. | DEC-11; WAI-ARIA Menu pattern | | ||
| | FR20 | With `header` or `footer` on a sub-menu (`DropdownMenuSubMenu`), its surface is a modal dialog named by its row (AR7), holding the header, the sub-menu's `role="menu"` (rows only), then the footer. It sits in a host `DropdownMenu` renders beside its menu, never inside a menu; under a root with no such host (`ContextMenu`) the sub-menu shows its rows alone. On a fine pointer the dialog is the flyout; on a compact touch display, the drilled-in view, which replaces the root's menu and regions until popped. A region keeps its own keys and presses. The rows are one Tab stop: Tab and Shift+Tab walk the header's controls, the rows, then the footer's, and wrap; from a deeper flyout, that flyout closes and the walk goes on from the rows. An open lands as for any sub-menu (FR11, FR12), not on a region. Escape in the dialog, outside an IME composition and unless a control there handled it, returns focus to the row and leaves the root open. A pointer leaving the flyout keeps it open until focus leaves, and it closes with the flyout holding its row. Without either prop nothing changes. | Current behavior; FR15; AR5 | | ||
| | FR21 | Under a finger, in the touch modes (`presentation` not pinned to `'popover'`), the pointer menu MUST grow out of the Trigger button on the theme's entrance duration and easing tokens and collapse back into it on the theme's exit duration and easing (`spec:AST-094` FR7, FR31, FR45), and MUST close as a pan that starts outside it begins (FR32); a pinned `'popover'` keeps the layer's own entrance. A pan dismissal is the menu's own close: focus that was in the menu returns to the Trigger button, whose `aria-expanded` turns false. A mouse keeps the layer's own entrance and exit. Implementation gap: the menu keeps the layer's own entrance and exit under a finger too, and has no pan-outside dismissal of its own. | `spec:AST-094` FR31, FR32; `family:overlay-dismissal` AV6 | | ||
| | FR22 | Under a finger, in the touch modes, where the primary pointer is coarse (a phone or a tablet), a tap or a drag that lands while the pointer menu enters MUST reach nothing: it closes nothing, scrolls nothing, moves no focus and clicks nothing (`spec:AST-094` FR35). The finger that opened the menu still picks the row it lifts on (FR8). Keys and assistive technology stay live. A mouse, a pinned `'popover'` and a finger where the primary pointer is fine (a touchscreen laptop) hold nothing, as for `component:Drawer`. | `spec:AST-094` FR35; `architecture:layer-runtime` | |
There was a problem hiding this comment.
Non-blocking: "A mouse … hold nothing" leaves out the case the AST-094 sentence now applies to DropdownMenu: after a finger, the first press of a mouse that has not moved lands on the entering menu's shield and does nothing. A follow-up could add that clause here.
`spec:AST-094` says that after a finger, the first press of a mouse that has not moved since lands on the entering presentation's shield and does nothing. meta#8458 brought DropdownMenu under that rule, so FR22 now says so too, and the entrance hold's ref comment keeps only its constraint.
`spec:AST-094` says that after a finger, the first press of a mouse that has not moved since lands on the entering presentation's shield and does nothing. meta#8458 brought DropdownMenu under that rule, and FR22 now says so too.
`spec:AST-094` records that after a finger, the first press of a mouse that has not moved since lands on the entering presentation's shield and does nothing, where FR35 keeps today's behavior for a mouse. meta#8458 brought DropdownMenu under that, and FR22 now notes it as an implementation gap.
* docs(DropdownMenu): FR22 notes the still-mouse shield gap `spec:AST-094` records that after a finger, the first press of a mouse that has not moved since lands on the entering presentation's shield and does nothing, where FR35 keeps today's behavior for a mouse. #8458 brought DropdownMenu under that, and FR22 now notes it as an implementation gap. * docs(DropdownMenu): list the still-mouse gap in the current state The record's Current-state impact gains the still-mouse gap that FR22 notes, and the readiness row of the summary table names it, so the summary no longer reads FR22's mouse clause as met. * docs(DropdownMenu): the still-mouse gap reads as a known defect The Current-state impact entry for the still-mouse gap now reads as a known defect against FR22's touch-only scope, with its fix: a press whose own pointerType is mouse passes the shield. * docs(DropdownMenu): the still-mouse defect entry states only the gap The Current-state impact entry keeps the known defect against FR22's touch-only scope, without a cause or a planned change.
User impact
On a phone, a second tap that lands while a DropdownMenu is still opening closes it halfway, and focus falls to the page. UIMenu on iOS takes no input until it is presented: the menu finishes opening and stays.
Found comparing our menus with UIKit's (iOS 26.2).
Problem and solution fit
The browser's light dismiss closes a popover on any press outside it, even mid-entrance. A quick second tap is common on a phone, so the person loses the menu and focus lands somewhere unexpected.
The fix uses the layer runtime's entrance hold, as Dialog, Drawer and MobileNav do. While the menu enters under a finger, a transparent shield covers the viewport: a tap there closes, scrolls, focuses and clicks nothing. Keys and assistive technology stay live. The finger that opened the menu still slides onto a row: the press model's hit test sees through that shield only.
Expected behavior and authority
spec:AST-094FR35 (current): a tap or drag that lands while an anchored menu is entering must not close it, scroll the page or move focus. This PR records that for DropdownMenu ascomponent:DropdownMenuFR22, and adds DropdownMenu to FR35's coarse-pointer exception.Smallest restoration
presentation="popover"hold nothing.Evidence
Writing rubric results
R1, R2, R3, R5: none. R4: no rejected alternatives in the record. R6: no open questions added.
Scope
Testing
789fc14 is one commit on main bfc1dcc. Local gates there: the core type check,
pnpm lint, strict lint, Prettier, the knowledge, changeset and SYNC checks,review:pre, the coreuisuite (19,335 passed; the 4 RadioListItem and Switch source-build tests fail the same way on main in this checkout, where a Babel preset does not resolve), andtouch-support-bundlewith resolution-compat (25 of 25). On fresh Storybook builds: the newDropdownMenuEntranceHoldspec, twice per case, in Chromium (6 of 6) and in WebKit through the container recipe (6 of 6); the menu press, drag-highlight, hand-off, TabList menu and input-hold interaction specs in Chromium (23 of 23); and the DropdownMenu, hooks, MoreMenu, ContextMenu, Breadcrumbs, TableCellMenus, PressModality and CardPress a11y specs in Chromium (236 of 236).NOT-FOR-LANDING arms on the same main: 4d4761e (main with the new tests only) fails the tap case 2 of 2, as the menu closes; 094757a (without the hit test's see-through) fails the finger case 2 of 2, as nothing is picked. The browser spec lengthens the entrance to 10 s with a style tag and ends it with
animation.finish(), so no case races the machine. The opening tap's own click lands on the trigger, so the click log ignores only clicks on the trigger. Four touch cases inMenuPress.a11y.chromium.spec.tsnow wait for the menu to finish entering before they press a row, and the drill-in spec counts only the popovers a person can perceive: the hold's shield is an aria-hidden popover while the menu enters.On a phone, on the DropdownMenu Default story:
Card 77 passed: Owner decision, Nov 5 2026 (Cindy).