Theme: Update color space registration to avoid side effects#77653
Merged
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
aduth
commented
Apr 24, 2026
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import './register-color-spaces'; |
Member
Author
There was a problem hiding this comment.
This is another good example of the code quality benefit. Nothing in this file actually depends directly on registered color spaces, so we were dragging in this side-effect without any purpose for doing so.
|
Size Change: -270 B (0%) Total Size: 7.98 MB 📦 View Changed
ℹ️ View Unchanged
|
|
Flaky tests detected in bdc6b24. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/26227641989
|
22 tasks
ciampo
approved these changes
Apr 27, 2026
| // See: https://colorjs.io/docs/procedural | ||
| ColorSpace.register( sRGB ); | ||
| ColorSpace.register( OKLCH ); | ||
| ColorSpace.register( P3 ); |
Contributor
There was a problem hiding this comment.
Noting that, with this PR, we're not registering the P3 space anymore — it looks like it should be fine / correct, but probably better to have confirmation from @jsnajdr too
Member
There was a problem hiding this comment.
We used to use P3 in the taperChroma code, but we no longer do that.
In our code, when we are referencing color spaces, we always import the space object (import { P3 }) and don't reference spaces by string ids ("p3"). That means we shouldn't need to register any color space at all.
The only exception is color string parsing. If we want to pass #fff or hsl(h s l) or oklch( l c h) or color(display-p3 x y z) values to our color APIs, the respective spaces need to be registered first. When our functions like customRgbFormat register the sRGB space, they do it only because their argument can be an sRGB string, not because they use the space internally.
In this sense, all our calls to ensureColorSpacesRegistered should register the same spaces. If they register different spaces, it's random and accidental: the inputs we happen to use in our code and tests happen to be in certain spaces.
There are other exceptions, but they are bugs:
- in
findColorMeetingRequirementswe usespaceId: 'oklch'instead ofspace: OKLCH, forcing registration. That can be fixed. - the
toGamutCSSfunction usesColorSpaces.get('oklch')(we even mention this in a code comment) but that's also a bug, in the colorjs.io library itself. It should import and use theOKLCHobject instead. We should report and fix it upstream. Other parts of the same library do it right. ThecontractWCAG21function uses thexyz_d65space internally, but doesn't force registration.
Member
Author
There was a problem hiding this comment.
If we want to pass
#ffforhsl(h s l)oroklch( l c h)orcolor(display-p3 x y z)values to our color APIs, the respective spaces need to be registered first.
I guess there's an open question here: Do we actually want to support all of these? I feel like we've been largely assuming that we'd support hex values for things like ThemeProvider seed values. If we only support that, we only need sRGB. We also need OKLCH until the aforementioned fix to toGamutCSS. Is that acceptable?
Member
Author
There was a problem hiding this comment.
Thanks for the deep analysis here @jsnajdr . After some more iterations, I think I got us into a better state here in terms of what we're expecting:
- Register
sRGBanywhere we're parsing some string value. - Register
OKLCHto handle the upstream bug.
Registering only sRGB means we don't accept HSL and OKLCH inputs. I think that's acceptable? We could always revisit later. It makes things a lot simpler to just register the one color space any time we're parsing inputs.
For the upstream issue I created a pull request at color-js/color.js#734 and referenced it in inline comments explaining why we have to to do the OKLCH registration.
With this, we don't need a ensureColorSpacesRegistered at all. We just inline the color registration where it's needed, as it's needed.
Contributor
There was a problem hiding this comment.
Registering only sRGB means we don't accept HSL and OKLCH inputs
If this affects the public API of ThemeProvider, we should document it (ie. the color needs to be a valid hex color string, or something like that?)
Member
Author
There was a problem hiding this comment.
If this affects the public API of
ThemeProvider, we should document it (ie. the color needs to be a valid hex color string, or something like that?)
Probably, yes. Right now we use pretty generic "seed color" phrasing that doesn't really specify any relevant limitations. And technically the changes here would lose support for some values that would have previously worked, though I don't know how much we'd consider that breaking since, to your point, we never really documented the extent of support of the public API.
Practically, once we choose what to support:
- Document it in the public API
- Probably just mark it as a breaking change for exhaustiveness sake (if we drop any current spaces, that is)
| ): { l: number; c: number } | PlainColorObject { | ||
| // `toGamut` with `method: 'css'` internally resolves OKLCH via the | ||
| // registry, regardless of the target gamut. | ||
| ensureColorSpacesRegistered( OKLCH ); |
Contributor
There was a problem hiding this comment.
We should probably also register whatever gamut is passed as a function argument? eg
const gamut = options.gamut ?? sRGB;
ensureColorSpacesRegistered( OKLCH, gamut );
Member
Author
There was a problem hiding this comment.
As long as we can expect options.gamut to be a colorspace object (which it is in how we use it) then we don't need to have it pre-registered. The internal logic for toGamut will use it directly.
Comment on lines
3
to
14
| const OKLCH = { id: 'oklch' }; | ||
| const sRGB = { id: 'srgb' }; | ||
| return { | ||
| __esModule: true, | ||
| OKLCH, | ||
| sRGB: {}, | ||
| P3: {}, | ||
| HSL: {}, | ||
| ColorSpace: { register: jest.fn() }, | ||
| sRGB, | ||
| P3: { id: 'p3' }, | ||
| HSL: { id: 'hsl' }, | ||
| ColorSpace: { register: jest.fn(), registry: {} }, | ||
| to: jest.fn( () => [ 0, 0, 0 ] ), | ||
| get: jest.fn( () => 0 ), | ||
| }; |
Contributor
There was a problem hiding this comment.
Nit: ColorSpace.registry stays {} for the lifetime of the test, which means ensureColorSpacesRegistered always invokes register on every call, not testing the idempotency of the code.
Maybe we could write a more sophisticated mock registry (ie register: jest.fn( ( s ) => { registry[ s.id ] = s; } ) with a shared local registry object)?
Member
Author
1e0aab3 to
bec4035
Compare
aduth
commented
May 14, 2026
Comment on lines
-35
to
+33
| export function getContrast( colorA: ColorTypes, colorB: ColorTypes ): number { | ||
| export function getContrast( | ||
| colorA: string | PlainColorObject, | ||
| colorB: string | PlainColorObject | ||
| ): number { |
Member
Author
There was a problem hiding this comment.
The change to the types here is a subset of what ColorTypes supports, and more accurately reflects the types of values we would realistically expect to be passed. Other types (ColorObject, ColorConstructor) could include options that we're not necessarily supporting (e.g. { spaceId: 'hsl', ... }).
aduth
commented
May 14, 2026
Comment on lines
-16
to
+25
| const allBgColors = lStops.flatMap( ( l ) => | ||
| // Generate a set of HSL colors across a broad perceivable range to test | ||
| // support for building ramps with various combinations of lightness, | ||
| // saturation, and hue. Convert to hex strings to mirror real-world | ||
| // consumer usage (`ThemeProvider` is initialized with a hex value). | ||
| const allBgColors: string[] = lStops.flatMap( ( l ) => | ||
| sStops.flatMap( ( s ) => | ||
| hStops.map( ( h ) => `hsl(${ h }deg ${ s }% ${ l }%)` ) | ||
| hStops.map( ( h ) => | ||
| getColorString( { | ||
| space: HSL, | ||
| coords: [ h, s, l ] as [ number, number, number ], | ||
| alpha: 1, | ||
| } ) | ||
| ) |
Member
Author
There was a problem hiding this comment.
I had to change the logic here in 4faed73 to convert the HSL values to hex. HSL is convenient for the type of combination testing we're exercising here, but as mentioned in previous comments I'm proposing to remove HSL string support from the library itself. We could pass a PlainColorObject and allow buildRamp to accept it, but I think this more accurately reflects real-world usage we're trying to support. The snapshot has to be updated because the conversion from HSL to hex loses some precision.
@jsnajdr I seem to recall you mentioning several months back that this was something you were expecting to want to do anyways, or maybe I'm misremembering.
Member
There was a problem hiding this comment.
The snapshot has to be updated because the conversion from HSL to hex loses some precision.
getColorString does two things: convert the color to sRGB, and serialize it as hex. Only the second part loses precision, because it rounds the color coords to a closest 1/256 value.
If you do just the HSL to sRGB conversion with serialize( to( color, sRGB ) ), you'll be getting rgb(r g b) strings with (almost) precise coords. If you try that, maybe the snapshot won't need any update.
I seem to recall you mentioning several months back that this was something you were expecting to want to do anyways, or maybe I'm misremembering.
I remember that I didn't like using the hex format, because it does the coarse rounding. Which is mostly invisible to human eye, but introduces a lot of quirky results into unit tests etc. A value changes from 0.499999 to 0.500001, a delta very safely under any "epsilon" constant used, but then a result changes substantially, by 0.4%.
Member
Author
There was a problem hiding this comment.
If you do just the HSL to sRGB conversion with
serialize( to( color, sRGB ) ), you'll be gettingrgb(r g b)strings with (almost) precise coords. If you try that, maybe the snapshot won't need any update.
Yeah, that's a good call 👍 Updating in d34fdba restores the original snapshot.
But this does imply that we support more than just hex values for seed colors, as this is now generating rgb(...) strings. Both are supported through sRGB, so it seems fine, but just want to flag for exactness about what types of "seed colors" we're expecting to support.
Member
There was a problem hiding this comment.
But this does imply that we support more than just hex values for seed colors
I thinks this is not an issue at all. We support any seed color without limits. The colors don't need to be rounded to particular "hex grid", and they can even be out of gamut (in sRGB, coordinates <0 or >1 are also often valid colors), the library will do all the mapping and rounding internally.
Member
Author
There was a problem hiding this comment.
We support any seed color without limits.
But we've never supported "any" color? At least talking about ThemeProvider color prop as the public API. Someone could pass a lab color string value as a seed and it wouldn't have worked previously, because we didn't register the Oklab color space.
So a specific question I'm trying to address in this pull request is which color spaces we claim to support and registering/documenting accordingly. As mentioned in #77653 (comment) , I limited this down to sRGB which includes hex and rgb strings (and technically OkLCh until the upstream bug is fixed). If we want to support others, we'll need to register those, and document accordingly.
Member
There was a problem hiding this comment.
But we've never supported "any" color?
We never supported "any" color space, but within a given space, we support any color. The hex format supports colors only from a finite 256x256x256 cube, but the rgb space in general supports the entire cube, even including values outside the cube. Whatever rgb(r g b) value you pass, you can be sure that it's a good color or can be mapped to one.
Member
Author
There was a problem hiding this comment.
Right, I think I follow. So with these changes, we support hex values and rgb(r g b) strings, which can support any color. We no longer support HSL-formatted strings. Given the former (that any color is supported) the latter seems reasonable?
Member
There was a problem hiding this comment.
Yes, there are no limits on the actual color you want to pass. We only limit the input spaces to one. Even wide-gamut colors can be expressed in sRGB. The coordinates are weird, like -0.1 or 1.05, but possible.
d34fdba to
170e45f
Compare
Member
Author
ciampo
approved these changes
May 20, 2026
| } | ||
|
|
||
| function getOKLCHValues( hex: string ) { | ||
| ColorSpace.register( sRGB ); |
Contributor
There was a problem hiding this comment.
Probably fine, but wanted to mention that getOKLCHValues gets called a few lines above to compute PRIMARY_SEED_OKLCH, meaning that the newly added color space registration will be invoked early (at parsing time).
Should be fine, given that this script runs at build time.
Member
Author
There was a problem hiding this comment.
That's a good call-out, and I agree it's not a problem for the moment with how we use the script, but it is a side-effect that could create issues down the line.
Separately, a couple things we could do to improve this:
- We don't need to register colorspaces if we give
colorjs.ioplain color objects with thespacedefined to a specific colorspace object, so we could avoid the global colorspace registration if we instead construct it this way. The problem here is that we'd have to deconstruct the hex string into its rgb coordinates, which is doable but a bit messy (AI-generated code at the bottom of the comment). - With how we're using the constant value, we could probably afford to not have those be constants and instead variables that are defined within
computeBrandFallback(the one function that uses the variables).
hexToSRGB
function hexToSRGB( hex: string ): PlainColorObject {
let digits = hex.replace( /^#/, '' );
if ( digits.length === 3 ) {
digits = digits
.split( '' )
.map( ( ch ) => ch + ch )
.join( '' );
}
const r = parseInt( digits.slice( 0, 2 ), 16 ) / 255;
const g = parseInt( digits.slice( 2, 4 ), 16 ) / 255;
const b = parseInt( digits.slice( 4, 6 ), 16 ) / 255;
return { space: sRGB, coords: [ r, g, b ], alpha: 1 };
}| colorA: string | PlainColorObject, | ||
| colorB: string | PlainColorObject | ||
| ): number { | ||
| ColorSpace.register( sRGB ); |
Contributor
There was a problem hiding this comment.
Is it worth adding a comment re. what @jsnajdr flagged about the implicit xyz_d65 color space dependency?
Member
There was a problem hiding this comment.
The xyz_d65 dependency is an internal detail of contrastWCAG21, it's something that nobody really needs to know about 🙂 The sole reason for the sRGB registration here is that getContrast accepts RGB strings as input.
Few lines below we do have a comment about registration of OKLCH, which is a workaround for a toGamut bug. Only here the internal toGamutCSS behavior has consequences for the caller.
Co-authored-by: Sculptor <sculptor@imbue.com>
Co-authored-by: Sculptor <sculptor@imbue.com>
Co-authored-by: Sculptor <sculptor@imbue.com>
Co-authored-by: Sculptor <sculptor@imbue.com>
Co-authored-by: Sculptor <sculptor@imbue.com>
Co-authored-by: Sculptor <sculptor@imbue.com>
Co-authored-by: Sculptor <sculptor@imbue.com>
Co-authored-by: Sculptor <sculptor@imbue.com>
Co-authored-by: Sculptor <sculptor@imbue.com>
Co-authored-by: Sculptor <sculptor@imbue.com>
Terrazzo is ESM-only, which our current version of Jest doesn't support
Restore more of the original implementation. We don't need to expand the argument types for buildRamp.
Preserve alpha from seed value
Preserves color precision
170e45f to
bdc6b24
Compare
ciampo
added a commit
that referenced
this pull request
May 27, 2026
Addresses mirka's review on #78664: - Compare resolved seed colours against `DEFAULT_SEED_COLORS` with `colorjs.io/fn`'s `equals` so any parseable representation of the same colour (uppercase hex, `rgb(...)`, alternate spaces) counts as default and triggers the `<style>`-skip optimisation. `try/catch` guards keep unparseable seeds on the existing emission path so the clear error from `legacyWpAdminThemeOverridesCSS` (per #77653) still surfaces. Lift `ColorSpace.register(sRGB)` to module scope now that registration is shared between `equals` and `to(...HSL)`. - Add `packages/theme/src/design-tokens.scss` so the `wp-build` style pipeline emits `build/styles/theme/design-tokens.css` (plus rtl and minified variants) from the existing prebuilt CSS as the single source of truth. - Register a `wp-design-tokens` PHP handle in `lib/client-assets.php`, add it as a dependency of `wp-base-styles`, and prepend it to `$wp_edit_blocks_dependencies` so the editor iframe's `<head>` receives the same `:root` token block as the parent document. Closes the legacy `--wp-components-*` regression for the `<StyleProvider document={iframeDocument}> + <ThemeProvider>` pattern without inlining ~10 KB of CSS into the JS bundle.
ciampo
added a commit
that referenced
this pull request
May 27, 2026
- Move the `--wp-components-*` legacy alias declarations from `@wordpress/theme`'s prebuilt CSS into a `:root` block shipped by `@wordpress/components` (new `_legacy-component-css-vars.scss`), where they live as package-internal compat tokens. - Rename the PHP handle `wp-design-tokens` -> `wp-theme` to match the per-package convention (`wp-base-styles`, `wp-components`). - Restore per-call `ColorSpace.register(sRGB)` instead of the module-scope registration (preserves the #77653 side-effect cleanup); add a comment explaining what `equals` can throw on. - Add `use-theme-provider-styles` unit tests covering default exact/uppercase-hex/rgb equivalence, cursor-only overrides, custom primary, and unparseable seeds. - Update the `AcrossIframes` story so it no longer relies on a `<style data-wpds-theme-provider-id>` always being present in the default case; clone the prebuilt token stylesheet into the iframe. - Tighten the package CHANGELOGs (consumer-focused phrasing, American spelling).
ciampo
added a commit
that referenced
this pull request
May 27, 2026
Addresses mirka's review on #78664: - Compare resolved seed colours against `DEFAULT_SEED_COLORS` with `colorjs.io/fn`'s `equals` so any parseable representation of the same colour (uppercase hex, `rgb(...)`, alternate spaces) counts as default and triggers the `<style>`-skip optimisation. `try/catch` guards keep unparseable seeds on the existing emission path so the clear error from `legacyWpAdminThemeOverridesCSS` (per #77653) still surfaces. Lift `ColorSpace.register(sRGB)` to module scope now that registration is shared between `equals` and `to(...HSL)`. - Add `packages/theme/src/design-tokens.scss` so the `wp-build` style pipeline emits `build/styles/theme/design-tokens.css` (plus rtl and minified variants) from the existing prebuilt CSS as the single source of truth. - Register a `wp-design-tokens` PHP handle in `lib/client-assets.php`, add it as a dependency of `wp-base-styles`, and prepend it to `$wp_edit_blocks_dependencies` so the editor iframe's `<head>` receives the same `:root` token block as the parent document. Closes the legacy `--wp-components-*` regression for the `<StyleProvider document={iframeDocument}> + <ThemeProvider>` pattern without inlining ~10 KB of CSS into the JS bundle.
ciampo
added a commit
that referenced
this pull request
May 27, 2026
- Move the `--wp-components-*` legacy alias declarations from `@wordpress/theme`'s prebuilt CSS into a `:root` block shipped by `@wordpress/components` (new `_legacy-component-css-vars.scss`), where they live as package-internal compat tokens. - Rename the PHP handle `wp-design-tokens` -> `wp-theme` to match the per-package convention (`wp-base-styles`, `wp-components`). - Restore per-call `ColorSpace.register(sRGB)` instead of the module-scope registration (preserves the #77653 side-effect cleanup); add a comment explaining what `equals` can throw on. - Add `use-theme-provider-styles` unit tests covering default exact/uppercase-hex/rgb equivalence, cursor-only overrides, custom primary, and unparseable seeds. - Update the `AcrossIframes` story so it no longer relies on a `<style data-wpds-theme-provider-id>` always being present in the default case; clone the prebuilt token stylesheet into the iframe. - Tighten the package CHANGELOGs (consumer-focused phrasing, American spelling).
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
3 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.
What?
Updates
@wordpress/themecolor ramp generation logic to avoid relying on module-level side-effects registering color spaces with the colorjs.io dependency, instead registering them lazily as needed.Why?
The original motivation was noticing that it's not possible to use
@wordpress/uicomponents referencing published packages via https://esm.sh/ , due to an error with side-effect handling causing this line to throw an error due to color spaces not being registered. The root issue may lie within this service itself, but it prompted a rethink of whether we should have these side effects at all. Being able to support this service isn't a hard requirement, but it would enable quick prototyping without a complicated build setup.Separately, we should aim to isolate and eliminate side effects generally:
These changes are similar to what we've previously done in
@wordpress/datethrough #75831 and discussion in #76865, where date functions initialize setup requirements "just in time" rather than having a module-level setup.How?
Moves color space registration into a new idempotent function, and updates call-sites which depend on said color space registration to call this function with their intended color space requirements.
In practice, this ended up being a bit more complicated than expected, due to "implied" dependencies through functions like
toandparse(hex conversions implicitly requiringsRGB) and internal logic oftoGamut. These were discovered and documented through AI implementation. I've largely left the inline comments in-tact to clarify this. The registration function itself also documents these quirks for future reference.I'm open to simplifying this to restore the "global pool of color spaces" approach to avoid having to deal with these quirks, but that also lose the "code quality" benefit raised above in "Why?".
Testing Instructions
Verify theme token build produces no changes locally, as confirmation that the build-time color generation is unaffected by this refactor:
Verify tests pass:
Observe no visual differences in color theming behavior through Storybook examples, compared to live examples.
npm run storybook:devUse of AI Tools
Used Cursor with Claude Opus 4.7 to both investigate the behavior around side effects and requirements for registering color spaces, as well as implementation of the refactor. Manually-prompted iterations for idempotency behavior (relying on ColorSpace as the source of truth), individual color registration, and registering as close to color transformations as possible (vs. entrypoints).