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
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsAlertDialog (@astryxdesign/core) · View in Storybook
ContextMenu (@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.
|
| Component | JavaScript | CSS |
|---|---|---|
| core/AlertDialog | +27.2KB |
+64B |
| core/BreadcrumbItem | +15.2KB |
0B |
| core/ContextMenu | +15.1KB |
0B |
| core/DropdownMenu | +29.8KB |
0B |
| core/DropdownMenuSubMenu | +14.8KB |
0B |
| core/Markdown | +15.1KB |
0B |
| core/MoreMenu | +29.8KB |
0B |
| core/Pagination | +14.6KB |
0B |
| core/PowerSearch | +14.7KB |
0B |
| core/Selector | +14.8KB |
0B |
| core/Table | +15.0KB |
0B |
| core/TableCell | +15.1KB |
0B |
| core/TableHeaderCell | +15.1KB |
0B |
Bundle Size Summary
Gzip figures measure each package's entry file, not the code an import pulls in.
| Package | Entry (ESM) | Entry gzip before | Entry gzip after | Delta |
|---|---|---|---|---|
| @astryxdesign/core | 4.9KB | 1.2KB | 1.2KB | 0B |
| Package stylesheet | Gzip before | Gzip after | Delta |
|---|---|---|---|
| @astryxdesign/core | 34.3KB | 34.3KB | +43B |
Accessibility Audit
Status: No accessibility violations detected.
Visual Regression
55 of 502 shot(s) changed.
A change here is a question, not a failure: check whether the after is the
picture you intended. Record that review on the PR. Baseline maintenance is an
explicit dispatch of CI; this report never rewrites the baseline or adds a release gate.
| component | story | theme | mode | pixels |
|---|---|---|---|---|
| Selector | Indicator Space Bottom Sheet Narrow End RTL | neutral | light | 753,933 |
| Selector | Indicator Space Bottom Sheet Wide Start | neutral | light | 740,072 |
| RichTextEditor | Markdown parity: side by side | probe | dark | 558,840 |
| RichTextEditor | Markdown parity: side by side | probe | light | 540,756 |
| Chat | Full AI Chat | probe | light | 230,823 |
| Chat | Full AI Chat | probe | dark | 229,805 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Collapsible | Dividers | probe | dark | 66,445 |
| Collapsible | Dividers | probe | light | 66,445 |
| AppShell | Playground | neutral | dark | 65,198 |
| AppShell | Playground | neutral | light | 63,788 |
| Chat | Default | neutral | dark | 36,304 |
| Chat | Default | neutral | light | 35,767 |
| Chat | Empty State | probe | light | 23,531 |
| Chat | Empty State | probe | dark | 22,622 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | dark | 15,378 |
| ChatAutoScroll | Scroll Behavior Comparison | neutral | light | 14,107 |
| Chat | With Attachments | probe | light | 13,406 |
| Chat | With Attachments | probe | dark | 11,232 |
and 35 more.
Generated by PR Enrichment workflow | Storybook | Sandbox | View full report
![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 cannot be approved yet. It intentionally changes module:DropdownMenu/useMenuPress FR2 and FR7 for fingers and adds the public touchTracking option, but the amendment lives only in the description. Write the amended claims into useMenuPress.spec.md on this pull request; an engineering owner then records the decision by commenting /approve-spec <full head SHA> (the author may do so as an engineering owner). These fixes are needed whatever that decision is:
- Attach the selection recognizer only while the menu is open: today every mounted DropdownMenu installs the gesture host's document and window listeners, which is what fails
CommentThread.test.tsx:552intest-ui (2/4). - End the gesture with nothing acting when a second finger lands, as FR4 requires; the recognizer currently ignores it and the first finger's lift still picks a row.
- Make
.changeset/dropdownmenu-touch-selection.mdapatchwith a[feature]tag so required lint passes. - Land the predecessor pull requests first so this diff is its own 1,076 lines rather than 32,868.
[Automated review]
| detach: () => void; | ||
| } | null>(null); | ||
| useIsomorphicLayoutEffect(() => { | ||
| const element = isSelectionEnabled ? options.menuRef.current : null; |
There was a problem hiding this comment.
This attaches the selection recognizer whenever touchTracking is 'uikit' and the menu element exists, which for DropdownMenu is from its first commit, while the menu is closed. GestureHost.attach then installs 14 document and window listeners on every page that renders a menu; CI test-ui (2/4) fails on CommentThread.test.tsx:552 because of the new document pointercancel listener. Attach only while the menu is open (or on the first touch that lands in it) and detach on close.
There was a problem hiding this comment.
Fixed at 141c0a0: the recognizer attaches only while the menu shows. A menu in a popover follows the popover's beforetoggle; one outside a popover attaches while it is in the document. That replaces the isOpen option, which is gone. A jsdom case checks that a closed popover adds no document listeners and that the open attaches.
| const isSelectionEnabled = | ||
| options.touchTracking === 'uikit' && (options.isEnabled ?? true); | ||
| useIsomorphicLayoutEffect(() => { | ||
| selection.setEnabled(isSelectionEnabled); |
There was a problem hiding this comment.
architecture:react-component-runtime INV3 asks a new Effect to name the external system it synchronizes. Turning isEnabled off mid-press cancels the recognizer here, which reaches a consumer's onHighlight(null) through the tracker; a one-line comment naming the gesture recognizer covers it.
There was a problem hiding this comment.
Fixed at 141c0a0: each effect names what it synchronizes (the recognizer's listener, its enabled flag, the gesture host).
| } | ||
|
|
||
| protected override acceptsTouch(touch: GestureTouch): boolean { | ||
| // One finger selects; another landing while it is down is ignored. |
There was a problem hiding this comment.
Current module:DropdownMenu/useMenuPress FR4 says the model MUST treat a second pointer as the end of the gesture with nothing acting. Ignoring the second finger lets the first finger's lift still run a row, and FR4 is not among the claims this pull request says it amends. Cancel (or fail) the press on a second touch and cover it in the DropdownMenu finger suite.
There was a problem hiding this comment.
Fixed at 141c0a0: a second finger cancels a press that has begun, or fails one that hasn't, so nothing acts. The touch itself isn't taken, so the browser's pinch is unaffected, and a late click for the first finger is swallowed. Two jsdom cases with two finger() contacts, before and after the row lights; both fail on the old code.
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| '@astryxdesign/core': minor | |||
There was a problem hiding this comment.
Required lint fails on this file: the body needs a [category] tag, and while core is 0.x only [breaking] may declare minor (scripts/check-changesets.mjs). patch with a [feature] tag passes.
There was a problem hiding this comment.
Fixed at 141c0a0: patch with a [feature] tag.
| }); | ||
| }); | ||
|
|
||
| it('a mouse released outside closes the menu; a finger released outside leaves it open', async () => { |
There was a problem hiding this comment.
The finger half of this case still dispatches touch-type pointer events, which the 'uikit' path ignores (useMenuPress.ts:912), so it passes without reaching finger tracking. Driving it with the finger() helper keeps current FR3 (a finger released outside leaves the menu open) covered on the new path.
There was a problem hiding this comment.
Fixed at 141c0a0: the tracked-finger suite has a finger() case for FR3 (a finger lifted outside acts on nothing and leaves the menu open). DropdownMenu tracks every finger this way now, Android included.
4cf6728 to
3a5d742
Compare
There was a problem hiding this comment.
This pull request cannot be approved yet. It intentionally changes module:DropdownMenu/useMenuPress FR2 and FR7 for fingers and adds the public touchTracking and isOpen options, but the amendment lives only in the description: write the amended claims and both options into useMenuPress.spec.md on this pull request, and an engineering owner then records the decision by commenting /approve-spec <full head SHA> (the author may do so as an engineering owner). These fixes are needed whatever that decision is:
- End the gesture with nothing acting when a second finger lands, as FR4 requires; today the recognizer ignores it and the first finger's lift still picks a row.
- Make
.changeset/dropdownmenu-touch-selection.mdapatchwith a[feature]tag so the required lint check passes. - Document
options.isOpeninuseMenuPress.doc.mjs, and drive the FR3 finger test through the new finger path. - Land the predecessor pull requests first so this diff is its own 1,103 lines rather than 32,895.
[Automated review]
| protected override acceptsTouch(touch: GestureTouch): boolean { | ||
| // One finger selects; another landing while it is down is ignored. | ||
| if (this.numberOfTouches >= 1) { | ||
| return false; | ||
| } | ||
| return super.acceptsTouch(touch); |
There was a problem hiding this comment.
Current module:DropdownMenu/useMenuPress FR4: "The model MUST treat pointercancel and a second pointer as the end of the gesture with nothing acting". Refusing the second touch here means the arena delivers it to nothing, so the first finger's lift still ends this recognizer and the tracker runs the row under it; FR4 is not among the claims the description amends. Cancel (or fail) the press when a second touch lands, and cover it in the DropdownMenu finger suite with two finger() contacts.
There was a problem hiding this comment.
Fixed at 141c0a0; see the reply above (a second finger ends the press with nothing acting, covered with two finger() contacts).
| @@ -0,0 +1,7 @@ | |||
| --- | |||
| '@astryxdesign/core': minor | |||
There was a problem hiding this comment.
Required lint fails on this file at check:changesets ("body must start with a [category] tag"), and while core is 0.x only [breaking] may declare minor (scripts/check-changesets.mjs:91-94). '@astryxdesign/core': patch with a body that starts with [feature] passes.
There was a problem hiding this comment.
Fixed at 141c0a0: patch with a [feature] tag.
| * recognizer only while it is, so a closed menu adds no listeners. | ||
| * @default true | ||
| */ | ||
| isOpen?: boolean; |
There was a problem hiding this comment.
isOpen is a new public option of an exported hook, and the record's Public contract row does not name it (nor touchTracking). Its default (true) also means a third-party caller who opts into 'uikit' and leaves it out attaches the gesture host's document and window listeners whenever the menu element exists. Worth settling with the owner whether the hook can work this out itself (for instance by attaching on the first touch inside the menu) before it becomes API.
There was a problem hiding this comment.
Fixed at 141c0a0: isOpen is gone. The hook works it out from the menu's popover (attached while it shows), so a third-party caller who opts into 'uikit' adds no listeners while the menu is closed. touchTracking is now in the record's Public contract row as proposed.
| useEffect(() => selection.addListener(onSelection), [selection, onSelection]); | ||
| const isSelectionEnabled = | ||
| options.touchTracking === 'uikit' && | ||
| (options.isEnabled ?? true) && | ||
| (options.isOpen ?? true); | ||
| useIsomorphicLayoutEffect(() => { | ||
| selection.setEnabled(isSelectionEnabled); | ||
| }, [selection, isSelectionEnabled]); |
There was a problem hiding this comment.
architecture:react-component-runtime INV3 asks a new Effect to name the external system it synchronizes. Neither the subscription nor the enabled-flag sync does, and the second can reach a consumer's onHighlight(null): when isOpen or isEnabled turns false mid-press, setEnabled(false) cancels the recognizer and its listener cancels the tracker. One comment each naming the gesture recognizer covers it.
There was a problem hiding this comment.
Fixed at 141c0a0: both effects name the gesture recognizer they synchronize, including that turning it off mid-press cancels the tracker.
| }); | ||
| }); | ||
|
|
||
| it('a mouse released outside closes the menu; a finger released outside leaves it open', async () => { |
There was a problem hiding this comment.
The finger half of this case still dispatches touch-type pointer events, which the 'uikit' path ignores (useMenuPress.ts:916-922), so it passes without reaching finger tracking. Driving it with the finger() helper over rows placed with placeRows keeps current FR3 (a finger released outside leaves the menu open) covered on the new path.
There was a problem hiding this comment.
Fixed at 141c0a0; see the FR3 reply above.
| { | ||
| name: 'options.touchTracking', | ||
| type: "'pointer' | 'uikit'", | ||
| description: | ||
| "How a finger or pen that lands in the menu is tracked. 'pointer' (the default): the highlight follows it from the touch-down. 'uikit': as the list of an iOS menu tracks it \u2014 the row under a resting finger lights after UIKit\u2019s content-touch delay (150 ms), a tap acts at the lift, and in a menu that scrolls a finger that travels first scrolls it (nothing lights or acts) while in a menu that fits it slides the highlight at once. A mouse is unaffected.", | ||
| }, |
There was a problem hiding this comment.
options.isOpen is missing here, though this list documents every other option except the hitTest test seam. A builder who follows the docs and opts into 'uikit' will leave isOpen at its default and keep the recognizer attached while the menu is closed; an entry beside touchTracking saying why to pass it fixes that.
There was a problem hiding this comment.
isOpen is removed, so there is nothing to document. The touchTracking entry says DropdownMenu and its sub-menus ask for 'uikit'.
…presentation (#8273) ## Exact decision needed Whether callers can keep a stable component's existing surface when the component gains touch behavior, and through what API. ## Existing authority searched - `architecture:public-component-api` INV4 (keep derivable decisions internal), INV9 (released behavior changes under `spec:AST-017`), INV3 (one prop, one concept). - `spec:AST-002` FR2–FR3: derivable differences, thresholds, and tuning stay internal. - `spec:AST-017` FR3 and DEC-17: a functional default change names the prop that restores the old default; FR21 and FR27: an omitted experimental opt-in leaves stable behavior unchanged, which covers experimental surfaces only. - No current record requires a caller opt-out for touch behavior on a stable component. - Search terms: presentation, opt-out, touch default, restoring prop, adaptive, touch mode. Overlapping open pull requests: #8229 (Popover `presentation`), #8195 (DropdownMenu touch tracking), #8052 (AST-094 resolver), #8062 (shared touch resolver), #7977 and #8001 (touch sheets behind `experimental.presentation`). ## Decision - `architecture:public-component-api` INV16: a stable component that, under a coarse pointer, presents a different surface or switches how an open surface follows input takes one `presentation` prop, and a value such as `presentation="popover"` keeps its fine-pointer surface on every device. Touch behavior is the default where it serves touch users; the caller's explicit choice always wins. Every such touch mode is reachable by callers; thresholds and tuning stay internal. Touch styling and touch-only behavior other records define (press paint, the iOS text-size floor, a row swipe, a tooltip's touch trigger) stay with those records. - `spec:AST-017` FR3 and DEC-23: for a touch default, that `presentation` value is the restoring input the Changeset names. ## Rejected alternatives - Touch behavior that callers cannot turn off or pin. ## Claim scope and non-goals Caller control over touch presentations and modes on stable components. Which components adopt touch defaults, the touch behavior itself, and whether a given touch default is breaking stay with their owners. ## Evidence and implementation relationship Selector, MultiSelector, DropdownMenu, MoreMenu, ContextMenu, and the date and time inputs already take `presentation`. Related open work, not edited here: #8229 adds `presentation` to Popover (its `'popover'` default conforms); #8195 adds a touch-tracking mode to DropdownMenu, which INV16 requires callers to be able to reach; the `spec:AST-094` revision in #8052 carries the touch presentation resolver. No implementation is included. ## Writing rubric results - R1 narration: none. - R2 research in the record: none. - R3 evidence naming a change: none; verification names test states. - R4 rejected alternatives over one line or outside a decision: one, inside DEC-23, one line. - R5 pull request or issue references remaining: none. - R6 ruled open questions still listing options: none. ## Scope - [x] This PR contains specification records only. - [x] It records observable behavior and preserves equivalent internal implementations. - [x] Any named internal mechanism is intentionally public, names its dependent caller/system, and explains why an equivalent implementation would not satisfy the contract. - [x] Verification states the evidence layer and failure signal without prescribing CI job/workflow topology. - [x] It records one intentional durable decision, not a rescue for a separable tagalong. - [x] The canonical knowledge owner, affected current claims, and overlapping open work are named; any ownership collision is resolved or called out for human decision. - [x] Adjacent API, visual, accessibility, composition, and implementation facts remain outside scope unless this exact decision depends on them. - [x] Public text and artifacts contain no internal Meta context. ## Validation - `pnpm check:knowledge` (CI lint) - Prettier - `git diff --check`
|
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.
|
…ry pair, meta#8195 re-takes
3a5d742 to
141c0a0
Compare
github-actions
Bot
added
the
needs:design-review
141c0a0 to
215eebb
Compare
215eebb to
c190837
Compare
…ose finds it The unmount kept the node of the first attach(null) in the task, so after attach(null), attach(next), attach(null) it landed a waiting Reduce Motion move on the node already replaced, and next stayed at its dip's opacity. A dispose right after attach(null) landed on no element, leaving the detached node at opacity 0. The latest detached node is now kept while its unmount is pending; the unmount and dispose both land there. Two tests fail on the previous head.
…ile it ran A page opened after startCompositorTrace and closed before stop() was still credited: its frames are in the browser-wide record, and neither check saw it. The record now watches for new pages from start to stop, in the page's own context and through the browser's target discovery in every context, and stop() throws if one opened. Negative controls: a page opened and closed mid-record, and one in another context, both make stop() refuse; alone, it records.
…rns to joins that dip Under Reduce Motion a call that names an opacity while an earlier dip's move still waits to land took the dip branch. So a call naming the opacity the dip already returns to, with no travel of its own, opened a second dip from where the first had got to: mid fade out the element faded out twice, and in the dip's own task the browser got a second, identical fade. A swipe row's release does exactly that: its move's layer plays the travel as a dip, then its state layer lands opacity 1 on the same element in the same task. Such a call now joins the waiting dip: the planner returns joins, the fade goes on as it was, the call takes the landing over and settles when the dip ends, and the call it replaces lands with it, each value keeping the call whose move it is. A call that names travel, or another opacity, plans as before. Four tests (the planner's, mid dip, in the dip's task, and a release played then landed) fail on the previous code.
Inside a trace, waitForAnimation and watch.started() read the element through handles, which find no element rather than wait for one, so a wait on an element the page removed rejected with "no animation started on the element" instead of "the element is not in the document", unlike the untraced path. At its deadline, a traced wait with no element now rejects as missing. Traced twin of the missing-element self-test; it fails on the previous code. Co-authored-by: Christopher Chedeau <vjeuxx@gmail.com>
There was a problem hiding this comment.
This pull request needs changes before it can be approved. The finger model in an open DropdownMenu follows the engineering owner's ruling on the content-touch delay and matches its governing claims, so what remains is scope:
- The
architecture:layer-runtimeINV13 and Exit copy text is still here, although an engineering owner asked for it to land only through #8186. - The diff carries fourteen open predecessor pull requests (35,670 authored lines against this change's own 1,900), including AlertDialog's new
presentationprop, so landing them first, or rebasing once they merge, keeps this pull request to its own change.
[Automated review]
| correct, and the runtime polls nothing: an anchor that a scrolled | ||
| container hides is followed again when an intersection observer reports it | ||
| back in view. | ||
| - **INV13 — One exit copy per closing layer.** A layer's close plays on at most |
There was a problem hiding this comment.
An engineering owner asked for INV13 and its Exit copy section to come out of this pull request, since they belong to #8186; they are still here at this head, with their verification row, change-coupling and owning-code lines.
| onAction, | ||
| width = 400, | ||
| width: widthProp, | ||
| presentation = 'adaptive', |
There was a problem hiding this comment.
This default moves every existing AlertDialog to the touch alert under a coarse primary pointer. spec:AST-094 FR25 lists the released defaults touch presentations move and AlertDialog's is not one, and the draft AlertDialog record names no presentation. It arrives from #8083 and #8084; landing those first keeps it out of this pull request.
…med or not The planner's comment on the cross-fade's length said it is not a theme role and no theme lengthens it. Under spec:AST-094 DEC-11 no motion role is themeable: the fade's length is no role at all, and nothing lengthens it.
A promotion keeps the stylesheet's will-change reasons beside its own, and read them with getComputedStyle as it started: in a drag's first move, that forces a style recalculation inside the input handler. The stylesheet's value is now read in a task of its own after the element is attached, and again after each demotion, so a promotion uses what was read. One that comes before that read (an element attached in this task that moves at once) still reads it then. A test that counts style reads in the promoting task fails on the previous code; one for the element that moves at once passes on both.
…rred unmount runs It read will-change right after attach(null), before the unmount's microtask, so it passed while that microtask could still promote the element and leave timers behind. It now awaits the microtask, then checks will-change and that no timer is left. It fails on meta#8521's earlier head (97436b2), before the unmount cleared what the landing rebuilt.
…pseudo-element support
…ith one passive listener and press-time document listeners
family:action-groups FR5 (and the AST-094 FR19/FR24 exception proposed with
it): the behavior applies to every pointer, mouse and pen included, so a
group must hear a press where the primary pointer is fine, where the host
otherwise registers nothing. attach() takes { actionGroup: true }: under a
fine pointer the group element gets one passive pointerdown listener; a
press on it adds document pointermove, pointerup and pointercancel
listeners (and window blur, plus the click listener for the click the lift
owes), and they go at the release. Only the group's own recognizers and
consumer take that press. Under a coarse pointer the host's usual listeners
serve the group, as every other recognizer.
Tests (failing first): idle under a fine pointer, only the group's one
passive pointerdown and nothing for a recognizer that is not a group; a
press adds the document listeners and the release removes them; a
recognizer that is not a group on the press path takes no part; detach
removes the group listener, and a coarse pointer keeps the usual
listeners.
…ond route around ./internal/* Lab's per-module tsconfig paths (core's motion, gestures and touchPresentation) and the Sandbox/docsite resolver that served them opened a route around architecture:react-component-runtime INV13's ./internal/* subpath exports. Nothing in Lab imports those paths here; they're dropped, and the import guard now fails on any Lab import core doesn't export and on any Lab tsconfig alias for core. Co-authored-by: Christopher Chedeau <vjeux@users.noreply.github.com>
…n a larger screen Cindy's D27: menus stay beside their trigger; the covering placement is rejected. The covering placement (centered over the trigger, 8 px off the viewport's sides, opening upward from the lower half, its height cap and first-row handoff) and its UIKit constants are removed; the menu takes the layer's placement on every screen. A mouse open still creates no touch motion (spec:AST-094 FR24), and the docs keep the content-touch-delay wording. Tests: a phone's menu opens at the layer's placement and keeps the caller's (jsdom), and below its trigger without covering it, from the upper and the lower half (real browser). Co-authored-by: Christopher Chedeau <vjeux@users.noreply.github.com>
… detaches The deferred unmount demotes the detached node, and demote asks for a fresh will-change read; the detached node now gets none, so no timer outlives the element (architecture:react-component-runtime INV5). Co-authored-by: Christopher Chedeau <vjeux@users.noreply.github.com>
…edge-zone strip decides at UIKit's 15 px The host's arena environment had no viewSize, so under the real host a screen-edge pan never saw its edge zone while a touch was down and decided at 10 px, while its attach-time touch-action (read through the default environment) had already claimed the axis (spec:AST-094 FR18). Co-authored-by: Christopher Chedeau <vjeux@users.noreply.github.com>
…into the menu placement # Conflicts: # .changeset/gesture-browser-negotiation.md # .changeset/uikit-constants-table.md
There was a problem hiding this comment.
This pull request needs changes before it can be approved. The finger timing in an open DropdownMenu follows current spec:AST-094 FR11, and what remains is listed below:
- The
architecture:layer-runtimeINV13 and Exit copy text is still here, although an engineering owner asked for it to land only through #8186. - In a menu whose rows fit, the selection recognizer claims the finger once its press begins, while FR11 says that press stays unclaimed; leaving it unclaimed, as in a menu that scrolls, fixes it.
- The diff carries seventeen open predecessor pull requests (31,798 authored lines against this change's own 1,905), including AlertDialog's new
presentationprop, so landing them first, or rebasing once they merge, keeps this pull request to its own change.
[Automated review]
| override get claimsTouches(): boolean { | ||
| return !this.scrolls(); | ||
| } |
There was a problem hiding this comment.
In a menu that fits this returns true, so the press claims the finger once it begins (150 ms in, or after 10 px) and the gesture host gives the menu root its blocking touchmove listener. Current spec:AST-094 FR11 now records the ruling on this menu: "A press in a list that no scroll can claim stays unclaimed, which FR19 allows (DEC-14)." Returning false here, as a menu that scrolls already does, matches it; the menu's touch-action: pinch-zoom (useMenuPress FR4) still keeps a one-finger move from panning. The test at menuSelectionRecognizer.test.ts:129-132 and the header comment at :16-21 would change with it.
| - **INV13 — One exit copy per closing layer.** A layer's close plays on at most | ||
| one exit copy, made by `placeLayerExitCopy`: drawn where the layer was drawn, | ||
| inert, hidden from assistive technology, without ids, form names, or form | ||
| owner, shown only in the top layer, and removed by its owner when the close | ||
| settles. Placing a second copy for the same layer removes the first; another | ||
| layer's copy stays. |
There was a problem hiding this comment.
An engineering owner asked for INV13 and its Exit copy section to come out of this pull request, since they belong to #8186; they are still here at this head, with their verified_by, verification, change-coupling and owning-code lines.
| onAction, | ||
| width = 400, | ||
| width: widthProp, | ||
| presentation = 'adaptive', |
There was a problem hiding this comment.
This default moves every existing AlertDialog to the touch alert under a coarse primary pointer. spec:AST-094 FR25 lists the released defaults touch presentations move and AlertDialog's is not one, and the draft AlertDialog record names no presentation. It arrives here from #8083 and #8084; landing those first keeps it out of this pull request.
| onActivate: row => | ||
| activateRef.current(row, syntheticRelease(row, releaseRef.current)), | ||
| })); | ||
| useEffect(() => () => trackerRef.current?.dispose(), []); |
There was a problem hiding this comment.
Minor: architecture:react-component-runtime INV3 asks a new Effect to name the external system it synchronizes; a one-line comment saying this releases the tracker's fade timers at unmount covers it.
|
In a menu whose rows fit, the press still claims the finger once it begins, which the review's second point flags: |
Depends on #8089, #8508, #8110. Review only this PR's change: vjeux/astryx@08a9bc9...eb42283
User need
A finger in an open DropdownMenu lights its row at once, and rows as a long menu scrolls. iOS waits, and scrolls with nothing lit.
Evidence
iOS 26.2 simulator, Safari (blue dot: finger). Resting finger (4× slower): main lights the row on the touch, this PR 152 ms later. Quick drag: main lights rows as it scrolls; this PR lights none.
Android Chromium 158 (emulator, finger injected at 60 Hz): at rest, ~180 ms after main; nothing lit in the drag.
Problem and solution fit
iOS 26.2, 3- and 30-row menus:
That is the list's content-touch delay (150 ms), now under every finger, Android included (AST-094); a mouse keeps the pointer model.
MenuSelectionGestureRecognizerbegins after that delay for a still finger, at the lift for a tap, at once after travel past the slop in a menu that fits; in a scrolling menu, travel fails it and the browser scrolls. A second finger ends it, nothing acting (FR4). It drives the shared tracker (#8110) while the menu shows. Where the primary pointer is fine (FR19, FR24) the menu joins no gesture system; a finger's tap acts once through the press model.Current specification
spec:AST-094FR26–FR30 govern a finger in the menu, its rows taking FR11's cell profile.module:DropdownMenu/useMenuPress(current) points there, unchanged.Public delta and caller burden
useDropdownMenuPress, DropdownMenu: under a finger the trigger opens as the finger presses (AST-094 FR29) #8078) also tracks a finger that lands in the open menu, and itsisOpenstays internal too. DropdownMenu passes it with the touch modes, and its sub-menus take the root's (DropdownMenu: under a finger the trigger opens as the finger presses (AST-094 FR29) #8078);presentation="popover"keeps the pointer model.useMenuPressis unchanged.hooks/menuSelectionRecognizer.ts, imported once by useMenuPress.Design and alternatives
Rejected: a pointer-machine timer (only an unclaimed recognizer leaves scrolling to the browser).
Writing rubric results
R1 narration: none.
R2 research: none.
R3 evidence: none in the record.
R4 rejected alternatives: none.
R5 change references: none.
R6 open questions: none.
Scope
Testing
pnpm vitest runhooks, DropdownMenu, ContextMenu, MoreMenu, Selector, MultiSelector, motion, gestures and utils at this head: 2843 passed, incl. a closed menu adding no listeners (DropdownMenu itself, without a visibility stub), a fine primary pointer joining no gesture system while a finger's tap acts once (it fails if the touch model's gate on the recognizer is removed), and a second finger on another row ending a coarse finger's press with its lit row cleared, in the gesture system's touch stream.pnpm test:interactionDropdownMenuTouch* on that build: Chromium 28/28, WebKit (Playwright image) 23 + 5 skipped (no native pan from scripted touches); the DropdownMenu and MoreMenu a11y specs (Chromium) 152 passed, and fix(DropdownMenu): a cancelled touch clears the row a finger lit, in sub-menus too (spec:AST-094 FR28) #8508's window-blur case runs on the gesture host's fix (fix(gestures): pointer-read fingers get the blur refusal and the pinch rule of the Touch Events path #8309 1be6b6c; MenuPress 18/18 at this head). The edge-autoscroll proof now lifts 12 px above the menu's bottom edge: at 4 px the point sat in the menu's 4 px bottom padding, so once the list reached its end no row was under the lift and nothing was picked (DropdownMenuTouchSelection at this head: Chromium 8/8, WebKit 7 + 1 skipped).pnpm build,pnpm typecheckandCI=true pnpm lintwith no error; CLI isolation and golden reads 47/47; the CI duration guard's and motion node-load tests;check:knowledge,check:changesets.Stack (merge in this order): #8076 and #8298 → #8078 → #8508 and #8186 → #8080; #8446 → #8089; #8083 → #8084 and #8346 → #8110; then this PR.