CustomGradientPicker: Support CSS variables #63915
Conversation
mirka
added
[Type] Enhancement
mirka
changed the title
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @gwwar, @editandpost. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
| const { gradientAST, hasGradient } = getGradientAstWithDefault( value ); | ||
| const [ normalizedValue, setNormalizedValue ] = useState( | ||
| !! getCSSVariablesInString( value ?? '' ).length | ||
| ? 'linear-gradient( #fff, #fff )' // prevent FOUC when there are CSS variables |
There was a problem hiding this comment.
If I understand correctly, this fallback gradient will only be shown for a very small time, until the onChange callback of the CSSVariableReplacer component fires with the parsed gradient.
Therefore, there shouldn't be any changes to how the gradient falls back to the DEFAULT_GRADIENT in the getGradientAstWithDefault function.
| onChange: ( args: { | ||
| replacedCssString: string; | ||
| computedVariables: ComputedCSSVariables; | ||
| } ) => void; |
There was a problem hiding this comment.
For completeness, it would probably be a good idea to also pass back the original string
| <VisuallyHidden> | ||
| <div ref={ ref } /> | ||
| </VisuallyHidden> |
There was a problem hiding this comment.
Nitty, but do we need the extra div ?
| } catch ( error ) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( | ||
| 'wp.components.CSSVariableReplacer failed to parse the CSS string with error', |
There was a problem hiding this comment.
Should we add the CSS string in the error message?
| ); | ||
| } | ||
|
|
||
| onChange( { replacedCssString, computedVariables } ); |
There was a problem hiding this comment.
In case of an error, should we still fire onChange? If we do, should we have a flag to signal that parsing failed?
| closingParen + 1, | ||
| ]; | ||
| const raw = str.slice( start, end ); | ||
| const value = raw.match( /--[\w-]+/ )?.[ 0 ]; |
There was a problem hiding this comment.
Should this regex also be declared above so we don't redeclare it every time?
| end, | ||
| raw, | ||
| value, | ||
| fallback: raw.match( /,(.+)\)/ )?.[ 1 ].trim(), |
There was a problem hiding this comment.
Same about this regex and the rest of the regexes in here. In addition to the small performance benefit, it can actually make the code more readable if regexes are declared as constants and referenced by name.
| /** | ||
| * Replace all `var()` functions in a CSS string with their computed values. | ||
| */ | ||
| export function replaceCSSVariablesInString( |
There was a problem hiding this comment.
Have you considered introducing a caching layer here? There seem to be a bunch of regex operations here, and with large CSS strings, caching could potentially have a real, measurable impact. Or is the effect dependency management of CSSVariableReplacer enough? Again, I'm concerned that this one could be used directly and then caching won't be leveraged.
| varFunctions.forEach( ( { start, end, value, fallback } ) => { | ||
| const replacement = | ||
| computedVariables[ value ] || | ||
| replaceCSSVariablesInString( fallback ?? '', computedVariables ); |
There was a problem hiding this comment.
If fallback is empty, do we even need to call replaceCSSVariablesInString? Can't we just use an empty string without calling replaceCSSVariablesInString?
| ); | ||
| } catch ( error ) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn( |
There was a problem hiding this comment.
Can we use the warning package instead? Seems like it's the recommended way to trigger developer errors and warnings now; see #63610
| replaceCSSVariablesInString( fallback ?? '', computedVariables ); | ||
|
|
||
| if ( ! replacement ) { | ||
| throw new Error( `No value found for CSS variable ${ value }.` ); |
There was a problem hiding this comment.
If this function is part of the public API, throwing an error can be a bit invasive. I see we're throwing it here to catch it in the CSSVariableReplacer but it may not be ideal if someone tries to consume replaceCSSVariablesInString directly and it errors just like that.
|
I've tested this with the Greyd WP theme and a new set of gradients with the primary color and can confirm the PR works. The only thing I noticed is that when switching style variations, the colors of the previously selected style variation stay in the gradient picker. I have to reload the editor to get the new colors. This is not the case for colors, these get updated as soon as the style variation is switched. However when I pick one of these, the correct primary color is selected, it's just the preview in the gradient picker that does not get updated. |
|
Hey @luminuu , it would be great if you could share more details about the issue that you're encountering? From a quick read, it sounds like the problem may rely on the consumers of |
|
@ciampo I've made a short video showing this behaviour: CleanShot.2024-08-16.at.09.57.33.mp4This doesn't happen with solid colors, they get updated immediately. It's just the gradients that are stuck with the colors of the previous style variation. |
Fixes #43715
Fixes #28254
What?
Add CSS variable support to CustomGradientPicker.
Why?
The gradient parser that we're using under the hood doesn't support CSS variables, and throws an error. We
catchthe error so it doesn't crash, but the UI looks unresponsive. It would be nice if we could read the computed style values of the CSS variables, so we could present the user with the actual colors in the gradient picker so they can be tweaked.This is not the first time we wanted the computed values of CSS variables (e.g. automated contrast checking for our
Themecomponent). I designed the solution in a generic way because think it might be useful in other cases as well.How?
Introduce a generic
CSSVariableReplacercomponent (internally) that takes a CSS string, gets the computed values of any CSS variables in it, replaces the CSS variables in the original string with computed values, and allows the consumer to retrieve that data via anonChangecallback. Inserting this component in different locations in the DOM will get the computed style values from that specific context.Ideally we would use a library for the CSS string parsing, but I couldn't find a suitable one for our specific needs, and a general CSS parser like
postcssis too big for us to add to the bundle just for this purpose. Fortunately the parsing logic we need here is not too complex, consisting of a standard parentheses matching algorithm and some simple regular expressions.Deferred for future enhancement
In the
onChangecall of CustomGradientPicker, it would be nice if we could maintain the untouched CSS variables even when one of the values are modified. In this PR, all of the variables will flatten into "literal" values once any one of them is modified. (This is the current behavior in trunk, so not a regression.)Testing Instructions
Possible ways to test
packages/components/src/utils/test/css-variables.ts), try to write a test case that breaks it.wp-admin/site-editor.php?path=%2Fwp_global_styles) and choose the "Sherbet" style, which has a theme gradient. In the editor, see the block inspector for a Paragraph block and open the Color ▸ Background popover. In the Gradient tab, you should be able to choose the first Theme gradient. (Unfortunately the other two Theme gradients do not work because they are invalid.)