Global Styles: simplify how we register preset metadata

[oandregal]

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

• Make sure the automated tests still pass.
• Use a theme with theme.json and make sure the presets (custom properties and classes) are generated properly.

[ajlende]

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

[ajlende]

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.

[oandregal]

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).

[oandregal]

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.

[ajlende]

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.

[ajlende]

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.

[oandregal]

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 :)

[oandregal]

#35318 is for making remove_insecure_styles work with duotone data.

[ajlende]

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 it color: 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. 😅

[oandregal]

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.

[ajlende]

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?

[ajlende]

Big picture (not for this PR) I don't think PRESETS_METADATA should be handling classes. It seems like something that could be left up to the block supports to do themselves like generating SVGs that duotone has to do. The classes are used directly by the block supports to apply presets.

I still want to add classes like .has-$slug-duotone for the duotone presets, but it's challenging because the filter doesn't apply to the root of the block. Maybe we can do something with them to make that a little easier if we move them out.

[oandregal]

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 remove_insecure_properties issue: as soon as we expose the duotone in the global styles sidebar (expected for the 5.9 cycle) we need to have this in place at the framework level, so I'd rather have it ready now with small changes. It's internal code, so we can always revisit later.

[youknowriad]

LGTM 👍

[oandregal]

Created a follow-up at #35248

I'm now looking at retrieving the proper value for duotone.

[oandregal]

#35255 allows value_func in remove_insecure_settings.