Skip to content

fix(studio): parse modern CSS colors without a new dependency - #3931

Merged
miguel-heygen merged 15 commits into
mainfrom
fix/studio-native-css-colors
Oct 2, 2026
Merged

miguel-heygen merged 15 commits into
mainfrom
fix/studio-native-css-colors

Conversation

@jrusso1020

@jrusso1020 jrusso1020 commented Sep 14, 2026 •

Copy link
Copy Markdown
Collaborator

Studio’s color parser treats modern CSS colors and named gradient stops as unparseable, so the inspector can display black and gradient insertion can copy a neighboring stop instead of using the colour the gradient paints there.

Use the browser’s CSS parser to convert modern colors into sRGB through relative-color serialization, and share the parser with gradient interpolation. Extend the existing hex/RGB path to preserve alpha for short/long alpha hex and space-separated RGB. This is an alternative to #3752 with no new dependencies or lockfile changes. Modern-color conversion requires browser support for relative CSS colors; server-side parsing retains the hex/RGB path.

Validation: 43 focused Vitest tests pass (colorValue 21, gradientValue 7, colorBrowserCanvas 15), plus the colour-literal ratchet. A real Chrome 146 regression script covers 18 color cases, computed-style picker conversion, alpha preservation, and two gradient cases. Studio typecheck, oxlint, oxfmt, and the fallow audit pass. Run the browser checks with bun run --cwd packages/studio test:colors:browser.

Follow-up commit: the inline text toolbar drops its own canvas fallback and uses parseCssColor alone, and rgb() channels must be well-formed numbers (so rgb(1. 2 3) is rejected, as CSS does). Rebased onto current main.

The browser path first tries color(from <value> srgb r g b / alpha), which needs Chrome 119+ or Safari 18+ and keeps alpha at full precision. If the browser cannot resolve that, it reads the colour itself from the canvas and accepts a #rrggbb or rgba() result, which every browser gives for named, hsl() and percent rgb() colours (with 8-bit alpha). In an older browser, legacy colours still resolve (checked in Chrome 113: red, hsl(210 40% 50%), rgb(100%, 0%, 0%)), while oklch(), lab() and color() return null and the picker shows black, as on main.

Both reads go through a canvas fillStyle, so nothing is rasterised. An invalid assignment leaves the previous fillStyle in place, so each value is read twice, after resetting to transparent and then to white; if the two reads differ, the canvas rejected it and the parser returns null.

A stop inserted into a gradient now takes the colour the gradient paints at that point: color-mix(in oklab, <left>, <right> <p>%), or in srgb when both stops are legacy colours, resolved by the same parser. That follows the browser's gradient interpolation, including premultiplied alpha (checked in Chrome 147: oklch(0.7 0.15 200) to red gives #B28979, (178, 137, 121), the painted midpoint; #ff000000 to #0000ffff gives rgba(0, 0, 255, 0.5)). When the browser cannot resolve the mix (no DOM, or an older browser), the stop falls back to the per-channel sRGB blend, which is exact for opaque legacy stops. The space is chosen for the whole gradient, as Chrome does: Oklab when any stop is a modern colour, sRGB when every stop is a legacy colour (hex, named, rgb, hsl, hwb). A stop that is a bare var() is unknown until it resolves (a modern function with a var() inside is still modern), so it neither forces Oklab nor rules it out; then the space is sRGB unless another stop is modern.

Context-dependent colours (light-dark(), currentcolor, var()) never go through the parser's canvas, which has no colour scheme or element to resolve them against. The inline text toolbar's picker then reads the selected text's computed colour, i.e. what the browser paints, so light-dark(white, red) in a dark-scheme composition shows red. The real-Chrome script covers that dark-scheme selection.

Unit tests load the colour modules against a fake canvas that serialises like Chrome 147 (colorBrowserCanvas.test.ts, kept inside a test file so its Chrome serialisations stay out of the colour-literal ratchet), so the modern path, alpha, the currentcolor exclusion, the parse order and the gradient mix each fail a unit test when broken.

