Global styles: check for recursive dynamic reference in the site editor #43166
Conversation
ramonjd
added
[Type] Bug
|
Size Change: +40 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
| export function getValueFromVariable( features, blockName, variable ) { | ||
| if ( ! variable || typeof variable !== 'string' ) { | ||
| if ( variable?.ref && typeof variable?.ref === 'string' ) { | ||
| const refPath = variable.ref.split( '.' ); | ||
| variable = get( features, refPath ); | ||
| // Presence of another ref indicates a reference to another dynamic value. | ||
| // Pointing to another dynamic value is not supported. | ||
| if ( ! variable || !! variable?.ref ) { |
There was a problem hiding this comment.
do we need .? here since we have already checked that variable is defined?
There was a problem hiding this comment.
I left it in there since variable is reassigned above: variable = get( features, refPath );
There was a problem hiding this comment.
sorry, wasn't 100% clear, and just a nit really. On line 240 we check again if it is defined with if ( ! variable .... I thought we would only need .? if we were only checking for .ref, eg. if ( !! variable?.ref ) ? ... it is Friday afternoon though, so I may be missing something 😄
There was a problem hiding this comment.
Ah sorry 🤦 was my Friday bad.
Good question. I wanted to check for both just in case the ref pointed to a dead end, not just another ref.
I'll update the comment.
Adding tests to global styles, just in case
| @@ -277,6 +277,11 @@ export function getStylesDeclarations( | |||
| if ( typeof ruleValue !== 'string' && ruleValue?.ref ) { | |||
| const refPath = ruleValue.ref.split( '.' ); | |||
| ruleValue = get( tree, refPath ); | |||
| // Presence of another ref indicates a reference to another dynamic value. | |||
| // Pointing to another dynamic value is not supported. | |||
| if ( ! ruleValue || !! ruleValue?.ref ) { | |||
There was a problem hiding this comment.
I ran out of time to add tests for this today. Will do later or in a follow up
|
I was thinking about this yesterday and wasn't sure where it was best to catch this issue, if it's best to handle this when we are resolving what ref is pointing to or right at the start when we are sanitizing the theme.json. The syntax is valid, but not the value being referenced. I don't have an answer to that! In any case, this is working for me and it looks good, thanks for the follow up! |
This is an excellent point, thanks for raising. I naively didn't probe much further than the JS error stack. 😀 Fetching and sanity checking refs during resolution sounds like it might be more bug-proof. I'm happy to go further on this as as a follow up next week or help test PRs. |
I've spent some time testing a couple of backend alternative approaches. The assumption I made was to move the work of So {
"version": 2,
"styles": {
"color": {
"background": {
"ref": "styles.color.text"
},
"text": "green"
}
}
}Would become {
"version": 2,
"styles": {
"color": {
"background": "green",
"text": "green"
}
}
}This could happen in the constructor, or indirectly via The challenge is that it would involve the duplication of a bunch of existing processes, e.g., Furthermore, we need to watch out for The catch is that we'd want to make sure all theme data are merged first, so perhaps there's a case for I've created a test PR of this idea: I did also test updating the methods in Gutenberg_REST_Global_Styles_Controller that retrieve theme and theme variations. This is how the frontend gets the theme.json (and its variations), and here we could parse the style nodes and ensure that any ref values are resolved. I thought it might be an alternative, and save a whole bunch of refactoring of |
The editor did not correctly resolve dynamic refs and the style engine does not yet support resolution. This commit adds a utility function to resolve dynmamic refs and add conditions to prevent recursive refs.
ramonjd
changed the title
|
I've also created a follow up to remove the duplicated code: |
I thought about this too! The problem with that is that then the editor loses the information that that value is a ref to another value, we should aim to expose this to the user at some point so they can decide if they want to replace a value that was initially referencing another one. |
Oh! That's a tricky one. I wonder how we'd translate it to the UI... 🤔 If Gutenberg did perform resolution on the backend, maybe it could replace the circular ref value with some other error identifier that the UI knows how to handle? Might be too tightly coupled. |
I think the backend should provide the frontend with valid values and let the frontend resolve what the ref is. If it's a recursive reference, then it's not a valid value, so we are not letting that reach the frontend. If it's a valid ref, then we let the frontend handle getting the content of that. Right now, resolving it to what the final value is, in the future, we can think how to expose that the value is actually a reference (though I don't know how that would look like technically or UI wise!) |
The editor did not correctly resolve dynamic refs and the style engine does not yet support resolution. This commit adds a utility function to resolve dynmamic refs and add conditions to prevent recursive refs.
Fixes #43154
What?
This PR checks for a recursive dynamic reference in
getValueFromVariable()and bails if found.Why?
When clicking on "Styles", the site editor crashes where a dynamic references points to another dynamic reference.
2022-08-12.11.44.32.mp4
Testing Instructions
Add a recursive reference to your theme.json, e.g.,
{ "version": 2, "settings": { "appearanceTools": true }, "styles": { "color": { "background": { "ref": "styles.color.text" }, "text": { "ref": "styles.color.background" } } } }Open up the site editor. You should see the
gutenberg/lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php
Line 969 in 34932d7
Click on the "Styles" button.
The editor shouldn't crash.