Repository navigation
test(Popover): browser proofs of the touch presentation's grow and shrink - #9823
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
added
the
needs:spec-owner-review
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
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not ready yet: two spec decisions need an engineering owner, and four changes are needed after that.
Needs an owner decision
- Popover's touch grow and shrink play the theme's entrance and exit tokens, but the current AST-094 FR4, FR7 and FR31 require named motion roles and UIKit's exit. An engineering owner needs to update those clauses on
main(the open #9706 proposes this), or the motion needs to conform. - The new layer-runtime INV13 and its Exit copy section need an engineering owner's approval, given by commenting
/approve-specwith the full head SHA (the author can do this as an engineering owner).
Blocking
- A popover nested in another layer that closes in the same step loses its shrink, because
placeLayerExitCopyputs the copy inside the closing outer layer (layerExitCopy.ts:221); rebase onto #8186's current head, which lays it after the outermost layer. - A tap outside during the growth closes the popover (at 53 ms and 85 ms in the built story), against AST-094 FR35 and Popover FR9; land the input hold (#8223) with this change and add a tap-during-growth test.
- The branch carries 3,894 authored lines against
main(#8186, #8187 and these proofs); declare the stack with one identicalStack: #8186 → #8187 → #9823line in all three descriptions, or split it. usePopoverSurfaceMotionadds its ownbeforetogglelistener and unmount Effect on the layer, beside the oneuseLayeralready binds; haveuseLayerreport the start of a close and drop the duplicate.
The unit suites and the Chromium proofs pass when run; two non-blocking notes are inline.
[Automated review]
| // clip. A popover's user-agent style scrolls as well. | ||
| style.setProperty('overflow-x', layerStyle.getPropertyValue('overflow-x')); | ||
| style.setProperty('overflow-y', layerStyle.getPropertyValue('overflow-y')); | ||
| element.after(copy); |
There was a problem hiding this comment.
Blocking: when an enclosing layer closes in the same step (for example a tap outside two nested auto popovers), this puts the copy inside the hidden outer layer, so it has a 0 × 0 box and the shrink is never drawn. #8186's current head lays the copy after the outermost layer and keeps what it inherited; rebasing onto it fixes this.
| motion.exit(); | ||
| } | ||
| }; | ||
| element.addEventListener('beforetoggle', handleBeforeToggle); |
There was a problem hiding this comment.
Blocking: useLayer already binds beforetoggle and toggle on this element. A second listener here, plus the unmount Effect that removes it, duplicates that concern. Let useLayer report the start of a close (an internal callback that usePopover passes through) and drop this listener and the Effect.
| * easing. A tap opens the popover, and a tap has no velocity, so no spring | ||
| * plays. | ||
| */ | ||
| export const POPOVER_ENTRANCE_TOKENS: ThemeTimingTokens = { |
There was a problem hiding this comment.
Needs an owner decision: the current AST-094 FR4 and FR31 time touch motion by named motion roles, and FR7 asks for UIKit's exit. These theme-token pairs follow the rule proposed in #9706, which has not landed.
| } | ||
| }; | ||
|
|
||
| const enter = (measure: () => PopoverSurfaceGeometry) => { |
There was a problem hiding this comment.
Blocking: nothing holds input while the growth runs, so a tap outside 53 ms in closes the popover. AST-094 FR35 and Popover FR9 say a touch during the growth is ignored. The hold in #8223 needs to land with this change, with a test.
| ).toBeNull(); | ||
| }); | ||
|
|
||
| it('with a mouse (a fine primary pointer), keeps the layer\u2019s own entrance and closes at once', () => { |
There was a problem hiding this comment.
Non-blocking: this test has the same body as the one at line 1656 under another name. One of them can go.
| }); | ||
| } | ||
|
|
||
| /** The elements a `<form>` can own; `form=""` matches no form's id. */ |
There was a problem hiding this comment.
Non-blocking: this doc comment documents nothing (the next line documents copies). Remove it, as #8186's current head does.
fe384c3 to
1b9a771
Compare
There was a problem hiding this comment.
Not ready yet: two spec decisions need an engineering owner, and one change is needed in any case.
Needs an owner decision
- Popover's touch grow and shrink play the theme's
--duration-fast-max/--ease-entranceand--duration-exit/--ease-exittokens. The current AST-094 FR4, FR7 and FR31 require named motion roles and UIKit's exit, so an engineering owner needs to update those clauses onmain(the open #9706 proposes this), or the motion needs to conform. - The new layer-runtime INV13 and its Exit copy section are new requirements in a current record. An engineering owner approves them by commenting
/approve-specwith the full head SHA, and the author may do so as an engineering owner.
Blocking
- The branch carries 4,219 authored lines against
main(#8186, #8187 and these proofs), and its stack declaration does not hold because #8223's description has noStack:line and #8223 is not stacked on this head; add the identical line to #8223 and rebase it onto this head, or drop #8223 from the line in all three descriptions.
The nested exit copy and the duplicate close listener from the last round are fixed, and the unit suites and Chromium proofs pass. A tap during the growth still closes the popover, as it does on main today; the input hold in #8223 covers that. Three notes are inline.
[Automated review]
| * easing. A tap opens the popover, and a tap has no velocity, so no spring | ||
| * plays. | ||
| */ | ||
| export const POPOVER_ENTRANCE_TOKENS: ThemeTimingTokens = { |
There was a problem hiding this comment.
Needs an owner decision: AST-094 FR4 and FR31 currently time touch motion by named motion roles, and FR7 asks for UIKit's exit. These token pairs follow the rule the open #9706 proposes as FR45, which is not on main yet.
| element: HTMLElement; | ||
| motion: PopoverSurfaceMotion; | ||
| } | null>(null); | ||
| // Whether the presentation in progress is the touch one. |
There was a problem hiding this comment.
Non-blocking: this comment only restates isTouchPresentationRef, so it can go.
| * It is shown only in the top layer, as a manual popover, so a clipping, | ||
| * transformed or sticky ancestor neither cuts nor moves it. A fixed box or a | ||
| * larger `z-index` is no substitute. Where the browser refuses the show | ||
| * during the closing popover's own toggle (Chromium 150 and later), the copy |
There was a problem hiding this comment.
Non-blocking: the browser spec's header says the refusal was seen on Android Chromium 158, and that the engines it runs do not refuse. Name the engine the evidence shows here and in the record's Exit copy section.
1b9a771 to
749d1e5
Compare
|
Updated to 749d1e5, on #8187's new head (caad921), answering the 02:36Z review:
|
There was a problem hiding this comment.
Changes needed: two blocking findings, and approval also waits on a spec change reaching main.
Blocking
- The branch carries 4,215 authored lines against
main(#8186, #8187 and these proofs), and the stack declaration does not hold because #8223's description has noStack:line and #8223 is not stacked on this head; add the identical line to #8223 and rebase it onto this head, or drop #8223 from the line in all three descriptions. - Miss:
popoverSurfaceMotion.tsbuilds its own theme-timed curves, repeating the finite-difference velocity of SegmentedControl'stimedCurveand the channel mapping ofmapProgressinmotion/progress.ts; return one timedMotionCurvedriver frommotion/themeTiming.tsand build the scale and opacity channels withmapProgress.
Waiting on the spec
Popover's touch grow and shrink play the theme's entrance and exit tokens, which the current AST-094 FR4, FR7 and FR31 do not allow yet, so, as an engineering owner decided, approval waits for #9706's FR45 to land on main and for this branch to rebase onto it. The new layer-runtime INV13 and Exit copy text already carry an engineering owner's approval on #8186. Two non-blocking notes stand: a comment that restates isTouchPresentationRef with the long placeLayerExitCopy JSDoc, and the engine named for the refused show.
[Automated review]
| } from './popoverSurfaceFrame'; | ||
|
|
||
| /** The progress per second `t` seconds after the start. */ | ||
| function progressVelocity(timing: ThemeTiming, t: number): number { |
There was a problem hiding this comment.
Blocking (Miss): this repeats the finite-difference velocity of timedCurve in SegmentedControl/useSegmentedSelectionMotion.ts, and surfaceCurves below hand-writes the chain rule that mapProgress in motion/progress.ts owns. Return one timed MotionCurve driver from motion/themeTiming.ts and build the scale and opacity channels with mapProgress.
749d1e5 to
b0a37a1
Compare
|
/approve-spec 5b74d42 |
There was a problem hiding this comment.
Changes requested. This pull request is reviewed on its own: its Stack: line declares #8186 → #8187 → #9823 → #8662, but #8662's head does not contain this head, so the stack does not hold.
Blocking
- Regression: without a holding stack, this branch carries 4,357 authored lines of #8186, #8187 and these proofs with no exemption, so merge this head into #8662 (or rebuild #8662 on it) so the declared stack holds again.
Nothing else blocks: the architecture:layer-runtime INV13 and component:Popover FR9 and PR4 texts are approved for this head, the rest follows spec:AST-094 FR31, FR35, FR45, FR7 and FR5, and the Popover, exit-copy and layer unit suites pass at this head. Once the stack holds, the review on #8662 judges this pull request with the rest of the stack.
[Automated review]
| // The page itself blocks once the growth has run a frame. A block sent from | ||
| // here starts later on a busy machine. | ||
| const opened = await pageNow(page); | ||
| const blocking = blockMainThread(page, 400, { |
There was a problem hiding this comment.
Non-blocking (Miss): spec:AST-094 FR5's verification row asks for an animation that keeps advancing "through a 1 s main-thread block"; this blocks 400 ms inside a 600 ms growth. A 1.2 s growth, as the rendering proof uses, would fit a 1 s block.
There was a problem hiding this comment.
Taking this in a follow-up right after the group merges: a 1 s block inside a 1.2 s growth, as FR5's verification row asks.
| const FORM_CONTROLS = | ||
| 'button, fieldset, input, object, output, select, textarea'; | ||
|
|
||
| /** Whether `node` is a custom element defined as form-associated. */ |
There was a problem hiding this comment.
Non-blocking (Miss): this JSDoc restates the function's name, so it can go.
There was a problem hiding this comment.
That JSDoc is in #8186's exit-copy change; it goes in the docs follow-up right after the group merges.
There was a problem hiding this comment.
Approved. This pull request is judged on its own increment, the Chromium and WebKit proofs of Popover's touch grow, shrink and input hold in PopoverTouchMotion.interaction.browser.spec.ts, with #8186 and #8187 as context. The declared stack holds again now that #8662's head contains this head, so the earlier size finding no longer applies, and CI is green here. Miss (non-blocking): the blocked-thread proof uses a 400 ms block where FR5's verification asks for 1 s, which you have already taken as a follow-up. This pull request lands only when it and every pull request below it (#8186, #8187) are approved.
[Automated review]
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.
The touch-motion spec's cases for the growth that starts on the first frame after the open, folded from meta#8662 with the change they prove.
|
/approve-spec 773765a |
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #8662.
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.
github-actions
Bot
added
the
needs:spec-owner-review
|
/approve-spec 6286e09 |
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
Approved. This review covers the stack #8186 → #8187 → #9823 together; a pull request lands only when it and every pull request below it are approved.
#8186: approve
No blocking findings.
#8187: approve
No blocking findings; the two next-frame record lines now sit with the code that implements them.
#9823: approve
Tests only; no blocking findings. Non-blocking, already noted: the blocked-main-thread proof blocks 400 ms where the touch-presentation spec's FR5 verification asks for a 1 s block, which you have taken as a follow-up.
Whole stack
Pass: the combined change follows the touch-presentation spec and the record text approved for each head, adds no public API, and CI is green.
[Automated review]
There was a problem hiding this comment.
Changes requested. This review covers the stack #8186 → #8187 → #9823 together; a pull request lands only when it and every pull request below it are approved.
#8186: request changes
- Newly live check: when a closing layer sits inside another layer,
layerExitCopy.ts:267-268inserts the exit copy and then reads the host's computed style inkeepInherited, a read after a write thatarchitecture:react-component-runtimeINV18 rules out; callkeepInheritedbeforeouter.after(copy).
#8187: request changes
- Newly live check: the growth's frame callback in
popoverSurfaceMotion.ts:255-298writesanimation-name,scaleandopacity, reads both boxes, writes the origin and start frame, then reads the theme tokens throughtimingOn, which forces six style-and-layout passes in the frame that first shows the popover (none for a mouse) against INV18; read the boxes and the entrance timing in one pass first, then write, and name the read's reason atboxOf.
#9823: approve
Tests only, with no blocking findings; the non-blocking note stands that the blocked-main-thread proof blocks 400 ms where the FR5 verification asks for 1 s.
Whole stack
Fail: the combined change follows the touch-presentation spec and adds no public API, but carries the two layout-read contradictions above.
[Automated review]
| element.style.setProperty('animation-name', 'none'); | ||
| animator.set({scaleX: 1, scaleY: 1, opacity: 0}); | ||
| present(null, measure()); |
There was a problem hiding this comment.
Newly live check (architecture:react-component-runtime INV18): this callback writes animation-name, scale and opacity, then reads both boxes; present() then writes the origin and the start frame and reads the theme tokens through timingOn (line 298) before it plays and shows the hold's shield. In Chromium phone emulation that forces six style-and-layout passes in the frame that first shows the popover; a mouse opening forces none. Read the two boxes and the entrance timing in one pass first, then write. The microtask at lines 267-271 has already turned the CSS entrance off, and settledBox reads a layer's box without its transform.
| outer.after(copy); | ||
| keepInherited(copy, outer.parentElement); |
There was a problem hiding this comment.
Newly live check (architecture:react-component-runtime INV18): keepInherited reads the host's computed style right after outer.after(copy) writes. Call it before the insertion; it reads only the host and writes only on the still-detached copy.
| } from './popoverSurfaceMotion'; | ||
|
|
||
| function boxOf(element: Element): PopoverSurfaceRect { | ||
| const rect = element.getBoundingClientRect(); |
There was a problem hiding this comment.
INV18 also asks each remaining read to name its reason in a comment: one line here (where the popover meets its source) covers it.
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 0c9ab62 |
There was a problem hiding this comment.
Approved pending CI: CI is still running on this head. This review covers the stack #8186 → #8187 → #9823 together; a pull request lands only when it and every pull request below it are approved.
#8186: approve
No blocking findings; the nested exit copy now reads its host's style before it is inserted.
#8187: approve
No blocking findings; the growth's first frame now reads both boxes and the timing before it writes. Non-blocking: the enter JSDoc in popoverSurfaceMotion.ts still says the element is invisible when measure reads it.
#9823: approve
Tests only, with no blocking findings; the non-blocking note stands that the blocked-main-thread proof blocks 400 ms where the FR5 verification asks for 1 s.
Whole stack
Pass: the combined change follows the touch-presentation spec and the layout-read rule, and adds no public API.
[Automated review]
| * `measure` reads the geometry there, with the element invisible at its | ||
| * full size. During a dismissal, the popover grows back from where the |
There was a problem hiding this comment.
Non-blocking (INV29): the frame no longer writes opacity 0 before measure runs, so "with the element invisible" no longer holds. Suggested: "measure reads the geometry there, at the element's full size, before that frame writes."
Stack: #8186 → #8187 → #9823
Merge group: #8186, #8187, #9823
These three pull requests merge back to back, as one unit. This pull request adds only
packages/core/src/Popover/__tests__/PopoverTouchMotion.interaction.browser.spec.ts, on #8187's head (51f8457). Review only this pull request's change: vjeux/astryx@51f8457...0c9ab62. #8187 came to over 2,000 authored lines with these proofs, so they moved here. The cases for the hold while the popover grows came from #8223, which is closed into #8187 and this pull request.Intent
These are real-browser proofs of #8187's touch presentation, in Chromium and in WebKit's engine (the iOS engine). They show what unit tests cannot: compositor frames, a blocked main thread, and the rendering work per frame.
What the proofs check
@storyboard.The spec reads the timing tokens from the story's theme where the popover is. So a theme's values are the ones checked.
The spec imports
testandexpectfrom@astryxdesign/a11y-spec/playwright, asmain's browser specs do.Evidence
At this head (0c9ab62), after
pnpm storybook:build:Element.animatewith no frame loop, and it waits on that start in the page. The harness's animation recorder reads every animation each frame, and on this anchored layer each read is a style update and a layout; armed during the growth, it gave every frame a Layout and a Paint, so the proof measured the recorder instead of the popover. The proof's growth and shrink run 1.2 s (a style rule sets--duration-medium-minand--duration-exiton the layer), and each window starts 400 ms in, after the one-off work as the content appears (the focus ring and the touch hit areas' first measurement), so it spans many frames on a loaded machine too. Both windows must show no Layout and no Paint. A long task is judged by its thread time, which a loaded machine does not stretch. A positive control moves the trigger by a pixel in each frame the page draws over the same span: at least 2 frames are drawn, and every one of them must show work. This holds on a page with no other frame work. With an empty pagerequestAnimationFrameloop running, Chromium lays out and paints the anchored growth every frame (94 Layouts and 94 Paints in its 780 ms window; a local probe, not part of the diff), while the shrink's copy, which is not anchored, still does none. So the zeros also show that no frame loop runs during the motions, Popover's own included.anchor-name. Before that fix, the copy laid out on every frame of the shrink.CI=trueon the spec, the public text check, a full build, typecheck, the Storybook build, and the Popover, motion and Layer unit suites) exits 0 at this head.Testing