Before

Text coloured oklch(0.7 0.15 200), picker opened from the Text section's Color row: the swatch and hex read black (#000000).

Before

After

The same element at this head: the swatch and hex read #00B9C3.

After

jrusso1020 and others added 2 commits October 2, 2026 08:48
…b numbers

The inline text toolbar kept its own canvas fallback for named colours.
parseCssColor now resolves them, so the toolbar uses it alone.
rgb() channels must be well-formed numbers; that replaces the NaN check
after parsing and rejects rgb(1. 2 3) as CSS does.
@miguel-heygen
miguel-heygen force-pushed the fix/studio-native-css-colors branch from a35bfd5 to 37e5e3c Compare October 2, 2026 13:07
Comment thread packages/studio/src/components/editor/colorValue.ts Fixed
Comment thread packages/studio/src/components/editor/colorValue.ts Fixed
…the parser

The rgb() number pattern now has one way to match each digit run, which clears
CodeQL's slow-regex alerts. The canvas reset uses transparent so the Studio
colour-literal ratchet stays at its baseline.
@github-actions

github-actions Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 1216 (base branch 1216), smooth 1120 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (1)

…ur syntax

parseBrowserColor reads the colour itself from the canvas first and accepts a
#rrggbb or rgba() result, then tries color(from ...). Named, hsl() and percent
rgb() colours resolve again in the text toolbar on Chrome before 119 and Safari 17.
@miguel-heygen
miguel-heygen marked this pull request as ready for review October 2, 2026 15:58

@somanshreddy somanshreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at 37f7b4a2. This is a comment, not an approval. I did my own pass and an independent Codex pass in a separate checkout, then checked each finding at this head. I ran the unit suites and the PR's browser script on Chromium 154, probed edge cases in the real browser, and mutation-tested the parser's guards. CI is green (88 passed, 2 skipped).

