Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsContextMenu (@astryxdesign/core) · View in Storybook
MultiSelector (@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/AlertDialog | +4.5KB |
+61B |
| core/AppShell | +3.7KB |
+60B |
| core/Avatar | +4.5KB |
+53B |
| core/AvatarGroup | +4.5KB |
+51B |
| core/AvatarGroupOverflow | +4.6KB |
+54B |
| core/Banner | +4.6KB |
+63B |
| core/BaseTypeahead | +4.6KB |
+80B |
| core/BottomSheet | +15.2KB |
+2B |
| core/BreadcrumbItem | +4.6KB |
+50B |
| core/Button | +4.6KB |
+65B |
| core/Calendar | +4.6KB |
+61B |
| core/Carousel | +6.0KB |
+64B |
| core/ChatComposer | +5.1KB |
+60B |
| core/ChatComposerInput | +5.0KB |
+63B |
| core/ChatComposerTokenElement | +5.0KB |
+63B |
| core/ChatDictationButton | +4.6KB |
+64B |
| core/ChatLayout | +17.4KB |
+100B |
| core/ChatLayoutScrollButton | +4.6KB |
+67B |
| core/ChatSendButton | +4.6KB |
+63B |
| core/CheckboxInput | +4.6KB |
+63B |
| core/CheckboxList | +4.5KB |
+50B |
| core/CheckboxListItem | +4.6KB |
+60B |
| core/CodeBlock | +4.6KB |
+61B |
| core/ComplexSelector | +4.6KB |
+64B |
| core/ContextMenu | +5.9KB |
+62B |
| core/ContextMenuItem | +6.0KB |
+64B |
| core/DateInput | +15.2KB |
+73B |
| core/DateRangeInput | +5.7KB |
+60B |
| core/DateTimeInput | +15.1KB |
+70B |
| core/DialogHeader | +4.6KB |
+62B |
| core/DrawerHeader | +4.6KB |
+60B |
| core/DropdownMenu | +5.9KB |
+72B |
| core/DropdownMenuSubMenu | +4.6KB |
+67B |
| core/Field | +4.5KB |
+52B |
| core/FieldLabel | +4.5KB |
+52B |
| core/FileInput | +4.6KB |
+61B |
| core/HoverCard | +4.5KB |
+51B |
| core/IconButton | +4.6KB |
+65B |
| core/InputGroup | +4.5KB |
+51B |
| core/LayerProvider | +4.5KB |
+64B |
| core/Lightbox | +4.6KB |
+64B |
| core/Link | +4.6KB |
+56B |
| core/ListItem | +4.6KB |
+60B |
| core/Markdown | +6.0KB |
+62B |
| core/MobileNav | +4.6KB |
+61B |
| core/MobileNavToggle | +4.6KB |
+65B |
| core/MoreMenu | +5.8KB |
+72B |
| core/MultiSelector | +6.1KB |
+88B |
| core/NumberInput | +4.6KB |
+59B |
| core/Pagination | +5.9KB |
+90B |
| core/Popover | +17.0KB |
+62B |
| core/PowerSearch | +13.8KB |
+92B |
| core/RadioList | +4.5KB |
+51B |
| core/RadioListItem | +4.6KB |
+60B |
| core/SegmentedControl | +4.5KB |
+52B |
| core/Selector | +5.9KB |
+88B |
| core/SideNav | +3.7KB |
+62B |
| core/SideNavCollapseButton | +4.6KB |
+63B |
| core/SideNavHeading | +4.6KB |
+63B |
| core/SideNavItem | +4.6KB |
+63B |
| core/Slider | +4.5KB |
+51B |
| core/StatusDot | +4.5KB |
+51B |
| core/Stepper | +4.6KB |
+63B |
| core/Switch | +4.6KB |
+63B |
| core/TabList | +4.5KB |
+50B |
| core/TabMenu | +4.6KB |
+64B |
| core/Table | +5.7KB |
+60B |
| core/TableCell | +5.9KB |
+59B |
| core/TableHeaderCell | +5.9KB |
+60B |
| core/TableSelectionToolbar | +4.6KB |
+61B |
| core/TextArea | +4.5KB |
+60B |
| core/TextInput | +4.6KB |
+64B |
| core/Thumbnail | +4.6KB |
+62B |
| core/TimeInput | +15.6KB |
+61B |
| core/Toast | +4.6KB |
+62B |
| core/ToggleButton | +4.6KB |
+65B |
| core/Tokenizer | +6.2KB |
+101B |
| core/Toolbar | +4.6KB |
+52B |
| core/Tooltip | +4.5KB |
+50B |
| core/TopNav | +4.6KB |
+61B |
| core/TopNavHeading | +4.6KB |
+66B |
| core/TopNavMegaMenu | +4.6KB |
+62B |
| core/TopNavMenu | +4.6KB |
+65B |
| core/Typeahead | +4.6KB |
+78B |
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 | 35.4KB | 35.5KB | +102B |
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 504 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
1663f60 to
a9d43aa
Compare
a9d43aa to
3c17f4f
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec states that a Popover should shrink above the software keyboard (AST-059 FR3 allows a component's lower cap, but AST-059 DEC-1 and the layer runtime's INV11 give the viewport inset to the layer), and the pull request also adds unapproved requirements in the draft AST-094, in useMenuPress (FR9, DEC-8 and the touch open row) and in DropdownMenu FR8. An engineering owner can provide or identify the governing spec for the keyboard rule (component Popover, AST-094 or the layer runtime) and record the decision by commenting /approve-spec 3c17f4ffe30c4d670e599e52ca23eb255dd7b206 on this pull request; the author may do so as an engineering owner. Whatever the ruling, these need fixing:
usePopoverKeyboardAvoidanceremoves a caller's inlinemax-block-sizeandoverflow-yfrom the surface on every close, so a Popover givenstyle={{maxBlockSize: 160}}reopens uncapped.- With the keyboard up, tapping the scrolled-in Save button lands no click in Chromium touch, because the blur restores the full height between
mousedownandmouseup. - A popover placed above its trigger is re-positioned rather than shortened from its top, and still ends under the keyboard.
- The useMenuPress record lists a 500 ms long-press delay while the code defaults to 150 ms.
- The compare holds 33,282 authored lines from the dependency stack; land those first so this pull request carries only its 702-line change.
[Automated review]
| const restore = () => { | ||
| style.removeProperty('max-block-size'); | ||
| style.removeProperty('overflow-y'); | ||
| }; |
There was a problem hiding this comment.
restore() removes max-block-size and overflow-y from the surface whether or not this hook set them, and it runs on every close (cleanup) and before every re-measure. The surface also carries the caller's style prop, which React re-applies only when it changes, so a <Popover style={{maxBlockSize: 160, overflowY: 'auto'}}> reopens uncapped after its first close (Chromium: 160 px on the base, 381 px here). Remember what the hook wrote and remove only that, or restore the previous inline values.
There was a problem hiding this comment.
Fixed in cd5c2f1: the hook records the caller's inline max-block-size and overflow-y when the popover opens and puts them back when its cap comes off, so it removes only what it wrote. Unit cases cover a style cap that needs no keyboard cap and one under it; both failed before.
| // Falling (it uncovers the full height as it goes) or at once. | ||
| avoid(transition.to); |
There was a problem hiding this comment.
A tap on a button in the capped popover blurs the field, the guide publishes a predicted hide from focusout, and this restores the full height between mousedown and mouseup. In Chromium touch the tap on the scrolled-in Save note then sends mouseup to a tip and click to the content: Save gets no click. Keeping the cap until the hide's start (or until the click has dispatched) would let the tap land; worth a case in PopoverKeyboard.interaction.browser.spec.ts.
There was a problem hiding this comment.
Fixed in cd5c2f1: while a pointer that went down inside the popover is pressed, a falling keyboard's height change waits for that press's click, or for 350 ms after a lift that makes no click. A new Chromium case, "with the keyboard up, a tap on the scrolled-in Save note clicks it", failed before (0 clicks) and passes now; unit cases cover both the click and the no-click paths.
| const limit = viewportHeight - inset - popover.keyboardClearancePx; | ||
| if (surface.bottom <= limit) { | ||
| return null; | ||
| } | ||
| return Math.max(0, limit - surface.top); |
There was a problem hiding this comment.
This assumes the surface's top stays where it is, but the layer re-positions the shorter box from its anchored side. A popover placed above a trigger near the bottom, capped to 351 px, moved from 16–505 px to 131–482 px and still ended 105 px under a keyboard top at 377 px. Compute the cap from the edge the layer holds, or skip it when it cannot bring the bottom above the keyboard.
There was a problem hiding this comment.
Fixed in cd5c2f1: after capping, the hook measures again. If the bottom did not come up, because the layer holds the bottom edge (a popover above its trigger), it takes the cap off and leaves the popover where the layer placed it. The unit case "leaves a popover whose bottom the layer holds as it is" failed before.
| @@ -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 says 500 ms, while useMenuPress.ts now defaults longPressDelayMs to 150 ms (menu.holdOpenMs, plus 150 ms inside a scroller). An owner approving this record would approve a number the code does not ship; please update the row, or the code.
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.
3c17f4f to
fc6757e
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec says a Popover should shrink above the software keyboard (AST-059 FR3 allows a component's lower cap, but AST-059 DEC-1 and the layer runtime's INV11 give the viewport inset to the layer), and the pull request also adds unapproved requirements in the draft AST-094, in useMenuPress (FR9, DEC-8 and the touch open row) and in DropdownMenu FR8. An engineering owner can name the record that should govern the keyboard rule (Popover, AST-094 or the layer runtime) and record the decision by commenting /approve-spec fc6757e15246a0f0b1fc6c91005eb39397aad18e on this pull request; the author may do so as an engineering owner. Whatever the ruling, these need fixing:
usePopoverKeyboardAvoidanceremoves a caller's inlinemax-block-sizeandoverflow-yfrom the surface on every close, so a Popover givenstyle={{maxBlockSize: 160}}reopens uncapped.- A tap on a button in the capped popover blurs the field, and the full height comes back between the tap's press and release, so the tap can miss the Save button the cap made reachable.
- A popover placed above its trigger is shortened from its top and still ends under the keyboard.
- The useMenuPress record lists a 500 ms long-press delay while the code defaults to 150 ms, and AST-094 FR13 does not state the iOS-only rule that Popover now follows and DropdownMenu does not.
- The compare holds 33,976 authored lines from the dependency stack; land those first so this pull request carries only its 780-line change.
[Automated review]
| const restore = () => { | ||
| style.removeProperty('max-block-size'); | ||
| style.removeProperty('overflow-y'); | ||
| }; |
There was a problem hiding this comment.
restore() removes max-block-size and overflow-y from the surface whether or not this hook set them, and it runs before every measurement and on every close (:189). A caller's style={{maxBlockSize: 160, overflowY: 'auto'}} is merged onto this same element, and React will not write it again while its value is unchanged, so the Popover reopens without it. Remember the inline values before the first write and put exactly those back.
| // Falling (it uncovers the full height as it goes) or at once. | ||
| avoid(transition.to); |
There was a problem hiding this comment.
A tap on a button in the capped popover blurs the field; the guide publishes a predicted hide from focusout, and this branch restores the full height at once, between the tap's mousedown and mouseup, so the click can land elsewhere than the Save the cap made reachable. Keep the cap until the viewport shows the keyboard going down, or until the tap's click has dispatched.
There was a problem hiding this comment.
Fixed in cd5c2f1, as in the thread above: the cap holds through a press inside the popover until its click, and the Chromium Save-tap case covers it.
| if (surface.bottom <= limit) { | ||
| return null; | ||
| } | ||
| return Math.max(0, limit - surface.top); |
There was a problem hiding this comment.
This assumes the top edge stays put, but the layer places the popover with position-area: a popover above its trigger keeps its bottom edge, so the shorter box moves its top down and still ends under the keyboard. Compute the cap from the edge the layer holds, or skip it when it cannot clear the keyboard.
There was a problem hiding this comment.
Fixed in cd5c2f1, as in the thread above: when the layer holds the bottom edge, the cap comes off and the popover is left where it is.
| @@ -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 says 500 ms, while useMenuPress.ts now defaults longPressDelayMs to 150 ms (menu.holdOpenMs, and 300 ms inside a scrolling region). An owner approving this record would approve a number the code does not ship.
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.
fc6757e to
84ceb52
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec says a Popover should shrink above the software keyboard (AST-059 FR3 allows a component's lower cap, while AST-059 DEC-1 and the layer runtime's INV11 give the viewport inset to the layer), and the pull request also adds unapproved requirements in the draft AST-094, in useMenuPress (FR9, DEC-8 and the touch open row) and in DropdownMenu FR8. An engineering owner can name the record that should govern the keyboard rule (Popover, AST-094 or the layer runtime) and record the decision by commenting /approve-spec 84ceb521692e1e863e2cf46f77a7608b1c9838b5 on this pull request; the author may do so as an engineering owner. Whatever the ruling, these need fixing:
usePopoverKeyboardAvoidanceremoves a caller's inlinemax-block-sizeandoverflow-yfrom the surface on every close, so a Popover givenstyle={{maxBlockSize: 160}}reopens uncapped.- A Popover whose content fits now declares
touch-action: none, which blocks page pinch-zoom over it;pinch-zoom, as the menus use, keeps the page still and zoomable. - The 744 px compact test contradicts AST-012 FR9 (width below 768 px or height below 500 px), and the touch Reduce Motion fades run longer than the 200 ms that the reduced-motion design rule allows.
- A tap on a button in the capped popover restores the full height between press and release, and a popover placed above its trigger is shortened from its top and still ends under the keyboard.
- The useMenuPress record says 500 ms and "the same settle rule" where the code ships 150 ms and a 10 px handoff, and the compare holds 34,027 authored lines from the dependency stack; land those first so this pull request carries only its 780-line change.
[Automated review]
| const {style} = surface; | ||
| let isDisposed = false; | ||
| const guide = getKeyboardLayoutGuide(win); | ||
| const restore = () => { |
There was a problem hiding this comment.
restore() removes max-block-size and overflow-y from the surface whether or not this hook set them, and it runs before every measurement and on every close. The surface stays mounted while closed and React re-writes a caller's style only when its value changes, so a Popover given style={{maxBlockSize: 160, overflowY: 'auto'}} reopens without them. Recording the inline values before the first write and putting exactly those back would keep the caller's styles.
| return; | ||
| } | ||
| // Falling (it uncovers the full height as it goes) or at once. | ||
| avoid(transition.to); |
There was a problem hiding this comment.
A tap on a button in the capped popover blurs the field; the guide publishes a predicted hide from focusout, and this branch restores the full height at once, between the tap's mousedown and mouseup, so the click can land on another element than the Save it was aimed at. Keeping the cap until the viewport reports the keyboard going down (or the tap's click has dispatched) avoids that.
| if (surface.bottom <= limit) { | ||
| return null; | ||
| } | ||
| return Math.max(0, limit - surface.top); |
There was a problem hiding this comment.
This assumes the top edge holds, but the layer places the box with position-area against its trigger: a popover above its trigger keeps its bottom edge, so the cap moves its top down and its bottom stays under the keyboard.
| // scroll: the page behind the popover stays where it is, as it does | ||
| // behind a UIKit popover. A scrolling surface keeps its own pan above. | ||
| surfaceStill: { | ||
| touchAction: 'none', |
There was a problem hiding this comment.
touch-action: none also stops page pinch-zoom for any finger that lands on the popover, on every touch screen. pinch-zoom still admits no one-finger pan and leaves the pinch to the browser, which is what DropdownMenu does for a menu whose rows fit (touchPinchZoom) and what draft AST-094 FR10 asks for.
| 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.
Current AST-012 FR9 defines compact as a viewport width below the effective md point (768) or a height below 500, and says the web touch presentation resolver keeps no size numbers of its own. With 744 here, a coarse-pointer screen 744–767 px wide and at least 500 px tall gets the popover where FR9 makes it compact.
| prefersReducedMotion: boolean, | ||
| ): SpringDescription { | ||
| if (prefersReducedMotion) { | ||
| return springs.menuReducedMotion; |
There was a problem hiding this comment.
Under reduced motion with a coarse pointer, the current reduced-motion design rule (DR2) asks for an opacity-only fade that finishes in 200 ms or less. menuReducedMotion is a 0.4 s critically damped spring, about 82% of the way at 200 ms.
| @@ -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 says 500 ms, while useMenuPress.ts now defaults longPressDelayMs to 150 ms (menu.holdOpenMs, and 300 ms inside a scrolling region). An owner approving the record would approve a number the code does not ship.
| | FR7 | While a press is tracked in a menu that scrolls, a pointer resting within the edge zone of the menu's top or bottom MUST scroll the menu toward that edge and re-read the row under the pointer as the rows move. | `useMenuPress.test.tsx` autoscroll case | verify | | ||
| | FR8 | On a mouse a DropdownMenu trigger MUST open its menu on press-down (button 0, no Control), and a drag from the trigger into the menu that releases over an enabled row MUST act on it. The opening gesture's release MUST act only if the pointer entered the menu or the press lasted the settle time; otherwise nothing acts and the menu stays open. Pressing the trigger of an open menu MUST close it and MUST NOT reopen it in the same gesture. | Proposed DEC-4; `DropdownMenu.test.tsx` press model suite, `menuPressGesture.test.ts` trigger states | proposed; pending owner review | | ||
| | FR9 | Under a finger a tap on the trigger MUST open the menu through its click. A press held on the trigger for the long-press delay MUST open the menu with the finger down, prevent the default of the finger's later `touchmove`s until the gesture ends, and continue under FR1–FR4 with the same settle rule. | Proposed DEC-4; `DropdownMenu.test.tsx` held-touch case, `useMenuPress.test.tsx` | proposed; pending owner review | | ||
| | FR9 | By default a finger's tap on the trigger MUST open the menu through its click, and a press held for the long-press delay MUST open it with the finger down and prevent the default of the finger's later `touchmove`s until the gesture ends. With touch open `press` the press-down MUST open it at once, and a cancel before the pointer enters the menu MUST close it again. Both continue under FR1–FR4 with the same settle rule. | Proposed DEC-4, DEC-8; `DropdownMenu.test.tsx` touch cases, `useMenuPress.test.tsx` | proposed; pending owner review | |
There was a problem hiding this comment.
Both finger opens are said to continue "with the same settle rule", but under a finger the machine replaces the settle rule with a 10 px handoff (MENU_PRESS_HANDOFF_PX, menuPressGesture.ts:324-349, :364-369) that no record states.
84ceb52 to
cd5c2f1
Compare
There was a problem hiding this comment.
This needs an engineering owner's decision before it can be approved: no current spec says a Popover should shrink above the software keyboard (AST-059 FR3 allows a component's lower cap, while AST-059 DEC-1 and the layer runtime's INV11 give the viewport inset to the layer), and the pull request also adds unapproved requirements in the draft AST-094, in useMenuPress (FR9, DEC-8 and the touch open row) and in DropdownMenu FR8. An engineering owner can name the record that should govern the keyboard rule (Popover, AST-094 or the layer runtime) and record the decision by commenting /approve-spec cd5c2f1acdb0b4b961199b57d1626232d5ca494c on this pull request; the author may do so as an engineering owner. The inline-style, mid-tap and above-trigger fixes in this push look right in the code; whatever the ruling, these still need fixing:
- A Popover whose content fits declares
touch-action: none, which blocks page pinch-zoom over it;pinch-zoom, as the menus use, keeps the page still and zoomable. - The 744 px compact test contradicts AST-012 FR9 (width below 768 px or height below 500 px), and the touch Reduce Motion fades run longer than the 200 ms the reduced-motion design rule allows.
- The useMenuPress record says 500 ms and "the same settle rule" where the code ships 150 ms and a 10 px handoff, and AST-094 FR13 keys touch behaviour on a coarse pointer where Popover keys on WebKit on iOS.
- The compare holds 34,358 authored lines from the dependency stack; land those first so this pull request carries only its 993-line change.
[Automated review]
| // scroll: the page behind the popover stays where it is, as it does | ||
| // behind a UIKit popover. A scrolling surface keeps its own pan above. | ||
| surfaceStill: { | ||
| touchAction: 'none', |
There was a problem hiding this comment.
touch-action: none also stops page pinch-zoom for any pinch that starts on a Popover whose content fits, on every touch screen. The menus keep the pinch with touch-action: pinch-zoom (current useMenuPress FR4), and this head's own draft AST-094 FR10 and FR18 say page pinch-zoom must stay the browser's. pinch-zoom still admits no one-finger pan, so the page behind stays still.
| 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.
Current AST-012 FR9 defines compact as a viewport width below the effective md point (768) or a height below 500, and says the web touch presentation resolver keeps no size numbers of its own. With 744 here, a coarse-pointer screen 744–767 px wide (an iPad mini in portrait) gets the anchored popover in every adaptive presentation instead of the sheet.
| prefersReducedMotion: boolean, | ||
| ): SpringDescription { | ||
| if (prefersReducedMotion) { | ||
| return springs.menuReducedMotion; |
There was a problem hiding this comment.
Under reduced motion with a coarse pointer, current design rule DR2 asks for an opacity-only fade that finishes in 200 ms or less. springs.menuReducedMotion (response 0.4 s, critically damped) is about 82% of the way at 200 ms and settles near 420 ms; the iOS Popover's springs.popoverPresent fade settles near 350 ms.
| @@ -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 says 500 ms, while useMenuPress.ts defaults longPressDelayMs to 150 ms (menu.holdOpenMs), and 300 ms inside a scrolling region. An owner approving the record would approve a number the code does not ship.
| | FR7 | While a press is tracked in a menu that scrolls, a pointer resting within the edge zone of the menu's top or bottom MUST scroll the menu toward that edge and re-read the row under the pointer as the rows move. | `useMenuPress.test.tsx` autoscroll case | verify | | ||
| | FR8 | On a mouse a DropdownMenu trigger MUST open its menu on press-down (button 0, no Control), and a drag from the trigger into the menu that releases over an enabled row MUST act on it. The opening gesture's release MUST act only if the pointer entered the menu or the press lasted the settle time; otherwise nothing acts and the menu stays open. Pressing the trigger of an open menu MUST close it and MUST NOT reopen it in the same gesture. | Proposed DEC-4; `DropdownMenu.test.tsx` press model suite, `menuPressGesture.test.ts` trigger states | proposed; pending owner review | | ||
| | FR9 | Under a finger a tap on the trigger MUST open the menu through its click. A press held on the trigger for the long-press delay MUST open the menu with the finger down, prevent the default of the finger's later `touchmove`s until the gesture ends, and continue under FR1–FR4 with the same settle rule. | Proposed DEC-4; `DropdownMenu.test.tsx` held-touch case, `useMenuPress.test.tsx` | proposed; pending owner review | | ||
| | FR9 | By default a finger's tap on the trigger MUST open the menu through its click, and a press held for the long-press delay MUST open it with the finger down and prevent the default of the finger's later `touchmove`s until the gesture ends. With touch open `press` the press-down MUST open it at once, and a cancel before the pointer enters the menu MUST close it again. Both continue under FR1–FR4 with the same settle rule. | Proposed DEC-4, DEC-8; `DropdownMenu.test.tsx` touch cases, `useMenuPress.test.tsx` | proposed; pending owner review | |
There was a problem hiding this comment.
Both finger opens are said to continue "with the same settle rule", but under a finger the machine hands off after MENU_PRESS_HANDOFF_PX of travel instead (menuPressGesture.ts:324-368), which no record states.
cd5c2f1 to
1dee9e0
Compare
github-actions
Bot
added
the
needs:design-review
…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.
There was a problem hiding this comment.
Approval waits on an engineering owner's decision about spec text this pull request carries from its dependencies: architecture:layer-runtime INV13 and its Exit copy section (from #8186), and two new spec:AST-057 sentences that put the row swipe's slide and spring on theme motion roles, neither of which is on main or approved for this revision. An engineering owner records that decision by commenting /approve-spec 33c21c5efa2c42ed166a9fa00e17eb3d2540c415 on this pull request, and the author may do so as an engineering owner; otherwise INV13 should land through #8186 and the AST-057 sentences go back to main's text. The keyboard cap itself follows current spec:AST-059 FR8–FR10, and two changes are needed whatever that decision is:
- Land or rebase the dependency stack so this pull request holds only the 2,368-line keyboard cap; today 41,264 of its 43,632 authored lines come from fourteen open dependency pull requests.
- Bring
architecture:layer-runtimeINV11, INV12 and its owning code along with the cap, as the current-state impact inspec:AST-059describes, or get an engineering owner's agreement that the record may follow.
[Automated review]
| - **Gesture tuning.** Axis-lock distance, commit ratio, fling velocity, | ||
| resistance past the panel: behavior constants in source, not public API and | ||
| not design tokens (the same ruling the touch press model received for its | ||
| clocks). The row's slide and spring motion plays on theme motion roles | ||
| (`spec:AST-094` FR4). |
There was a problem hiding this comment.
These sentences arrived with the #8186 merge and are not on main: current spec:AST-057 keeps "slide and spring durations" among the swipe's behavior constants ("not public API and not design tokens"), and no row-swipe code is in this pull request. Restore main's text here and under the API table (lines 223-226), unless an engineering owner approves the change for this head.
| 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.
Review only this PR's change: vjeux/astryx@66038f6...33c21c5
Depends on #8187, which carries #8176.
An open review asks to re-measure the cap when the runtime swaps in a new layer element while it is open (FR8), and to rebase once the dependencies land; the cap's measuring 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, the software keyboard covered the lower part of an anchored layer (a Popover, a Selector's list, a menu), buttons included: someone typing into its search field could not reach what was under the keyboard. Safari's keyboard does not shrink the layout viewport, so the layer runtime's size cap never saw it.
What led to it
We are bringing Astryx's touch components to the behavior of native iOS controls. On iOS, a popover shrinks with its top held so that it ends 10 pt above the keyboard. The owner decided the rule for Astryx (#8208 (comment)), recorded by #8506 (
spec:AST-059FR8–FR10 and DEC-6,architecture:layer-runtimeINV11).Options and tradeoffs
Why this choice
It matches iOS and the owner's decision, and joins the runtime's existing caps, so every layered component gets it from one rule; lists and menus fold it into their own 300 px.
How to check it
PopoverKeyboardinteraction specs and the list and menu specs pass in Chromium and WebKit.Android has not been run yet, so the changeset is scoped to iOS.
Evidence
iOS 26.2 Safari, before and after (after: the layer spans 68–339 px against the keyboard's top at 349):
Opened with the keyboard already up (before, it ran to 509):