Theme Provider: Unify color schemes using CSS custom properties

[SirLouen]

In the series #75053

Why?

Instead of building from the same color scheme repeated over and over again through different parts of the code, why not unifying them?

How?

This is not final, I'm playing around with the idea of exposing certain parameters from Core:
See this Core PR: WordPress/wordpress-develop#10825

And fetching them unified from there.

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: SirLouen <sirlouen@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[aduth]

I feel like this background handling should probably happen in Root and not here. It makes sense to me that the primary color should always correspond to the user's color preference, but there's a lot more variability in background colors (white, accent, etc.) that we wouldn't want to link the default background to the menu color.

[SirLouen]

Yes, could be an option.

[aduth]

Separately, I think ThemeProvider should have built-in support for passing var(--foo) as color values, with this resolution happening inside the component.

[SirLouen]

Interesting, I was also struggling in my mind, with this idea because of the sass limitations, you mean something like

resolveCSSVariable( 'var(--primary)' )

To return #007cba

Something like:

const match = value.match( /^var\(\s*(--[\w-]+)(?:\s*,\s*(.+))?\s*\)$/ );
const propertyName = match[ 1 ];
const fallback = match[ 2 ];

[aduth]

I mean that we could do something like:

<ThemeProvider color={ { bg: 'var(--wp-admin-color-menu-background)' } } />

...and it would "just work".

But I tinkered with this idea a bit locally and it's kinda tricky, because ThemeProvider has to be adaptive to its rendered context (i.e. where it is in the DOM hierarchy since CSS properties cascade and be overridden by DOM ancestors), whereas here you can be sure that the property is defined on document.body.

[SirLouen]

Yep, it seems that it will require at least an extra render before getting the property we need to define the right variable to display. I'm still trying to figure out why the colors were included manually. I think that @youknowriad holds the key to this knowledge in #23048 (prob forgot, a little old) and you added this

const THEME_PRIMARY_COLORS = new Map< string, string >( [
	[ 'light', '#0085ba' ],
	[ 'modern', '#3858e9' ],
	[ 'blue', '#096484' ],
	[ 'coffee', '#46403c' ],
	[ 'ectoplasm', '#523f6d' ],
	[ 'midnight', '#e14d43' ],
	[ 'ocean', '#627c83' ],
	[ 'sunrise', '#dd823b' ],
] );

Quite recently in #74011

[SirLouen]

Judging by the conversation context in #23048 it feels that back in the day GB was significantly disconnected from Core, prob back then Gutenberg Core Committers did not even had Core Committing access, so lending the whole color palette was the fastest way to proceed.

Is interesting to see this file history now:

packages/base-styles/index.js

Many colors in the scheme were being exported.

[aduth]

Yep, it seems that it will require at least an extra render before getting the property we need to define the right variable to display.

Yeah, I had run into this as well when testing. We might be able to avoid a second render if we do something with useEffect instead. That would require some manual DOM manipulation, though depending on the performance cost of ThemeProvider rendering, it could well be worth it.

prob back then Gutenberg Core Committers did not even had Core Committing access, so lending the whole color palette was the fastest way to proceed.

Yeah, it was probably the easiest option at the time. But we could always "do it right" and update accordingly once those core changes become accessible within Gutenberg.

[ciampo]

I mean that we could do something like:

<ThemeProvider color={ { bg: 'var(--wp-admin-color-menu-background)' } } />
...and it would "just work".

But I tinkered with this idea a bit locally and it's kinda tricky, because ThemeProvider has to be adaptive to its rendered context (i.e. where it is in the DOM hierarchy since CSS properties cascade and be overridden by DOM ancestors), whereas here you can be sure that the property is defined on document.body.

Adding support to ThemeProvider for accepting CSS custom properties would definitely be non-trivial, and would have potential drawbacks.

• Having to parse the CSS custom property first would cause a FUOC / layout shift
• the element that is used to resolve the value in the CSS cascade matters and can lead to unexpected results (we can't assume it's always document.body)
• running getPropertyValue or getComputedStyle can have runtime performance consequences

Having said that, we can work on a PoC and take it from there.

[aduth]

I'm curious why it wasn't implemented this way in the first place 🤔 Maybe we're missing some historical context. I'll add some reviewers who may be able to shed some more light, or maybe we can review the original discussions behind this implementation. My first thought was maybe to avoid directly tying to WordPress internals, but we're already doing that with relying on the body class names.