Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
There was a problem hiding this comment.
This pull request can't be approved yet: Popover's touch presentation rests only on draft spec:AST-094, added in this stack, so an engineering owner needs to approve that spec for this head or name the record that should govern it, settling FR4 (touch springs bypass the theme's motion tokens), FR7 (touch exits animate) and FR13 (the coarse-pointer resolver). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Separately, the diff against main is the whole unmerged stack (about 30,500 changed lines, including the gesture, motion, harness and DropdownMenu touchOpen changes), so please rebase once the predecessors land, so that it contains only the Popover commit.
- The two new Popover Effects (
Popover.tsx:428,usePopoverSurfaceMotion.ts:88) should each say which external system they synchronize. Popover.doc.mjsand the changeset say "under a finger", but the motion follows the device's coarse primary pointer, so a mouse on a tablet gets it and a finger on a touchscreen laptop does not.
[Automated review]
| // it, as a UIKit popover (usePopoverSurfaceMotion.ts); a mouse keeps the | ||
| // layer's entrance. | ||
| const {contentRef: surfaceRef} = popover; | ||
| useIsomorphicLayoutEffect(() => { |
There was a problem hiding this comment.
architecture:react-component-runtime INV3 asks every new Effect to name the external system it synchronizes. This one only refreshes presentSurfaceRef for the show handler, and the comment above it describes the motion instead. Either say what it keeps in sync, or assign the ref without an Effect.
There was a problem hiding this comment.
Fixed in 98eefa0: the comment above the Effect now names what it synchronizes. It keeps the layer's show handler, an event callback outside React, in step with the latest presentation call and the anchor element it reads, and it has nothing to clean up.
| return motion; | ||
| }, []); | ||
|
|
||
| useEffect( |
There was a problem hiding this comment.
This cleanup-only Effect releases the popover element's beforetoggle listener and its compositor motion (and any exit copy). A one-line comment naming that resource would satisfy INV3 of architecture:react-component-runtime.
There was a problem hiding this comment.
Fixed in 98eefa0: the cleanup Effect now says what it releases when the component unmounts: the popover element's beforetoggle listener, its compositor animations and any exit copy still on screen.
| usage: { | ||
| description: | ||
| 'A click-triggered overlay anchored to a button or trigger element. Use it for secondary actions, inline confirmations, or supplementary information that does not warrant a full dialog. For hover previews use HoverCard, for brief helper text use Tooltip.', | ||
| 'A click-triggered overlay anchored to a button or trigger element. Use it for secondary actions, inline confirmations, or supplementary information that does not warrant a full dialog. For hover previews use HoverCard, for brief helper text use Tooltip. Under a finger it is presented like a popover on iOS: it grows out of its trigger on UIKit\u2019s spring and shrinks back into it on dismissal; with reduced motion it fades on the same springs.', |
There was a problem hiding this comment.
"Under a finger" describes a per-input switch, but the code decides by the device: readTouchPresentation(...).isTouch is (pointer: coarse) (usePopoverSurfaceMotion.ts:101-103). A mouse on a tablet gets this motion and a finger on a touchscreen laptop does not. Suggest "On a device whose primary pointer is coarse (phones and tablets)…"; the same applies to the zh and dense copies and the changeset.
There was a problem hiding this comment.
Fixed in 98eefa0: the docs (all three copies) and the changeset now say "On an iPhone or iPad (iOS, whose primary pointer is coarse)", which matches the resolver's iOS idiom: WebKit on iOS with a coarse primary pointer.
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsMultiSelector (@astryxdesign/core) · View in Storybook
Popover (@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/BottomSheet | +15.1KB |
+2B |
| core/ChatLayout | +16.1KB |
+39B |
| core/DateInput | +14.1KB |
+3B |
| core/DateTimeInput | +14.2KB |
+3B |
| core/Popover | +12.8KB |
+4B |
| core/PowerSearch | +13.9KB |
+4B |
| core/TimeInput | +14.1KB |
+4B |
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 | +26B |
Accessibility Audit
Status: 2 accessibility violation(s) found — 2 serious.
ChatComposer - 1 issue(s)
- 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
- Rule:
color-contrast· Affects 1/19 stories · Learn more - WCAG: 1.4.3 (Level AA)
- Rule:
ChatComposerInput - 1 issue(s)
- 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
- Rule:
color-contrast· Affects 1/18 stories · Learn more - WCAG: 1.4.3 (Level AA)
- Rule:
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 | 440,175 |
| Chat | Full AI Chat | probe | dark | 438,105 |
| AppShell | With Mobile Nav | probe | light | 217,010 |
| AppShell | With Mobile Nav | probe | dark | 216,960 |
| Chat | Empty State | probe | light | 122,185 |
| Chat | Empty State | probe | dark | 120,076 |
| 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 |
| 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
e370bdb to
7ffbca4
Compare
There was a problem hiding this comment.
This pull request can't be approved yet: Popover's iOS touch presentation rests only on draft spec:AST-094, added in this stack, so an engineering owner needs to approve that record for this head or name the one that should govern it, settling the iOS-only gate the code applies (FR13 as drafted keys touch behaviour on any coarse pointer), FR4 (touch springs bypass the theme's motion tokens) and FR7 (touch exits animate). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Separately, the diff against main is the whole unmerged stack (about 31,000 authored lines), so please rebase once the predecessors land so that it contains only the Popover commit.
- The
useMenuPressrecord still gives a 500 ms hold-open while the code now defaults to 150 ms, and it does not list the new publiccontinueGesture. - The two new Popover Effects (
Popover.tsx:429,usePopoverSurfaceMotion.ts:89) should each say which external system they synchronize.
[Automated review]
| @@ -140,6 +140,7 @@ Consumer migration instructions belong in consumer docs and release notes. | |||
| | stray-click window | 400 ms, or the next `pointerdown` | how long after a tracked release the browser's click is still swallowed | `MENU_PRESS_STRAY_CLICK_MS` | `module:DropdownMenu/useMenuPress` | stable | | |||
| | settle time | 300 ms | proposed: how long an opening press must last before its release may act | `MENU_PRESS_SETTLE_MS` | `module:DropdownMenu/useMenuPress` | proposed | | |||
| | long-press delay | 500 ms | proposed: how long a finger rests on the trigger before the menu opens under it | `longPressDelayMs` | `module:DropdownMenu/useMenuPress` | proposed | | |||
There was a problem hiding this comment.
This row still gives 500 ms, but MENU_PRESS_LONG_PRESS_MS now reads menu.holdOpenMs (150, motion/uikit/menu.ts:43) and useMenuPress.doc.mjs documents a default of 150. The public-contract row also omits the new continueGesture and MenuPressContinuation. Please make the record match the code, or keep the 500 ms default, in the pull request that owns this change.
There was a problem hiding this comment.
This record comes from the DropdownMenu press change below this one in the stack, and that change is being brought in line with the code (the 150 ms hold-open and continueGesture). Nothing to change in this pull request.
| thresholds its UIKit counterpart uses: 0.5 s and 10 pt for a plain long | ||
| press, the measured values for context menus and menus. A rubber band MUST | ||
| use the coefficient its use measures. | ||
| - **FR13 — One touch-presentation resolver.** Whether a component shows its |
There was a problem hiding this comment.
FR13 keys touch behaviours on a coarse pointer, but the code in this stack also gates Popover's and DropdownMenu's motion on iOS WebKit (IOS_SUPPORTS_QUERY in utils/touchPresentation.ts, cited as "D-01", which no record defines). If iOS only is intended, the governing record needs to say so; the Non-goals leave that per-component decision to each component's record.
There was a problem hiding this comment.
7ffbca4 to
98eefa0
Compare
There was a problem hiding this comment.
This pull request can't be approved yet: Popover's iOS touch presentation rests only on draft spec:AST-094, added in this stack, so an engineering owner needs to choose the gate for UIKit motion (iOS only, as Popover does, or every coarse pointer, as FR13 is drafted and DropdownMenu does) and approve that record for this head or name the one that should govern it, including FR4 (touch springs bypass the theme's motion tokens) and FR7 (touch exits animate). An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request; the author may do so if they are an engineering owner. Separately, the diff against main is still the whole unmerged stack (about 31,000 authored lines), so please rebase once the predecessors land so that it contains only the Popover commit.
- Under Reduce Motion the Popover and touch-menu fades run 532–588 ms on springs, while
design:reduced-motionDR2 onmainnow requires an opacity-only fade of 200 ms or less. - The touch resolver's
(width < 744px)compact edge contradictsspec:AST-012FR9 onmain(compact below themdpoint, 768 by default), which also moves the adaptive bottom-sheet boundary. - The
useMenuPressrecord still gives a 500 ms hold-open while the code defaults to 150 ms, and it does not listcontinueGesture.
[Automated review]
| export const TOUCH_QUERY = '(pointer: coarse)'; | ||
|
|
||
| /** The viewport is compact: layout behaviours. */ | ||
| export const COMPACT_QUERY = '(width < 744px), (height < 500px)'; |
There was a problem hiding this comment.
spec:AST-012 FR9 on main defines compact as a viewport width below the effective md point (768 by default) or a height below 500, and names the web touch presentation resolver as one of its users, with no size numbers of its own. With (width < 744px) a coarse-pointer screen 744–767 px wide (an iPad mini in portrait) is not compact here but is under FR9, and the adaptive sheet query built on this constant disagrees with it too. Please take the width edge from the md point.
| springs.popoverPresent, | ||
| velocity, | ||
| ), | ||
| {crossfade: springs.popoverPresent}, |
There was a problem hiding this comment.
Under Reduce Motion this cross-fade follows springs.popoverPresent, so the fade-in lasts the spring's settle time, 532 ms (549 ms for the dismiss below). design:reduced-motion DR2 on main requires a touch entrance or exit to change opacity only and finish in 200 ms or less, and its verification fails on a spring. A timed fade of 200 ms or less would conform.
| } | ||
|
|
||
| /** Reduce Motion: a cross-fade on the menu's own spring, as UIKit's. */ | ||
| const REDUCED_MOTION = {crossfade: springs.menuReducedMotion} as const; |
There was a problem hiding this comment.
The same DR2 issue for the touch menu: a cross-fade on springs.menuReducedMotion runs about 588 ms, and it applies on every coarse pointer, not only on iOS.
|
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.
|
98eefa0 to
c8c4113
Compare
…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.
…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.
BottomSheet.test.tsx's imports from both sides are kept; the search goldens are regenerated after the remaining merges.
… guide The keyboard layout guide (this pull request) reads where focus moves to tell a keyboard that comes up from one that stays: meta#8559 removed the store's focus reports because nothing there read them, so they return here with their first reader. One pair of capture listeners on the document, attached while anything subscribes, reports each focusin and focusout with the current height; BottomSheet's reveal still acts on viewport reports only.
Conflicts were in this stack's own keyboard files: the browser-support sentence keeps ChatLayout's composer and takes meta#8559's page wording, BottomSheet's test imports follow the sheetKeyboard split, and the keyboard notes keep both the sheet's and ChatLayout's Chrome 124 findings. The search goldens are regenerated after the merge.
…op-level page The composer's tap route, its scroll-free focus and Done's park attach only where the keyboard is measurable, which excludes a page inside an iframe (spec:AST-099 FR7); there the browser's own focus applies. The docs and changeset now say so.
…e Popover touch presentation AST-057 takes the dependencies' text, which is main's (meta#8646): the tuning bullet this branch once restored is superseded, and the Popover touch presentation does not touch row actions.
… ChatLayout BottomSheet's useSheetKeyboard moved to keyboard/sheetKeyboard.ts, and it subscribes to the keyboard layout guide and rides Gboard with rideKeyboard as ChatLayout does, but the two modules' headers named only ChatLayout, and BottomSheetPanel's related-files list and the keyboard notes still pointed at keyboardHeight.ts. They now name the hook and its file.
…the Popover touch presentation
There was a problem hiding this comment.
Changes are needed before this can be approved: Popover's grow and shrink under a finger follows the current Popover and touch-presentation specs, but the branch still carries about 39,000 lines of open predecessor work and layer-runtime record text an engineering owner asked to drop. Please address these, then rebase once #8076, #8186 and the keyboard chain land so the diff is only the Popover change:
- Restore
main'sdocs/architecture/layer-runtime.md, since INV13 and the Exit copy section are being decided on #8186. - Play the two popover springs through registered motion roles, as AST-094 FR4 and FR31 require now that DEC-15 lets a theme's knobs reach every role, or have an engineering owner amend those claims.
- Take the presentation hold from the layer runtime (#8587) instead of
isTouchPresentationRef, per layer-runtime INV14. - Add recordings from a physical iPhone in Safari and a physical Android phone in Chrome, which AST-094's platform support asks for.
[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 this pull request's layer-runtime text to go back to main, since INV13 and the Exit copy section are being decided on #8186. Please restore main's version of this file here; the Current gaps line about Popover's latch can go with it once Popover takes the runtime's hold.
| // The popover's two springs are this component's measured values, not | ||
| // motion roles (touch motion stays internal, spec:AST-094 DEC-11): they | ||
| // play as given, so no table is registered and no other row is bundled. |
There was a problem hiding this comment.
AST-094 FR4 requires every spring a touch presentation plays to be a motion role ("MUST NOT play a literal"), FR31 grows and shrinks a Popover "on its motion roles", and since DEC-15 a theme's tempo and character reach every FR4 role (AST-108 FR3). These two springs stay out of SPRING_ROLES, so they would keep UIKit's literal timing once the knobs land. Registering them as internal roles (DEC-15 keeps per-role tables non-public), or an engineering owner amending FR4 and FR31, resolves it.
| detach: () => void; | ||
| } | null>(null); | ||
| // Whether the presentation in progress is the touch one. | ||
| const isTouchPresentationRef = useRef(false); |
There was a problem hiding this comment.
This keeps the chosen presentation from the open until the close, which is the presentation hold layer-runtime INV14 gives the runtime ("A component that needs one of these uses the runtime's rather than building its own"). Please take it from the runtime's hold (#8587) before this lands, or record the latch through a record change an engineering owner approves.
| * | ||
| * Touch-only code a pointer page carries (architecture:component-style-authoring | ||
| * INV6): components use the global touch module once it is installed | ||
| * (`TouchSupport.install()`, proposed in #8464), so per-component lazy swaps |
There was a problem hiding this comment.
TouchSupport.install() did not land: react-component-runtime INV13 says components import the shared touch modules statically and "There is no install call". This justification should cite INV13 instead.
|
At 66038f6:
|
Review only this PR's change: vjeux/astryx@2ed0645...66038f6
Depends on #8186 and #8176 (in 2ed0645). Merge order: #8076 → #8186 → this pull request → #8208 (with #8176) → #8223.
An open review asks to take the presentation hold from the layer runtime (INV14), restore main's layer-runtime document, and rebase once the dependencies land; the code described here changes with that.
This pull request shows a merge conflict with main in the CLI's icon-set search goldens; it comes from the keyboard module below it (#8559), which resolves it, and this branch takes that fix with its next push.
Why
On a phone, Popover opens with the desktop entrance (a short fade and a 5% scale) and vanishes at once when dismissed, where an iOS popover grows out of its control and shrinks back into it. A mouse keeps today's motion.
What led to it
We are bringing Astryx's touch components to the behavior of native iOS controls, with one touch model for every touch device (
spec:AST-094, #8378). For the Popover we measured iOS 26.2's popover on the simulator: it grows out of the point that meets its source, with scale (0.01 → 1), size and opacity on one spring (stiffness 331.901, damping 29.149), wider before taller, and dismisses along the same path on its own spring (283.973 / 28.648), interruptibly.Options and tradeoffs
clip-path: not composited, so it stutters whenever the page is busy.Why this choice
It follows iOS frame by frame (the scale tracks the spring within 0.009), keeps moving with the main thread blocked, and can be presented again mid-dismissal; Reduce Motion gets a 200 ms opacity fade. No API change: under a finger it writes inline
scale,opacityandtransform-origin, plus a short-lived inert exit copy. The arrow comes later.How to check it
Evidence
iOS 26.2 simulator, before and after:
Main thread blocked 400 ms; presented again mid-dismissal:
Android 15 emulator (Chromium), before and after; Reduce Motion: