Border radius presets: support mixed values in the rangecontrol slider #73033
Conversation
…e unparseable strings. Added tests for clamp, min, max, and calc values, ensuring correct behavior with mixed and simple values.
ramonjd
added
[Type] Bug
|
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. |
| const parsedQuantitiesAndUnits = Object.values( values ).map( ( value ) => { | ||
| const newValue = parseQuantityAndUnitFromRawValue( value ); | ||
| if ( typeof value === 'string' && newValue[ 0 ] === undefined ) { | ||
| return [ value, '' ]; |
There was a problem hiding this comment.
This will return the string no matter what it is. I think that's okay since the value should match what's in the presets array in order for the steppers to work.
The Theme JSON class sanitizes the values on the backend.
There was a problem hiding this comment.
At a glance, this looks ok to me as well.
There's a possibility the values are values originating from the preset values but instead custom values entered by the user. These would be entered via the custom value inputs and should be safe simple CSS values.
The Theme JSON class does sanitize style values on the backend but that happens here:
It might pay to dig a little deeper and confirm safety for the application of styles in the editor. If there's an issue there it wouldn't be unique to this fix or PR.
There was a problem hiding this comment.
I looked a little closer and didn't see any obvious issues but it might pay to get a fresher set of eyes on it.
|
Size Change: +23 B (0%) Total Size: 2.45 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in e324343. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19127877332
|
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
There was a problem hiding this comment.
This is testing well for me ✨
✅ Could replicate the original issue
✅ Applying this fix resolves the problem
✅ Slider behaviour for "split" controls continues to function as before
✅ New unit tests are passing (bonus points for these 🙇 )
| Before | After |
|---|---|
Screen.Recording.2025-11-06.at.5.51.34.pm.mp4 |
Screen.Recording.2025-11-06.at.5.59.08.pm.mp4 |
|
Thanks for the quick fix! While testing this pull request, I noticed that the radius preset values cannot be saved in the global styles. However, this is not caused by this pull request. 4e08f5aa0a23b9248ea4d3a3f67e4e5d.1.mp4 |
t-hamano
added
the
Backport to WP 6.9 Beta/RC
|
I also think it's worth backporting this PR to 6.9. |
SirLouen
left a comment
SirLouen
left a comment
There was a problem hiding this comment.
At first everything looks good to me, the solution is hacky. Basically with this new conditional, theoretically we could be ignoring parseQuantityAndUnitFromRawValue all around and do exactly the same as for the "complex" scenario, returning values without units as they will get equally to the CSS
Also, these two tests make me doubt about the whole usefulness of how the function is designed. What's the point of returning value=>'apples' or value=>32, unit=>apples? I'm checking uses of getAllValue and I can't really see a place where knowing the unit is actually useful.
it( 'should preserve string values that cannot be parsed at all (no numeric prefix)', () => {
// Values with no numeric prefix cannot be parsed, so they should be preserved
const unparseableValue = 'apples';
const values = {
bottomLeft: unparseableValue,
bottomRight: unparseableValue,
topLeft: unparseableValue,
topRight: unparseableValue,
};
expect( getAllValue( values ) ).toBe( unparseableValue );
} );
it( 'should parse numeric prefix from partially parseable values', () => {
// Values with numeric prefix get parsed, so "32apples" becomes "32"
const partiallyParseableValue = '32apples';
const values = {
bottomLeft: partiallyParseableValue,
bottomRight: partiallyParseableValue,
topLeft: partiallyParseableValue,
topRight: partiallyParseableValue,
};
expect( getAllValue( values ) ).toBe( '32' );
} );cc @ramonjd
Thanks for the comment. I agree, there's room for a refactor here and some iteration. The target of this PR is to fix the bug right now so that the feature works. I reckon tightening up the logic should be in a follow up PR. Feel free to open one if other folks don't get to it first!
Granted, it's pretty permissive so keen to hear how it can be improved in a follow up. If I'm not mistaken, knowing the unit is useful when switching between the preset range and the custom unit control. The unit, if known, will set the unit drop down. cc @aaronrobertshaw for fact check. I'm not a computer today, but feel free to merge this and we can do follow ups for global styles et. al. |
It appears that the unit is just to select among all the values the ones with more coincidences. I feel that the unit tests are adapting to the function and not the other way around. Reading the function and the original PR #31585 its not easy to guess what was the author thinking with all those conditionals, so in this case tests are the best docs.
So I would also add, for example, these tests it( 'same values with same amount of mixed units', () => {
const values = {
bottomLeft: '2px',
bottomRight: '2rem',
topLeft: '2px',
topRight: '2rem',
};
expect( getAllValue( values ) ).toBe( '2px' );
} );
it( 'same values with different amount of mixed units', () => {
const values = {
bottomLeft: '2px',
bottomRight: '2rem',
topLeft: '2rem',
topRight: '2rem',
};
expect( getAllValue( values ) ).toBe( '2rem' );
} );
it( 'mixed values with same units', () => {
const values = {
bottomLeft: '2px',
bottomRight: '3px',
topLeft: '4px',
topRight: '5px',
};
expect( getAllValue( values ) ).toBe( undefined );
} );Finally, after digging a little bit more, I believe that your patch should be integrated in the |
The |
As per the PR linked (#31585), you can see the UI design being implemented used the existing UnitControl component which only allows setting simple CSS values. Originally, I believe this and the spacing controls were based off the
I'm on board with fixing the immediate issue, minimising disruptive changes before the RC, and revisiting or consolidating logic as a follow up 👍 |
|
Yes, for now, let's merge this pull request as is. |
github-actions
bot
removed
the
Backport to WP 6.9 Beta/RC
#73033) * Enhance getAllValue function to handle complex CSS values and preserve unparseable strings. Added tests for clamp, min, max, and calc values, ensuring correct behavior with mixed and simple values. * Remove comment Co-authored-by: ramonjd <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: SirLouen <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: benoitchantre <[email protected]>
github-actions
bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.9 branch to get it included in the next release: 91cc49e |
|
Thanks for merging, I think this is a good fix for now, too 👍
I believe I've seen this one before, I think we might not be decoding values correctly for the global styles case when using the presets. I can do a little digging. |
I think you're on the money here. There was a similar issue with the block inspector border panel. Once the inspector and global styles sidebars were combined it's become a little trickier to cover the two potential formats e.g. |
|
I was also digging into the reported issue and it appears that the data is saved to global styles fine. It is just the handling within the border radius control component that needs fixing. |
That's what I'm seeing, too. I think I've got a quick fix working. I can throw up a PR shortly. |
Fixes #73018
What?
Update the
getAllValuefunction to handle complex CSS values and preserve unparseable strings.Added tests for clamp, min, max, and calc values, ensuring correct behavior with mixed and simple values.
Why?
When
parseQuantityAndUnitFromRawValuecan't parse a clamp value like"clamp(1rem, 2vw, 3rem)", it returns[undefined, undefined].Without the fix, that becomes
[undefined, undefined], and the clamp value is lost.How?
Check that the value is a string and is unparseable. Return the original value.
Testing Instructions
Here is some test JSON to apply to your theme.
Head to the editor and try to apply these border radius presets to a Group or other block.
You should be able to apply all presents to all, or single corners.
Screenshots or screencast
Kapture.2025-11-06.at.18.14.07.mp4