-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Global Styles: simplify how we register preset metadata #35228
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the simplification of css_var
and classes
—tested out locally and everything still seems to work fine!
Just had one comment on properties
that I think needs some change
* - path => where to find the preset within the settings section | ||
* | ||
* - value_key => the key that represents the value | ||
* | ||
* - value_func => optionally, instead of value_key, a function to generate | ||
* the value that takes a preset as an argument | ||
* | ||
* - css_var => name of the var to generate. The "$slug" substring will be | ||
* replaced by the slug of each preset. For example, | ||
* given a preset for color with two values whose slugs are "black" and "white", | ||
* the string "--wp--preset--color--$slug" will generate two variables: | ||
* "--wp--preset--color--black" and "--wp--preset--color--white". | ||
* | ||
* - classes => array containing a structure with the classes to | ||
* generate for the presets, where for each array item | ||
* the key is the class name and the value the property name. | ||
* The "$slug" substring will be replaced by the slug of each preset. | ||
* For example: | ||
* 'classes' => array( | ||
* '.has-$slug-color' => 'color', | ||
* '.has-$slug-background-color' => 'background-color', | ||
* '.has-$slug-border-color' => 'border-color', | ||
* ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
properties
is missing in the documentation here, but since the purpose of the properties being checked is for the generated .has-$slug-$property
classes, is a separate properties
array really needed here?
Duotone, for example, never sets the filter
property from settings—that's left up to styles
to handle. We're only setting the --wp--preset--duotone--$slug
variable from this set of metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The properties
are used exclusively by remove_insecure_properties
(the kses method) and nowhere else. This method was reusing the css_var_infix
, but that didn't work for duotone because the infix is not the name of the property to be used (filter
) but other thing (duotone
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this PR, remove_insecure_properties
understands which property maps to the duotone data (before it tested duotone: value
, now it tests filter: value
). The second step is to also teach it that the property value can come from value_func
(it assumes value_key
for everything). For simplicity, I'll do that in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that the CSS that needs to be checked in remove_insecure_properties
is this for duotone:
--wp--preset--duotone--slug: url('#wp-duotone-slug');
It has nothing to do with the filter
property. That check should be in remove_insecure_styles
.
And this for things like color:
--wp--preset--color--slug: #000;
.has-slug-color {
color: var(--wp--preset--color--slug);
}
.has-slug-background-color {
background-color: var(--wp--preset--color--slug);
}
.has-slug-border-color {
border-color: var(--wp--preset--color--slug);
}
So it makes sense to check the classes
like before—not a new properties
key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The properties array isn't used to generate the CSS, so we're not actually testing the generated CSS by using the properties
array. The generated CSS is coming from classes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, #35255 is the continuation of this work. It updates remove_insecure_settings
to consider presets that use the value_func
. Welcome reviews :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#35318 is for making remove_insecure_styles
work with duotone data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the well-written explanation! I've also stepped through remove_insecure_properties
and into safecss_filter_attr
in a debugger to get a more detailed idea of exactly what is going on.
In the case of settings, kses wouldn't know how to validate
--wp--preset--color--slug: user_color_value
hence why we pass itcolor: user_color_value
instead.
This is the core of what feels weird to me that I was trying to get at. Maybe safecss_filter_attr
isn't the right tool for what we're trying to do. A function that is aware of the CSS custom properties and doesn't have to substitute CSS properties where the variable is used would be less confusing and could be made to be more robust.
Arguably, in certain situations we could use a single property (
color
instead of[ color, background-color, border-color]
) but that presumes some knowledge about how kses works internally, which should be transparent to us.
We have to add properties in the first place because we know how kses (safecss_filter_attr
) works internally—it can't handle CSS custom properties. If we wanted to be truly transparent to how kses works, we should be able to pass the properties directly to either kses or a new function that can handle them as they appear in the stylesheet.
Does this help? I'm happy to jump into a call to go through this as well.
If you want to talk more about it, I'd be happy to hop on a call and write up a summary for here. I might not be explaining my perspective on this very well. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to add properties in the first place because we know how kses (safecss_filter_attr) works internally
Not really! We add properties because that's how the CSS vars are going to be used in the classes & styles the engine generates. For example, color presets generate the vars plus the classes:
.has-slug-color { COLOR: <our-css-var>; }
.has-slug-background-color { BACKGROUND-COLOR: <our-css-var>; }
.has-slug-border-color { BORDER-COLOR: <our-css-var>; }
And they are also expected to be used in the corresponding theme.json
styles: styles.border.color
, styles.color.background
, styles.color.text
, etc.
Those are the properties we need to check for.
We could argue that this info is already in the classes
key of the presets_metadata
array. However, not all presets generate classes (e.g. duotone), hence why we need a separate properties
key for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could argue that this info is already in the classes key of the presets_metadata array. However, not all presets generate classes (e.g. duotone), hence why we need a separate properties key for clarity.
Couldn't remove_insecure_properties
keep track of the CSS custom properties that we generate and use, and then check them without needing to manually add the properties
key?
Big picture (not for this PR) I don't think I still want to add classes like |
That's an interesting idea, it could work nicely. It also seems like something that takes a while to refactor 😅 I wouldn't want to wait until that happens to fix the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Created a follow-up at #35248 I'm now looking at retrieving the proper value for duotone. |
#35255 allows |
Follow-up to #34667
This PR simplifies the preset registration and adds a new setting that presets need to fill (
properties
) for kses to use in validating the styles. In doing so, we prepare the code to allow user-provided duotones to be validated by kses in a further follow-up.How to test
theme.json
and make sure the presets (custom properties and classes) are generated properly.