Repository navigation
fix(core): focus traps and the keyboard model are one per page across swizzled copies (AST-110 FR3) - #10831
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.
|
|
/approve-spec 1cd7019 |
github-actions
Bot
added
the
needs:design-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved: all four pull requests in the stack pass review, and required CI passed on this head. Each pull request lands only when it and every pull request below it are approved.
#10807: approve. No blocking findings.
#10817: approve. No blocking findings.
#10822: approve. No blocking findings.
#10831: approve. No blocking findings.
Whole stack: pass. The increments build on each other cleanly, and the net public API is unchanged.
Optional follow-ups:
- Add a nested-region Tab case to
focusKeyboard.shared.test.tsx. It is the lead claim of #10831, and a two-load probe confirms the fix. - In
docs/specs/AST-110-swizzle-output/spec.md, the FR3 sentence "unless the allowlist names it with a reason" is stricter than the check, which also acceptsopenentries. The gap line could name the unchecked rows instead of "no check onmain". - The #10807 and #10817 descriptions have stale baseline counts. The #10817 description calls
SharedBrand's member optional, but the code makes it required.
[Automated review]
PR Analysis Report🔎 Preview ·
|
| Component | Size | LoC |
|---|---|---|
| ChatComposer | 79.7 kB (+252 B) · 12.0 kB CSS (0) | — |
| BottomSheet | 69.2 kB (+243 B) · 7.7 kB CSS (0) | — |
| ChatComposerInput | 77.5 kB (+243 B) · 11.3 kB CSS (0) | — |
130 more components
| Component | Size | LoC |
|---|---|---|
| ChatComposerTokenElement | 77.5 kB (+243 B) · 11.3 kB CSS (0) | — |
| TimeInput | 164.0 kB (+234 B) · 16.0 kB CSS (0) | — |
| charts/Chart | 206.4 kB (+232 B) · 494 B CSS (0) | — |
| charts/ChartTooltip | 183.6 kB (+222 B) · 422 B CSS (0) | — |
| Typeahead | 82.0 kB (+222 B) · 12.4 kB CSS (0) | — |
| DateInput | 188.8 kB (+211 B) · 17.0 kB CSS (0) | — |
| DateTimeInput | 210.4 kB (+207 B) · 18.7 kB CSS (0) | — |
| SideNavHeading | 76.9 kB (+196 B) · 11.3 kB CSS (0) | — |
| lab/TransferListSelector | 111.2 kB (+191 B) · 1.4 kB CSS (0) | — |
| PowerSearch | 292.0 kB (+188 B) · 24.2 kB CSS (0) | — |
| TabMenu | 73.2 kB (+185 B) · 11.1 kB CSS (0) | — |
| BaseTypeahead | 73.1 kB (+184 B) · 10.9 kB CSS (0) | — |
| lab/Comments | 354.5 kB (+182 B) · 2.5 kB CSS (0) | — |
| SideNavItem | 71.7 kB (+180 B) · 11.1 kB CSS (0) | — |
| BreadcrumbItem | 86.0 kB (+178 B) · 12.1 kB CSS (0) | — |
| ComplexSelector | 72.3 kB (+178 B) · 11.8 kB CSS (0) | — |
| DropdownMenuSubMenu | 71.4 kB (+177 B) · 9.7 kB CSS (0) | — |
| TopNavHeading | 76.5 kB (+175 B) · 11.2 kB CSS (0) | — |
| ChatLayout | 84.1 kB (+172 B) · 10.5 kB CSS (0) | — |
| TopNavMenu | 75.9 kB (+171 B) · 11.2 kB CSS (0) | — |
| Tokenizer | 93.8 kB (+169 B) · 18.8 kB CSS (0) | — |
| lab/CommentThread | 347.5 kB (+166 B) · 2.4 kB CSS (0) | — |
| Popover | 92.3 kB (+158 B) · 14.2 kB CSS (0) | — |
| TopNavMegaMenu | 71.0 kB (+155 B) · 11.8 kB CSS (0) | — |
| ButtonGroup | 42.1 kB (+153 B) · 3.7 kB CSS (0) | — |
| Markdown | 190.1 kB (+151 B) · 20.7 kB CSS (0) | — |
| Table | 127.2 kB (+151 B) · 18.4 kB CSS (0) | — |
| lab/CommentComposer | 153.7 kB (+151 B) · 1.9 kB CSS (0) | — |
| ContextMenu | 110.3 kB (+148 B) · 17.4 kB CSS (0) | — |
| SideNav | 116.0 kB (+148 B) · 11.4 kB CSS (0) | — |
| lab/ChatReactionBar | 108.7 kB (+148 B) · 1.1 kB CSS (0) | — |
| MoreMenu | 98.7 kB (+147 B) · 17.6 kB CSS (0) | — |
| TableCell | 111.6 kB (+147 B) · 17.9 kB CSS (0) | — |
| AppShell | 92.9 kB (+145 B) · 13.5 kB CSS (0) | — |
| lab/TourStep | 98.6 kB (+145 B) · 590 B CSS (0) | — |
| Lightbox | 123.8 kB (+144 B) · 10.6 kB CSS (0) | — |
| Pagination | 111.7 kB (+144 B) · 17.8 kB CSS (0) | — |
| TableHeaderCell | 111.2 kB (+143 B) · 17.6 kB CSS (0) | — |
| Drawer | 51.9 kB (+140 B) · 8.3 kB CSS (0) | — |
| MultiSelector | 96.3 kB (+140 B) · 16.8 kB CSS (0) | — |
| DropdownMenu | 98.5 kB (+135 B) · 17.6 kB CSS (0) | — |
| DateRangeInput | 94.3 kB (+134 B) · 12.5 kB CSS (0) | — |
| LayoutPanel | 21.9 kB (+133 B) · 4.9 kB CSS (0) | — |
| Selector | 101.6 kB (+133 B) · 17.4 kB CSS (0) | — |
| lab/ChatEmojiPicker | 107.5 kB (+131 B) · 773 B CSS (0) | — |
| BottomSheetSwitcher | 18.6 kB (+128 B) · 3.4 kB CSS (0) | — |
| AvatarGroup | 47.1 kB (+127 B) · 5.7 kB CSS (0) | — |
| AlertDialog | 106.8 kB (+123 B) · 14.6 kB CSS (0) | — |
| Dialog | 24.9 kB (+123 B) · 6.9 kB CSS (0) | — |
| TableSelectionToolbar | 71.6 kB (+121 B) · 13.2 kB CSS (0) | — |
| richtext/RichTextEditorToolbar | 280.2 kB (+121 B) · 454 B CSS (0) | — |
| CommandPalette | 74.5 kB (+118 B) · 12.7 kB CSS (0) | — |
| TabList | 44.8 kB (+118 B) · 6.5 kB CSS (0) | — |
| lab/Schedule | 107.3 kB (+116 B) · 3.9 kB CSS (0) | — |
| lab/CommentPin | 58.3 kB (+114 B) · 1.2 kB CSS (0) | — |
| Toolbar | 44.7 kB (+113 B) · 8.1 kB CSS (0) | — |
| MobileNav | 71.8 kB (+110 B) · 10.6 kB CSS (0) | — |
| NavHeadingMenu | 16.9 kB (+109 B) · 3.5 kB CSS (0) | — |
| SegmentedControl | 97.3 kB (+109 B) · 6.9 kB CSS (0) | — |
| DropdownMenuRadioItem | 23.4 kB (+108 B) · 6.6 kB CSS (0) | — |
| DropdownMenuCheckboxItem | 23.3 kB (+107 B) · 6.6 kB CSS (0) | — |
| TreeList | 45.6 kB (+106 B) · 5.6 kB CSS (0) | — |
| lab/NavigationScreen | 83.3 kB (+102 B) · 1.5 kB CSS (0) | — |
| lab/NavigationStack | 83.3 kB (+102 B) · 1.5 kB CSS (0) | — |
| DropdownMenuItem | 22.5 kB (+100 B) · 6.3 kB CSS (0) | — |
| lab/TabBar | 81.3 kB (+100 B) · 1.8 kB CSS (0) | — |
| TopNav | 73.5 kB (+94 B) · 10.9 kB CSS (0) | — |
| Collapsible | 28.4 kB (+79 B) · 4.5 kB CSS (0) | — |
| ContextMenuItem | 32.8 kB (+79 B) · 6.3 kB CSS (0) | — |
| InfoTip | 47.6 kB (+76 B) · 5.4 kB CSS (0) | — |
| lab/SelectableCardGroup | 65.4 kB (+76 B) · 162 B CSS (0) | — |
| Avatar | 44.3 kB (+75 B) · 5.6 kB CSS (0) | — |
| CheckboxList | 69.7 kB (+74 B) · 7.7 kB CSS (0) | — |
| ResizeHandle | 22.1 kB (+73 B) · 4.3 kB CSS (0) | — |
| lab/DialogHeroHeader | 84.3 kB (+73 B) · 372 B CSS (0) | — |
| CheckboxListItem | 72.4 kB (+71 B) · 11.4 kB CSS (0) | — |
| AvatarGroupOverflow | 50.0 kB (+70 B) · 5.7 kB CSS (0) | — |
| Link | 54.1 kB (+70 B) · 7.0 kB CSS (0) | — |
| FieldLabel | 52.1 kB (+69 B) · 5.7 kB CSS (0) | — |
| FileInput | 70.3 kB (+69 B) · 10.9 kB CSS (0) | — |
| Slider | 110.2 kB (+69 B) · 8.8 kB CSS (0) | — |
| lab/TransferList | 103.4 kB (+69 B) · 1.2 kB CSS (0) | — |
| RadioList | 57.4 kB (+68 B) · 7.3 kB CSS (0) | — |
| lab/EmojiPicker | 88.1 kB (+68 B) · 751 B CSS (0) | — |
| ListItem | 71.4 kB (+67 B) · 11.4 kB CSS (0) | — |
| RadioListItem | 68.5 kB (+67 B) · 10.1 kB CSS (0) | — |
| lab/DiffViewer | 88.4 kB (+66 B) · 2.1 kB CSS (0) | — |
| lab/ListInput | 89.3 kB (+65 B) · 922 B CSS (0) | — |
| Field | 56.0 kB (+64 B) · 7.2 kB CSS (0) | — |
| HoverCard | 28.7 kB (+63 B) · 4.4 kB CSS (0) | — |
| InputGroup | 57.6 kB (+63 B) · 7.8 kB CSS (0) | — |
| StatusDot | 32.2 kB (+62 B) · 5.3 kB CSS (0) | — |
| TextInput | 68.9 kB (+62 B) · 11.2 kB CSS (0) | — |
| Thumbnail | 65.6 kB (+62 B) · 11.6 kB CSS (0) | — |
| Calendar | 72.3 kB (+61 B) · 10.6 kB CSS (0) | — |
| CodeBlock | 76.4 kB (+61 B) · 11.3 kB CSS (0) | — |
| NumberInput | 72.8 kB (+61 B) · 11.4 kB CSS (0) | — |
| Tooltip | 30.5 kB (+61 B) · 4.4 kB CSS (0) | — |
| lab/TabBarMoreList | 93.6 kB (+61 B) · 1.8 kB CSS (0) | — |
| richtext/RichTextEditor | 299.6 kB (+61 B) · 2.6 kB CSS (0) | — |
| Stepper | 62.9 kB (+60 B) · 10.1 kB CSS (0) | — |
| TextArea | 63.4 kB (+60 B) · 9.3 kB CSS (0) | — |
| Toast | 99.2 kB (+60 B) · 10.3 kB CSS (0) | — |
| Banner | 63.3 kB (+59 B) · 10.5 kB CSS (0) | — |
| SideNavCollapseButton | 62.1 kB (+59 B) · 10.0 kB CSS (0) | — |
| Button | 61.3 kB (+58 B) · 10.0 kB CSS (0) | — |
| ChatDictationButton | 61.9 kB (+58 B) · 10.1 kB CSS (0) | — |
| richtext/RichTextView | 277.7 kB (+58 B) · 2.3 kB CSS (0) | — |
| charts/ChartLegend | 53.4 kB (+57 B) · 224 B CSS (0) | — |
| IconButton | 61.3 kB (+57 B) · 10.0 kB CSS (0) | — |
| ChatSendButton | 61.7 kB (+56 B) · 10.0 kB CSS (0) | — |
| DialogHeader | 64.1 kB (+56 B) · 11.5 kB CSS (0) | — |
| MobileNavToggle | 61.6 kB (+55 B) · 10.0 kB CSS (0) | — |
| Switch | 104.8 kB (+55 B) · 9.9 kB CSS (0) | — |
| ChatLayoutScrollButton | 61.8 kB (+54 B) · 10.2 kB CSS (0) | — |
| Text | 13.5 kB (+54 B) · 5.4 kB CSS (0) | — |
| CheckboxInput | 65.4 kB (+53 B) · 9.2 kB CSS (0) | — |
| DrawerHeader | 64.3 kB (+52 B) · 12.9 kB CSS (0) | — |
| Spinner | 26.4 kB (+49 B) · 6.2 kB CSS (0) | — |
| CommandPaletteInput | 33.5 kB (+48 B) · 7.5 kB CSS (0) | — |
| Timer | 15.6 kB (+48 B) · 6.0 kB CSS (0) | — |
| ToggleButton | 87.8 kB (+48 B) · 10.1 kB CSS (0) | — |
| Heading | 14.1 kB (+45 B) · 5.4 kB CSS (0) | — |
| Timestamp | 34.7 kB (+45 B) · 10.2 kB CSS (0) | — |
| Carousel | 71.0 kB (+42 B) · 10.9 kB CSS (0) | — |
| ChatToolCalls | 33.7 kB (+41 B) · 8.3 kB CSS (0) | — |
| lab/LineClamp | 15.6 kB (+41 B) · 168 B CSS (0) | — |
| ChatMessageList | 29.9 kB (+38 B) · 7.0 kB CSS (0) | — |
| ProgressBar | 11.9 kB (0) · 6.3 kB CSS (0) | — |
| lab/TabBarItem | 3.9 kB (0) · 1.8 kB CSS (0) | — |
Package size: @astryxdesign/core 7.29 MB packed (+678 B); no new dependencies.
API changes: none in this pull request
Committed API report behind this build: cli 36 rows, core 36 rows; run pnpm api:report.
Not run on this PR: add ci:run-visual or ci:run-rtl to run them.
Generated by PR Enrichment workflow | View full report
1cd7019 to
e8eeb8f
Compare
|
/approve-spec e8eeb8f |
There was a problem hiding this comment.
Changes requested: #10817 and #10822 each need one fix, and a pull request lands only when it and every pull request below it are approved.
#10948: approve, with no blocking findings.
#10817: request changes
- Miss: the new FR3 sentence in
docs/specs/AST-110-swizzle-output/spec.md(lines 102-106) requires Core's internal shared-realm helpers and the source check's allowlist, whicharchitecture:knowledge-contractsINV18 does not allow in a product requirement, so keep the observable guarantee in FR3 (one versioned slot per page, React-bound state one per React) and move the helpers, the check and the allowlist to an architecture record or the FR7 verification row.
#10822: request changes
- Miss: the title,
.changeset/scroll-lock-swizzle-shared.mdand the description say a swizzled overlay kept its own page scroll lock, but swizzle copies importuseScrollLockfrom@astryxdesign/core/hooksand never carryuseScrollLock.ts,rootScrollStyles.tsorpageHold.ts, and closing the overlay on top already kept the page locked, so describe the fixed case as two loads of Core's scroll-lock modules on one page where closing the overlay that opened first unlocked it, name selection locks as the part that reaches swizzled copies, and drop the stuck-hiddensentence.
#10831: approve, with no blocking findings; it lands after #10817 and #10822.
Whole stack: fail on the two findings above; the net public API is unchanged.
- Optional: add a nested-region Tab case to
focusKeyboard.shared.test.tsx, since it is the lead claim of #10831.
[Automated review]
sharedRealm.ts gains the shapes the FR3 fixes need, each a thin layer over realmValue and sharedKey: sharedState for counters, flags and host objects; sharedMap, sharedWeakMap and sharedWeakSet for registries and per-document stores; sharedStore for a value and its listeners; sharedBrand and the SharedBrand type for brands; sharedSequence for id counters; and resetSharedStateForTest. The source check asserts that every shared slot is named with a literal name and version, once. AST-110 FR3 states that cross-module state is held through these helpers.
e8eeb8f to
fee30cf
Compare
|
/approve-spec fee30cf |
github-actions
Bot
removed
the
needs:design-review
|
/approve-spec 63307d2 |
cixzhang
added
ci:run
There was a problem hiding this comment.
Changes requested: the focus-trap and keyboard changes are sound, but two changesets that came back in the merge from main need to go.
- Regression:
.changeset/scroll-lock-swizzle-shared.mdand.changeset/shared-realm-helpers.mdrepeat the release notes 0.7.12 already published for #10822 and #10817 (packages/core/CHANGELOG.mdlines 128 and 222), so the next changelog would announce those fixes again under this pull request; delete both and keep.changeset/focus-keyboard-swizzle-shared.md.
The new cases in focusKeyboard.shared.test.tsx, including the nested focus-region Tab case, fail on main and pass on this head. One optional wording suggestion for the changed comment in useFocusTrap.ts is inline.
[Automated review]
…a focus-region comment
|
/approve-spec 7ea1f91 |
There was a problem hiding this comment.
Approved. One CI job fails at this head, but not because of this change: pr-a11y (audit and guards) reports 26 Layer story plays with "No open layer", and the same plays fail in a Storybook built from the base commit, so the merge guard waits on a fix for those plays on main.
Focus regions, the Escape-trap signal, focus return and the keyboard model now keep one shared state per page, and the new two-load cases fail without the change and pass with it.
- Optional: the changeset's "no longer both answer the same Tab" describes a symptom the base does not show; there the outer region answers alone and focus leaves the inner overlay (inline suggestion).
[Automated review]
|
/approve-spec 1d1d1af |
There was a problem hiding this comment.
Approved. Every CI check on this head passes, including the heavy test, build and docsite-test lanes.
Focus regions, the Escape-trap count, focus return and the on-screen keyboard model now keep one state per page across a swizzled copy and the package, and the four new two-load cases fail without the change and pass with it.
- Heads-up: #11027 landed on
mainafter this head and edits the same block ofscripts/shared-identity.baseline.json, so mergingmainwill need that file resolved. - Optional: the changeset's "no longer both answer the same Tab" describes a symptom the base does not show; there the outer region answers alone and focus leaves the inner overlay (see the inline suggestion on line 5).
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] A swizzled overlay and the package's components now share one set of focus regions and one mobile keyboard model: nested focus regions from the package and a copy no longer both answer the same Tab, and package and copied fields agree on the keyboard's height, its layout guide and which elements ride it. No API changes. |
There was a problem hiding this comment.
Optional wording: "nested focus regions from the package and a copy no longer both answer the same Tab" names a symptom the base does not show. Each region's Tab handler returns once another has called preventDefault (useFocusTrap.ts:402), so on the base only the outer region answers and focus leaves the inner overlay (the new case lands on A). Suggest: "a Tab inside a copy's focus region nested in a package region (or the reverse) now wraps inside the inner region instead of moving to the outer region's first control".
|
/approve-spec 14cf8df |
There was a problem hiding this comment.
Approved. CI passes on this head, including the test, build and docsite-test jobs that the earlier run cancelled for capacity.
Focus regions, the Escape-trap count, focus return and the on-screen keyboard model keep one state per page across a swizzled copy and the package: the four two-load cases fail without the change and pass with it, and in a real browser Tab from a copied focus region nested in a package region now wraps inside the copied region.
- Optional: the changeset's "no longer both answer the same Tab" describes a symptom the base does not show; there the outer region answers alone and focus leaves the inner overlay (see the inline suggestion on line 5).
[Automated review]
User impact
A product engineer who swizzles an overlay or a field and keeps the package's other components gets two copies of the page's focus regions and of the mobile keyboard model:
After this change both copies use one set of focus regions and one keyboard model per page.
Problem and solution fit
useFocusTrapkeeps its region containers and its Escape-trap count,focusReturnits temporary-focus and removal state, and the keyboard modules their per-window stores, guides, takeovers, ride registries and targets in module variables. A swizzled copy carries its own copies.spec:AST-110FR3: coordinating state lives in one store per page, held through Core's shared-realm helpers.Each moves onto the helpers from #10817, with no other change:
useFocusTrap: Escape-trap count →sharedState; active region containers →sharedState(() => new Set()).focusReturn:temporaryFocus,focusDetachedAtRemovalStart→sharedWeakMap.keyboardHeight:stores,focusTakeovers,touchStartWatches→sharedWeakMap.keyboardLayoutGuide:guides→sharedWeakMap.keyboardTarget:currentRides,riddenAnimators→sharedWeakMap;targets→sharedWeakSet.useMobileKeyboard: the learned reveal-at-report windows →sharedState.The baseline loses these 12 entries.
layoutViewport's scrollbar-thickness cache moves from "open" to "allowed" as a memo cache (a second measurement per copy changes no result).Expected behavior and authority
spec:AST-110FR3 (approved,authority: draft).Evidence
keyboard/focusKeyboard.shared.test.tsx): the package does not see a copy's active Escape trap; two loads get two layout guides for one window; the package does not recognize a keyboard target a copy registered; and, for the lead fix, with a copy's focus region opened inside a package region, Tab from the copy region's last control lands on the package region's first control (A) instead of wrapping to the copy's first (B1). That last case fails on the PR below and passes here.tscpasses for both the source and build configurations.scripts/shared-identity.test.mjspasses with the entries removed.Scope
Components: Dialog, Popover, BottomSheet, Drawer, and every component with a focus region or a keyboard-aware field
Testing
pnpm exec vitest run --project ui packages/core/src/keyboard packages/core/src/hooks packages/core/src/utils packages/core/src/Dialog packages/core/src/BottomSheet packages/core/src/Popover packages/core/src/Drawerpnpm exec vitest run --project node scripts/shared-identity.test.mjstsc --noEmit -p packages/core(andtsconfig.build.json)