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📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsContextMenu (@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/DropdownMenu | +2.1KB |
0B |
| core/MoreMenu | +2.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.6KB | 34.6KB | 0B |
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
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.
…llowance under every primary pointer The useMenuPress record's FR9 puts the trigger's held finger under spec:AST-094 FR29 under every primary pointer (FR43), so a finger on a touchscreen laptop may drift 13 pt before the open, as on a phone. The trigger read (pointer: coarse) as its press started and took the long press's 10 px where the pointer was fine. Three cases fail until it does not: a 12 pt drift opens under a fine pointer too, a (12, 12) settle opens there, and nothing reads a pointer query, the trigger's press included.
…lowance under every primary pointer The useMenuPress record's FR9 governs the trigger's held finger by spec:AST-094 FR29 under every primary pointer (FR43). The trigger now hands the long press FR29's 13 px drift allowance whatever the primary pointer, so it no longer reads (pointer: coarse) as its press starts, and the menus drop their coarsePointer import. ContextMenu keeps the long press's own allowable movement. The changeset says the same.
Strict ESLint reported six unused bindings in files this pull request carries: the host's HAPTIC_SWITCH_ATTRIBUTE import, two unused imports in the pan's tests (scrollProbe, ScreenEdgePanGestureRecognizer), the swipe test's drift, the presented-hit-testing test's gestures, and a GestureHost test's before, which now asserts what it was read for: a second finger's moves leave the pan where it was.
…eal angular speed The release velocity of a rotation whose listener sets rotation = 0 on each change, at 2 degrees every 16 ms, read 4.363 rad/s against the 2.182 rad/s of the same stream without a re-base: the test asserted only more than 1.5. It now asserts the 2.18 rad/s (spec:AST-100 FR2), and fails.
The rotation's velocity tracker samples the raw unwrapped angle, which setting rotation does not move, so the setter's shift of the samples read each re-base as extra turn and doubled the release velocity of a listener that re-bases on every change. The setter no longer touches the tracker.
…mates while it runs The proof read the element's animations 230 ms in, after the dip had ended, and asserted only that every animated key is opacity, which an empty list passes: with the cross-fade removed from the plan (the move still landing), the proof still passed. It now reads the keyframe keys right after the call starts and asserts they include opacity and nothing else; the same mutant fails it. Chromium and WebKit pass. (F1's in-flight read from c648034.)
…roller, and its claimed moves are cancelled under pointer events Two comparisons stop at the shadow boundary, each failing a case: - once the host's listener on a recognizer's inner node has seen the finger, isFingerDownIn reads it as outside a light-DOM scroller around that root, so a scroll under it is not the finger's (FR20); - under inputMode 'pointer-events', a claimed pan on a shadow host cancels none of its finger's moves over the inner content, since the finger's target is the inner node and its touches reach the host retargeted (FR19).
…the flat tree isFingerDownIn and the pointer-events finger match compare with containsAlongFlatTree, as the scroll watch already does: a finger on content inside an open shadow root is down in the scroller around that root, and its touches, retargeted to the shadow host, match its pointer finger, so a claimed pan cancels its moves there (spec:AST-094 FR19, FR20).
…achments asks, and only then The host let the last attachment decide whether an element is an action group (useGestureRecognizer always passes the option), and never recomputed it at a detach: a plain attachment after the group's turned the group off, and a group's attachment released left a recognizer that never asked taking presses under a fine pointer. Fails until the host counts the attachments that ask.
…ents asks The host counts the attachments that pass actionGroup: true and keeps the element a group while one remains, recomputed at each detach. An attachment that passes false, as useGestureRecognizer does by default, no longer turns a group off, and a group's last asking attachment taking its leave ends the group.
… that content The presented hit test takes the document's elementFromPoint, which answers a shadow host for anything inside its open root, and compares with Node.contains, which stops at the boundary: a recognizer on a control inside a moving sheet's shadow root gets no finger, and the tap's click goes to the host. Fails until the hit test descends into open roots and compares along the flat tree.
Where a moving element is drawn away from its layout box, the host hit-tests what is drawn under the finger with the document's elementFromPoint, which answers a shadow host for anything inside its open root. The hit test now descends through each open root's own elementFromPoint and compares with the movers along the flat tree, so a recognizer on a control inside a moving sheet's shadow root gets its finger and the tap clicks that control.
…nt lets go The host's touch-action rules sit in an adopted sheet or a style element per document or shadow root, and were never removed, so a page whose last recognizer detached kept them, against spec:AST-094 FR24 (nothing touch-related remains after the last instance unmounts). Fails until a root's rules go with its last marked element.
…th its last marked element The touch-action writer records which elements it marked in each document or shadow root, and when the last one is unmarked (its last recognizer detached) it removes that root's adopted sheet or style element, so nothing touch-related remains after the last instance unmounts (spec:AST-094 FR24). A later mark writes the rules again; an element marked in another root first lets go of the old one.
… the inline style it reads - The motion registry, the host's engine gate and the README name the presented hit test as an engine workaround (spec:AST-013 FR6) with its review condition (re-measure on each iOS release; remove it once iOS WebKit hit-tests a compositor-moved element where it is drawn) and its regression fixture. - While a curve runs the animator holds the value it starts from inline, so presentation.ts and the README no longer say the inline style holds the end.
…core/internal/touch Core ships the internal entry @astryxdesign/core/internal/touch, which re-exports the gesture primitives for Astryx's own packages, and the Lab-import guard fails a Lab import of core outside core's exports (architecture:react-component-runtime INV13). The README and the barrel's header named internal workspace imports and said nothing here is exported; they now name that entry.
…ill needs the element, and the listeners that cancel The release note, the stream's header, the host's comment and the README said the stream is one passive listener per event type and that a document-capture stopPropagation no longer starves the recognizers. Such a stop never hid a listener on the same node, and some subscriptions cancel. They now say what changed: an app's window-capture stopPropagation, or a document-capture stopImmediatePropagation registered before core, no longer hides a touch; a recognizer that must claim still needs the touch to reach its element; the gesture host's touchend, click and contextmenu cancel while a claimer is attached and the page hold's touchmove while it holds; and the stream stays ahead of an app's window-capture listener only until a type's listener is re-added. The README's list of blocking touchmove listeners gains the claimed touch's target and the document while a claiming element moves.
…default it moves
The {stylexOptions} form of astryxStylex now passes StyleX
treeshakeCompensation: true unless the caller sets it. spec:AST-017 FR3
asks the release note to name the old and new default and the option
that restores the old one; it now does.
…er and for the two stops the input stream fixes - A still finger with sub-pixel jitter over a scroll view keeps its tap and scrolls nothing (spec:AST-094 FR18–FR22). - A tap is still recognized under an app's window-capture stopPropagation, and under a document-capture stopImmediatePropagation registered before core's: the recognizers hear touches on the window's capture phase. Chromium and WebKit pass.
…s once Where nothing that claims is attached, the host's touchend listener is passive and cannot cancel the engine's own click after a retargeted tap clicks what is drawn. The shared click guard swallows that late click whatever is attached; this case pins it, as asked on meta#8293.
This pull request sat on meta#8076 and carried its old copies of the harness meta#8045 and meta#8046 landed on main. meta#8360 carries meta#8076 with main's harness, so after the merge this pull request's own diff is the presentation resolver's commits. The one conflict, the gestures README, keeps both sides: this pull request's paragraph on the presentation prop ends the touch presentation section, and meta#8360's migration section follows it.
…esture host's subscriptions The gesture host subscribes no click: the one cancelling click subscriber is the click guard's, added at a window's first arm and kept. The release note, the stream's header and the gestures README listed click with the host's touchend and contextmenu; they now name the guard's own.
… as a dependency meta#8627 is now one commit on main; meta#8360 carries meta#8076, meta#8073 and meta#8446. useMenuPress.ts: the trigger half lives in menuTriggerPress.ts, so it takes meta#8360's trigger changes there. The trigger's holds run on the long-press detector and pass FR29's 13 px drift allowance as each press starts, under every primary pointer (FR43). The hold duration comes from motion/uikit/longPress.ts. MENU_PRESS_TRIGGER_DRIFT_PX is re-exported beside MENU_PRESS_LONG_PRESS_MS. The stray-click window is the shared click guard's, from meta#8360.
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.
Depends on #8078. Review only this PR's change: vjeux/astryx@dcda3d0...f21f4f3
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, which is right for a mouse, whose highlight follows hover (
spec:AST-017FR59). Under the touch model, a finger's on every device unlesspresentation="popover"(spec:AST-094FR43), a cancelled finger's lit row now clears, in the root menu and in its flyouts. A window blur withdraws the finger's gesture: the row clears, nothing runs, and its listeners end at its lift, so none outlives it. A tap on the trigger that the blur interrupts keeps its click, so the menu still opens (a reviewer found the lift swallowing it underpresentation="popover").Expected behavior and authority
spec:AST-094FR28: "A cancelled touch, a second finger, or the window losing focus ends the gesture: nothing acts and the lit row clears."Smallest restoration
Options considered:
Evidence
presentation="popover"keep it lit.presentation="popover") failed before the fix; the tap's click now opens the menu.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 (they fail on #8078's head), and the menu-press accessibility specs in Chromium. A
[fix]changeset records the change. Main'stouch-support-bundletest fails here, and #8671 updates it.Stack (merge in this order): #8360, #8298 and #8627 (any order) → #8078 → this PR.