fix(core): resolve Toast fallback viewport's theme mode instead of OS preference#3743
Open
let-sunny wants to merge 1 commit into
Open
fix(core): resolve Toast fallback viewport's theme mode instead of OS preference#3743let-sunny wants to merge 1 commit into
let-sunny wants to merge 1 commit into
Conversation
|
@let-sunny is attempting to deploy a commit to the Meta Open Source Team on Vercel. A member of the Team first needs to authorize it. |
… preference useToast()'s fallback viewport mounts via createRoot() on a detached div, so Toast's useTheme() call can't see ThemeContext and falls back to prefers-color-scheme. When that disagrees with the app's actual <Theme mode>, the toast's inverted-surface text/icon can compute to the exact same color as its own background. Bridge the gap the same way facebook#1587 already does for CSS: read the mode off <html data-theme> (present means an explicit app mode, absent means "system", where the existing OS-preference fallback is already correct) and re-provide it into the fallback tree via ThemeContext, kept live with a MutationObserver since that attribute changes whenever <Theme mode> does.
d5b3695 to
04d8608
Compare
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
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
useToast()'s fallback viewport (the no-LayerProviderpath) can show a fully invisible toast: its body text and dismiss icon compute to the exact same color as the toast's own background. This PR fixes that by re-providing the app's theme mode into the fallback tree, reusing thedata-themeattribute that #1587 already keeps in sync on<html>.The bug fires whenever the app's
<Theme mode="light"|"dark">disagrees with the OS'sprefers-color-scheme. And the no-provider setup is not an edge case — it's exactly whatuseToast.doc.mjsdocuments and theNoProviderStorybook story demonstrates.Root cause: the fallback mounts
<ToastViewport>viacreateRoot()on a<div>appended todocument.body— a new, disconnected React tree, not a portal. React context doesn't cross that boundary. So whenToast.tsxcallsconst {mode} = useTheme()to pick its inverted-surface polarity, there's noThemeContextto read and the hook falls back to OS preference.#1586/#1587 already solved the CSS half of this disconnected-tree problem by syncing
data-astryx-theme/data-themeonto<html>; the JS half —useTheme()'s context-basedmode— was never covered. That split is what makes the worst case so bad: the toast's surface color (CSS, fixed) and its text color (JS, still wrong) come from two sources that can disagree, and they converge on the same color.The fix reads that same
data-themeattribute: present means an explicit app mode, absent meansmode="system", where the existing OS-preference fallback is already correct, so the provider deliberately supplies nothing.Aside on why this went unnoticed: the
NoProviderstory exists precisely to demonstrate the fallback, but the globalwithThemedecorator inapps/storybook/.storybook/preview.tsxwraps every story in<LayerProvider>. The story that showcases the fallback never actually exercises it.Change
packages/core/src/Toast/useToast.tsx:readRootThemeMode()readsdocument.documentElement.getAttribute('data-theme')and returns'light' | 'dark' | null.nullcovers bothmode="system"and no root<Theme>at all — the same "let OS preference decide" semanticsuseTheme()already has.FallbackThemeProviderwraps the fallback's<ToastViewport>and re-providesThemeContextwith that mode. AMutationObserverondata-themekeeps it live, since fix(theme): sync data-xds-theme to <html> for root provider; add LayerProvider to sandbox #1587 updates the attribute whenever<Theme mode>changes. (Same observer pattern asOutline/useOutlineFromDOM.ts.)valuetoggles between the derived mode andnull. An earlier version omitted the Provider element when mode was unknown — that changes the tree shape on every mode transition, remountsToastViewport, and silently drops any toast on screen. The new tests caught it.{mode, theme: null}. The null is not a shortcut: only the mode is reflected to the DOM, while the theme object exists solely in React context (CSS receives the theme separately, through the theme stylesheet anddata-astryx-theme), so there is nothing theme-shaped for the fallback to read. It's also safe for anything that renders inside the fallback tree — toast bodies are arbitraryReactNode— becauseuseThemealready coalescesctx?.theme ?? nulland resolves a null theme to the default tokens, exactly what the no-provider path returns today. The one fieldToast's polarity decision actually reads,mode, is now correct.packages/core/src/theme/useTheme.ts: one type-only change.ThemeContextValue.themewidens toDefinedTheme | nullso the provider can express "mode known, theme not" honestly instead of casting. The null-theme state has always existed at runtime via the no-provider path; this just lets the type say so.Themealways provides a real theme,useThemeis the type's only reader (verified repo-wide), and the hook's public return type (UseThemeReturn) is unchanged — no consumer ofuseTheme()sees a null or gains a null check.packages/core/src/Toast/useToast.test.tsx(new): co-located tests for the fallback's theme provision..changeset/toast-fallback-theme-mode.md: patch bump for@astryxdesign/core.Alternatives considered
This PR reads the DOM attribute; the alternative was capturing the real
ThemeContextvalue at theuseToast()call site (it runs in the app tree) and re-providing it into the fallback root — the same cross-root plumbingFallbackCapturealready does forToastContext, in the opposite direction, and it would carry the full theme object rather than just the mode. Two reasons for the attribute:<html>sync as the robust core fix. This PR consumes the attribute that fix already maintains, adding no new state.useToastwould subscribe every component that calls the hook to theme changes — a re-render per mode switch across the app, for a hook that is deliberately subscription-free today.Happy to reshape toward context capture if that's the preferred direction.
Verification
New tests (3): the bug case asserts the fallback resolves the app's mode, and fails with the fix reverted;
mode="system"keeps today's OS-preference behavior; theLayerProviderpath is untouched.Toast/+theme/suites: 245/245. Typecheck and lint clean. (The monorepo-wide test run also shows 6 pre-existingpackages/clifailures that reproduce identically on a cleanupstream/maincheckout — A/B verified, unrelated.)End-to-end against the built
dist/output, driven with Playwright (colorScheme: 'dark'emulating OS dark).data-astryx-mediais the attributeMediaThemesets fromuseTheme()'s resolved mode:data-astryx-mediabefore fixmode="light"(the bug)light(wrong)dark(correct)rgb(37, 37, 42)→rgb(255, 255, 255)mode="system"(no regression)lightlight(unchanged)rgb(37, 37, 42)(unchanged)LayerProvider(control)darkdark(unchanged)rgb(255, 255, 255)(unchanged)* (If you re-verify from a source rebuild and the toast background doesn't flip in the fallback case, that's the pre-existing [Bug] Built theme.css pins color-scheme to "light dark", so <Theme mode> can't force light/dark on <html> #3658, fix pending in fix(cli): make theme-build color-scheme decl mode-aware #3660 — unrelated here, and it doesn't affect the
data-astryx-media/icon evidence above.)Repro
With OS
prefers-color-scheme: darkemulated (PlaywrightcolorScheme: 'dark'), the dismiss icon and body text compute to the same color as the toast's own background.Screenshots
Before — the toast in the bottom-right is a blank dark rectangle. Its surface color comes from the CSS side (correct since #1587) while its text/icon polarity comes from the JS mode (wrong in the fallback), and the two converge on the same color:
Control — identical conditions but with
LayerProvider. This is also what the fallback renders like after this fix: