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.
|
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix works: in Chromium, a drill-in tap's click delivered about 0.7 s late now leaves the sub-menu open, and the new jsdom cases for the late click and the key press fail without the change. Approval waits on an engineering-owner decision about the proposed amendment to module:DropdownMenu/useMenuPress FR5, whose stray-click window currently reads "400 ms, or the next pointerdown": an engineering owner records it by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner), and FR5 and that window row should then state the new rule. The decision should settle:
- the 2 s cap after a touch release, which also swallows a click with no press (
detail1) for 2 s after a finger slide that sends no click; - the new
keydowndisarm, which applies after every release; - whether pen, which the record counts with touch as "a finger", keeps 400 ms.
[Automated review]
PR Analysis Report
The exact-head preview is not available yet. Generated by PR Enrichment workflow | View current CI run |
b353805 to
1a905ea
Compare
github-actions
Bot
added
the
needs:spec-owner-review
There was a problem hiding this comment.
The fix works in Chromium: a drill-in tap's own click delivered 0.7 s late now leaves the sub-menu open, and the new jsdom cases fail without the change. Approval waits on an engineering owner deciding the proposed amendment to module:DropdownMenu/useMenuPress FR5 and its stable stray-click window ("400 ms, or the next pointerdown"): the 2 s touch cap, the keydown disarm, and pen staying at 400 ms although the record counts pen as a finger. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request (the author may do so as an engineering owner); since a new commit invalidates the approval, please first fix the record so the approved head describes the code:
- PR2 still says the swallower's click listener is installed once per document, while
useMenuPress.tsnow adds and removes its listeners at every arm; amend PR2 in the proposal or keep one installation. - The proposed FR5 sentence says an assistive activation after the release still acts, but a click with
detail1 and no press before it is swallowed for 2 s after a finger slide; state that cost. - The window row's "measured about 0.7 s late" is not backed by the pull request's WebKit figure (432 ms), and measurements stay out of records; drop it.
[Automated review]
| | FR2 | While a press is held inside a menu, the highlighted row MUST be the enabled row under the pointer, and nothing MUST be highlighted over a divider, a heading, a disabled row, or outside the menu. A mouse moving with no button down moves the highlight the same way (existing hover-to-focus). | `architecture:interaction-modality` allowed variation for menus; `useMenuPress.test.tsx` | verify | | ||
| | FR3 | A press that began inside a menu and is released outside it MUST act on nothing. Under a mouse the menu MUST then close; under a finger or pen it MUST stay open. | DEC-3; `useMenuPress.test.tsx`, `DropdownMenu.test.tsx`, `Selector.test.tsx` | verify | | ||
| | FR4 | A menu whose rows fit MUST declare `touch-action: pinch-zoom`; one that scrolls MUST declare `touch-action: pan-y pinch-zoom` and `overscroll-behavior: contain`. Neither admits a one-finger pan the menu does not scroll, and both leave a two-finger pinch to the browser, so the page stays zoomable over an open menu. The model MUST treat `pointercancel` and a second pointer as the end of the gesture with nothing acting, and MUST NOT re-implement scrolling. | Browser scroll ownership; WCAG 2.2 SC 1.4.4 under `spec:AST-020` FR1; `DropdownMenu.test.tsx` touch-action case, `useMenuPress.test.tsx`, `MenuPinchZoom.a11y.chromium.spec.ts` | verify | | ||
| | FR5 | The click the browser reports for a gesture this model tracked MUST NOT act on any row; only the release acts. A click with no tracked gesture behind it, or one whose `detail` is 0, MUST act on its target as before. The swallower arms at a tracked release and disarms at the next `pointerdown` or after the stray-click window. Proposed, pending owner review: after a touch's release the window is 2 s, and the next `keydown` also disarms it, so a late click from a busy main thread acts on nothing while a keyboard or assistive activation after the release still acts; a pen's window stays 400 ms. | One gesture, one activation; `useMenuPress.test.tsx` FR5 suite | verify | |
There was a problem hiding this comment.
The proposed sentence says the keydown disarm means "a keyboard or assistive activation after the release still acts", but for up to 2 s after a touch release that sent no click (a finger slide) a click with detail 1 and no press before it is swallowed: the case useMenuPress.test.tsx:332 calls an assistive technology's activation. Please state that cost here, or drop "or assistive".
| | pointer type | `mouse`, `touch`, `pen` | the gesture's own pointer; `touch` and `pen` are "a finger" | from the event | `module:DropdownMenu/useMenuPress` | stable | | ||
| | enabled-row selector | caller string | the rows a release may act on; anything else inside the root highlights nothing | required | `module:DropdownMenu/useMenuPress` | stable | | ||
| | highlight | DOM focus, or a caller callback | one lit row; focus with `preventScroll` in a menu, `aria-activedescendant` in a picker | focus | `module:DropdownMenu/useMenuPress` | stable | | ||
| | stray-click window | 400 ms, or the next `pointerdown`; proposed: 2 s after a touch's release, and the next `keydown` too | how long after a tracked release the browser's click is still swallowed; a touch's click comes when the tap recognizer sends it, which a busy main thread holds back (WebKit, measured about 0.7 s late), while a mouse's or a pen's follows its release at once | `MENU_PRESS_STRAY_CLICK_MS`, `MENU_PRESS_TOUCH_CLICK_MS` | `module:DropdownMenu/useMenuPress` | stable; touch window proposed | |
There was a problem hiding this comment.
"measured about 0.7 s late" is not backed by this pull request's WebKit figure (the click 432 ms after the release), and the writing rubric keeps measurements out of a record (R2). Suggest keeping only the fact: a busy main thread holds the click back.
| // The swallower's listeners are held only while it is armed | ||
| // (react-component-runtime INV5): attached at a tracked release, removed when | ||
| // it disarms. |
There was a problem hiding this comment.
PR2 in useMenuPress.spec.md still reads "The window capture click listener is installed once per document and armed per gesture", and this pull request leaves it unchanged while the listeners are now added and removed at every arm. INV5 allows the old document singleton because PR2 names its lifetime, so either keep one installation or add a proposed PR2 amendment beside the FR5 one.
|
Posted by Cindy's agent on her behalf.
vjeux, could you update the PR to match this? |
github-actions
Bot
removed
the
needs:spec-owner-review
|
Done in 4604a3c, as
|
There was a problem hiding this comment.
Cancelling the tracked finger's touchend is the right fix under spec:AST-094 FR21: with real touch input in Chromium the tap now ends with a cancelled touchend and no click, and the new jsdom cases fail without the change. Two small changes are needed before approval:
- Arm the canceller only where the primary pointer is coarse, since FR19 and FR24 keep a finger on a touchscreen whose primary pointer is fine on the browser's defaults with no extra touch listener (
useMenuPress.ts:610,:622). - Disarm the stray-click swallower once the
touchendis cancelled, because no click follows it, and today a click withdetail1 and no press within 400 ms of a tap is swallowed althoughmodule:DropdownMenu/useMenuPressFR5 says it must act.
[Automated review]
| // Detach before acting: the activation usually closes the menu. | ||
| endGesture(); | ||
| armStrayClick(doc); | ||
| if (native?.pointerType === 'touch') { |
There was a problem hiding this comment.
This arms on every touch release whatever the primary pointer, so on a touchscreen whose primary pointer is fine a finger's tap also loses its click and runs a touch-only listener and timer, which spec:AST-094 FR19 and FR24 rule out there (FR24's one exception is the released module:DropdownMenu/useMenuPress FR9 behaviour). Arm only where the primary pointer is coarse, as FR13 and FR19 decide it, here and at line 622, with a jsdom case for a fine primary pointer.
There was a problem hiding this comment.
Fixed in f0cc114: the canceller arms only where the primary pointer is coarse, read at the release with matchMedia(TOUCH_QUERY) (spec:AST-094 FR13, FR19), here and in the settle branch. Where it is fine, a finger keeps the browser's defaults, with no touch listener or timer (FR24), and its stray click is still swallowed as released. The new jsdom case "where the primary pointer is fine, a finger keeps the browser's defaults…" spies on document.addEventListener and fails if either gate is removed.
| // A touch the browser took for a scroll ends with an uncancelable touchend, | ||
| // and no click follows it anyway. | ||
| if (event.cancelable) { | ||
| event.preventDefault(); |
There was a problem hiding this comment.
No click follows a cancelled touchend, so the swallower armed beside this now waits out its 400 ms and eats a press-less click with detail 1 (a script's, say), which FR5 says must act; before, the tap's own click disarmed it. Calling disarmStrayClick() here after preventDefault() fixes it, and every case in useMenuPress.test.tsx still passes with that change.
There was a problem hiding this comment.
Fixed in f0cc114: once the touchend is cancelled, disarmStrayClick() runs, since no click follows it, so a later click with no press behind it acts (module:DropdownMenu/useMenuPress FR5). The new jsdom case "once the touchend is cancelled, the swallower stands down…" fails without that line.
There was a problem hiding this comment.
Cancelling the tracked finger's touchend fixes the late drill-in click at its source, as spec:AST-094 FR21 requires, and both earlier review points are addressed: the canceller stays off where the primary pointer is fine, and the swallower stands down after a cancelled touchend. Two small changes remain before approval:
- Add a fine-pointer case where the finger lifts outside the menu, since removing the settle branch's coarse-pointer gate (
useMenuPress.ts:642-647) currently leaves every hook and menu test passing, so FR24's verification is not met there. - Scope the changeset to a coarse primary pointer, because a finger on a touch laptop keeps the 400 ms window.
Rebasing onto main will also let the interaction proofs job run; it currently stops because this branch predates its config.
[Automated review]
| expect(onSelect.mock.calls).toEqual([['A'], ['B']]); | ||
| }); | ||
|
|
||
| it('where the primary pointer is fine, a finger keeps the browser\u2019s defaults: no touch listener, no touchend cancelled, its stray click still swallowed (FR19, FR24)', () => { |
There was a problem hiding this comment.
This case releases on a row, so it pins only the act branch's gate: with isCoarsePrimaryPointer(doc) removed from the settle branch (useMenuPress.ts:642-647) all 39 cases here still pass. A fine-pointer case where the finger lifts outside the menu, asserting no touchend listener and an uncancelled touchend, would cover it; spec:AST-094 FR24's verification expects a touch listener on a fine pointer to fail a test.
There was a problem hiding this comment.
Added in e78fa8b: "where the primary pointer is fine, a finger that lifts outside the menu adds no touch listener and its touchend is not cancelled (FR19, FR24)". It goes through the settle branch and fails with that branch's coarse-pointer gate removed (useMenuPress.ts:642-647).
The interaction-proofs job stops because this branch predates its config; that routing is being fixed at its origin (#8457), after which the job re-runs here, so no rebase onto main for it.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] A finger's tap that a DropdownMenu (or any menu on the press model) tracks now cancels that touch's `touchend`, so the browser sends no click and no compatibility mouse events for it (`spec:AST-094` FR21). Before, a busy main thread could hold that click past the 400 ms stray-click window, and after a tap that drilled into a sub-menu it picked the sub-menu row that had come under the finger and closed the menu. A mouse or pen release keeps the 400 ms window. |
There was a problem hiding this comment.
On a touchscreen whose primary pointer is fine, a finger keeps the 400 ms window too (the file header says so at useMenuPress.ts:30-34). Suggest starting with "Where the primary pointer is coarse (a phone or tablet), a finger's tap ...".
There was a problem hiding this comment.
Done in e78fa8b: the changeset now opens "Where the primary pointer is coarse (a phone or tablet), …", and ends with a mouse or pen release, and a finger where the primary pointer is fine (a touch laptop), keeping the 400 ms window.
There was a problem hiding this comment.
This is ready for approval: cancelling a tracked finger's touchend where the primary pointer is coarse removes the late drill-in click at its source, as spec:AST-094 FR21 requires, and in Chromium phone emulation a drill-in tap whose click is held back 450 ms closes the menu on main but leaves the sub-menu open here, since no click is sent. Both earlier review points are addressed: the new fine-pointer lift-outside case fails without the settle branch's coarse-pointer gate, and the changeset is scoped to a coarse primary pointer. Optionally, useMenuPress.doc.mjs could say that on a coarse pointer the browser's click after a tap is no longer sent, rather than swallowed.
[Automated review]
There was a problem hiding this comment.
Changes requested: cancelling a tracked finger's touchend fixes the late drill-in click at its source, as spec:AST-094 FR21 requires, and in Chromium phone emulation a drill-in tap whose click is held back 450 ms closes the menu on main, while here no click is sent and the sub-menu stays open. Since this branch was written, AST-094's FR43 has made gestures attach for every pointer and removed the fine-pointer exception that FR19 and FR24 used to state, so FR21 now also covers a finger on a touchscreen whose primary pointer is fine. Please arm the canceller for every tracked finger release (drop the isCoarsePrimaryPointer condition at useMenuPress.ts:630 and :642-647), turn the two fine-pointer cases at useMenuPress.test.tsx:424 and :443 into cases that expect a cancelled touchend, and drop the fine-pointer clause from the file header and the changeset. If a fine-pointer finger should keep its click, an engineering owner would need to amend AST-094 first.
[Automated review]
| // Detach before acting: the activation usually closes the menu. | ||
| endGesture(); | ||
| armStrayClick(doc); | ||
| if (native?.pointerType === 'touch' && isCoarsePrimaryPointer(doc)) { |
There was a problem hiding this comment.
spec:AST-094 FR43 now attaches gestures for every pointer, and FR19 no longer exempts a finger on a touchscreen whose primary pointer is fine, so FR21's touchend cancel applies there too. Please arm the canceller for every tracked finger release here and at lines 642-647, and drop isCoarsePrimaryPointer with its TOUCH_QUERY import.
| expect(onSelect.mock.calls).toEqual([['A'], ['B']]); | ||
| }); | ||
|
|
||
| it('where the primary pointer is fine, a finger keeps the browser\u2019s defaults: no touch listener, no touchend cancelled, its stray click still swallowed (FR19, FR24)', () => { |
There was a problem hiding this comment.
This case and the one at line 443 pin the fine-pointer exception that FR19 and FR24 no longer state; with the canceller armed for every finger they are the only two cases in the file that fail. Please turn them into cases that expect a cancelled touchend.
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] Where the primary pointer is coarse (a phone or tablet), a finger's tap that a DropdownMenu (or any menu on the press model) tracks now cancels that touch's `touchend`, so the browser sends no click and no compatibility mouse events for it (`spec:AST-094` FR21). Before, a busy main thread could hold that click past the 400 ms stray-click window, and after a tap that drilled into a sub-menu it picked the sub-menu row that had come under the finger and closed the menu. A mouse or pen release, and a finger where the primary pointer is fine (a touch laptop), keep the 400 ms window. |
There was a problem hiding this comment.
Once the canceller arms for every finger, please drop the coarse-pointer opening and "and a finger where the primary pointer is fine (a touch laptop)" so the note matches the behavior.
There was a problem hiding this comment.
This looks ready to approve: a finger's tracked release in a press-model menu now cancels its touchend on every device, as spec:AST-094 FR21 and FR43 require, and the coarse-pointer gate from the previous revision is gone. In Chromium phone emulation, a drill-in tap whose click is held back 450 ms closes the menu on main but leaves the sub-menu open here, where no click is sent, and the two rewritten fine-pointer cases fail against the previous gated hook. No further changes are needed.
[Automated review]
…er picks a row On a busy phone, the browser's click for a tap that opens a sub-menu can arrive after the 400 ms stray-click window, once the sub-menu has put a new row under the finger, and pick that row. The press model now cancels the touchend of a finger whose release it handled, on every device (spec:AST-094 FR21, FR43), so that click is never created. When it cancels one, the stray-click swallower armed for that release stands down, so a later click with no press behind it still acts (module:DropdownMenu/ useMenuPress FR5). The next pointer press, or the stray-click window, ends the wait. Keyboard and assistive-technology activation is unchanged. Tests: useMenuPress's touch-end cases, and the drill-in accessibility case in real Chromium, where a click follows the tap without the change. A [fix] changeset.
0c3fcc6 to
a6d1db9
Compare
There was a problem hiding this comment.
This looks ready to approve: a finger's tracked release in a press-model menu now cancels its touchend on every device, as spec:AST-094 FR21 and FR43 require, so a click a busy page delivers late can no longer pick the row a drill-in put under the finger. In Chromium phone emulation, a drill-in tap whose click is held back 450 ms closes the menu on main but leaves the sub-menu open here, and eight of the eleven new jsdom cases fail against main's hook. Optionally, useMenuPress.doc.mjs could say that the browser's click after a tracked touch is no longer sent, rather than swallowed.
[Automated review]
User impact
On a busy phone, a tap that opens a DropdownMenu sub-menu can also pick a row inside it and close the menu: the browser's click for that tap arrives late, after the sub-menu has put a new row under the finger.
What led to it: a drill-in test with a blocked main thread on the iOS 26.2 simulator.
Problem and solution fit
WebKit sends a tap's click when its tap recognizer runs, which a busy page delays past the 400 ms window in which the menu ignores stray clicks. The press model now cancels the
touchendof a finger whose release it handled, on every device, so that click is never created. Keyboard and assistive-technology activation is unchanged.Expected behavior and authority
spec:AST-094FR21 (current): when a gesture takes a touch, itstouchendis cancelled and no click reaches the page; FR43 makes that hold on every device.module:DropdownMenu/useMenuPressFR5: a tracked gesture's click must not act on any row.Smallest restoration
touchend(chosen): the browser's own way to say the gesture took the touch; one listener inuseMenuPress.ts, no API change.#8360 replaces the stray-click swallower this listener stands down; whichever lands second moves that call to the click guard's disarm.
Evidence
iOS 26.2 simulator, Safari, main thread blocked 400 ms from 30 ms after the tap on "Move to", 6 runs each. On main the late click picks "Folder A" and closes the menu in 3 of 6; with this change no click comes and the sub-menu stays, 6 of 6.
Scope
Testing
With touch emulation in Chromium, on the DropdownMenu "Submenu" story: tap "Move to" in the open menu; the sub-menu opens and stays. Tap a row; it runs once.
On main d73908f, 8 of
useMenuPress's touch-end cases fail and so does the drill-in accessibility case in Chromium (a click follows the tap); all pass here.Local gates at a6d1db9: build, typecheck, lint,
pnpm test:ui(16,457),pnpm test:node(all but #8520's two knowncache.test.mjsfailures), CLI (47), Storybook, the DropdownMenu and Menu accessibility specs in Chromium (229), Prettier, and the knowledge, changeset and repo checks.