No blockers. The fix does what the description says. On Chromium, the picker reads oklch(0.7 0.15 200) as #00b9c3, and all 18 cases in native-css-colors.mjs pass. A few things I checked hold up:

  • The double-read rejection is sound, including inputs that equal one of the two sentinels:
    • Canvas (#ffffff), color(srgb 1 1 1) and color(srgb 0 0 0 / 0) are accepted;
    • inherit, initial and rgb(1. 2 3) return null.
  • The new rgb() patterns run in linear time. 50,000-character adversarial inputs take 0.2–4 ms. CodeQL's two alerts were on 96bbfc81, and nothing is open at this head.
  • The canvas context is cached, so a parse costs a few fillStyle assignments.

Should fix

  1. The headline conversion is only tested by a script that CI never runs. No workflow calls test:colors:browser. The unit tests' fake canvas only knows legacy spellings, so the color(from … srgb r g b / alpha) branch never succeeds under Vitest. I applied 9 mutations to the parser:

    Mutation Unit tests Browser script
    Drop the color(from …) branch (oklch, lab, display-p3 and color-mix go back to black) 63/63 pass (colour, gradient, panel and toolbar suites) fails
    Ignore alpha in parseSrgbSerialization 1,765 pass (all editor suites) fails
    Drop the currentcolor/var( exclusion 1,765 pass fails
    Drop the CSS.supports gate 1,765 pass passes
    The other 5: the double-read compare, the white reset, the number pattern, 8-digit-hex alpha, % channels fail —

    So the main thing this PR fixes could regress with CI still green. Either add the script to ci.yml, which already runs Chrome for timeline-virtualization.mjs and studio-open-counts.mjs, or give the fake canvas a color(srgb …) serialisation so the second branch has unit coverage. Codex reached the same conclusion.

  2. The colour of an inserted gradient stop still doesn't match what the browser paints (gradientValue.ts:388-401). It is in two ways:

    • Modern stops. Chrome interpolates a gradient that has any non-legacy stop in Oklab. For linear-gradient(90deg, oklch(0.7 0.15 200) 0%, red 100%), the rendered midpoint pixel is (178, 136, 120), but a stop inserted at 50% is #805D62 (128, 93, 98). Inserting it visibly darkens the middle of the gradient.
    • Alpha. Gradients interpolate with premultiplied alpha, and this blend doesn't. For #ff000000 → #0000ffff, the midpoint renders as (128, 128, 255) on white, while the inserted rgba(128, 0, 128, 0.5) renders as (191, 127, 191). The same happens with transparent → red.

    The alpha case already happens on main for rgba() and transparent stops. This PR extends it to 8-digit hex, and the modern-stop case is new now that those stops parse. One change closes both: build color-mix(in oklab, <left>, <right> <p>%), using in srgb when both stops are legacy, and run it through parseCssColor. I checked this in Chromium:

    • color-mix(in oklab, oklch(0.7 0.15 200), red) gives (178, 137, 121), which matches the painted pixel.
    • color-mix(in srgb, #ff000000, #0000ffff) gives (0, 0, 255, 0.5).

    Keep the current blend as the fallback when the browser can't resolve the mix. Codex raised both cases as blocking. I rate them should-fix because main copies the neighbouring stop, which is also wrong. Either way, the "blending it" claim in the description only holds for legacy colours.

Lower priority

  1. Trying the legacy serialisation first loses alpha. hsl(0 100% 50% / 0.001) parses with alpha 0, because the canvas returns an 8-bit rgba() and the precise path is never tried. Trying color(from …) first, with legacy as the fallback, keeps full precision in current browsers and behaves the same in older ones. This was Codex's suggestion. The impact is small, because formatAlpha rounds to hundredths anyway (mergeColorWithExistingAlpha("#123456", "rgb(0 0 0 / 0.001)") already gives alpha 0).

  2. Context-dependent colours resolve against a detached canvas (colorValue.ts:78-97).

    • light-dark(red, blue) parses as red whatever the composition's colour scheme. The canvas rejects it as a direct value, and it gets through via the color(from …) wrapper.
    • AccentColor resolves the same way.
    • \63 urrentcolor, an escaped currentcolor, gets past the regex and parses as black.

    The toolbar reads authored inline-span styles, not computed ones, so an authored light-dark() does reach this code. These could be refused like currentcolor, or resolved on the real element.

  3. CSS.supports is called outside the try (colorValue.ts:81). An environment with a CSS global but no supports throws instead of returning null. That isn't realistic in a browser, but moving the check inside the try is free. No test notices if the gate is removed (table above).

Nits

  • propertyPanelColor.tsx:196: the comment says the 3/6-digit gate covers "both lengths parseCssColor accepts", which is no longer true. A pasted #0f172acc in the hex field still neither previews nor commits. Either accept 4/8 digits, carrying their alpha, or update the comment.
  • InlineTextToolbar.tsx:263 has no test with a modern colour. The browser script calls toColorPickerValue directly instead of mounting the toolbar.
  • Without a DOM, signs, exponents and none (rgb(+1e2 0 0)) still return null. That matches the description's "retains the hex/RGB path", so this is just for the record.

— Somu

…paints there

The stop colour is color-mix() in oklab, or srgb when both stops are legacy,
resolved by the browser, so it matches the painted midpoint including
premultiplied alpha; the channel blend stays as the fallback. The parser tries
color(from ...) before the legacy read, keeping alpha precise, and calls
CSS.supports inside its try. A Chrome-147-shaped fake canvas covers the modern
path in unit tests.

@somanshreddy somanshreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of the 37f7b4a2 → 1dc7875f delta. I re-checked my earlier findings by number but didn't re-review the full PR. This is a comment, not an approval, and it has no blockers. The delta is five commits on top of my last head (no rebase): colorBrowserCanvas.test.ts is new, and colorValue.ts, gradientValue.ts, colorValue.test.ts and a comment in propertyPanelColor.tsx changed. My own pass plus a static Codex pass on the delta went into this, and I checked each point at this head. The colour suites pass: colorValue, colorBrowserCanvas, gradientValue, propertyPanelColor, InlineTextToolbar and hexRatchet give 84 of 84. CI is green (94 passed).

Earlier findings, by number

  1. The modern-colour branch now has unit coverage: fixed, with a different fix. The browser script still isn't in any workflow. Instead, the fake canvas in colorBrowserCanvas.test.ts now returns recorded Chrome 147 color(srgb …) serialisations, so the color(from …) branch runs under Vitest. I re-ran my mutation table against the five colour suites:

    Mutation Round 1 Now
    Drop the color(from …) branch passed every unit test caught (9 fail)
    Ignore alpha in parseSrgbSerialization passed caught (3)
    Drop the currentcolor/var( exclusion passed caught (1)
    Drop the CSS.supports gate passed still passes
    Double-read compare, white reset, number pattern, 8-digit hex alpha, % channels caught caught

    The new gradient logic is pinned too. Seven mutations all fail a test:

    • dropping the color-mix() path;
    • always mixing in sRGB, or always in Oklab;
    • deciding the space from the two neighbours only;
    • treating a bare var() as modern;
    • treating hwb() as non-legacy;
    • zeroing the fallback alpha.

    One caveat: the fake canvas is a lookup table, and it stubs CSS.supports as always true. It proves the branches run, but it can't catch Chrome changing a serialisation. The browser script remains the only end-to-end check, and CI still doesn't run it.

  2. Inserted gradient stop colour: fixed. I checked it in Chromium 154 by rendering each gradient 1001 px wide on white and comparing the painted pixel with the inserted stop painted on its own:

    Gradient, insert position Painted Inserted stop
    oklch(0.7 0.15 200) → red, 50% (178, 137, 121) (178, 137, 121)
    #ff000000 → #0000ffff, 50% (128, 128, 255) (127, 127, 255)
    red, blue 50%, oklch(…), 25% (140, 83, 163) (140, 83, 162)
    transparent → red, 50% (255, 128, 128) (255, 127, 127)
    color(display-p3 1 0 0) → blue, 50% (153, 81, 160) (153, 81, 160)
    hwb red → hwb blue, red → blue (50% and 10%) match within 1 match within 1

    Every case matches within 1/255. That covers Oklab for modern stops and premultiplied alpha for translucent ones.

  3. Alpha precision: fixed. color(from …) is tried first now, so hsl(0 100% 50% / 0.001) parses with alpha 0.001 in Chromium (it was 0), and the fake canvas has a test row for it.

  4. Context-dependent colours: unchanged. In Chromium, light-dark(red, blue) still parses as red, AccentColor as (0, 117, 255), and an escaped \63 urrentcolor as black. It's still low priority.

  5. CSS.supports outside the try: fixed. canResolveInBrowser now runs inside it. Removing the gate still passes every test, as in round 1; that's fine to leave.

Nits:

  • Hex-field comment: fixed. The comment at propertyPanelColor.tsx:196 now says the field edits RGB only.
  • No modern-colour toolbar test: unchanged.
  • No-DOM signs, exponents and none: unchanged. That's by design.

The hex ratchet holds. hexRatchet.test.ts keeps a per-file baseline of colour literals in non-test files.

  • The red came from the intermediate colorCanvasTestUtils.ts in 3a6d0e47. It wasn't a .test.ts file, so its recorded colours counted against the baseline.
  • 17315476 moved them into colorBrowserCanvas.test.ts, which the ratchet skips.
  • hex-baseline.json is unchanged in the delta, so the gate wasn't loosened.

New, low priority

  • An explicit interpolation method now mixes in the wrong space (gradientValue.ts:403-414, found by Codex, confirmed). The cause is that parseGradient doesn't parse a leading in <space>. It turns in srgb (or 90deg in oklch) into a pseudo-stop at 0% and drops the angle: 90deg reads back as 180. That part predates this PR. What's new is that the pseudo-stop now counts as a modern colour, which forces Oklab.
    • For linear-gradient(in srgb, red 0%, blue 100%) at 50%, the inserted stop was #800080 at 37f7b4a2, which matched the painted colour. It is now #8C53A2.
    • For in hsl or in oklch gradients, neither version matches what Chrome paints. I measured (255, 0, 255) painted against (140, 83, 162) inserted for in hsl.
    • Parsing the method onto the model and mixing in that space (including hue direction) would fix all of it. Because the model already rewrites these gradients, I'd treat this as part of that parser gap rather than a blocker here.
  • A bare var() stop's space is a guess. If --third resolves to an oklch() value, Chrome interpolates the whole gradient in Oklab, but the insert uses sRGB. If the var() stop is one of the two neighbours, the mix can't resolve and the insert copies the left stop. That can't be known statically, and the tests document the choice, so it's fine as is. (Codex found this.)
  • Intervals under 1% get the wrong ratio (gradientValue.ts:388). Math.max(1, …) turns red 0%, blue .5% at .25% into a 25% mix (#BF0040) instead of the midpoint. It behaves the same at 37f7b4a2, so it predates the delta; the line only moved. (Codex found this.)

— Somu

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the whole PR at 1dc7875f. No blockers. I'd approve this, but this account opened the PR, so GitHub only allows a comment here. Another reviewer's approval is needed for the merge queue.

What I checked

  • Parser. parseSerializedColor runs first, then the canvas. 3, 4, 6 and 8-digit hex and space-separated rgb() with percents now parse in pure JS. Values with var() or currentcolor never reach the canvas. Reading fillStyle twice, after resetting it to transparent and then white, correctly catches values the canvas rejects (inherit returns null).
  • Callers. toColorPickerValue, colorFromCss, mergeColorWithExistingAlpha, ColorField and the inline toolbar all just get more inputs resolved. None of them relied on null meaning "not a colour". Typed hex in ColorField is still limited to 3 or 6 digits, and the comment there is accurate. Dropping the toolbar's own canvas fallback loses nothing, since parseCssColor now covers it.
  • Real Chrome (headless shell 152). test:colors:browser passes. My own extra cases resolve as expected:
    • RED → (255, 0, 0)
    • rgba(255,0,0,50%) → (255, 0, 0, 0.5)
    • #f008, lab(50 40 30), light-dark(red, blue) and color-mix(...) resolve
    • color(display-p3 1 0 0) is gamut-clipped to (255, 0, 0)
    • rgb(1. 2 3), currentColor, var(--x) and inherit return null
  • Inserted stops against painted pixels. I inserted a stop at 30% into six gradients and read the pixel Chrome paints there. Every one matched within 2/255 per channel and 0.01 alpha. The cases mixed legacy and modern colours, used alpha on both sides (#ff000033 → oklch(...), rgb(255 0 0 / 0.2) → color(display-p3 0 1 0 / 0.9)) and included a three-stop gradient.
  • Tests. 606/606 in the 36 editor colour and property-panel test files. CI is 94/94.
  • Mutations. Four of five turned the tests red: always using sRGB (3 failures), dropping the currentcolor/var() guard (1), reading fillStyle only once (1), and ignoring alpha in 8-digit hex (2). The fifth is the nit below.

Nit

  • gradientValue.ts:414: the || !isLegacy(left) || !isLegacy(right) clause does nothing. A stop that is neither legacy nor var() already passes stops.some(isModern), so the clause only fires when a neighbour is a bare var(). That mix is then refused by canResolveInBrowser and falls through to the fallback, which returns left.color either way. Removing the clause keeps all 42 colour tests green. It also slightly contradicts the description ("neither forces Oklab nor rules it out"), since a var() neighbour does force in oklab into the string. Fine to drop it or leave it.

I agree with Somu's low finding that in <space> is ignored. That gap is older than this PR.

— Rames

right: string,
ratio: number,
): string {
const oklab = stops.some((stop) => isModern(stop.color)) || !isLegacy(left) || !isLegacy(right);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this clause never changes the result. A non-legacy, non-var() neighbour already makes stops.some(isModern) true, so it only fires for a bare var() neighbour, and canResolveInBrowser refuses any mix containing var(). Deleting it keeps all 42 colour tests green.

— Rames

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested at 1dc7875f007faf2310e0c85209ab52a8b7804fde for one browser-reproduced, PR-introduced picker regression.

Resolve contextual colors in the selected element's color scheme (packages/studio/src/components/editor/colorValue.ts:94). In a Chromium composition with html { color-scheme: dark } and selected inline text <span style="color: light-dark(red, white)">, the text paints white and its computed color is white. The toolbar receives the authored light-dark(...) value, but the new conversion uses a detached Studio-side canvas with the light scheme, serializes it as red, and shows #ff0000 in the picker. The merge-base picker showed #ffffff for the same selection. An edit through that picker can therefore replace correctly rendered white text with red. Please resolve contextual values against the selected element/iframe's computed style or equivalent context, and cover a dark-scheme inline selection in the browser test. This is not a generic objection to modern color support; light-dark() is a concrete newly wrong selection path.

I verified this with a real Chromium iframe selection fixture and traced the selection-to-picker call path; I did not run the entire Studio browser suite. The low-alpha stop-rounding edge is a lower-priority precision follow-up, not a second hold.

— Review by tai (pr-review)

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A correction to my review above: this should be fixed before merge. I listed light-dark() as resolving correctly, but I only tested it outside any colour-scheme context. I've reproduced the dark-scheme regression another reviewer reported.

Repro in real Chrome (headless shell 152): an iframe with color-scheme: dark and text styled color: light-dark(red, white).

  • The text's computed colour is rgb(255, 255, 255).
  • color(from light-dark(red, white) srgb r g b / alpha) on a canvas serialises as color(srgb 1 0 0) (red). That happens whether the canvas belongs to the parent document or the iframe's.
  • A plain light-dark(red, white) is rejected by canvas fillStyle. So before this PR it never resolved to red.

So parseBrowserColor turns any light-dark() value into its light colour, whatever scheme the element actually renders in. The picker then shows red for white text, and committing from it can write red. The fix is probably to treat light-dark() like currentcolor and var(): either resolve it from the element's computed colour, or keep it out of the canvas path.

— Rames

@miguel-heygen

Copy link
Copy Markdown
Collaborator

Thanks, good catch. Fixed at 27ccd13.

  • Root cause: color(from light-dark(…) srgb r g b / alpha) resolves to the light colour on any detached canvas, even one created by a dark-scheme document, and a plain fillStyle rejects light-dark(). So the shared parser now refuses light-dark() the same way it refuses currentcolor and var().
  • Picker: when the authored value needs the element's context, the inline toolbar's picker reads the computed colour of the element that paints the selected text. That is the same character the swatch's colour comes from, so whole-element selections, leading spaces and uncoloured text in between all pick the right element.
  • Browser test: tests/e2e/native-css-colors.mjs now selects inside a dark-scheme iframe in real Chromium: a whole-element selection, and a mixed span / text / span selection. Both fail on the previous code (the parse gives white, the picker gives the parent's #008000) and pass now, with the picker at #ff0000.
  • Gradient nit: the neighbour clause is gone too; the whole-gradient check already covers it.

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at 27ccd1327d52968f71d1515e37097aed0ba02e94; this lifts my changes request at 1dc7875f007faf2310e0c85209ab52a8b7804fde. I rechecked the original dark-scheme selection regression in Chromium: selected <span style="color: light-dark(red, white)"> paints white inside the dark iframe, the selected span is identified as the painted element, and the native picker now resolves to #ffffff rather than the prior head's #ff0000. The four-commit fix delta and selection-to-picker path show no new blocker. The targeted color/editor suites passed 125/125 and the Chromium color exercise passed its 18 cases plus the dark-iframe case.

A distinct Studio toolbar swatch still paints the authored light-dark(...) in its light parent document, so that swatch can appear red while the dark-iframe text is white. This was already true at the merge base and is not the picker regression I blocked; it is a separate nonblocking follow-up for contextual swatch rendering.

— Review by tai (pr-review)

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 2, 2026

@somanshreddy somanshreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review of the 1dc7875→27ccd132 delta; full PR not re-reviewed. This is a comment, not an approval. I did my own pass and a static Codex pass on the delta, then checked each point at this head. tai's approval (review 5397382605) already covers the native-picker result and the targeted suites, so I don't repeat those here.

The light-dark() fix works in real Chromium. I probed resolvePickerColor(…, readFirstPaintedElement(range, "color"), …) with puppeteer on Chrome:

Case Picker Painted
light-dark(red, white) on a span, dark scheme #ffffff white
the same, light scheme #ff0000 red
light-dark() inherited from the host, no span colour #ffffff white
color-mix(in srgb, light-dark(red, white), blue), dark #8080ff color(srgb 0.5 0.5 1)
currentcolor from a blue parent #0000ff blue
escaped \63 urrentcolor from a green parent #008000 green
var(--c) #0a141e rgb(10, 20, 30)
AccentColor #0075ff rgb(0, 117, 255)
authored oklch(0.7 0.15 200) (round-trips, no computed read) #00b9c3 —
mixed red then lime #ff0000 (first coloured character) —
empty selection default, no throw —

The escaped currentcolor I flagged last round now resolves correctly on this path. element.style normalises it, so the exclusion catches it. The PR's browser script passes, and the colour and inline-text suites pass 144 of 144. CI is green (94 passed).

Worth fixing: the fix's runtime path isn't pinned in CI. I mutation-tested it against src/components/editor and hexRatchet (1,794 tests), plus the browser script:

  • Caught: dropping the light-dark( exclusion, picking the first painted character instead of the first coloured one, and dropping painter from the runs.
  • Caught only by the browser script, which CI still doesn't run: dropping the getComputedStyle fallback in resolvePickerColor. All unit tests stay green.
  • Caught by nothing: passing null instead of readFirstPaintedElement(range, "color") at InlineTextToolbar.tsx:255. The unit tests and the browser script both pass, because the script calls the helpers directly rather than going through the toolbar. That one-line revert would bring back the reported regression with CI green.

A toolbar test that mounts a dark-scheme light-dark() selection and asserts pickerColour, or adding the browser script to ci.yml, would close it.

Low priority:

  • The visible swatch can still show the light colour. swatchBackground(placement.colours, …) paints the raw authored value in Studio's document. I checked main, and it does the same, so this predates the PR, as tai said.
  • An authored value that parses wins over what's painted. For example, inline color: red under a .forced { color: blue !important } rule shows red. Main behaves the same way. (Codex)
  • ::first-letter colours aren't seen, because the painter is an element, not a pseudo-element. (Codex)
  • Other parseCssColor callers: Codex flagged them, since light-dark() now parses to null everywhere. The property panel reads computedStyles (PropertyPanel.tsx:114), so I don't think authored light-dark() reaches it on the usual path. Gradient-stop insertion between two light-dark() stops falls back to copying the left stop. That's rare and no worse than main.
  • The isLegacy clause removed from mixLikeTheGradient: I agree with Rames that it was dead. The gradient cases in the browser script still pass.

— Somu

Merged via the queue into main with commit 5c52f72 Oct 2, 2026
94 checks passed
@miguel-heygen
miguel-heygen deleted the fix/studio-native-css-colors branch October 2, 2026 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants