Repository navigation
refactor(ui): migrate renderer modals onto Base UI Dialog (#520) - #548
Merged
Merged
Conversation
…I Dialog (#520 PR7) Migrate the two remaining hand-written top-level modals onto Base UI DialogRoot/DialogContent so focus trap, Esc, and focus restoration come from the same shell SearchModal/PermissionDialog already use. SearchModal already rendered via DialogContent but still called useModalA11y for initial focus + suppressFocusRestore; replace with DialogContent initialFocus={inputRef} and a finalFocus callback that returns false when navigating to a result so the destination owns focus. - keyboard-help.tsx, command-palette.tsx: hand-written role="dialog" + aria-modal + useModalA11y -> DialogRoot/DialogContent - search-modal.tsx: drop residual useModalA11y; DialogContent initialFocus/ finalFocus instead - dialog-source-contract.test.ts (new): ban hand-written role="dialog" / aria-modal outside the Drawer-migration allowlist (provider-config-sheet, provider-oauth-section, bot-chat-settings-page move to Base UI Drawer in a follow-up commit) - drop orphan maka-palette-backdrop CSS (positioning now via DialogContent top-[12vh] className) - update contracts that previously asserted on useModalA11y Verification: dialog-source + modal-lifecycle + command-palette-a11y + search-modal-lifecycle + renderer-utility contracts 51/51; desktop 2020/2020; typecheck clean; sidebar-search-modal-open + command-palette-open screenshots vs fresh cc3fc2c baseline 0.05% residual.
…og (#520 PR7) Migrate WeChatScanLoginModal + WechatQrLoginModal in bot-chat-settings-page from hand-written role="dialog" + aria-modal + useModalA11y onto Base UI DialogRoot/DialogContent (same shell as commit 1). Drop the now-orphan settingsModalBackdrop + settingsWechatQrBackdrop CSS rules (backdrop now comes from DialogContent). Remove bot-chat-settings-page from the dialog-source-contract Drawer-migration allowlist (only provider-config-sheet + provider-oauth-section remain, pending Base UI Drawer in the next commit). Verification: dialog-source + settings-form-a11y + model-oauth-section + modal-lifecycle + renderer-style-pruning + app-region-hygiene contracts pass; desktop 2020/2020; typecheck clean; settings-bots + settings-general screenshots vs fresh cc3fc2c baseline 0 diff.
…ortal container (#520 PR7) Migrate ProviderConfigSheetOverlay + ClaudeSubscriptionModal + SubscriptionLoginModal from hand-written role="dialog" + useModalA11y + the 120-line useProviderSheetBackgroundInert DOM walker onto Base UI Dialog. Dialog.Portal container=.settingsSurface keeps the backdrop + popup INSIDE the settings surface (nested visual preserved: scrim covers only the surface, rounded corners stay aligned to the modal). Base UI's modal layer now owns focus trap, Esc, and aria-hidden on the settings nav/list. Dialog.Backdrop forceRender because Base UI skips nested backdrops by default; provider sheets are side panels that want the scrim. This retires the last useModalA11y consumers, so modal-a11y.ts + components.tsx's useModalA11y export are deleted, and the dialog-source-contract Drawer-migration allowlist is now empty. Verified via CDP probe (storybook + electron) on the real ProviderConfigSheetOverlay inside a .settingsSurface: backdrop renders (absolute inset 0, providerConfigOverlay CSS), sheet is portal'd into the surface, nav/main get aria-hidden="true" + data-base-ui-inert, sheet gets role="dialog". Behavior change: clicking the settings nav area while a sheet is open now closes the sheet (Base UI dismiss on backdrop click) instead of being silently swallowed by the old inert — matches PermissionDialog and standard modals. Verification: dialog-source + model-oauth + settings-form-a11y + dead-export + search-modal-lifecycle contracts pass; desktop 2019/2019; typecheck clean; settings-bots + settings-data screenshots vs fresh cc3fc2c baseline ~0 diff.
…form
- KeyboardHelpModal + WeChatScanLoginModal + WechatQrLoginModal: pass
showClose={false} to DialogContent. These three keep their own in-header
close button (with a specific aria-label like "关闭快捷键面板"), and
DialogContent's default showClose was rendering a duplicate. Provider sheets
are unaffected — they use BaseDialog.Popup, not DialogContent/createModalContent.
- dialog-source-contract: FORBIDDEN_PATTERNS now matches the JSX expression
form (role={'dialog'}, role={"dialog"}, role={`dialog`}, same for
aria-modal) in addition to the literal form, with a coverage test that
asserts both positive and benign-negative samples.
- ProviderSheet: document the singleton assumption on the `.settingsSurface`
querySelector (SettingsModal is conditional-mounted, at most one surface).
Two review items intentionally not changed (reasoning in PR review reply):
querySelector owner-ref (YAGNI — .settingsSurface is a singleton today) and
the StrictMode focus-restore behavior test (the commit-1 static finalFocus
test already fails if the false branch is removed).
…label
- dialog-source-contract: FORBIDDEN_PATTERNS regex now tolerates spaces
around `=` and inside `{}`, and matches `aria-modal={true}` (bare boolean,
no quotes) in addition to the quoted form. Positives add `aria-modal={true}`,
`aria-modal = { true }`, `role = {'dialog'}`, `role = "dialog"`. Still regex,
no AST — same rationale as the P2 fix.
- WeChatScanLoginModal: in-header close aria-label "关闭" → "关闭微信扫码登录".
The other two retained close buttons already had specific labels
(关闭快捷键面板, 关闭微信扫码登录), so all three internal close buttons now
carry concrete copy.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Converge every hand-written renderer modal onto Base UI Dialog so focus trap, Esc, restore-focus, and aria-hidden come from one shell instead of the self-rolled
useModalA11y+ per-siterole="dialog"/aria-modal+ the 120-lineuseProviderSheetBackgroundInertDOM walker. Three commits, each an independent revert boundary.Why
Refs #520 (PR7 dialog/drawer slice). Five modal surfaces were still hand-writing dialog semantics on top of
useModalA11y, and the three provider sub-sheets additionally ran a hand-rolled DOM walker (useProviderSheetBackgroundInert) to simulate nested-modal background inert. Base UI Dialog already does all of this — keeping the hand-rolled versions is duplicated maintenance and an a11y regression risk.Scope
Changed (commit 1 → 2 → 3):
d5d0a9d2— KeyboardHelpModal + CommandPalette hand-writtenrole="dialog"+useModalA11y→DialogRoot/DialogContent; SearchModal drops residualuseModalA11yin favor ofDialogContent initialFocus/finalFocus. Addsdialog-source-contractbanning hand-writtenrole="dialog"/aria-modaloutside a Drawer-migration allowlist.d0413022— WeChatScanLoginModal + WechatQrLoginModal →DialogContent; drops orphansettingsModalBackdrop+settingsWechatQrBackdropCSS.fee54b12— ProviderConfigSheetOverlay + ClaudeSubscriptionModal + SubscriptionLoginModal → Base UI Dialog nested +Dialog.Portal container=.settingsSurfaceso the scrim stays inside the settings surface (nested visual preserved).Dialog.Backdrop forceRenderbecause Base UI skips nested backdrops by default. Retires the lastuseModalA11yconsumers → deletesmodal-a11y.ts+components.tsxexport + theuseProviderSheetBackgroundInertwalker. Allowlist now empty.Not included (PR7 remainder, separate PRs):
Verification
dialog-source,modal-lifecycle,command-palette-a11y-copy,search-modal-lifecycle,renderer-utility-primitives,model-oauth-section,settings-form-a11y,ui-tsx-dead-export,renderer-style-pruning,app-region-hygieneall pass.npm run -w @maka/desktop test: 2019/2019.npm run typecheck: clean.cc3fc2cdbaseline:sidebar-search-modal-open0.05%,command-palette-open0.05%,settings-bots~0,settings-data~0..settingsSurface: backdrop renders (absolute inset 0, providerConfigOverlay CSS), sheet portals into the surface, nav/main getaria-hidden="true"+data-base-ui-inert, sheet getsrole="dialog". Probe scripts were local-only and deleted.User-facing impact
One behavior change in commit 3: clicking the settings nav area while a provider sheet is open now closes the sheet (Base UI dismiss-on-backdrop-click) instead of being silently swallowed by the old
inert. This matches PermissionDialog and other standard modals (click outside = close).Reviewer notes
Dialog.Portal container(nested visual) andDialog.Backdrop forceRender(Base UI skips nested backdrops by default). Both are explained in theProviderSheetdoc comment and verified by the CDP probe.