Repository navigation
feat(Popover): under a finger, it grows out of its trigger and shrinks back into it - #8187
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🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| LayerProvider | 4.1 kB (0) · 20 B CSS (0) | — | ✓ · ✓ · ✗ |
| lab/ChatEmojiPicker | 🔴 92.3 kB (+17.7 kB) · 773 B CSS (0) | — | — · — · — |
| lab/ChatReactionBar | 🔴 92.9 kB (+17.7 kB) · 1.1 kB CSS (0) | — | — · — · — |
112 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| lab/CommentComposer | 🔴 132.7 kB (+17.4 kB) · 1.9 kB CSS (0) | — | — · — · — |
| Popover · Storybook | 🔴 77.8 kB (+17.0 kB) · 13.4 kB CSS (0) | 1,490 (+451) | ✓ · ✓ · ✓ |
| lab/TourStep | 🔴 83.8 kB (+16.9 kB) · 590 B CSS (0) | — | — · — · — |
| lab/CommentThread | 🔴 291.2 kB (+15.2 kB) · 2.4 kB CSS (0) | — | — · — · — |
| lab/Comments | 🔴 296.9 kB (+15.2 kB) · 2.5 kB CSS (0) | — | — · — · — |
| SideNav | 81.8 kB (+249 B) · 10.7 kB CSS (0) | — | — · — · — |
| AppShell | 81.8 kB (+191 B) · 12.8 kB CSS (0) | — | — · — · — |
| DateTimeInput | 193.5 kB (+97 B) · 18.0 kB CSS (0) | — | — · — · — |
| Tokenizer | 80.3 kB (+66 B) · 17.8 kB CSS (0) | — | — · — · — |
| ChatComposerInput | 66.2 kB (+65 B) · 10.6 kB CSS (0) | — | — · — · — |
| ChatComposerTokenElement | 66.2 kB (+64 B) · 10.6 kB CSS (0) | — | — · — · — |
| lab/TransferListSelector | 92.4 kB (+57 B) · 1.9 kB CSS (0) | — | — · — · — |
| ChatComposer | 68.2 kB (+56 B) · 11.2 kB CSS (0) | — | — · — · — |
| BreadcrumbItem | 75.8 kB (+49 B) · 11.5 kB CSS (0) | — | — · — · — |
| DropdownMenuSubMenu | 64.5 kB (+48 B) · 9.4 kB CSS (0) | — | — · — · — |
| FieldLabel | 42.9 kB (+47 B) · 5.2 kB CSS (0) | — | — · — · — |
| PowerSearch | 264.2 kB (+47 B) · 23.1 kB CSS (0) | — | — · — · — |
| charts/Chart | 173.8 kB (+44 B) · 494 B CSS (0) | — | — · — · — |
| richtext/RichTextEditorToolbar | 239.8 kB (+43 B) · 454 B CSS (0) | — | — · — · — |
| CheckboxListItem | 66.2 kB (+42 B) · 10.9 kB CSS (0) | — | — · — · — |
| Slider | 97.0 kB (+42 B) · 8.3 kB CSS (0) | — | — · — · — |
| Toast | 89.1 kB (+42 B) · 9.6 kB CSS (0) | — | — · — · — |
| lab/TransferList | 85.4 kB (+42 B) · 1.7 kB CSS (0) | — | — · — · — |
| TabMenu | 63.0 kB (+39 B) · 10.4 kB CSS (0) | — | — · — · — |
| RadioListItem | 63.0 kB (+37 B) · 9.7 kB CSS (0) | — | — · — · — |
| Switch | 94.8 kB (+37 B) · 9.5 kB CSS (0) | — | — · — · — |
| lab/ListInput | 91.0 kB (+37 B) · 1.6 kB CSS (0) | — | — · — · — |
| richtext/RichTextView | 270.9 kB (+37 B) · 2.3 kB CSS (0) | — | — · — · — |
| Tooltip | 25.8 kB (+36 B) · 4.3 kB CSS (0) | — | — · — · — |
| Table | 99.6 kB (+35 B) · 17.2 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditor | 290.6 kB (+35 B) · 2.6 kB CSS (0) | — | — · — · — |
| MultiSelector | 88.3 kB (+33 B) · 16.2 kB CSS (0) | — | — · — · — |
| Thumbnail | 55.9 kB (+33 B) · 10.9 kB CSS (0) | — | — · — · — |
| lab/Schedule | 98.9 kB (+33 B) · 3.9 kB CSS (0) | — | — · — · — |
| SideNavItem | 61.6 kB (+32 B) · 10.4 kB CSS (0) | — | — · — · — |
| HoverCard | 24.3 kB (+31 B) · 4.3 kB CSS (0) | — | — · — · — |
| TableHeaderCell | 91.0 kB (+31 B) · 16.7 kB CSS (0) | — | — · — · — |
| TextInput | 59.4 kB (+31 B) · 10.5 kB CSS (0) | — | — · — · — |
| ListItem | 65.2 kB (+30 B) · 10.9 kB CSS (0) | — | — · — · — |
| Typeahead | 69.9 kB (+30 B) · 11.8 kB CSS (0) | — | — · — · — |
| Carousel | 59.5 kB (+29 B) · 10.3 kB CSS (0) | — | — · — · — |
| Field | 46.8 kB (+29 B) · 6.7 kB CSS (0) | — | — · — · — |
| charts/ChartTooltip | 151.2 kB (+28 B) · 423 B CSS (0) | — | — · — · — |
| FileInput | 60.2 kB (+28 B) · 10.3 kB CSS (0) | — | — · — · — |
| lab/EmojiPicker | 77.6 kB (+28 B) · 751 B CSS (0) | — | — · — · — |
| CodeBlock | 66.2 kB (+27 B) · 10.6 kB CSS (0) | — | — · — · — |
| StatusDot | 27.5 kB (+27 B) · 5.3 kB CSS (0) | — | — · — · — |
| TableCell | 91.4 kB (+27 B) · 16.9 kB CSS (0) | — | — · — · — |
| lab/SelectableCardGroup | 56.1 kB (+27 B) · 162 B CSS (0) | — | — · — · — |
| InputGroup | 48.5 kB (+25 B) · 7.3 kB CSS (0) | — | — · — · — |
| ChatSendButton | 52.0 kB (+24 B) · 9.3 kB CSS (0) | — | — · — · — |
| CheckboxList | 50.0 kB (+24 B) · 7.5 kB CSS (0) | — | — · — · — |
| Link | 48.5 kB (+24 B) · 6.9 kB CSS (0) | — | — · — · — |
| RadioList | 48.2 kB (+24 B) · 6.8 kB CSS (0) | — | — · — · — |
| TimeInput | 144.0 kB (+24 B) · 15.1 kB CSS (0) | — | — · — · — |
| Toolbar | 39.4 kB (+24 B) · 7.9 kB CSS (0) | — | — · — · — |
| CheckboxInput | 59.9 kB (+22 B) · 9.0 kB CSS (0) | — | — · — · — |
| InfoTip | 43.0 kB (+22 B) · 5.3 kB CSS (0) | — | — · — · — |
| ChatLayoutScrollButton | 52.1 kB (+21 B) · 9.5 kB CSS (0) | — | — · — · — |
| ComplexSelector | 62.4 kB (+21 B) · 11.0 kB CSS (0) | — | — · — · — |
| ContextMenu | 90.2 kB (+21 B) · 16.5 kB CSS (0) | — | — · — · — |
| Markdown | 161.5 kB (+21 B) · 19.5 kB CSS (0) | — | — · — · — |
| NumberInput | 62.8 kB (+21 B) · 10.7 kB CSS (0) | — | — · — · — |
| Stepper | 53.3 kB (+21 B) · 9.5 kB CSS (0) | — | — · — · — |
| Banner | 53.6 kB (+20 B) · 9.8 kB CSS (0) | — | — · — · — |
| Button | 51.5 kB (+19 B) · 9.3 kB CSS (0) | — | — · — · — |
| SegmentedControl | 89.0 kB (+19 B) · 6.8 kB CSS (0) | — | — · — · — |
| SideNavCollapseButton | 52.4 kB (+19 B) · 9.4 kB CSS (0) | — | — · — · — |
| lab/CommentPin | 52.6 kB (+19 B) · 1.2 kB CSS (0) | — | — · — · — |
| lab/DiffViewer | 77.9 kB (+19 B) · 2.1 kB CSS (0) | — | — · — · — |
| Calendar | 62.4 kB (+18 B) · 9.9 kB CSS (0) | — | — · — · — |
| TableSelectionToolbar | 61.1 kB (+18 B) · 12.4 kB CSS (0) | — | — · — · — |
| ToggleButton | 52.5 kB (+18 B) · 9.4 kB CSS (0) | — | — · — · — |
| AvatarGroup | 42.1 kB (+17 B) · 5.6 kB CSS (0) | — | — · — · — |
| ChatLayout | 54.9 kB (+17 B) · 9.8 kB CSS (0) | — | — · — · — |
| SideNavHeading | 66.6 kB (+17 B) · 10.6 kB CSS (0) | — | — · — · — |
| AvatarGroupOverflow | 44.4 kB (+16 B) · 5.5 kB CSS (0) | — | — · — · — |
| DropdownMenu | 85.8 kB (+16 B) · 16.6 kB CSS (0) | — | — · — · — |
| MobileNavToggle | 51.9 kB (+16 B) · 9.3 kB CSS (0) | — | — · — · — |
| TopNav | 63.2 kB (+16 B) · 10.2 kB CSS (0) | — | — · — · — |
| DialogHeader | 54.4 kB (+15 B) · 10.8 kB CSS (0) | — | — · — · — |
| TextArea | 53.8 kB (+15 B) · 8.9 kB CSS (0) | — | — · — · — |
| AlertDialog | 95.6 kB (+14 B) · 13.7 kB CSS (0) | — | — · — · — |
| ChatDictationButton | 52.1 kB (+14 B) · 9.4 kB CSS (0) | — | — · — · — |
| DrawerHeader | 54.6 kB (+14 B) · 12.2 kB CSS (0) | — | — · — · — |
| IconButton | 51.6 kB (+14 B) · 9.3 kB CSS (0) | — | — · — · — |
| TabList | 40.1 kB (+14 B) · 6.4 kB CSS (0) | — | — · — · — |
| MobileNav | 61.5 kB (+12 B) · 9.9 kB CSS (0) | — | — · — · — |
| TopNavHeading | 66.0 kB (+12 B) · 10.4 kB CSS (0) | — | — · — · — |
| Avatar | 39.4 kB (+11 B) · 5.5 kB CSS (0) | — | — · — · — |
| DateRangeInput | 81.4 kB (+10 B) · 11.8 kB CSS (0) | — | — · — · — |
| Lightbox | 59.7 kB (+9 B) · 9.7 kB CSS (0) | — | — · — · — |
| MoreMenu | 86.1 kB (+8 B) · 16.6 kB CSS (0) | — | — · — · — |
| TopNavMegaMenu | 60.9 kB (+5 B) · 11.2 kB CSS (0) | — | — · — · — |
| TopNavMenu | 65.8 kB (+3 B) · 10.5 kB CSS (0) | — | — · — · — |
| charts/ChartLegend | 35.6 kB (+1 B) · 224 B CSS (0) | — | — · — · — |
| ChatMessageList | 23.8 kB (0) · 6.9 kB CSS (0) | — | — · — · — |
| ChatToolCalls | 27.7 kB (0) · 8.3 kB CSS (0) | — | — · — · — |
| CommandPalette | 59.3 kB (0) · 12.4 kB CSS (0) | — | — · — · — |
| CommandPaletteInput | 27.9 kB (0) · 7.5 kB CSS (0) | — | — · — · — |
| Heading | 8.0 kB (0) · 5.3 kB CSS (0) | — | — · — · — |
| Timer | 9.6 kB (0) · 5.9 kB CSS (0) | — | — · — · — |
| ProgressBar | 11.5 kB (−1 B) · 6.2 kB CSS (0) | — | — · — · — |
| Spinner | 20.3 kB (−1 B) · 6.1 kB CSS (0) | — | — · — · — |
| Text | 7.5 kB (−1 B) · 5.3 kB CSS (0) | — | — · — · — |
| Timestamp | 23.0 kB (−1 B) · 9.6 kB CSS (0) | — | — · — · — |
| Selector | 88.8 kB (−2 B) · 16.6 kB CSS (0) | — | — · — · — |
| DateInput | 167.1 kB (−3 B) · 16.1 kB CSS (0) | — | — · — · — |
| Pagination | 98.8 kB (−14 B) · 17.0 kB CSS (0) | — | — · — · — |
| BaseTypeahead | 61.9 kB (−17 B) · 10.3 kB CSS (0) | — | — · — · — |
| layer | — | — | — · ✓ · — |
| Layer | — | — | ✓ · ✓ · — |
Package size: @astryxdesign/core 6.74 MB packed (+26.8 kB); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 19 rows, core 21 rows; run pnpm api:report.
Touched components checked; add ci:run-visual, ci:run-a11y or ci:run-rtl to run the full suites.
Generated by PR Enrichment workflow | 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
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #8662.
|
/approve-spec a46586c |
|
/approve-spec a46586c |
… landed; uikit index header lists popover and zoomMedia)
|
/approve-spec e8cd6f6 |
There was a problem hiding this comment.
Changes are needed before this can be approved. Under a finger, Popover's grow, hold and shrink follow current spec:AST-094 FR31, FR35, FR45 and FR7 and the component:Popover FR9 row an engineering owner approved for this head, and a mouse keeps today's entrance.
Required change:
- Regression: #9823 is still built on this pull request's previous head, so the declared stack
#8186 → #8187 → #9823 → #8662does not hold, and this pull request stands alone at 3,329 authored lines with no exemption and with its real-browser proofs in #9823; rebuild #9823 and #8662 on this head so the four are reviewed as the stack they declare.
The Popover, Layer and motion unit suites, Core's typecheck and every CI check pass on this head.
[Automated review]
There was a problem hiding this comment.
Approved. Under a finger, Popover now grows out of its trigger on the theme's entrance tokens, ignores touches until it has grown, and shrinks back into it on the theme's exit tokens, as current spec:AST-094 FR31, FR35, FR45 and FR7 and the component:Popover FR9 row approved for this head describe; a mouse keeps today's entrance.
The earlier request for changes is resolved: the declared stack #8186 → #8187 → #9823 → #8662 now holds, so the size falls under the stack and #9823's real-browser proofs apply. The Popover, Layer and motion unit suites and every CI check pass on this head. This pull request lands with its stack, once #8186 below it is approved as well.
[Automated review]
|
/approve-spec f642349 |
There was a problem hiding this comment.
Changes are needed before this can be approved. Under a finger, Popover's grow, hold and shrink follow current spec:AST-094 FR31, FR35, FR45 and FR7, an engineering owner approved this head's record changes, and a mouse keeps today's entrance.
Required change:
- Regression: #8662 is still built on #9823's previous head, so the declared stack
#8186 → #8187 → #9823 → #8662does not hold, and this pull request stands alone at 3,329 authored lines with no exemption and with its Popover browser tests (spec:AST-094FR31 and FR35) and PR4 trace in #9823; rebuild #8662 on #9823's current head so the four are reviewed as the stack they declare.
The merge since the last review resolves its transientCopy.ts conflict correctly, and the Popover, Layer, motion, SideNav and AppShell unit suites and Core's typecheck pass on this head.
[Automated review]
There was a problem hiding this comment.
Approved. Under a finger, Popover now grows out of its trigger on the theme's entrance tokens, ignores touches until it has grown, and shrinks back into it on the theme's exit tokens, as current spec:AST-094 FR31, FR35, FR45 and FR7 and the component:Popover FR9 row approved for this head describe; a mouse keeps today's entrance.
The earlier request for changes on this head is resolved: #9823 and #8662 are now rebuilt on it, so the declared stack #8186 → #8187 → #9823 → #8662 holds, the size falls under the stack, and #9823's real-browser proofs apply. The Popover, Layer and motion unit suites and every CI check pass on this head. This pull request lands with its stack, once #8186 below it is approved as well.
[Automated review]
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #8662.
The growth and its input hold now start on the first frame after the open, not in the task that opens the popover, so the opening tap's task lays out no more than a mouse opening does. Folded from meta#8662, so the group's squashes apply in order: these lines are this pull request's own.
There was a problem hiding this comment.
Changes are needed before this can be approved. Popover's grow and shrink under a finger and its input hold follow the current touch-presentation spec, and the record changes carry an engineering owner's approval for this head. The remaining problem is the pull request's scope.
- Regression: the declared stack
#8186 → #8187 → #9823 → #8662no longer holds, because #9823 is built on this pull request's previous head, so this range stands alone at 3,329 authored lines, 1,508 of them #8186's exit copy; rebuild #9823 and #8662 on this head, or land #8186 first so this pull request carries only Popover's change.
Once the stack holds again, this pull request is reviewed as part of it, and Popover's browser proofs in #9823 come back with it.
[Automated review]
|
/approve-spec 9c96e82 |
There was a problem hiding this comment.
Changes are needed before this can be approved. Popover's grow, hold and shrink under a finger follow the current touch-presentation spec, the record changes carry an engineering owner's approval for this head, and the declared stack holds again, so the size concern is gone.
- The touch hold now starts on the first frame after the open (
popoverSurfaceMotion.ts), so a touch in that frame reaches the page, whichspec:AST-094FR35 does not allow without the exception that only #8662 records; move #8662's two record lines (the AST-094 line for Popover and Popover's Current-state impact) into this pull request with the code, for an engineering owner to approve at that head, or keep the hold starting in the opening task here and leave the next-frame start to #8662.
Non-blocking: the comment saying a tap that lands before the hold cannot dismiss the popover is not shown by any test.
[Automated review]
| * The hold starts with the growth, in its frame. Holding from the open | ||
| * would lay the page out twice in the opening task, since the shield's | ||
| * `showPopover()` lays it out. A touch that starts in the frame before the | ||
| * hold reaches the page. Its tap does not dismiss the popover: its lift | ||
| * lands on the hold. |
There was a problem hiding this comment.
This window is new here: the hold starts in the growth's frame, so a touch in the first frame after the open reaches the page. Current spec:AST-094 FR35 allows no such window, and the two lines that record it as Popover's exception (AST-094's "Popover's input hold (FR35) starts on the first frame after the open…" and Popover's Current-state impact) are still only in #8662. Move them into this pull request with the code, or keep the hold starting in the opening task here.
Non-blocking: "Its tap does not dismiss the popover: its lift lands on the hold" is shown by no case; a tap whose press and lift both land before that frame would reach the light dismiss.
The input hold starts on the first frame after the open, so a touch that lands within that frame reaches the page. spec:AST-094 and Popover's record state that window here, beside the code that opens it. Moved from meta#8662.
|
/approve-spec d99ab9c |
There was a problem hiding this comment.
Approved. Under a finger, Popover's grow, hold and shrink follow the current touch-presentation spec, and the two record lines for the first frame after the open, which held the last revision, now ship here with an engineering owner's approval for this head. This pull request is judged on its own change above #8186, which lands first.
Non-blocking: the comment at popoverSurfaceMotion.ts:152-154 says a tap in the frame before the hold does not dismiss the popover; no test shows it, and a tap whose press and lift both land before that frame would reach the light dismiss.
[Automated review]
| * `showPopover()` lays it out. A touch that starts in the frame before the | ||
| * hold reaches the page. Its tap does not dismiss the popover: its lift | ||
| * lands on the hold. |
There was a problem hiding this comment.
Non-blocking: no case shows "Its tap does not dismiss the popover: its lift lands on the hold." It holds only when the lift lands after the hold starts; a tap whose press and lift both land before that frame reaches the light dismiss. Suggested wording: "A touch that starts in the frame before the hold reaches the page. If its lift lands on the hold, the tap does not dismiss the popover." Or add that case.
keepInherited read the outer layer's parent's computed style right after the copy was inserted there: a style read after a write. It reads only that host and writes only on the still-detached copy, so it now runs before the insertion. What the copy inherits is unchanged.
The frame that starts a touch presentation wrote the CSS entrance off and the start scale, read both boxes, wrote the origin and the start frame, then read the theme's entrance tokens. Each read after a write restyled or laid the page out again. It now reads both boxes and the entrance timing in one pass, then writes, the start frame first: the motion engine may read the element's will-change as it sets it. The box needs no reset first: the opening task already took the CSS entrance off, and a dismissal leaves the element at full size. Shown again during the dismissal, the popover reads its entrance timing before its first write too.
|
/approve-spec 51f8457 |
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #9823.
Stack: #8186 → #8187 → #9823
Merge group: #8186, #8187, #9823
These three pull requests merge back to back, as one unit. #8186 is the layer runtime's exit copy, which this uses first. #9823 holds this change's browser proofs. While the popover grows, this change holds input with the layer runtime's input hold (#8342, on
main); that part came from #8223, which is closed into this one. The next-frame start and the two record lines that state its window came from #8662, which is closed into this one, so that the group's squashes apply in order: each upper pull request only adds. This pull request's change is five commits, 415707b, 456d9ab, 9c96e82, d99ab9c and the read-before-write commit in 51f8457, on #8186's head (30e42ae, which brings inmain). Review only this pull request's change: vjeux/astryx@30e42ae...51f8457.main's Wheel and date picker tables sit beside this change'spopoverlines in the UIKit table's index, and the rebase keeps all three.User need
On a phone, Popover's anchored presentation appears and goes at once. Under a finger, people expect a popover to grow out of what they tapped and to go back into it, as an iOS popover does. Then the motion shows where the popover came from and where it went. A tap that lands while it grows, or a drag started then, must not close it again: on iOS, UIKit ignores every touch while a popover presents.
Problem and solution fit
--duration-medium-minand--ease-entrance. That is the Storybook theme's duration step nearest iOS's growth, which comes within 2% of its end in 206 ms; the step is 225 ms in that theme. The shrink plays the theme's exit duration and easing,--duration-exitand--ease-exit, as every overlay's exit does. iOS's 249 ms shrink stays in the motion table as a reference. A theme that sets no exit duration gets--duration-fast, through the exit duration's default. A tap has no velocity, so no spring plays.MotionCurvewhere the popover is. The scale, length and opacity follow it throughmapProgress. A timing that does not resolve plays no motion.onCloseStartthatusePopoverpasses through. Popover adds no listener of its own.spec:AST-094FR35.Current specification
component:PopoverFR9 andspec:AST-094FR31: under a finger, Popover grows out of its trigger and shrinks back into it.spec:AST-094FR35 andcomponent:PopoverFR9: a touch during the growth is ignored, through the layer runtime's one input hold.spec:AST-094FR45, merged in docs(AST-094, AST-057): motion guidance; gesture springs; swipe-row returns #9915 (257573b). Popover's main interaction is a tap: "A component whose main interaction is a tap plays the theme's curve tokens (duration and easing, scaled bytempo), never a spring, since a tap carries no velocity; no character or option plays a platform-measured curve for it (owner decision, 2026-11-04)." The growth follows this line: "In a component whose main interaction is a tap, a motion a tap, key or the program starts whose record names no duration step, and for which the platform has a measured duration, plays the theme duration token whose value in the Storybook theme (neutral) is nearest that measurement, with the easing for its kind (entrance, exit or move); the measured value stays a reference in the component's motion table." The shrink follows: "A motion whose record names its step keeps it: a tap-started overlay exit plays--duration-exitand--ease-exit(FR7); an in-place toggle cross-fade is not an exit, andcomponent:ProgressBarFR4's fill keeps--duration-medium."component:PopoverFR9 to name the growth's tokens,--duration-medium-minand--ease-entrance, and the 206 ms measurement they are chosen against, followingspec:AST-094FR45's nearest-step rule. Its state reads "Implemented; gap: the open-to-close latch waits for the shared presentation hold".component:PopoverPR4 (Specs decision, Nov 4 2026): on a page with no other frame work, the touch growth and shrink do no Layout or Paint per frame. A known platform cost: in Chromium, a page with its ownrequestAnimationFrameloop makes an anchored, animating layer lay out and paint every frame.spec:AST-094FR35 holds input while a touch presentation enters. This change records Popover's one exception beside the code that opens it.spec:AST-094's Current-state impact gains: "Popover's input hold (FR35) starts on the first frame after the open, so a touch that lands within that frame reaches the page."component:Popovergains a Current-state impact section: "The input hold starts on the first frame after the open; a touch landing within that frame reaches the page."architecture:layer-runtimeINV14 gives presentation holds to the runtime, so FR9 records the latch as its gap.Writing rubric results
This change edits one row of
component:Popover(FR9: its invariant and its state). It also adds one performance line, PR4, and its verification row, and one Current-state impact line. Inspec:AST-094's Current-state impact, one sentence joins the bullet for DropdownMenu, ContextMenu and Popover.Public delta and caller burden
No public API change and no caller decision. The behavior changes only under a coarse primary pointer.
useLayerandusePopoverkeep their public options; the close start travels throughuseLayerInternalandusePopoverInternal.Design and alternatives
motion/uikit/popover.ts): it grows from a 0.01 scale and a 100 pt length.holdInput(element, {until: growth, scope: 'viewport'})from the layer runtime. The hold's shield covers the viewport from inside the layer, so a tap on it is a tap inside the popover, which the browser's light dismiss leaves open.data-astryx-popover-exitattribute has its owner module,privateAttributes/popoverExit.stylex.ts, which exports a writer and a reader.Evidence
At this head (51f8457) and at the unit's top (#9823's head, 0c9ab62), which contains it:
bank/C27/8187-red-first-e1-ryon the fork) has the old spring motion and fails 5 of 17, the token-timed motion cases; f782377 (bank/C27/8187-red-first-e3-ry) restores only the previous growth token,--duration-fast-max, and fails 4 of 17. c143208 (bank/C27/8187-red-first-e2-fold) is this head without the hold: its three hold tests fail, 3 of 20. None is for landing. This head passes 20 of 20. 58b937b (bank/C27/8187-red-first-nextframe) is this head without the next-frame start: four cases fail, 4 of 95 in the surface-motion and Popover suites, and this head passes 95 of 95. The close-start case inuseLayer.test.tsxfails without theuseLayerchange.useLayer.test.tsx, the UIKit table's source checks over the two new reference values, and the token test, which reads the Storybook theme's motion scale from its own source. CI runs the full suites.PopoverTouchMotion.interaction.browser.spec.ts, from test(Popover): browser proofs of the touch presentation's grow and shrink #9823, at test(Popover): browser proofs of the touch presentation's grow and shrink #9823's head): Chromium 14 of 14; WebKit 9 of 9, with the 5 Chromium-only proofs skipped. With chore(core): the layer runtime's exit copy, for a touch presentation's close #8186's exit-copy spec and the input-hold spec: Chromium 33 of 33; WebKit 26 of 26, with 7 Chromium-only cases skipped. The rendering proof and the blocked-main-thread proof each pass 5 of 5, 5 of 5 with the browser held to 2 cores at 1 worker, and 5 of 5 with 3 busy loops on those 2 cores. The rendering proof takes its windows from when each scale animation starts, without the harness's per-frame animation recorder (test(Popover): browser proofs of the touch presentation's grow and shrink #9823 has the detail): the growth and the shrink do no Layout or Paint per frame.showPopover), which is unchanged here; the presentation's own reads and writes force none.PopoverSecondDocumentandMobileNavTouchPresentation29/29.popover-uikit-presentation.md) adds: "A tap that lands while it grows is ignored, and a pan started then scrolls nothing." and "The growth starts in the next animation frame, before that frame paints, so the opening tap does no extra layout."CI=trueon the changed files, the public text check, a full build, typecheck and the unit suites above exit 0. At the unit's top, the Storybook build and the browser proofs exit 0.@astryxdesign/core/Popoverfrom each builtpackages/core/dist, minified, with React and StyleX external, gzip at the default level (architecture:component-style-authoringINV6). Againstmain(711acc6, 100,899 B), test(Popover): browser proofs of the touch presentation's grow and shrink #9823's head is 107,612 B (+6,713 B). Without the shared modules the import reaches (the motion engine with its spring, role and reduced-motion modules, the theme-timing reader, and the layer runtime's exit copy and input hold, which a page carries once), Popover goes from 87,613 B to 89,508 B (+1,895 B), under the 2 KB touch-only budget.astryx.cssis 41,946 B gzip, againstmain's 41,986 B.Scope
Testing
The two interaction commands run from #9823's head.