feat(ui): sync the mobile browser chrome with the dialog scrim - #9424
feat(ui): sync the mobile browser chrome with the dialog scrim#9424maxyinger wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: c4c7f4d The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-google-signin
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
📝 WalkthroughWalkthroughAdds browser-chrome synchronization to Mosaic Dialog. The new Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🔵 Low · up to The PR adds mobile browser-chrome synchronization to dialogs. It is mergeable with owner follow-up because several tests can produce false positives or leak state across tests; the supplied evidence does not establish a production behavior defect. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
packages/ui/src/mosaic/hooks/useAccessibleNameWarning.ts (1)
3-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the explanatory comments.
Keep a short JSDoc contract and, if needed, one terse comment for the deferred check. The current comments add procedural detail that the code already expresses.
As per coding guidelines, “Keep code comments minimal. Add comments only when critical to explain why a non-obvious change was made; never restate code behavior, and keep warranted comments to one terse line rather than a verbose multi-line block.”
Also applies to: 33-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/mosaic/hooks/useAccessibleNameWarning.ts` around lines 3 - 19, Reduce the JSDoc above the accessible-name warning hook to a brief contract describing the node and component parameters and the warning purpose. Remove procedural explanations of mounting, effects, role checks, and timing; retain at most one concise comment near the deferred check only if needed to explain its non-obvious rationale.Source: Coding guidelines
packages/ui/src/mosaic/components/dialog/keyboard-inset.ts (1)
75-81: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the release function idempotent.
The returned function decrements
listenerson every call. A second call on the same handle driveslistenersbelow zero. The check at Line 77 then never matches again, so theresizeandscrolllisteners and the--_cl-keyboard-insetproperty stay for the lifetime of the page.The sibling module guards against exactly this. See
packages/ui/src/mosaic/components/dialog/browser-chrome.tsLines 294-299, which tracks areleasedflag because "callers release ondata-ending-styleand again at unmount".The current caller in
packages/ui/src/mosaic/components/dialog/dialog.tsxLine 253 releases once, so this is not a live defect. Aligning the two modules removes the leak class.🛡️ Proposed fix
- return () => { - listeners--; - if (listeners === 0 && detach) { - detach(); - detach = null; - } - }; + let released = false; + return () => { + if (released) { + return; + } + released = true; + listeners--; + if (listeners === 0 && detach) { + detach(); + detach = null; + } + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/mosaic/components/dialog/keyboard-inset.ts` around lines 75 - 81, Make the cleanup function returned by the keyboard-inset setup idempotent by adding a per-handle released guard, following the pattern used by browser-chrome. In the returned function, exit immediately on subsequent calls; otherwise mark it released, decrement listeners once, and preserve the existing detach and reset behavior when the count reaches zero.packages/ui/src/mosaic/components/dialog/dialog.tsx (1)
216-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared "node state plus forwarded ref" logic.
Backdrop(Lines 216-228) andPopup(Lines 276-289) implement the same pattern: auseStatenode, auseCallbackref that callssetNodeand then forwards to the consumer ref. The two copies are identical apart from the element type.One extracted hook removes the duplication and keeps the forwarding rules in one place.
♻️ Proposed helper
function useNodeRef<T extends HTMLElement>( ref: React.ForwardedRef<T>, ): [T | null, (element: T | null) => void] { const [node, setNode] = React.useState<T | null>(null); const callback = React.useCallback( (element: T | null) => { setNode(element); if (typeof ref === 'function') { ref(element); } else if (ref) { ref.current = element; } }, [ref], ); return [node, callback]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/mosaic/components/dialog/dialog.tsx` around lines 216 - 228, Extract the duplicated node state and forwarded-ref logic into a generic useNodeRef hook in dialog.tsx, supporting HTMLElement subtypes and React.ForwardedRef values. Replace the local useState/useCallback implementations in both Backdrop and Popup with this hook, preserving their existing useBrowserChrome behavior and ref-forwarding semantics.packages/ui/src/mosaic/components/dialog/browser-chrome.ts (1)
47-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing explicit return types across the new dialog modules. Both files annotate most functions but leave a few unannotated, so the guideline is applied inconsistently within the same feature.
packages/ui/src/mosaic/components/dialog/browser-chrome.ts#L47-L48: annotatelayerDurationasnumberandlayerEaseas(t: number) => number, and annotatetoCss(Line 141),firstDuration(Line 193), andanimate(Line 199).packages/ui/src/mosaic/components/dialog/dialog.tsx#L339-L349: annotate the exportedDialogreturn type, and annotateuseBrowserChrome(Line 54),Root(Line 122), andDialogContent(Line 321).As per coding guidelines: "Always define explicit return types for functions, especially public APIs".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/mosaic/components/dialog/browser-chrome.ts` around lines 47 - 48, Add explicit return types to all listed functions: in browser-chrome.ts, annotate layerDuration as number, layerEase as (t: number) => number, and add appropriate return types to toCss, firstDuration, and animate; in dialog.tsx, annotate Dialog, useBrowserChrome, Root, and DialogContent. Preserve each function’s existing behavior and infer the types from their current return values.Source: Coding guidelines
.changeset/olive-doors-tell.md (1)
1-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis second empty changeset is redundant.
.changeset/dialog-browser-chrome.mdis also empty. One empty changeset satisfies the repository changeset check. Remove this file, or give the two files distinct release entries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/olive-doors-tell.md around lines 1 - 2, Remove the redundant empty changeset file represented by this diff, leaving the existing .changeset/dialog-browser-chrome.md as the single empty changeset that satisfies the repository check.packages/swingset/src/stories/dialog.component.stories.tsx (1)
39-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRoute the knob cast through the local
knobsAsPropshelper.
DefaultacceptsRecord<string, unknown>correctly, but it narrows the knobs with an inlineargs as { size?: DialogSize }cast. The story convention is a single localknobsAsPropshelper, so every knob-driven story casts the same way.As per path instructions for
packages/swingset/src/stories/*.stories.tsx: "Simple CVA story functions must acceptRecord<string, unknown>and cast through a localknobsAsPropshelper before rendering the typed component."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swingset/src/stories/dialog.component.stories.tsx` around lines 39 - 40, Update the Default story to use the local knobsAsProps helper for narrowing args instead of the inline `{ size?: DialogSize }` cast, while keeping its Record<string, unknown> parameter and existing rendering behavior unchanged.Source: Path instructions
packages/ui/src/mosaic/components/dialog/dialog.test.tsx (1)
500-514: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the fixed 250 ms sleep with fake timers.
This test waits a real 250 ms to prove the deferred teardown did not strip the tint. The delay is a magic number tied to the fade duration inside
browser-chrome.ts. If that duration grows, the test passes for the wrong reason. If CI is slow, the assertion still runs late but the suite pays the cost on every run.Use
vi.useFakeTimers()and advance past the teardown delay, or export the delay constant and derive the wait from it. The trailinguser.keyboard('{Escape}')also carries no assertion; add a short comment that it exists only to release the tint before teardown.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/mosaic/components/dialog/dialog.test.tsx` around lines 500 - 514, Update the dialog re-open regression test to use Vitest fake timers instead of the fixed 250 ms real-time sleep, advancing timers beyond the deferred teardown delay before asserting themeColor() remains present. Prefer the existing teardown-delay constant from browser-chrome.ts if available, and add a brief comment explaining that the final user.keyboard('{Escape}') only releases the tint before teardown.packages/swingset/src/stories/dialog.component.mdx (1)
182-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve the archetype mismatch for this page.
The story sets
group: 'Components'and this page renders a Playground and a<PropTable>, which is the Simple Components archetype. That archetype fixes the top-level order asPlayground,Props,Usage,Examples. This page inserts## Partsand## Stylingas extra top-level sections betweenUsageandExamples.Pick one archetype. Either demote
PartsandStylingto subsections ofUsage, or move the page to the Compound Components archetype and drop<Preview>and<PropTable>.As per path instructions for
packages/swingset/src/stories/{*.stories.tsx,*.mdx}: "Use the archetype determined bymeta.group, preserve its required headings and order, and do not invent or reorder top-level sections."Also applies to: 200-200
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swingset/src/stories/dialog.component.mdx` at line 182, Resolve the archetype mismatch in the dialog story by keeping the Components/Simple Components structure: retain the required top-level order of Playground, Props, Usage, and Examples, and demote Parts and Styling under Usage as subsections. Do not add extra top-level headings or switch archetypes while retaining Playground and PropTable.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/mosaic/components/dialog/browser-chrome.ts`:
- Around line 229-244: In
packages/ui/src/mosaic/components/dialog/browser-chrome.ts lines 229-244, move
the “Called by every mounted backdrop” JSDoc, including its `@param` backdrop tag,
to acquireBrowserChrome around line 259; leave resolveTint documented only by
the block describing its parameterless tint calculation. In
packages/ui/src/mosaic/components/dialog/dialog.styles.ts lines 153-188, move
the “Named for what the surface IS” JSDoc to sizes around line 230, where it
documents the prompt, card, and panel widths; do not leave it attached to
viewportSizes.
- Around line 148-157: Update the fallback in readBaseColor to use the CSS
system Canvas color instead of document.body’s computed backgroundColor when no
applicable theme-color meta exists. Preserve the existing meta-selection
behavior and ensure the fallback resolves the actual page canvas color,
including color-scheme-aware defaults.
In `@packages/ui/src/mosaic/components/dialog/dialog.test.tsx`:
- Around line 154-157: Update the nested-dialog comments to describe the actual
inner dialog size as prompt: in
packages/ui/src/mosaic/components/dialog/dialog.test.tsx lines 154-157 replace
the card wording, and in
packages/swingset/src/stories/dialog.component.stories.tsx line 159 replace card
with prompt. No implementation change is needed.
- Around line 516-532: Update the stacked-dialog test to await the asynchronous
theme-color acquisition before asserting the meta count, ensuring it verifies a
single shared meta after both dialogs are active. Then close the inner dialog
and confirm the meta remains, close the outer dialog, and assert the theme-color
meta is removed, covering the complete “only with the last” behavior.
- Around line 537-543: Move the console.warn spy restoration for the affected
tests into an afterEach hook, ensuring every created spy is restored on both
passing and failing paths; remove the inline warn.mockRestore() calls while
preserving each test’s warning assertions.
In `@packages/ui/src/mosaic/hooks/useAccessibleNameWarning.ts`:
- Around line 37-47: Update the accessible-name check in the warning logic
around labelledBy so any existing DOM element resolved by aria-labelledby is
treated as a valid name source, without requiring non-empty textContent.
Preserve the existing aria-label and component title handling, and avoid
introducing a text-only validation that rejects valid aria-label or
alternative-content targets.
---
Nitpick comments:
In @.changeset/olive-doors-tell.md:
- Around line 1-2: Remove the redundant empty changeset file represented by this
diff, leaving the existing .changeset/dialog-browser-chrome.md as the single
empty changeset that satisfies the repository check.
In `@packages/swingset/src/stories/dialog.component.mdx`:
- Line 182: Resolve the archetype mismatch in the dialog story by keeping the
Components/Simple Components structure: retain the required top-level order of
Playground, Props, Usage, and Examples, and demote Parts and Styling under Usage
as subsections. Do not add extra top-level headings or switch archetypes while
retaining Playground and PropTable.
In `@packages/swingset/src/stories/dialog.component.stories.tsx`:
- Around line 39-40: Update the Default story to use the local knobsAsProps
helper for narrowing args instead of the inline `{ size?: DialogSize }` cast,
while keeping its Record<string, unknown> parameter and existing rendering
behavior unchanged.
In `@packages/ui/src/mosaic/components/dialog/browser-chrome.ts`:
- Around line 47-48: Add explicit return types to all listed functions: in
browser-chrome.ts, annotate layerDuration as number, layerEase as (t: number) =>
number, and add appropriate return types to toCss, firstDuration, and animate;
in dialog.tsx, annotate Dialog, useBrowserChrome, Root, and DialogContent.
Preserve each function’s existing behavior and infer the types from their
current return values.
In `@packages/ui/src/mosaic/components/dialog/dialog.test.tsx`:
- Around line 500-514: Update the dialog re-open regression test to use Vitest
fake timers instead of the fixed 250 ms real-time sleep, advancing timers beyond
the deferred teardown delay before asserting themeColor() remains present.
Prefer the existing teardown-delay constant from browser-chrome.ts if available,
and add a brief comment explaining that the final user.keyboard('{Escape}') only
releases the tint before teardown.
In `@packages/ui/src/mosaic/components/dialog/dialog.tsx`:
- Around line 216-228: Extract the duplicated node state and forwarded-ref logic
into a generic useNodeRef hook in dialog.tsx, supporting HTMLElement subtypes
and React.ForwardedRef values. Replace the local useState/useCallback
implementations in both Backdrop and Popup with this hook, preserving their
existing useBrowserChrome behavior and ref-forwarding semantics.
In `@packages/ui/src/mosaic/components/dialog/keyboard-inset.ts`:
- Around line 75-81: Make the cleanup function returned by the keyboard-inset
setup idempotent by adding a per-handle released guard, following the pattern
used by browser-chrome. In the returned function, exit immediately on subsequent
calls; otherwise mark it released, decrement listeners once, and preserve the
existing detach and reset behavior when the count reaches zero.
In `@packages/ui/src/mosaic/hooks/useAccessibleNameWarning.ts`:
- Around line 3-19: Reduce the JSDoc above the accessible-name warning hook to a
brief contract describing the node and component parameters and the warning
purpose. Remove procedural explanations of mounting, effects, role checks, and
timing; retain at most one concise comment near the deferred check only if
needed to explain its non-obvious rationale.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 7abc39dd-277b-4479-af22-8457b53380d0
📒 Files selected for processing (23)
.changeset/dialog-browser-chrome.md.changeset/olive-doors-tell.mdpackages/headless/src/primitives/dialog/README.mdpackages/headless/src/primitives/dialog/dialog-backdrop.tsxpackages/headless/src/primitives/dialog/dialog-context.tspackages/headless/src/primitives/dialog/dialog-popup.tsxpackages/headless/src/primitives/dialog/dialog-root.tsxpackages/headless/src/primitives/dialog/dialog-viewport.tsxpackages/headless/src/primitives/drawer/drawer-context.tspackages/swingset/src/stories/dialog.component.mdxpackages/swingset/src/stories/dialog.component.stories.tsxpackages/ui/src/mosaic/components/dialog.tsxpackages/ui/src/mosaic/components/dialog/browser-chrome.tspackages/ui/src/mosaic/components/dialog/dialog.styles.tspackages/ui/src/mosaic/components/dialog/dialog.test.tsxpackages/ui/src/mosaic/components/dialog/dialog.tsxpackages/ui/src/mosaic/components/dialog/index.tspackages/ui/src/mosaic/components/dialog/keyboard-inset.tspackages/ui/src/mosaic/components/popover/popover.tsxpackages/ui/src/mosaic/hooks/useAccessibleNameWarning.tspackages/ui/src/mosaic/primitives/dialog.tsxpackages/ui/src/mosaic/styles/index.tspackages/ui/src/mosaic/tokens.stylex.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/clerk-ios(auto-detected)clerk/cli(auto-detected)clerk/clerk-android(auto-detected)
💤 Files with no reviewable changes (3)
- packages/headless/src/primitives/drawer/drawer-context.ts
- packages/ui/src/mosaic/primitives/dialog.tsx
- packages/ui/src/mosaic/components/dialog.tsx
| function readBaseColor(): string { | ||
| const metas = document.head.querySelectorAll<HTMLMetaElement>('meta[name="theme-color"]'); | ||
| for (const meta of metas) { | ||
| const media = meta.getAttribute('media'); | ||
| if (!media || window.matchMedia(media).matches) { | ||
| return meta.content; | ||
| } | ||
| } | ||
| return getComputedStyle(document.body).backgroundColor; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A transparent body background resolves to black, so the derived base colour is wrong.
readBaseColor falls back to getComputedStyle(document.body).backgroundColor when the app ships no theme-color meta. On a page that sets no explicit background-color on <body>, that computed value is rgba(0, 0, 0, 0).
readColor at Line 119 keeps only the r, g, b channels and discards alpha. Transparent black therefore resolves to [0, 0, 0]. Two consequences follow:
resolveTintcomposites the scrim over black, so the tint is near-black instead of a dimmed version of the page's real colour.animatewrites that value todocument.body.style.backgroundColorat Line 206, so the canvas and the overscroll gutter turn black on a default white page.
The revert path is unaffected, because Line 331 restores the original inline value.
Resolve the fallback against the actual painted canvas. Canvas is the CSS system colour for the default page background, and it tracks color-scheme.
🐛 Proposed fix
function readBaseColor(): string {
const metas = document.head.querySelectorAll<HTMLMetaElement>('meta[name="theme-color"]');
for (const meta of metas) {
const media = meta.getAttribute('media');
if (!media || window.matchMedia(media).matches) {
return meta.content;
}
}
- return getComputedStyle(document.body).backgroundColor;
+ // A body with no background of its own computes to `rgba(0, 0, 0, 0)`, and `readColor` drops
+ // alpha — so an unset background would resolve to black. `Canvas` is what the browser actually
+ // paints there, and it tracks `color-scheme`.
+ const body = getComputedStyle(document.body).backgroundColor;
+ const html = getComputedStyle(document.documentElement).backgroundColor;
+ const opaque = (value: string) => Boolean(value) && !/^(transparent|rgba\(0, 0, 0, 0\))$/.test(value);
+ if (opaque(body)) {
+ return body;
+ }
+ return opaque(html) ? html : 'Canvas';
}Confirm that the canvas keeps the page's colour when the host app ships neither a theme-color meta nor a body background.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function readBaseColor(): string { | |
| const metas = document.head.querySelectorAll<HTMLMetaElement>('meta[name="theme-color"]'); | |
| for (const meta of metas) { | |
| const media = meta.getAttribute('media'); | |
| if (!media || window.matchMedia(media).matches) { | |
| return meta.content; | |
| } | |
| } | |
| return getComputedStyle(document.body).backgroundColor; | |
| } | |
| function readBaseColor(): string { | |
| const metas = document.head.querySelectorAll<HTMLMetaElement>('meta[name="theme-color"]'); | |
| for (const meta of metas) { | |
| const media = meta.getAttribute('media'); | |
| if (!media || window.matchMedia(media).matches) { | |
| return meta.content; | |
| } | |
| } | |
| // A body with no background of its own computes to `rgba(0, 0, 0, 0)`, and `readColor` drops | |
| // alpha — so an unset background would resolve to black. `Canvas` is what the browser actually | |
| // paints there, and it tracks `color-scheme`. | |
| const body = getComputedStyle(document.body).backgroundColor; | |
| const html = getComputedStyle(document.documentElement).backgroundColor; | |
| const opaque = (value: string) => Boolean(value) && !/^(transparent|rgba\(0, 0, 0, 0\))$/.test(value); | |
| if (opaque(body)) { | |
| return body; | |
| } | |
| return opaque(html) ? html : 'Canvas'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/components/dialog/browser-chrome.ts` around lines 148
- 157, Update the fallback in readBaseColor to use the CSS system Canvas color
instead of document.body’s computed backgroundColor when no applicable
theme-color meta exists. Preserve the existing meta-selection behavior and
ensure the fallback resolves the actual page canvas color, including
color-scheme-aware defaults.
| /** | ||
| * Called by every mounted backdrop. Refcounted like floating-ui's scroll lock, so stacked dialogs | ||
| * compose: the first open captures and tints, each further open re-derives from the deeper scrim, | ||
| * and only the last close restores. | ||
| * | ||
| * @param backdrop - the element whose computed background and transition timing drive both the | ||
| * target colour and how long it takes to get there. Reading the timing from CSS rather than | ||
| * duplicating a constant means the chrome automatically follows the sheet's longer fade on mobile. | ||
| */ | ||
| /** | ||
| * The colour the chrome should show right now: the captured base with every open dialog's scrim | ||
| * composited over it in order, so two stacked dialogs land on the same value their two backdrops | ||
| * do. Recomputed from scratch on every change rather than accumulated, which is what makes | ||
| * closing one of them exactly reversible. | ||
| */ | ||
| function resolveTint(): string | null { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc blocks are detached from the symbols they describe. In both files a doc block sits above the wrong declaration because a second block was inserted between it and its symbol. The intended target is then left undocumented.
packages/ui/src/mosaic/components/dialog/browser-chrome.ts#L229-L244: move the "Called by every mounted backdrop" block, including its@param backdroptag, down toacquireBrowserChromeat Line 259.resolveTinttakes no parameters.packages/ui/src/mosaic/components/dialog/dialog.styles.ts#L153-L188: move the "Named for what the surface IS" block down tosizesat Line 230. It describes theprompt,card, andpanelwidths, notviewportSizes.
As per coding guidelines: "All public APIs must be documented with JSDoc".
📍 Affects 2 files
packages/ui/src/mosaic/components/dialog/browser-chrome.ts#L229-L244(this comment)packages/ui/src/mosaic/components/dialog/dialog.styles.ts#L153-L188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/components/dialog/browser-chrome.ts` around lines 229
- 244, In packages/ui/src/mosaic/components/dialog/browser-chrome.ts lines
229-244, move the “Called by every mounted backdrop” JSDoc, including its `@param`
backdrop tag, to acquireBrowserChrome around line 259; leave resolveTint
documented only by the block describing its parameterless tint calculation. In
packages/ui/src/mosaic/components/dialog/dialog.styles.ts lines 153-188, move
the “Named for what the surface IS” JSDoc to sizes around line 230, where it
documents the prompt, card, and panel widths; do not leave it attached to
viewportSizes.
Source: Coding guidelines
| // A `panel` dialog (account profile) opening a `card` dialog (add an email address) is a | ||
| // real shape, so the `FloatingTree` nesting the headless README claims is exercised here | ||
| // rather than assumed. Dismissal must reach the topmost dialog only, and the body must | ||
| // stay locked until the last one closes. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The nested example is documented as a card dialog but renders a prompt. In both files the inner Dialog passes no size, so it resolves to the default prompt. The MDX at packages/swingset/src/stories/dialog.component.mdx line 352 and the story JSDoc at line 111 both say prompt, so these two comments are the stale ones.
packages/ui/src/mosaic/components/dialog/dialog.test.tsx#L154-L157: change "opening acarddialog (add an email address)" to "opening apromptdialog", or passsize='card'to the innerDialoginNestedif thecardshape is what the suite means to exercise.packages/swingset/src/stories/dialog.component.stories.tsx#L159-L159: change "withcarddialogs opened from inside it" to "withpromptdialogs opened from inside it".
📍 Affects 2 files
packages/ui/src/mosaic/components/dialog/dialog.test.tsx#L154-L157(this comment)packages/swingset/src/stories/dialog.component.stories.tsx#L159-L159
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/components/dialog/dialog.test.tsx` around lines 154 -
157, Update the nested-dialog comments to describe the actual inner dialog size
as prompt: in packages/ui/src/mosaic/components/dialog/dialog.test.tsx lines
154-157 replace the card wording, and in
packages/swingset/src/stories/dialog.component.stories.tsx line 159 replace card
with prompt. No implementation change is needed.
| it('keeps one meta for stacked dialogs and removes it only with the last', async () => { | ||
| const user = userEvent.setup(); | ||
| render( | ||
| <Dialog defaultOpen> | ||
| <div>Outer</div> | ||
| <Dialog trigger={addEmailTriggerShared}> | ||
| <div>Inner</div> | ||
| </Dialog> | ||
| </Dialog>, | ||
| ); | ||
|
|
||
| await user.click(screen.getByRole('button', { name: 'Add email' })); | ||
| expect(document.head.querySelectorAll('meta[name="theme-color"]')).toHaveLength(1); | ||
|
|
||
| await user.keyboard('{Escape}'); | ||
| expect(themeColor()).not.toBeNull(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This test does not cover the second half of its name, and the count assertion can pass early.
Two problems:
- The name promises removal "only with the last" dialog. The test closes the inner dialog and asserts the meta is still present. It never closes the outer dialog, so the removal path is untested.
- Line 528 asserts the meta count synchronously. The suite establishes at lines 463-465 that acquisition happens one frame after mount. A synchronous
toHaveLength(1)can therefore observe the outer dialog's meta before the inner dialog would have added a second one, so it does not prove the stack shares one meta.
💚 Proposed fix
await user.click(screen.getByRole('button', { name: 'Add email' }));
- expect(document.head.querySelectorAll('meta[name="theme-color"]')).toHaveLength(1);
+ // The inner dialog acquires a frame after mount, so settle before counting.
+ await waitFor(() => expect(screen.getByText('Inner')).toBeInTheDocument());
+ expect(document.head.querySelectorAll('meta[name="theme-color"]')).toHaveLength(1);
await user.keyboard('{Escape}');
expect(themeColor()).not.toBeNull();
+
+ await user.keyboard('{Escape}');
+ await waitFor(() => expect(themeColor()).toBeNull());
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/components/dialog/dialog.test.tsx` around lines 516 -
532, Update the stacked-dialog test to await the asynchronous theme-color
acquisition before asserting the meta count, ensuring it verifies a single
shared meta after both dialogs are active. Then close the inner dialog and
confirm the meta remains, close the outer dialog, and assert the theme-color
meta is removed, covering the complete “only with the last” behavior.
| const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); | ||
| render(<Dialog defaultOpen>Body</Dialog>); | ||
|
|
||
| await settle(); | ||
|
|
||
| expect(warn).toHaveBeenCalledWith(expect.stringContaining('no accessible name')); | ||
| warn.mockRestore(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore the console.warn spy in afterEach.
Four tests in this suite call warn.mockRestore() as the last statement of the test body. If an assertion above it throws, the restore never runs and console.warn stays mocked for every later test in the file. That hides warnings and can make an unrelated failure hard to diagnose.
Move the restore into a hook so it runs on both the pass and fail paths.
As per coding guidelines for **/*.{test,spec}.{jsx,tsx}: "Implement proper test isolation in React component tests" and "Use proper test cleanup in React component tests".
💚 Proposed fix
describe('accessible name warning', () => {
+ afterEach(() => vi.restoreAllMocks());
+
it('warns when the dialog has no accessible name', async () => {
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
render(<Dialog defaultOpen>Body</Dialog>);
await settle();
expect(warn).toHaveBeenCalledWith(expect.stringContaining('no accessible name'));
- warn.mockRestore();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); | |
| render(<Dialog defaultOpen>Body</Dialog>); | |
| await settle(); | |
| expect(warn).toHaveBeenCalledWith(expect.stringContaining('no accessible name')); | |
| warn.mockRestore(); | |
| describe('accessible name warning', () => { | |
| afterEach(() => vi.restoreAllMocks()); | |
| it('warns when the dialog has no accessible name', async () => { | |
| const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); | |
| render(<Dialog defaultOpen>Body</Dialog>); | |
| await settle(); | |
| expect(warn).toHaveBeenCalledWith(expect.stringContaining('no accessible name')); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/components/dialog/dialog.test.tsx` around lines 537 -
543, Move the console.warn spy restoration for the affected tests into an
afterEach hook, ensuring every created spy is restored on both passing and
failing paths; remove the inline warn.mockRestore() calls while preserving each
test’s warning assertions.
Source: Coding guidelines
| const labelledBy = node.getAttribute('aria-labelledby'); | ||
| const named = labelledBy | ||
| ?.split(/\s+/) | ||
| .filter(Boolean) | ||
| .some(id => node.ownerDocument.getElementById(id)?.textContent?.trim()); | ||
| if (named) { | ||
| return; | ||
| } | ||
| console.warn( | ||
| `[clerk] <${component}.Popup> renders a dialog with no accessible name. Pass \`aria-label\`, or render a \`<${component}.Title>\` inside it.`, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not use textContent as the accessible-name check.
A referenced element can provide its name through aria-label or alternative content while its textContent is empty. This code then emits a false warning. PopoverPopupProps supports arbitrary existing aria-labelledby targets. Accessible-name calculation recursively processes referenced elements. (w3.org)
Treat a resolved ID target as valid for this development warning, or use a standards-compliant accessible-name calculation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/hooks/useAccessibleNameWarning.ts` around lines 37 -
47, Update the accessible-name check in the warning logic around labelledBy so
any existing DOM element resolved by aria-labelledby is treated as a valid name
source, without requiring non-empty textContent. Preserve the existing
aria-label and component title handling, and avoid introducing a text-only
validation that rejects valid aria-label or alternative-content targets.
76a1f93 to
8b8b052
Compare
While a dialog is open, tints the browser's own chrome so it reads as one
continuous surface rather than a dimmed page inside undimmed furniture. Two
surfaces move together: `<meta name="theme-color">` for the address bar and
toolbar, and `<body>`'s background for the canvas outside the layout viewport —
the overscroll gutter and the strip revealed as the address bar collapses,
neither of which a `position: fixed` scrim covers.
It ships no colour of its own. The target is derived — the backdrop's computed
background composited over whatever the page already had — so it stays correct
if a consumer retunes the scrim. The meta is prepended rather than mutated, so
it overrides the app's own (including framework-managed tags like Next's
`viewport.themeColor`) and removing it restores theirs with no bookkeeping.
Refcounted across stacked dialogs, reverted exactly on close, and inert wherever
`theme-color` is ignored. Opt out with `syncBrowserChrome={false}`.
Split out of #9388: `animate` writes `document.body.style.backgroundColor` on
every frame, which is a full-viewport repaint per frame during the entrance.
That wants profiling on real mobile hardware before it ships. If it measures
badly, the fix is to set the body colour once at the target rather than
animating it — it is only ever visible outside the layout viewport, so nobody
sees it mid-entrance.
8b8b052 to
c4c7f4d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/ui/src/mosaic/components/dialog/dialog.test.tsx (1)
567-581: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the fixed 250 ms sleep, and drop the trailing
Escape.Two points:
- Line 578 sleeps for a hard-coded 250 ms. The value must track the teardown duration in
browser-chrome.ts. Prefer fake timers, orwaitForon an observable condition, so the test does not depend on a magic delay.- Line 580 presses
Escapeon a dialog rendered with a controlledopenprop and noonOpenChange. That key press cannot close the dialog, so the line has no effect.cleanup()at line 12 already unmounts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/mosaic/components/dialog/dialog.test.tsx` around lines 567 - 581, Update the dialog reopen test around the deferred teardown assertion to avoid the hard-coded 250 ms sleep by using fake timers tied to the teardown duration from browser-chrome.ts, or waitFor an observable teardown condition. Remove the trailing user.keyboard('{Escape}') call because this controlled dialog has no onOpenChange and cleanup already unmounts it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/mosaic/components/dialog/dialog.test.tsx`:
- Around line 554-565: Update the “opts out with syncBrowserChrome={false}” test
to await the same metadata-settling point used by the positive tests before
asserting themeColor(). Keep the expectation that themeColor() is null, but make
it occur after asynchronous acquisition has completed so the opt-out behavior is
actually validated.
- Around line 509-512: Update the dialog test cleanup hook to fully reset
browser-chrome state: await the pending teardown before removing theme-color
metadata, or reset the browser-chrome snapshot, layers, and teardown state after
cleanup. Ensure later dialogs cannot reuse a detached snapshot.meta.
---
Nitpick comments:
In `@packages/ui/src/mosaic/components/dialog/dialog.test.tsx`:
- Around line 567-581: Update the dialog reopen test around the deferred
teardown assertion to avoid the hard-coded 250 ms sleep by using fake timers
tied to the teardown duration from browser-chrome.ts, or waitFor an observable
teardown condition. Remove the trailing user.keyboard('{Escape}') call because
this controlled dialog has no onOpenChange and cleanup already unmounts it.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e2b7cb2-9706-40c6-b4c3-4048f11ee309
📒 Files selected for processing (3)
packages/swingset/src/stories/dialog.component.mdxpackages/ui/src/mosaic/components/dialog/dialog.test.tsxpackages/ui/src/mosaic/components/dialog/dialog.tsx
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/clerk-ios(auto-detected)clerk/cli(auto-detected)clerk/clerk-android(auto-detected)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/mosaic/components/dialog/dialog.tsx
| afterEach(() => { | ||
| document.head.querySelectorAll('meta[name="theme-color"]').forEach(m => m.remove()); | ||
| document.body.style.backgroundColor = ''; | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect module-level state and teardown timing in browser-chrome.ts.
fd -t f 'browser-chrome.ts' -x cat -n {}Repository: clerk/javascript
Length of output: 15916
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- dialog test setup and browser-chrome tests ---'
sed -n '500,590p' packages/ui/src/mosaic/components/dialog/dialog.test.tsx
printf '%s\n' '--- browser chrome imports and call sites ---'
rg -n -C 4 'acquireBrowserChrome|useBrowserChrome|data-(starting|ending)-style' packages/ui/src/mosaic packages/ui/src | head -240
printf '%s\n' '--- test configuration and relevant package scripts ---'
fd -t f '(vitest|jest|package.json)' . | head -80Repository: clerk/javascript
Length of output: 30789
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- remaining browser-chrome tests and imports ---'
sed -n '1,90p' packages/ui/src/mosaic/components/dialog/dialog.test.tsx
sed -n '567,670p' packages/ui/src/mosaic/components/dialog/dialog.test.tsx
printf '%s\n' '--- dialog browser-chrome lifecycle ---'
rg -n -C 12 'useBrowserChrome|acquireBrowserChrome|syncBrowserChrome' packages/ui/src/mosaic/components/dialog
printf '%s\n' '--- cleanup registration and test setup ---'
rg -n -C 8 'cleanup\(|afterEach|auto.*cleanup|`@testing-library/react`' packages/ui packages | head -240Repository: clerk/javascript
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- UI test configuration ---'
fd -t f -i 'vitest.config.*|vitest.setup.*' packages/ui -x sh -c 'echo "--- $1"; sed -n "1,180p" "$1"' sh {}
printf '%s\n' '--- duration token definitions and style injection ---'
rg -n -C 5 'durationVars|--cl-duration-fast|transitionDuration|stylex.inject|styleSheet|cssVars' packages/ui/src packages/ui/vitest.config.* packages/ui/vitest.setup.* 2>/dev/null | head -260
printf '%s\n' '--- exact hook and cleanup locations ---'
nl -ba packages/ui/src/mosaic/components/dialog/dialog.test.tsx | sed -n '1,16p;505,518p'Repository: clerk/javascript
Length of output: 23062
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- Mosaic duration declarations and dialog style application ---'
sed -n '270,300p' packages/ui/src/mosaic/tokens.stylex.ts
rg -n -C 8 'durationVars|durationDefaults|InternalThemeProvider|ThemeProvider' packages/ui/src/mosaic packages/ui/src/styledSystem | head -220
printf '%s\n' '--- dialog test helper and cleanup neighborhood ---'
rg -n -C 6 'function themeColor|const themeColor|afterEach\\(' packages/ui/src/mosaic/components/dialog/dialog.test.tsx
printf '%s\n' '--- source invariant verifier: cleanup, open tests, and teardown branch ---'
python3 - <<'PY'
from pathlib import Path
test = Path('packages/ui/src/mosaic/components/dialog/dialog.test.tsx').read_text()
chrome = Path('packages/ui/src/mosaic/components/dialog/browser-chrome.ts').read_text()
checks = {
'file cleanup calls RTL cleanup': 'afterEach(() => cleanup())' in test,
'file cleanup removes theme-color metas': 'querySelectorAll(\'meta[name="theme-color"]\')' in test,
'an open-dialog test omits explicit close': "render(<Dialog defaultOpen>Body</Dialog>);" in test,
'release schedules positive-duration teardown': 'teardown = window.setTimeout(finish, durationMs);' in chrome,
'zero-duration release finishes synchronously': 'if (durationMs <= 0)' in chrome and 'finish();' in chrome,
'new acquire reuses existing snapshot': 'if (!snapshot) {' in chrome and 'document.head.prepend(meta);' in chrome,
}
for name, result in checks.items():
print(f'{name}: {result}')
PYRepository: clerk/javascript
Length of output: 23895
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
test = Path('packages/ui/src/mosaic/components/dialog/dialog.test.tsx').read_text()
chrome = Path('packages/ui/src/mosaic/components/dialog/browser-chrome.ts').read_text()
styles = Path('packages/ui/src/mosaic/components/dialog/dialog.styles.ts').read_text()
tokens = Path('packages/ui/src/mosaic/tokens.stylex.ts').read_text()
checks = {
'RTL cleanup is registered': 'afterEach(() => cleanup())' in test,
'manual meta cleanup is registered': 'querySelectorAll(\'meta[name="theme-color"]\')' in test,
'an open dialog is rendered without an explicit close': "render(<Dialog defaultOpen>Body</Dialog>);" in test,
'release schedules delayed teardown': 'teardown = window.setTimeout(finish, durationMs);' in chrome,
'release clears state only in finish': 'snapshot = null;' in chrome and 'meta.remove();' in chrome,
'acquire reuses an existing snapshot': 'if (!snapshot) {' in chrome and 'document.head.prepend(meta);' in chrome,
'dialog uses a nonzero fast duration token': "default: durationVars['--cl-duration-fast']" in styles,
'fast duration token is nonzero': "'--cl-duration-fast': '0.1s'" in tokens,
}
for name, result in checks.items():
print(f'{name}: {result}')
print('--- lifecycle consequence ---')
print('If cleanup releases a layer with durationMs > 0, snapshot remains non-null until finish.')
print('If manual meta cleanup runs before finish, the next acquire clears teardown and reuses detached snapshot.meta.')
PY
nl -ba packages/ui/src/mosaic/components/dialog/dialog.test.tsx | sed -n '1,8p;505,515p;535,570p'Repository: clerk/javascript
Length of output: 762
Reset browser-chrome state during test cleanup.
cleanup() releases an open dialog, but browser-chrome.ts keeps snapshot until teardown finishes. The hook then removes the meta element immediately, so a later dialog can reuse a detached snapshot.meta. Wait for teardown before removing the meta, or reset snapshot, layers, and teardown in test cleanup.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/components/dialog/dialog.test.tsx` around lines 509 -
512, Update the dialog test cleanup hook to fully reset browser-chrome state:
await the pending teardown before removing theme-color metadata, or reset the
browser-chrome snapshot, layers, and teardown state after cleanup. Ensure later
dialogs cannot reuse a detached snapshot.meta.
| it('opts out with syncBrowserChrome={false}', () => { | ||
| render( | ||
| <Dialog | ||
| defaultOpen | ||
| syncBrowserChrome={false} | ||
| > | ||
| Body | ||
| </Dialog>, | ||
| ); | ||
|
|
||
| expect(themeColor()).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The opt-out test can pass even if the opt-out is broken.
Acquisition is asynchronous. Lines 530-532 and line 573 both wait for the meta because the backdrop carries data-starting-style on the mount frame. This test asserts toBeNull() synchronously, so it observes the state before acquisition could happen either way. Wait for the same settling point that the positive tests use, then assert that no meta appeared.
💚 Proposed fix
- it('opts out with syncBrowserChrome={false}', () => {
+ it('opts out with syncBrowserChrome={false}', async () => {
render(
<Dialog
defaultOpen
syncBrowserChrome={false}
>
Body
</Dialog>,
);
+ // Give acquisition the frame it needs in the enabled case, then prove nothing was added.
+ await settle();
expect(themeColor()).toBeNull();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('opts out with syncBrowserChrome={false}', () => { | |
| render( | |
| <Dialog | |
| defaultOpen | |
| syncBrowserChrome={false} | |
| > | |
| Body | |
| </Dialog>, | |
| ); | |
| expect(themeColor()).toBeNull(); | |
| }); | |
| it('opts out with syncBrowserChrome={false}', async () => { | |
| render( | |
| <Dialog | |
| defaultOpen | |
| syncBrowserChrome={false} | |
| > | |
| Body | |
| </Dialog>, | |
| ); | |
| // Give acquisition the frame it needs in the enabled case, then prove nothing was added. | |
| await settle(); | |
| expect(themeColor()).toBeNull(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/ui/src/mosaic/components/dialog/dialog.test.tsx` around lines 554 -
565, Update the “opts out with syncBrowserChrome={false}” test to await the same
metadata-settling point used by the positive tests before asserting
themeColor(). Keep the expectation that themeColor() is null, but make it occur
after asynchronous acquisition has completed so the opt-out behavior is actually
validated.
1 participant
Description
[WIP] — the behaviour is complete and tested; holding for the performance pass described at the bottom.
While a dialog is open, tints the browser's own chrome so it reads as one continuous surface rather than a dimmed page sitting inside undimmed furniture. Two surfaces move together:
<meta name="theme-color">— the address bar and toolbar on iOS Safari and Chrome/Firefox for Android.<body>'s background — this propagates to the canvas, which paints everything outside the layout viewport: the rubber-band overscroll gutter, the strip revealed as the address bar collapses, and the area behind the home indicator. Aposition: fixedscrim covers none of those, so without it the app's own colour shows through at the edges as an undimmed band.It ships no colour of its own. The target is derived — the backdrop's computed background composited over whatever the page already had — so it stays correct if a consumer retunes the scrim, and the colour work runs through a canvas rather than string parsing because a computed colour is not necessarily
rgb()(ours serialises asoklab(), where reading the numbers positionally turns white into black).The meta is prepended rather than mutated, so it overrides the app's own tag — including framework-managed ones like Next's
viewport.themeColor— and removing it restores theirs with no bookkeeping. Refcounted across stacked dialogs, reverted exactly on close, and inert wherevertheme-coloris ignored. Opt out withsyncBrowserChrome={false}.Split out of #9388 so the StyleX rebuild could land without waiting on this.
Before this is ready
willReadFrequently, colours resolve once per open, and the easing is sampled into a lookup table. It isanimate's per-frameapply(): it writesdocument.body.style.backgroundColorevery frame, and body background is not GPU-compositable, so that is a full-viewport repaint on every frame of the entrance. It also rewritesmeta.contentevery frame, which re-tints native chrome on iOS Safari.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change