Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
PR Analysis Report
Generated by PR Enrichment workflow | View current CI run |
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 fix itself (clearing the row a finger lit when the browser cancels its press, in DropdownMenu and its flyouts) matches spec:AST-094 FR28, but this pull request can't be approved as submitted. Against main its diff is about 24,600 changed lines, because it carries the four open pull requests it is stacked on (#8076, #8298, #8446 and #8078). Please land those first and rebase so this pull request contains only its own commit. While you're there, a test showing that a second finger, and not only pointercancel, clears the lit row would cover the rest of FR28.
[Automated review]
| fireEvent.pointerMove(row('B'), touch()); | ||
| expect(onHighlight).toHaveBeenLastCalledWith(row('B')); | ||
| fireEvent.pointerCancel(row('B'), touch()); | ||
| expect(onHighlight).toHaveBeenLastCalledWith(null); |
There was a problem hiding this comment.
FR28 also clears the lit row for a second finger and when the window loses focus. Both paths reach the same cancel input (useMenuPress.ts:857-867), but the second-pointer case below checks only that nothing acts, so a regression on those paths would still pass. A case that lights B, then lands a second pointer (and one that fires blur on the window) and expects onHighlight(null), would cover them.
|
At 36fd673:
|
There was a problem hiding this comment.
This pull request can't be approved as submitted. Its own fix (a cancelled finger clears the lit row in DropdownMenu and its flyouts) matches spec:AST-094 FR28 and its new tests, including the second-finger case, cover it, but against main the diff is still about 23,300 changed lines because it carries #8076, #8298, #8446 and #8078. Please land those in the stated order and rebase so this pull request contains only its own two commits. While you're there, a window-blur case and a cancel case in the real-Chromium menu test would cover the rest of FR28's verification.
[Automated review]
| expect(onFineHighlight).toHaveBeenLastCalledWith(row('B')); | ||
| }); | ||
|
|
||
| it('where the primary pointer is coarse, a second finger ends the gesture and clears the row the first one lit (spec:AST-094 FR28); where it is fine, the highlight stays', () => { |
There was a problem hiding this comment.
This second-finger case covers FR28's second trigger. The third, the window losing focus, reaches the same cancel input (useMenuPress.ts:850-852) but has no case: lighting B under touchModel="uikit", then firing blur on window, should end with onHighlight(null), and with B kept under the fine-pointer harness.
|
At 6ce4c57: the real-Chromium menu test adds FR28's other two cases, a cancelled touch and the window losing focus, each while a finger has a row lit. In both, the gesture ends with nothing acting, the row no longer lit, and the menu still open. Both fail on #8078's head, where the row stays lit. The blur case also showed a gap: the browser can keep the touch alive past the blur, and that touch's later lift ran the row through its click. A gesture the blur ended now swallows that click (module FR5), and a jsdom case pins it. Main's model has the same gap, and this closes it. |
meta#8078's module FR10 cases moved to the sub-menu pull request, so they leave this line too; the flyout's cancel case keeps this line's gesture-system finger. meta#8508's real-Chromium FR28 cases wait for this line's open animation to settle, as its own touch cases do; the blur case waits for the gesture host's fix.
There was a problem hiding this comment.
This pull request can't be approved as submitted. Its own fix, clearing the row a finger lit in DropdownMenu and its flyouts when the browser takes the touch, matches spec:AST-094 FR28, and the new cancel, second-finger and blur cases cover it, but against main the diff is still about 22,800 changed lines because it carries #8076, #8298, #8446 and #8078: please land those in the stated order and rebase so only this pull request's commits remain. The new blur commit also needs two changes:
- The listener
armStrayClickAtReleaseadds is removed only by that pointer's release, so when the page never sees a mouse press's release after a blur, the next mouse click anywhere is swallowed; dropping it at the nextpointerdownwould bound it. - The post-blur swallow also changes what a mouse does, so
spec:AST-017FR59 asks for a mouse case before and after, or the swallow can be kept to fingers.
[Automated review]
| function armStrayClickAtRelease(doc: Document, pointerId: number): void { | ||
| const onRelease = (event: PointerEvent) => { | ||
| if (event.pointerId !== pointerId) { | ||
| return; | ||
| } | ||
| doc.removeEventListener('pointerup', onRelease, true); | ||
| doc.removeEventListener('pointercancel', onRelease, true); | ||
| if (event.type === 'pointerup') { | ||
| armStrayClick(doc); | ||
| } | ||
| }; | ||
| doc.addEventListener('pointerup', onRelease, true); | ||
| doc.addEventListener('pointercancel', onRelease, true); | ||
| } |
There was a problem hiding this comment.
These listeners come off only when the same pointerId is released. If the page never sees that release (the button comes up after a switch to another tab, say), they stay, and a mouse keeps one pointerId, so the next mouse click anywhere in the document reaches armStrayClick here and is swallowed by onWindowClickCapture: a click with no tracked gesture behind it, which module FR5 says must act. Dropping the listener at the next pointerdown, as the swallower itself disarms, would bound it, and a case where the release never arrives and a later click still acts would pin it.
There was a problem hiding this comment.
Fixed: the wait now also ends at the next pointerdown, as the swallower itself disarms there. A new case checks that when the release never comes, a later click still acts.
| const blurredPointerId = pointerIdRef.current; | ||
| stepRef.current({type: 'cancel'}); | ||
| if (blurredPointerId != null) { | ||
| armStrayClickAtRelease(doc, blurredPointerId); | ||
| } |
There was a problem hiding this comment.
This arms for every pointer type, so after a window blur in the middle of a mouse press, that press's click no longer activates the row it lands on, which it does on main. spec:AST-017 FR59 asks a touch change that alters fine-pointer behavior to show fine-pointer evidence before and after, and every blur case uses a finger. Either a mouse case for this path or arming it only for touch and pen would settle it.
There was a problem hiding this comment.
Fixed: it arms only for a finger or pen now. A mouse press that the blur interrupts keeps its click, as on main, and a case pins that.
There was a problem hiding this comment.
This pull request can't be approved as submitted. Its own fix, clearing the row a finger lit in DropdownMenu and its flyouts when the browser takes the touch, matches spec:AST-094 FR28, and the window-blur change now keeps no listener past the gesture, but against main the diff is still about 22,700 authored lines because it carries #8076, #8298, #8446 and #8078. Please land those in the stated order and rebase so only this pull request's own commits remain; the carried copy of #8076 also predates main's rule that Core and Lab reach the touch modules through @astryxdesign/core/internal/touch, so it should not land through this branch.
[Automated review]
|
Thanks. This pull request's own change is the fix at vjeux/astryx@8014a27...8c2ce13. The rest lands first with #8076, #8298, #8446, #8627 and #8078. The carried #8076 is a copy from before |
There was a problem hiding this comment.
This pull request can't be approved as submitted. Its own fix, clearing the row a finger lit in DropdownMenu and its flyouts when the browser takes the touch, matches spec:AST-094 FR28 and its tests cover the cancel, second-finger and blur paths, but against main the diff is still about 19,500 authored lines because it carries #8078 and the pull requests it builds on. Two changes are needed:
- Land #8076, #8298 and #8446, then #8627 and #8078, and rebase so only this pull request's own commits remain; the carried copy of #8076 is also missing that pull request's latest gesture-host fix.
- Add a
@astryxdesign/core[fix]changeset, since the blur change also stops a finger's post-blur click from acting on released surfaces such as ContextMenu, Selector and the publicuseMenuPresshook (spec:AST-017FR7).
[Automated review]
There was a problem hiding this comment.
Changes requested: the spec:AST-094 FR28 fix itself matches the spec, but the branch still carries five open pull requests (#8078, #8627, #8298, #8446 and an older state of #8076), 18,536 of its 19,069 authored lines.
- Land those first and rebase this pull request to its 9-file change; the carried #8076 is already one commit behind its own head.
- Add a
[fix]changeset for@astryxdesign/core, since a finger's click after a window blur now acts on nothing on every menu, including ContextMenu, Selector and the publicuseMenuPress.
[Automated review]
| // these listeners until its release, which acts on nothing and whose | ||
| // click is swallowed (`spec:AST-094` FR28, module FR5). A mouse press | ||
| // ends as a cancel and keeps its click (`spec:AST-017` FR59). | ||
| stepRef.current({type: 'withdraw'}); |
There was a problem hiding this comment.
This withdrawal is not gated on the touch model, so it changes released behavior on every press-model surface: on ContextMenu, Selector's listbox, the sheet's action list, the public useMenuPress and a DropdownMenu pinned to presentation="popover", a finger's click after a window blur no longer acts, which it does on main. Please add a [fix] changeset for @astryxdesign/core that names this and the touch-mode clear (spec:AST-017 FR7).
There was a problem hiding this comment.
Changes requested: the spec:AST-094 FR28 fix itself matches the spec, but the branch still carries five open pull requests (#8078, #8627, #8298, #8446 and #8076, the last one commit behind its own head), all but 533 of its 19,241 authored lines.
- Land those first and rebase this pull request to its 9-file change.
- Add a
[fix]changeset for@astryxdesign/core: after a window blur, a finger's click no longer acts on any menu, including ContextMenu, Selector and the publicuseMenuPress. - A blur during a finger's tap on a pointer-model trigger (for example under
presentation="popover") now swallows the tap's click, so the menu no longer opens; settling a withdrawn trigger press withstray: falsewould keep FR29's open on a tap.
[Automated review]
| case 'withdraw': | ||
| return withdraw<T>(gesture.pointerType, null); |
There was a problem hiding this comment.
A finger held on a trigger under the pointer model (presentation="popover", a fine primary pointer, or the public useMenuPress with a trigger) is withdrawn here when the window blurs, and its lift then settles with stray: true (line 277). That arms the window's click swallower, so the browser's click for the tap never reaches the trigger and the menu does not open. An uninterrupted tap settles with stray: false (lines 340-342) precisely so that click passes, and on main the blur's cancel let it through. spec:AST-094 FR29 keeps “today's open on a tap” under presentation="popover", and the module's FR9 does the same for a finger where the primary pointer is fine. Keeping the press's origin in the withdrawn state and settling a withdrawn trigger press with stray: false would restore it; a case beside the one at menuPressGesture.test.ts:445 would pin it.
There was a problem hiding this comment.
Changes requested. The cancel-and-blur fix itself matches spec:AST-094 FR28 for the anchored menu and its flyouts, and a finger's lift after a window blur now acts on nothing, as the press model requires. Two things need to change before it can merge:
- Land #8076, #8298, #8360, #8446, #8627 and #8078 first, then merge
mainso this diff is only the 11-file fix; as submitted it carries 30,878 authored lines of those pull requests. - FR28 grants no
presentation="popover"exemption: drop thetouchModel === 'uikit'condition from both clears inuseMenuPress.ts, flip the popover assertion inDropdownMenuSubMenu.test.tsx, cover the bottom-sheet list, and make the Changeset name the surfaces that clear.
[Automated review]
| if ( | ||
| effect.clear === true && | ||
| optionsRef.current.touchModel === 'uikit' | ||
| ) { | ||
| highlight(null); | ||
| } |
There was a problem hiding this comment.
This check keeps FR28's clear from every surface that is not 'uikit': a DropdownMenu pinned to presentation="popover" (and its flyouts) and the bottom sheet's action list keep a cancelled finger's row lit. spec:AST-094 FR28 applies to every press-model surface under every pointer, and presentation="popover" keeps only FR29's tap open, FR32's outside dismissal and FR33's ContextMenu menu. The machine already limits clear to a finger or pen, so dropping the touchModel check here and in the withdraw case above keeps a mouse's highlight as spec:AST-017 FR59 asks.
| for (const coarse of [true, false]) { | ||
| stubPrimaryPointer(coarse); | ||
| expect(await litAfterCancel(), `coarse=${coarse}`).toBe(false); | ||
| expect(await litAfterCancel('popover'), `coarse=${coarse}`).toBe(true); |
There was a problem hiding this comment.
FR28's verification row says "a lit row after a cancel fails"; this assertion pins the lit row under presentation="popover", which FR28 does not exempt. It should expect false once the clear reaches the pinned menu.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] After the window loses focus during a finger's press, lifting that finger no longer acts on any menu (DropdownMenu, MoreMenu, ContextMenu, Selector, and menus built on the public `useMenuPress`), and a tap on a menu's trigger still opens it. Under a finger, a DropdownMenu or MoreMenu row lit by a press that is cancelled (a scroll, a second finger, the window losing focus) now clears, as on iOS (`spec:AST-094` FR28); a mouse keeps its highlight. |
There was a problem hiding this comment.
The second sentence tells callers that a DropdownMenu or MoreMenu row now clears, but under presentation="popover" and in the bottom-sheet presentation it still stays lit. Name the surfaces that clear once the gate is settled.
… moves (spec:AST-094 FR29, FR43) Under a finger or pen, on every device, a DropdownMenu or MoreMenu opened under a resting finger picked the row beneath it once the press had lasted 300 ms. The press machine's open phase now takes UIMenu's handoff: until the finger is 10 px, in a straight line, from the first position the open menu sees, nothing lights and a lift acts on nothing; then it slides and picks as usual. A mouse keeps the settle rule, and presentation="popover" pins the pointer model. - menuPressGesture: the handoff state and transitions, on internal types; the released menuPressStep keeps its public shapes. - useMenuPress: the internal useDropdownMenuPress entry takes a touch model; the public hook is unchanged. - menuPressValues: UIKit's handoff distance, copied from the constants table so a menu bundles the number, not the table. - DropdownMenu and MoreMenu run the touch model unless presentation is "popover"; docs and a changeset say so. - A real-browser spec (DropdownMenuTouchHandoff); core declares the interaction harness it imports.
f21f4f3 to
b53115c
Compare
github-actions
Bot
removed
the
needs:design-review
There was a problem hiding this comment.
Changes requested. The fix itself, clearing a finger's lit row on a cancel, a second finger or a window blur on every menu surface, matches spec:AST-094 FR28, and its new cases fail without it.
- Land #8627 and #8078 first and merge
main, or rebase the fix alone, because the branch carries both open pull requests: 3,257 authored lines around this pull request's 633. - In #8078, the public
useMenuPresskeeps the released 500 ms hold whenlongPressDelayMsis unset, whichspec:AST-094FR25 moves to FR29's timing, so give the hook FR29's timing there unless an engineering owner amends FR25. - In #8078,
isInScrollViewinmenuTriggerPress.tsre-walks the ancestors with its own overflow and fixed-bar rules after callingcreateNativeScrollProbe(...).isInsideScroller, which already owns both, so return the probe's answer and delete the walk.
[Automated review]
| // `touchModel` does not reach the model. | ||
| const {touchModel: _touchModel, ...publicOptions} = | ||
| options as DropdownMenuPressOptions; | ||
| return useDropdownMenuPress({...publicOptions, touchModel: 'pointer'}); |
There was a problem hiding this comment.
spec:AST-094 FR25 moves this hook's released 500 ms longPressDelayMs default to FR29's timing ("which a caller keeps by passing it"), and the module's long-press-delay row says the same. Forcing 'pointer' here keeps 500 ms and the settle rule for every caller that leaves longPressDelayMs unset. Taking 'uikit' when longPressDelayMs is unset, and 'pointer' when a caller passes it, would match FR25; the doc default, the two cases that pin 500 ms and the menu-press-handoff Changeset would follow.
| const doc = control.ownerDocument; | ||
| const view = doc.defaultView; | ||
| const root = doc.scrollingElement ?? doc.documentElement; | ||
| for ( | ||
| let node = control.parentElement; | ||
| node != null && node !== root && node !== doc.body; | ||
| node = node.parentElement | ||
| ) { | ||
| const style = view?.getComputedStyle(node); | ||
| const scrolls = | ||
| style != null && | ||
| ((/(auto|scroll)/.test(style.overflowY) && | ||
| node.scrollHeight > node.clientHeight) || | ||
| (/(auto|scroll)/.test(style.overflowX) && | ||
| node.scrollWidth > node.clientWidth)); | ||
| if (scrolls) { | ||
| return true; | ||
| } | ||
| if (style?.position === 'fixed') { | ||
| return false; | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
createNativeScrollProbe(control).isInsideScroller already applies the overflow-and-extent test and drops scrollers beyond a fixed ancestor (gestures/nativeScroll.ts). With this walk removed and the probe's answer returned, all 274 cases in useMenuPress.test.tsx and DropdownMenu.test.tsx pass, the fixed-bar, sticky and scroll-view cases included. Returning the probe's answer keeps one scroll-view rule.
| // A trigger in a fixed bar opens as the finger presses, and the page | ||
| // holds still under it as a bar's does in UIKit. The finger's compatibility mouse events must not follow it | ||
| // onto the trigger: their mousedown would take focus back from the | ||
| // menu. |
There was a problem hiding this comment.
Non-blocking: this comment holds two topics and a 116-column line. Keeping only the reason reads: "The finger's compatibility mousedown would take focus back from the menu. Cancel it."
| export type MenuPressPointerType = 'mouse' | 'touch' | 'pen'; | ||
|
|
||
| export type MenuPressGesture<T> = | ||
| /** @internal The machine's own shape, with the finger's handoff; not public contract. */ |
There was a problem hiding this comment.
Non-blocking: these internal types now also carry the withdrawn phase and the settle clear, so "with the finger's handoff" no longer describes them; @internal Not public contract. on each is enough.
… carry positions The released menuPressStep hands its events to the internal transition, which starts the finger's handoff when a move carries x and y. The public gesture drops the handoff origin, so a caller whose events carried positions got no pick from the opening finger. menuPressStep now strips the positions first, and a test whose events carry them pins it. Three comments are reworded.
b53115c to
a2f76fe
Compare
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
Changes requested. The fix itself, clearing a finger's lit row on a cancel, a second finger or a window blur on every menu surface, matches spec:AST-094 FR28, and its new cases fail without it.
- Land #8627 and #8078 first and merge
main, or rebase the fix alone, because the branch carries both open pull requests: 3,340 authored lines around this pull request's 644. - In #8078, the public
useMenuPressstill waits the released 500 ms whenlongPressDelayMsis unset (useMenuPress.ts:301-307), whichspec:AST-094FR25 moves to FR29's timing, so give the hook FR29's timing there unless an engineering owner amends FR25 and the module record.
Non-blocking: the FR8 verification row in DropdownMenu.spec.md still describes the 500 ms gap the branch removes.
[Automated review]
| // `touchModel` does not reach the model. | ||
| const {touchModel: _touchModel, ...publicOptions} = | ||
| options as DropdownMenuPressOptions; | ||
| return useDropdownMenuPress({...publicOptions, touchModel: 'pointer'}); |
There was a problem hiding this comment.
spec:AST-094 FR25 moves this hook's released 500 ms longPressDelayMs default to FR29's timing ("which a caller keeps by passing it"), and the module's long-press-delay row reads "unset, as spec:AST-094 FR29 states (none in a fixed bar; 150 ms in a scroll view)". Forcing 'pointer' here keeps 500 ms and the settle rule for every caller that leaves longPressDelayMs unset, Breadcrumbs, NavMenu, SideNav, TabList and TopNav included. Use FR29's timing when longPressDelayMs is unset and keep the set value when a caller passes it; then update the doc default, the three cases that pin 500 ms and the handoff Changeset.
| | FR5 | `DropdownMenu.test.tsx` press-model cases ("a finger that lands on one row and lifts on another acts on the second, once", "a mouse released outside closes the menu; a finger released outside leaves it open", "marks the menu as carrying the press model and owns touch scrolling"); `MenuPress.a11y.chromium.spec.ts`; the module record's own map | Finger and mouse release; scrolling menu | A row that acts on press-down, a mouse release outside that keeps the menu, or a missing press marker fails. | `audit:DropdownMenu/behavior` | | ||
| | FR6 | `DropdownMenu.test.tsx` focus-return cases ("restores focus to the trigger after keyboard dismissal", "returns focus to the trigger after pointer dismissal without a focus ring", "leaves focus on the control a press outside landed on") and the "focus return after a pointer pick" and "a trigger the caller renders" cases | Keyboard and pointer close; Button and custom trigger | A lost focus return, a ring after a pointer close, no ring after a keyboard close, or focus pulled off a control pressed outside fails. | `audit:DropdownMenu/accessibility` | | ||
| | FR7 | `DropdownMenu.test.tsx` menuMaxHeight cases | A cap above 300px; viewport bound | A menu clipped to 300px under a raised cap, or one that escapes the viewport, fails. Evidence gap: no test asserts that the touch sheet ignores the cap. | `audit:DropdownMenu/layout` | | ||
| | FR8 | `DropdownMenu.test.tsx` press-model trigger cases ("a mouse press on the trigger opens the menu and a drag-release acts on the row under it", "the opening release before the settle time acts on nothing and the menu stays", "pressing the trigger of an open menu closes it and does not reopen it in the same gesture", "under a coarse pointer a finger opens the menu as it presses the trigger (no scroll view), and a slide picks", "a tap on the trigger still opens through its click") | Mouse press-open; held finger; tap | A mouse that opens only on click, an opening release that acts early, or a re-open in the closing gesture fails. The held-touch case asserts the current 500 ms hold, the FR8 gap pending conformance. | `audit:DropdownMenu/behavior` | |
There was a problem hiding this comment.
Non-blocking: this row still ends with "The held-touch case asserts the current 500 ms hold, the FR8 gap pending conformance", but this branch removes the FR8 gap and renames the case. Drop that sentence.
| // The long-press detector of the system: the same hook ContextMenu spreads | ||
| // onto its trigger, driven here from the pointer stream this model already | ||
| // observes. Its touch handlers go unused; the hold duration, the movement | ||
| // tolerance and the timer are what matter, and they have one home. Inside | ||
| // a scroll view the touch model waits for the content-touch delay | ||
| // (`scroll.delaysContentTouchesMs`), so a scroll can claim the touch first. | ||
| // The press picks its detector when it lands. | ||
| // With `touchModel: 'uikit'`, only a trigger inside a scroll view waits: | ||
| // the content-touch delay, within the drift allowance. One in a fixed bar | ||
| // opens as the finger presses, so its detector never runs. |
There was a problem hiding this comment.
Non-blocking (INV29): this comment narrates the detector's reuse and covers three topics. Keeping only the reason reads: "Inside a scroll view, wait for the content-touch delay so a scroll can claim the touch first. A trigger in a fixed bar opens on the press and starts no hold."
…resses (AST-094 FR29) Under a finger or pen, on every device, a DropdownMenu or MoreMenu trigger in a fixed bar opens the menu as the finger presses, as UIMenu's pull-down button does. Inside a scroll view it opens once the 150 ms content-touch delay has passed with no scroll, within a 13 px drift allowance. The opening finger then picks only after the handoff distance. A mouse is unchanged, and presentation="popover" keeps the pointer model. - menuTriggerPress.ts: the trigger half of the press model (the press-down open, the held finger, the scroll-view wait), reached only by menus with a trigger. - useMenuPress: the trigger-less callers inside core's own menus (sub-menus, ContextMenu, Selector's list and the sheet's action list) take the internal useMenuSurfacePress, so they do not bundle the trigger half. - menuPressValues: UIKit's hold-open slop and content-touch delay, copied from the tables with drift tests. - A real-browser spec (DropdownMenuTouchPress) and an accessibility case. - The touch-support bundle check names the eight imports that now carry the trigger half's scroll-view wait.
…every menu surface Under a finger, on every device and every menu surface, a press that the browser takes (pointercancel, a second finger, a scroll) now clears the row the finger lit, as UIKit clears it when the list starts scrolling. That holds under any presentation, in sub-menus and in the bottom sheet's action list. When the window loses focus under a finger, its gesture is withdrawn: it stays tracked to its release, which acts on nothing and whose click is swallowed, and any new press ends it. A mouse keeps its highlight and its click. - menuPressGesture: the withdraw input and withdrawn phase, and the clear flag on a finger's cancel. - useMenuPress: both clears follow the machine's clear flag, which only a finger or pen sets. - MenuBottomSheetActionList: its list takes focus (tabIndex -1) when no row is lit, as DropdownMenu's own menu does, so a cleared row loses focus. - Tests: the machine, hook, sub-menu and sheet-list cases, and the accessibility case in real Chromium. A [fix] changeset.
a2f76fe to
6341ca0
Compare
There was a problem hiding this comment.
Changes requested. The fix itself, clearing a finger's lit row on a cancel, a second finger or a window blur on every menu surface and keeping a blurred finger's lift from acting, matches spec:AST-094 FR28, and its new cases fail without it.
- Land #8627 and #8078 first and merge
main, or rebase the fix alone, because the branch carries both open pull requests: 3,349 authored lines around this pull request's 644. - The carried #8078 code pins the public
useMenuPressto the released 500 ms hold whenlongPressDelayMsis unset (useMenuPress.ts:307-313), whichspec:AST-094FR25 moves to FR29's timing; #8531 makes that change but is not in this branch, so land it with #8078 before this fix or drop the carried commits.
Non-blocking: the useMenuSurfacePress comment still places the scroll probe in menuTriggerPress.ts, though it now lives in menuScrollView.ts.
[Automated review]
| // `touchModel` does not reach the model. | ||
| const {touchModel: _touchModel, ...publicOptions} = | ||
| options as DropdownMenuPressOptions; | ||
| return useDropdownMenuPress({...publicOptions, touchModel: 'pointer'}); |
There was a problem hiding this comment.
Carried from #8078: with longPressDelayMs unset, the public hook stays on the released 500 ms hold and settle rule, while spec:AST-094 FR25 moves that default to FR29's timing ("a caller keeps by passing it"). #8531 makes that change outside this branch; land it with #8078 before this fix, or drop the carried commits.
| * context menu, a Selector's list or a sheet's action list. It is the menu | ||
| * half alone, so the trigger half's long-press detectors and scroll probe |
There was a problem hiding this comment.
Non-blocking: the scroll probe now lives in menuScrollView.ts, which DropdownMenu passes in, so this sentence points at the wrong file. A shorter version that keeps the reason: "The menu half alone: a menu that never presses a trigger does not import the trigger half (menuTriggerPress.ts)."
Re-applied onto current main, stacked on #8078. It merges after #8531, a sibling on #8078 that moves the public hook's unset default (
spec:AST-094FR25).User impact
Under a finger, on any device, when a finger that lit a row in an open DropdownMenu loses its press (the page scrolls, a second finger lands, or the window loses focus), the row stays lit and reads as a pending pick. iOS clears it, and nothing runs.
What led to it: the UIKit comparison behind
spec:AST-094FR28; a reviewer asked to split this fix out of #8078.Problem and solution fit
The press model ended a cancelled gesture without acting but kept its highlight. That is right for a mouse, whose highlight follows hover (
spec:AST-017FR59). A cancelled finger's lit row now clears on every device (spec:AST-094FR43) and every menu surface: any presentation, sub-menus, and the bottom sheet's action list. A window blur withdraws the finger's gesture: the row clears, nothing runs, and its listeners end at its lift. A tap on the trigger that the blur interrupts keeps its click, so the menu still opens.Expected behavior and authority
spec:AST-094FR28 (current): "A cancelled touch, a second finger, or the window losing focus ends the gesture: nothing acts and the lit row clears." No record changes.Smallest restoration
Evidence
presentation="popover"too and in the bottom sheet's action list; a mouse keeps it lit.presentation="popover".Scope
Testing
On the DropdownMenu "Default" story, with touch emulation:
presentation="popover", press the trigger, switch windows, come back and lift: the menu opens.Automated: the cancel, second-finger and blur unit tests and the sub-menu cases fail with #8078's press model and menus restored, and pass here; so do the menu-press accessibility cases in Chromium. A
[fix]changeset records the change.Local gates at 6341ca0: the menu units pass (1,572 of 1,572) and the core type check; with #8078's press model restored 12 cases fail. The full gates are running.
Stack: #8627 → #8078 → this PR. Review only this PR's change: vjeux/astryx@4ad7e96...6341ca0