Repository navigation
chore(core): the layer runtime's exit copy, for a touch presentation's close - #8186
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
github-actions
Bot
added
the
needs:spec-owner-review
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This pull request targets main, so it currently carries its whole open stack (153 files, about 28,600 changed lines) rather than just the exit-copy commit, and approval waits on two things. First, the head adds draft requirements that main does not state (spec:AST-094 FR1–FR23 with DEC-1/DEC-2, the touchOpen: 'press' clause and DEC-8 in useMenuPress.spec.md, and the press-open sentence of DropdownMenu.spec.md FR8) and proposes amending current records (theme-tokens INV3, user-states DR1/DR3, the buttons press treatment and AST-025 FR14), so an engineering owner needs to approve or reject them by commenting /approve-spec followed by the full head SHA on this pull request; the author may do so if they are an engineering owner. Second, please land the stack first and rebase so this pull request contains only the exit-copy commit, bring useMenuPress.spec.md in line with the shipped 150 ms hold-open, 10 px handoff and continueGesture (it still says 500 ms and the settle rule), and add a nested sub-menu case for the new outside-pan dismissal.
[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 the long-press delay as 500 ms, but hooks/useMenuPress.ts:90 now defaults to menu.holdOpenMs (150 ms, plus 150 ms inside a scrolling region). Please update the row to what the code ships.
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.
| | 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.
FR9 says both modes continue “with the same settle rule”, but a finger now picks only after MENU_PRESS_HANDOFF_PX (10 px) of travel (hooks/menuPressGesture.ts:57), so a held finger released over a row before 10 px acts on nothing. Stating that here, and listing continueGesture in the public-contract row, would keep the record in line with 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.
| popover.hide(); | ||
| }, [popover]); | ||
|
|
||
| // A pan that starts outside the open menu, in its touch presentation, |
There was a problem hiding this comment.
This outside-pan dismissal is exercised only with the root menu open. The layer-runtime architecture's change coupling asks a local outside or touch change to verify nested surfaces: could you add a case with an open sub-menu flyout (and a header/footer sub-menu dialog), for a pan that starts outside the menu and one that starts inside the sub-menu?
There was a problem hiding this comment.
This dismissal comes from the DropdownMenu change below this one in the stack. The rule now lives in the layer runtime's usePanOutsideDismiss (#8198), which adds the nested cases: a pan inside a parent popover but outside its child closes only the child, and a pan in a dialog outside its popover closes only the popover. This pull request only adds the exit copy.
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| LayerProvider | 4.1 kB (0) · 20 B CSS (0) | — | ✓ · ✓ · ✗ |
| SideNav | 81.8 kB (+216 B) · 10.7 kB CSS (0) | — | — · — · — |
| AppShell | 81.7 kB (+175 B) · 12.8 kB CSS (0) | — | — · — · — |
112 more components
| Component | Size | LoC | Visual · A11y · RTL |
|---|---|---|---|
| charts/Chart | 173.8 kB (0) · 494 B CSS (0) | — | — · — · — |
| charts/ChartLegend | 35.6 kB (0) · 224 B CSS (0) | — | — · — · — |
| charts/ChartTooltip | 151.2 kB (0) · 423 B CSS (0) | — | — · — · — |
| AlertDialog | 95.6 kB (0) · 13.7 kB CSS (0) | — | — · — · — |
| Avatar | 39.4 kB (0) · 5.5 kB CSS (0) | — | — · — · — |
| AvatarGroup | 42.1 kB (0) · 5.6 kB CSS (0) | — | — · — · — |
| AvatarGroupOverflow | 44.4 kB (0) · 5.5 kB CSS (0) | — | — · — · — |
| Banner | 53.5 kB (0) · 9.8 kB CSS (0) | — | — · — · — |
| BaseTypeahead | 61.9 kB (0) · 10.3 kB CSS (0) | — | — · — · — |
| BreadcrumbItem | 75.7 kB (0) · 11.5 kB CSS (0) | — | — · — · — |
| Button | 51.5 kB (0) · 9.3 kB CSS (0) | — | — · — · — |
| Calendar | 62.4 kB (0) · 9.9 kB CSS (0) | — | — · — · — |
| Carousel | 59.4 kB (0) · 10.3 kB CSS (0) | — | — · — · — |
| ChatComposer | 68.2 kB (0) · 11.2 kB CSS (0) | — | — · — · — |
| ChatComposerInput | 66.2 kB (0) · 10.6 kB CSS (0) | — | — · — · — |
| ChatComposerTokenElement | 66.2 kB (0) · 10.6 kB CSS (0) | — | — · — · — |
| ChatDictationButton | 52.1 kB (0) · 9.4 kB CSS (0) | — | — · — · — |
| ChatLayout | 54.9 kB (0) · 9.8 kB CSS (0) | — | — · — · — |
| ChatLayoutScrollButton | 52.0 kB (0) · 9.5 kB CSS (0) | — | — · — · — |
| ChatMessageList | 23.8 kB (0) · 6.9 kB CSS (0) | — | — · — · — |
| ChatSendButton | 52.0 kB (0) · 9.3 kB CSS (0) | — | — · — · — |
| ChatToolCalls | 27.7 kB (0) · 8.3 kB CSS (0) | — | — · — · — |
| CheckboxInput | 59.9 kB (0) · 9.0 kB CSS (0) | — | — · — · — |
| CheckboxList | 50.0 kB (0) · 7.5 kB CSS (0) | — | — · — · — |
| CheckboxListItem | 66.1 kB (0) · 10.9 kB CSS (0) | — | — · — · — |
| CodeBlock | 66.1 kB (0) · 10.6 kB CSS (0) | — | — · — · — |
| CommandPalette | 59.3 kB (0) · 12.4 kB CSS (0) | — | — · — · — |
| CommandPaletteInput | 27.9 kB (0) · 7.5 kB CSS (0) | — | — · — · — |
| ComplexSelector | 62.4 kB (0) · 11.0 kB CSS (0) | — | — · — · — |
| ContextMenu | 90.2 kB (0) · 16.5 kB CSS (0) | — | — · — · — |
| DateInput | 167.1 kB (0) · 16.1 kB CSS (0) | — | — · — · — |
| DateRangeInput | 81.3 kB (0) · 11.8 kB CSS (0) | — | — · — · — |
| DateTimeInput | 193.4 kB (0) · 18.0 kB CSS (0) | — | — · — · — |
| DialogHeader | 54.4 kB (0) · 10.8 kB CSS (0) | — | — · — · — |
| DrawerHeader | 54.6 kB (0) · 12.2 kB CSS (0) | — | — · — · — |
| DropdownMenu | 85.8 kB (0) · 16.6 kB CSS (0) | — | — · — · — |
| DropdownMenuSubMenu | 64.5 kB (0) · 9.4 kB CSS (0) | — | — · — · — |
| Field | 46.7 kB (0) · 6.7 kB CSS (0) | — | — · — · — |
| FieldLabel | 42.9 kB (0) · 5.2 kB CSS (0) | — | — · — · — |
| FileInput | 60.2 kB (0) · 10.3 kB CSS (0) | — | — · — · — |
| Heading | 8.0 kB (0) · 5.3 kB CSS (0) | — | — · — · — |
| HoverCard | 24.3 kB (0) · 4.3 kB CSS (0) | — | — · — · — |
| IconButton | 51.6 kB (0) · 9.3 kB CSS (0) | — | — · — · — |
| InfoTip | 43.0 kB (0) · 5.3 kB CSS (0) | — | — · — · — |
| InputGroup | 48.5 kB (0) · 7.3 kB CSS (0) | — | — · — · — |
| Lightbox | 59.7 kB (0) · 9.7 kB CSS (0) | — | — · — · — |
| Link | 48.5 kB (0) · 6.9 kB CSS (0) | — | — · — · — |
| ListItem | 65.2 kB (0) · 10.9 kB CSS (0) | — | — · — · — |
| Markdown | 161.4 kB (0) · 19.5 kB CSS (0) | — | — · — · — |
| MobileNav | 61.5 kB (0) · 9.9 kB CSS (0) | — | — · — · — |
| MobileNavToggle | 51.9 kB (0) · 9.3 kB CSS (0) | — | — · — · — |
| MoreMenu | 86.1 kB (0) · 16.6 kB CSS (0) | — | — · — · — |
| MultiSelector | 88.2 kB (0) · 16.2 kB CSS (0) | — | — · — · — |
| NumberInput | 62.8 kB (0) · 10.7 kB CSS (0) | — | — · — · — |
| Pagination | 98.9 kB (0) · 17.0 kB CSS (0) | — | — · — · — |
| Popover | 60.8 kB (0) · 13.4 kB CSS (0) | — | — · — · — |
| PowerSearch | 264.2 kB (0) · 23.1 kB CSS (0) | — | — · — · — |
| ProgressBar | 11.5 kB (0) · 6.2 kB CSS (0) | — | — · — · — |
| RadioList | 48.2 kB (0) · 6.8 kB CSS (0) | — | — · — · — |
| RadioListItem | 62.9 kB (0) · 9.7 kB CSS (0) | — | — · — · — |
| SegmentedControl | 89.0 kB (0) · 6.8 kB CSS (0) | — | — · — · — |
| Selector | 88.8 kB (0) · 16.6 kB CSS (0) | — | — · — · — |
| SideNavCollapseButton | 52.4 kB (0) · 9.4 kB CSS (0) | — | — · — · — |
| SideNavHeading | 66.6 kB (0) · 10.6 kB CSS (0) | — | — · — · — |
| SideNavItem | 61.6 kB (0) · 10.4 kB CSS (0) | — | — · — · — |
| Slider | 97.0 kB (0) · 8.3 kB CSS (0) | — | — · — · — |
| Spinner | 20.3 kB (0) · 6.1 kB CSS (0) | — | — · — · — |
| StatusDot | 27.5 kB (0) · 5.3 kB CSS (0) | — | — · — · — |
| Stepper | 53.3 kB (0) · 9.5 kB CSS (0) | — | — · — · — |
| Switch | 94.8 kB (0) · 9.5 kB CSS (0) | — | — · — · — |
| Table | 99.6 kB (0) · 17.2 kB CSS (0) | — | — · — · — |
| TableCell | 91.4 kB (0) · 16.9 kB CSS (0) | — | — · — · — |
| TableHeaderCell | 91.0 kB (0) · 16.7 kB CSS (0) | — | — · — · — |
| TableSelectionToolbar | 61.1 kB (0) · 12.4 kB CSS (0) | — | — · — · — |
| TabList | 40.1 kB (0) · 6.4 kB CSS (0) | — | — · — · — |
| TabMenu | 63.0 kB (0) · 10.4 kB CSS (0) | — | — · — · — |
| Text | 7.5 kB (0) · 5.3 kB CSS (0) | — | — · — · — |
| TextArea | 53.8 kB (0) · 8.9 kB CSS (0) | — | — · — · — |
| TextInput | 59.4 kB (0) · 10.5 kB CSS (0) | — | — · — · — |
| Thumbnail | 55.9 kB (0) · 10.9 kB CSS (0) | — | — · — · — |
| TimeInput | 144.0 kB (0) · 15.1 kB CSS (0) | — | — · — · — |
| Timer | 9.6 kB (0) · 5.9 kB CSS (0) | — | — · — · — |
| Timestamp | 23.0 kB (0) · 9.6 kB CSS (0) | — | — · — · — |
| Toast | 89.1 kB (0) · 9.6 kB CSS (0) | — | — · — · — |
| ToggleButton | 52.4 kB (0) · 9.4 kB CSS (0) | — | — · — · — |
| Tokenizer | 80.3 kB (0) · 17.8 kB CSS (0) | — | — · — · — |
| Toolbar | 39.4 kB (0) · 7.9 kB CSS (0) | — | — · — · — |
| Tooltip | 25.8 kB (0) · 4.3 kB CSS (0) | — | — · — · — |
| TopNav | 63.1 kB (0) · 10.2 kB CSS (0) | — | — · — · — |
| TopNavHeading | 66.0 kB (0) · 10.4 kB CSS (0) | — | — · — · — |
| TopNavMegaMenu | 60.9 kB (0) · 11.2 kB CSS (0) | — | — · — · — |
| TopNavMenu | 65.8 kB (0) · 10.5 kB CSS (0) | — | — · — · — |
| Typeahead | 69.9 kB (0) · 11.8 kB CSS (0) | — | — · — · — |
| lab/ChatEmojiPicker | 74.6 kB (0) · 773 B CSS (0) | — | — · — · — |
| lab/ChatReactionBar | 75.2 kB (0) · 1.1 kB CSS (0) | — | — · — · — |
| lab/CommentComposer | 115.4 kB (0) · 1.9 kB CSS (0) | — | — · — · — |
| lab/CommentPin | 52.6 kB (0) · 1.2 kB CSS (0) | — | — · — · — |
| lab/Comments | 281.7 kB (0) · 2.5 kB CSS (0) | — | — · — · — |
| lab/CommentThread | 275.9 kB (0) · 2.4 kB CSS (0) | — | — · — · — |
| lab/DiffViewer | 77.9 kB (0) · 2.1 kB CSS (0) | — | — · — · — |
| lab/EmojiPicker | 77.6 kB (0) · 751 B CSS (0) | — | — · — · — |
| lab/ListInput | 91.0 kB (0) · 1.6 kB CSS (0) | — | — · — · — |
| lab/Schedule | 98.8 kB (0) · 3.9 kB CSS (0) | — | — · — · — |
| lab/SelectableCardGroup | 56.1 kB (0) · 162 B CSS (0) | — | — · — · — |
| lab/TourStep | 66.9 kB (0) · 590 B CSS (0) | — | — · — · — |
| lab/TransferList | 85.4 kB (0) · 1.7 kB CSS (0) | — | — · — · — |
| lab/TransferListSelector | 92.4 kB (0) · 1.9 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditor | 290.6 kB (0) · 2.6 kB CSS (0) | — | — · — · — |
| richtext/RichTextEditorToolbar | 239.7 kB (0) · 454 B CSS (0) | — | — · — · — |
| richtext/RichTextView | 270.9 kB (0) · 2.3 kB CSS (0) | — | — · — · — |
| layer | — | — | — · ✓ · — |
| Layer | — | — | ✓ · ✓ · — |
Package size: @astryxdesign/core 6.72 MB packed (+8.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
05b28df to
90f052b
Compare
github-actions
Bot
added
the
needs:design-review
github-actions
Bot
removed
the
needs:spec-owner-review
90f052b to
d8da416
Compare
vjeux
changed the title
There was a problem hiding this comment.
This pull request targets main, so it carries its whole open stack (194 files, about 31,000 changed lines) rather than only the exit-copy commit, and approval waits on an engineering owner's decision about the draft spec:AST-094 (FR1–FR23, DEC-1, DEC-2), its proposed amendments to current records, and the current claim that should govern the layer runtime's exit copy. An engineering owner records that decision by commenting /approve-spec followed by the full head SHA on this pull request, and the author may do so if they are an engineering owner; otherwise the record is settled in its own pull request. Independently of that decision, please:
- land the stack first and rebase so this pull request contains only the exit-copy commit;
- measure the copy's placement without the transform it copies from running animations, since in Chromium a layer caught at
scale: 0.8 0.6gets a copy 32 px away; - remove
nameandformfrom cloned form controls, since inserting the copy unchecks the layer's checked radio for good; - replace the
position: fixedandz-index: 2147483647fallback, which current AST-027 FR1 and FR8 rule out, and add real-browser evidence for the copy as AST-013 FR10 requires; - fix the changesets that name the refused role
timing.reducedMotionCrossFadeMsand contradict each other on Reduce Motion.
[Automated review]
| const place = () => { | ||
| style.setProperty('left', `${box.left}px`); | ||
| style.setProperty('top', `${box.top}px`); | ||
| const landed = copy.getBoundingClientRect(); |
There was a problem hiding this comment.
This measures the copy after commitPresented has written the running curve's scale/translate/transform, so the correction cancels the transform's offset too. In Chromium, a layer at left 100 caught at scale: 0.8 0.6 about 160px 0 stands at 132, and its copy is placed at left: 68px and shows at 100, 32 px away. Measuring before commitPresented (or with the transform properties set to none) keeps the copy where the layer stands.
| if (topLayer) { | ||
| copy.setAttribute('popover', 'manual'); | ||
| } | ||
| element.after(copy); |
There was a problem hiding this comment.
Inserting the deep clone here puts a checked radio with the same name and form into the document, which unchecks the original radio in the layer, and removing the copy later does not restore it (checked before, unchecked after, in Chromium). Removing name and form from the cloned form controls before this line avoids it.
| ); | ||
| style.setProperty('overflow', 'visible'); | ||
| // On top of the stacking order until it is in the top layer. | ||
| style.setProperty('z-index', '2147483647'); |
There was a problem hiding this comment.
A page-level z-index on a fixed box is what AST-027 FR1 and FR8 rule out (“a fixed position, or a larger number is not a substitute” for the top layer). Where the popover API is missing the copy keeps this for the whole close; could the fallback follow the layer runtime's reduced path, or skip the animated close?
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [feat] Themes can retune touch motion roles. `defineTheme({motion})` takes `springs`, `durations` and `curves` keyed by motion role, the table path of a measured touch motion value (`springs.sheetPresent`, `timing.reducedMotionCrossFadeMs`), at the root or in an adaptation rule, and writes one `--motion-*` variable per role it sets. A role the theme leaves out keeps its measured default, an unknown role id is refused, and the duration scale (`fast`, `medium`, `ratio`) is optional when a theme sets only roles. No shipped component reads the roles yet. Adaptation rules gain a `platform` condition: `ios` for the browsers of iPhone and iPad, or `other`. `astryx docs motion` lists every motion role with its CSS variable and default, and the theme authoring guide and template describe the new fields. |
There was a problem hiding this comment.
timing.reducedMotionCrossFadeMs is not a role on this head: expandMotionRoles throws “is not a motion role”, and reduced-motion-record.md says it is no longer a theme role. A different example role would keep the release note accurate.
|
Measured the light-dismiss case at this head under a 2-core load, 20 runs per arm:
So the case is green on CI's Chromium today and flaky from 150. A follow-up opens as soon as this stack merges, ahead of any Playwright update to Chromium 150 or later. It carries the frame wait (red first against the shim), pruning |
|
I'll run a full review to re-check this with your note. |
|
An update on There is no retention test because a test cannot force garbage collection, so none could fail on the old code. A unit test instead checks that an inner layer's copy is still shown above the outer layer's copy. |
|
I'll run a full review to re-check this with your note. |
|
/approve-spec 1cc771f |
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #8223.
The exit copy found the inner layers to raise through a module-level set of every copied layer. A layer React removed on close stayed reachable there until a later close showed a copy. The inner layers are inside the closing layer, so they are now found there, and nothing keeps a layer once it is gone. Chromium 150 and later refuse a show during another popover's toggle, so the copy can open in the next frame. The browser spec's light-dismiss case now reads the copy after a frame, as its nested-close cases do. The cap check's comment now names only the copy cap.
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. This is a draft from a fork, so CI runs once it is marked ready for review and a maintainer adds the ci:run label.
The exit copy is the one shared copy architecture:layer-runtime INV14 gives the runtime, it is shown only in the top layer as spec:AST-027 FR8 requires, and the layer-runtime text it adds has an engineering owner's approval for this head. No blocking issues.
- Miss: non-blocking, the
placeLayerExitCopyJSDoc states the empty-canvas rule twice (layerExitCopy.ts:160-161and:177-179); one sentence is enough. - The stack line in this description ends in #8662, while #8187 and #9823 end in #8223; make them identical.
[Automated review]
| * Its frames and media load nothing, and a canvas shows its pixels. Content | ||
| * holding a canvas that reads back empty gets no copy: the close is then | ||
| * immediate. Its descendants' keyframe animations do not start again. A |
There was a problem hiding this comment.
Miss, non-blocking (comment style): this repeats the empty-canvas rule from lines 160-161 and inertCopy's own contract. Suggested: drop "Its frames and media load nothing, and a canvas shows its pixels. Content holding a canvas that reads back empty gets no copy: the close is then immediate." and keep the statement in the first paragraph.
There was a problem hiding this comment.
Dropped in 0e69c33: the rule stays once, in the first paragraph.
The empty-canvas rule stays in the first paragraph, and the inert-copy paragraph no longer repeats it or `inertCopy`'s own contract.
There was a problem hiding this comment.
Approved pending CI: CI has not run on this head yet. This is a draft from a fork, so CI runs once it is marked ready for review and a maintainer adds the ci:run label.
The exit copy is the one shared copy architecture:layer-runtime INV14 gives the runtime, it is shown only in the top layer as spec:AST-027 FR8 requires, and the layer-runtime text it adds has an engineering owner's approval for this head. No blocking issues; the JSDoc now states the empty-canvas rule once.
- The stack line in this description ends in #8662, while #8187 and #9823 end in #8223; make them identical so the stack is reviewed as one.
[Automated review]
|
/approve-spec 0e69c33 |
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #8662.
…tion table keeps both rows)
|
/approve-spec 223875a |
There was a problem hiding this comment.
This pull request is reviewed as part of its stack; see the review on #8662.
|
/approve-spec 223875a |
|
/approve-spec 6ecb859 |
There was a problem hiding this comment.
Recommended for approval: no blocking issues at this head.
The exit copy follows the layer runtime's ownership of the one shared exit copy and the top-layer rule, the new layer-runtime Exit copy text and INV13 carry an engineering owner's approval for this exact head, and the unit tests and the browser spec pass in Chromium 149 and in Chromium 151, which refuses a show during another popover's toggle. CI is green on this head.
- Miss (non-blocking): the JSDoc on
isFormAssociated(packages/core/src/motion/transientCopy.ts:164) restates the function's name and can be dropped.
[Automated review]
| 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.
Miss, non-blocking (INV29): this JSDoc restates the function's name. Drop it; the comment above the form="" loop already gives the reason.
…side keepInherited)
There was a problem hiding this comment.
Recommended for approval: no blocking issues at this head, and CI is green.
The exit copy follows the layer runtime's ownership of the one shared exit copy and the top-layer rule, the new layer-runtime Exit copy text and INV13 carry an engineering owner's approval for this head, and the merge of main keeps the swap's new copy placement beside the shared inherited-value step.
- Non-blocking: the JSDoc on
isFormAssociated(packages/core/src/motion/transientCopy.ts:164) restates the function's name and can be dropped.
[Automated review]
|
/approve-spec d7905ea |
|
/approve-spec c2dc518 |
There was a problem hiding this comment.
Recommended for approval: no blocking issues at this head, and CI is green.
The exit copy follows the layer runtime's ownership of the one shared exit copy and the top-layer rule, the new layer-runtime Exit copy text and INV13 carry an engineering owner's approval for this head, and the latest merge of main changes none of this pull request's files.
- Non-blocking: the JSDoc on
isFormAssociated(packages/core/src/motion/transientCopy.ts:164) restates the function's name and can be dropped.
[Automated review]
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.
|
/approve-spec 30e42ae |
There was a problem hiding this comment.
Recommended for approval, pending CI: the required checks are still running on this head.
The exit copy follows the layer runtime's ownership of the one shared exit copy and the top-layer rule, the new layer-runtime Exit copy text and INV13 carry an engineering owner's approval for this head, and the latest commit reads the host's style before the copy is inserted without changing what the copy inherits.
- Non-blocking: the JSDoc on
isFormAssociated(packages/core/src/motion/transientCopy.ts:164) restates the function's name and can be dropped.
[Automated review]
Stack: #8186 → #8187 → #9823
Merge group: #8186, #8187, #9823
Touch exits animate on the theme's exit tokens (
spec:AST-094FR7); the copy is what they play on. No behavior change: the first caller is #8187, stacked on it.Why
The browser hides a popover the moment it closes, so a touch presentation has nothing left on screen to animate.
What led to it
DropdownMenu (#8080) and Popover (#8187) each need a copy to close on: no duplicate ids or form state, no
z-indexfight.Options and tradeoffs
Rejected: a copy per presentation (every fix lands once per component); keeping the real popover open through the animation (screen readers would find a closed layer). Chosen: one helper the layer runtime owns.
Why this choice
The real popover closes at once for focus and assistive technology; an inert copy plays the motion in the top layer.
architecture:layer-runtimeINV13 records it, and that the presentation plays its exit on the copy (spec:AST-094FR7).Review fixes
scaleandopacitystay; the box isuseLayer'ssettledBox; content past the copy cap gets no copy.inertCopycopies join no form and carry no anchor name.inertCopy: no test ids, label targets, autofocus or loading media; an empty canvas means no copy.restoreScrollOffsets, instantly.placeOver(a clipping ancestor cuts its host) andafterPopoverOperations(a task late).How to check it
layerExitCopyinteraction spec in Chromium and in WebKit.Writing rubric results
R1 narration, R2 research, R4 rejected alternatives and R5 change references: none. R3 evidence: INV13's row names the two test files. R6 open question: none.
Scope
Testing
At 30e42ae, a closing layer inside another layer reads its host's style before its copy is inserted there. So no style read follows that write (
architecture:react-component-runtimeINV18). The new unit case fails without the fix (e2b89fe) and passes with it. Layer, motion and Popover units: 1,479 passed; the exit-copy spec: Chromium ×3 15/15, WebKit 5/5.At 0e69c33, the exit copy's JSDoc states the empty-canvas rule once; only that comment changes, and the gates below carry over.
At 544ddee, a close finds the inner copies to raise inside the closing layer, so no module-level set keeps a closed layer; a unit case keeps the inner copy above the outer one. The light-dismiss case reads the copy after a frame, and the cap check's comment names only the copy cap. Local gates there: the core type check,
pnpm lint, strict lint, Prettier, the knowledge, changeset and SYNC checks,review:pre, the Layer, Popover and motion units (1,470 of 1,470), and the layerExitCopy spec in Chromium (25 of 25, five runs each) and in WebKit through the container recipe (15 of 15, three runs each). NOT-FOR-LANDING arms add a shim that refuses a show during another popover's toggle, as Chromium 150 and later do: without the frame wait (a223582) the light-dismiss case fails 11 of 20; with it (0328f70) it passes 20 of 20.At 1cc771f, main's
Layer/settledBox.ts(from #8208) is kept, with its header naming the exit copy as a reader: this pull request's code there was the same. Its other files carry the same change as before.At 1fda019, the browser spec's two nested-close cases wait one frame after the outer layer's close before they read, as its first case does: where the browser refuses a show during the toggle, the copy opens after the closing script. Only the spec changes. NOT-FOR-LANDING arms add a page shim that refuses a show during another popover's toggle: on the previous synchronous reads (c15460b) both cases fail, 4 of 4 runs; on this head's reads (f565842) all pass, in Chromium and WebKit. At this head: Prettier, lint, strict lint,
review:pre, and the spec twice per case in Chromium (10 of 10) and in WebKit through the container recipe (10 of 10).At 7b80000, the commit is rebased onto main 57aa93c, which adds
exitAnimationstoLayer/useLayer.tsxin the block this pull request moves intosettledBox.ts. The resolution keepsexitAnimationsinuseLayer.tsxand moves the rest as before; every other file carries the same change, and the record's added and removed lines are unchanged. Local gates there: the core type check,pnpm lint, strict lint, Prettier, the knowledge, changeset and SYNC checks,review:pre, the coreuisuite (18,704 passed; the 4 RadioListItem and Switch source-build tests fail the same way on main in this checkout), and, on a fresh Storybook build, the Layer, layout motion and closed-trigger-anchor interaction specs in Chromium (132 of 132), the exit-copy and closed-trigger-anchor specs in WebKit through the container recipe (7 passed; 1 case that main skips in WebKit), and the Layer, Popover and Tooltip a11y specs in Chromium (100 of 100).This head is the commit rebased onto main 8ddcfe1, which makes a layout motion's copy carry on its running CSS animations. Two conflicts in
motion/transientCopy.tswere resolved by keeping both sides:inertCopy's description names both the anchor name and the carried animations, andplaceOverrestores scroll offsets throughrestoreScrollOffsetsand then carries the animations as main does. The two copies differ on purpose. A layout motion's copy is laid out byplaceOver, and the CSS animations running in it carry on. The layer's exit copy never goes throughplaceOver: it writes the values its running animations hold, then stops them (animation-name: none), so its close plays on a still picture, as before the rebase. Six long comment sentences in the exit copy's source and tests are split, with no code change. Local gates at 634a5a8: the core type check,pnpm lint, strict lint, Prettier, the knowledge and changeset checks,review:pre, the coreuisuite (18,342 passed; the RadioListItem and Switch source-build tests fail the same way on main in this checkout, and a Selector bottom-sheet case that timed out on the loaded box passes three runs of three here and on main), and, on a fresh Storybook build, the layerExitCopy spec and main's layout motion and animator specs in Chromium (108 of 108), layerExitCopy in WebKit through the container recipe (5 of 5), and the Layer and Popover a11y specs in Chromium (66 of 66). Earlier: this head is e1d8e3a's commit rebased onto main f10bc91 with no conflict. Main now bars@playwright/testin browser specs, solayerExitCopy.interaction.browser.spec.tsimports from@astryxdesign/a11y-spec/playwright; every other line of the patch matches e1d8e3a's. Local gates at b83240b: the core type check,pnpm lint, strict lint (a copy with the old import trips the rule), Prettier, the knowledge and changeset checks,review:pre, the coreuisuite (18,156 passed; the RadioListItem and Switch source-build tests fail the same way on main in this checkout because a Babel preset does not resolve), and, on a fresh Storybook build, the layerExitCopy spec in Chromium (5 of 5) and in WebKit through the container recipe (5 of 5) and the Layer and Popover a11y specs in Chromium (66 of 66).Local gates at e1d8e3a: the core type check, strict lint, and the layerExitCopy Chromium spec (5 of 5, twice). At 4bf41537b5, which lacks only one cleanup line: the motion, Layer, Popover, DropdownMenu, Tooltip, ContextMenu and Selector units (2,208 of 2,208), Prettier,
review:preand the spec three times. Red arms (NOT-FOR-LANDING): withoutcolor-schemein the inherited list (3f1da31840), the nested copy'slight-dark()background is white and the unit case fails; without showing inner copies again (aeab98d83d), the outer copy covers the inner one. At bb99789: the core type check, strict lint, Prettier,review:pre, and the layerExitCopy Chromium spec (4 of 4, twice). The motion, Layer, Popover, DropdownMenu, Tooltip, ContextMenu and Selector units pass at c9f3aacd39 (2,208 of 2,208), which differs from this head only in one spec line. With 93f7cca's two sources under this head's tests (NOT-FOR-LANDING arm 2fcdab73dd), both new cases fail: the nested copy is not drawn. At 93f7cca: the motion, Layer, Popover, DropdownMenu, Tooltip, ContextMenu and Selector units (2,207 of 2,207), the core type check, strict lint, Prettier andreview:pre(no findings) pass. With 536095d'sinertCopyunder this head's tests (NOT-FOR-LANDING arm dd4a525f51), the new anchor-name case fails. At 536095d: the Layer, motion, swap-adopter and menu units (2,241 of 2,241), the core type check, strict lint, the knowledge check andreview:pre(no findings) pass. With main'sinertCopyunder this head's tests (NOT-FOR-LANDING arm 1be8016635), the new form case fails. Replayed onto main 2188aa9 from 5b3a49f with an equal own diff (4c7e8a797d), then the changes above.