Theme: Update color space registration to avoid side effects#77653
Open
Theme: Update color space registration to avoid side effects#77653
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: -241 B (0%) Total Size: 7.78 MB 📦 View Changed
ℹ️ View Unchanged
|
|
Flaky tests detected in 1e0aab3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/24895178397
|
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.
| ): { 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 );
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)?
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).