fix(react-grab): harden app theme detection (oklch backgrounds, color-scheme, body markers)#476
Merged
Merged
Conversation
Browsers serialize getComputedStyle().backgroundColor in the authored color space (oklch/oklab/color()) instead of always normalizing to rgb(). The rgb-only luminance parser therefore failed on modern theming setups and fell back to prefers-color-scheme, mis-detecting forced-light pages as dark for dark-OS visitors. Normalize background colors through a canvas parser and fall back to the document element when the body is transparent. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
…kers - color-scheme: light dark/dark light advertises both schemes; the active one follows the OS preference, not token order. Defer to luminance / prefers-color-scheme instead of always returning the first token, which mis-detected dark-OS visitors on dual-scheme sites. - Inspect <body> as well as <html> for theme markers (class, data-theme/ data-bs-theme/etc., presence attributes) for apps that theme the body. - Add e2e coverage for dual/single color-scheme and body-level markers. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
cursor
Bot
changed the title
Code-quality follow-up from a thermo-nuclear review: - Delete parse-css-color.ts (a second cached-canvas singleton + duplicate rgba type). Reuse the existing parseAnyColor + parseHexChannels stack, which already resolves oklch (Tailwind v4's format) via exact math and needs no getImageData (avoids canvas-fingerprinting perturbation). - Replace the three repeated 'html ?? body' ternaries in detectTheme with a firstThemeFromRoots(roots, classify) walk, surfacing the root-first vs body-first precedence as explicit data instead of burying the body-first luminance order in a separate helper. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
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
2 participants
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.
Problem
react-grab paints its overlay in the inverse of the detected app theme so it stays legible. On a light page, the host should carry
data-rg-theme="dark".Several real-world setups were mis-detected, leaving a low-contrast overlay (e.g. a clearly-light page reporting
data-rg-theme="light"):oklch()backgrounds — e.g. Tailwind v4, which authors colors inoklch.color-scheme— sites that opt into both schemes viacolor-scheme: light dark.<body>instead of<html>.Root causes & fixes
1.
oklchbackgroundsdetectTheme()falls back to a background-luminance heuristic, which parsedgetComputedStyle().backgroundColorwith an rgb()-only regex. Modern browsers serialize computed colors in the authored color space (verified in Chromium: anoklch(1 0 0)background comes back verbatim as"oklch(1 0 0)"). The regex never matched, luminance returnednull, and detection fell through toprefers-color-scheme— so a forced-light page looked dark to a dark-OS visitor.Fix: reuse the package's existing canonical color stack —
parseAnyColor(which already resolvesoklchvia exact OKLab→sRGB math, plus named/rgb/hsl/hwb via canvas) followed byparseHexChannels— for the luminance read. This needs nogetImageData, so it's also immune to canvas-fingerprinting perturbation (Brave/Tor/Firefox-RFP). Backgrounds in exotic syntaxesparseAnyColorcan't resolve (e.g.oklab()/color()) simply degrade to theprefers-color-schemefallback rather than being mis-read.2.
color-scheme: light dark/dark lightA dual
color-schemeadvertises support for both schemes; the active one is chosen by the OS preference (and reflected in the actual paint), not by token order. The previous code returned the first token (light), mis-detecting dark-OS visitors.Fix: when both schemes are listed, defer to luminance /
prefers-color-schemeinstead of guessing. Single-valuecolor-schemestill forces that theme.3. Theme markers on
<body>Detection only inspected
<html>, missing apps (and some Bootstrap/MUI setups) that put the theme class /data-*attribute on<body>.Fix: inspect both
<html>and<body>for class, theme attributes, and presence attributes. The mutation observer already watches both roots, so changes still re-trigger detection.Structure
detectTheme()is expressed as a smallfirstThemeFromRoots(roots, classify)walk, making the per-concern root precedence explicit data: markers andcolor-schemeare read root-first, the painted background body-first (frameworks usually paint the page background on<body>while declaring the theme on<html>).Tests
New
e2e/app-theme-detection.spec.tscovers all cases:color-scheme: light dark/dark lightdeferring to the OS preferencecolor-scheme: darkforcing dark against a light OS<body>Each was confirmed to fail against the pre-fix logic and pass with the fix. The existing
theme-customizationandedit-panel-colorsuites stay green (43/43 together).pnpm build,pnpm typecheck,pnpm lint, andpnpm formatare all clean.Includes a
patchchangeset forreact-grab.Code-quality pass
A thermo-nuclear review flagged that an earlier revision introduced a second cached-canvas color resolver duplicating
parse-any-color.ts. That utility was removed in favor of the canonical parser, and the repeatedhtml ?? bodybranches were collapsed into thefirstThemeFromRootswalk — net deletion of a file plus the duplicate type/canvas.Summary by cubic
Hardens app theme detection in
react-grabso the overlay picks the correct inverse theme on pages using modern CSS colors, dualcolor-scheme, or body-level theme markers. Works regardless of the user’s OS preference.Bug Fixes
oklch()/oklab()/color()values; falls back frombodytohtmlwhenbodyis transparent.color-scheme(light dark/dark light) as OS-decided; single-value still forces that theme.<body>as well as<html>.color-scheme, and body-level markers.Refactors
parseAnyColor+parseHexChannelsstack and removes the duplicate color parser.firstThemeFromRoots(...)walk to make root-first vs body-first precedence explicit.Written for commit 35ced63. Summary will update on new commits.