Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Global Styles: simplify how we register preset metadata #35228

Merged
merged 3 commits into from
Sep 30, 2021
Merged

Global Styles: simplify how we register preset metadata #35228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e9bdf05
Simplify preset metadata
oandregal Sep 29, 2021
12482fa
Add duotone test
oandregal Sep 29, 2021
dd796b4
Update docs
oandregal Sep 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
142 changes: 75 additions & 67 deletions lib/class-wp-theme-json-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,77 +141,72 @@ class WP_Theme_JSON_Gutenberg {
*
* This contains the necessary metadata to process them:
*
* - 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_infix => infix to use in generating the CSS Custom Property. Example:
* --wp--preset--<preset_infix>--<slug>: <preset_value>
*
* - classes => array containing a structure with the classes to
* generate for the presets. Each class should have
* the class suffix and the property name. Example:
*
* .has-<slug>-<class_suffix> {
* <property_name>: <preset_value>
* }
* - 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
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
* 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',
* )
Comment on lines +144 to +166
Copy link
Contributor

@ajlende ajlende Sep 29, 2021 •
edited

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.

Copy link
Member Author

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

Copy link
Member Author

@oandregal oandregal Sep 30, 2021 •
edited

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.

Copy link
Contributor

@ajlende ajlende Sep 30, 2021 •
edited

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.

Copy link
Contributor

@ajlende ajlende Sep 30, 2021 •
edited

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.

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Contributor

@ajlende ajlende Oct 4, 2021 •
edited

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

Copy link
Member Author

@oandregal oandregal Oct 6, 2021 •
edited

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.

Copy link
Contributor

@ajlende ajlende Oct 6, 2021 •
edited

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?

* - properties => array of CSS properties to be used by kses to
* validate the content of each preset
* by means of the remove_insecure_properties method.
*/
const PRESETS_METADATA = array(
array(
'path' => array( 'color', 'palette' ),
'value_key' => 'color',
'css_var_infix' => 'color',
'classes' => array(
array(
'class_suffix' => 'color',
'property_name' => 'color',
),
array(
'class_suffix' => 'background-color',
'property_name' => 'background-color',
),
array(
'class_suffix' => 'border-color',
'property_name' => 'border-color',
),
'path' => array( 'color', 'palette' ),
'value_key' => 'color',
'css_vars' => '--wp--preset--color--$slug',
'classes' => array(
'.has-$slug-color' => 'color',
'.has-$slug-background-color' => 'background-color',
'.has-$slug-border-color' => 'border-color',
),
'properties' => array( 'color', 'background-color', 'border-color' ),
oandregal marked this conversation as resolved.
Show resolved Hide resolved
),
array(
'path' => array( 'color', 'gradients' ),
'value_key' => 'gradient',
'css_var_infix' => 'gradient',
'classes' => array(
array(
'class_suffix' => 'gradient-background',
'property_name' => 'background',
),
),
'path' => array( 'color', 'gradients' ),
'value_key' => 'gradient',
'css_vars' => '--wp--preset--gradient--$slug',
'classes' => array( '.has-$slug-gradient-background' => 'background' ),
'properties' => array( 'background' ),
),
array(
'path' => array( 'color', 'duotone' ),
'value_func' => 'gutenberg_render_duotone_filter_preset',
'css_var_infix' => 'duotone',
'classes' => array(),
'path' => array( 'color', 'duotone' ),
'value_func' => 'gutenberg_render_duotone_filter_preset',
'css_vars' => '--wp--preset--duotone--$slug',
'classes' => array(),
'properties' => array( 'filter' ),
),
array(
'path' => array( 'typography', 'fontSizes' ),
'value_key' => 'size',
'css_var_infix' => 'font-size',
'classes' => array(
array(
'class_suffix' => 'font-size',
'property_name' => 'font-size',
),
),
'path' => array( 'typography', 'fontSizes' ),
'value_key' => 'size',
'css_vars' => '--wp--preset--font-size--$slug',
'classes' => array( '.has-$slug-font-size' => 'font-size' ),
'properties' => array( 'font-size' ),
),
array(
'path' => array( 'typography', 'fontFamilies' ),
'value_key' => 'fontFamily',
'css_var_infix' => 'font-family',
'classes' => array(),
'path' => array( 'typography', 'fontFamilies' ),
'value_key' => 'fontFamily',
'css_vars' => '--wp--preset--font-family--$slug',
'classes' => array(),
'properties' => array( 'font-family' ),
),
);

Expand Down Expand Up @@ -762,14 +757,16 @@ private static function compute_preset_classes( $settings, $selector, $origins )
$stylesheet = '';
foreach ( self::PRESETS_METADATA as $preset_metadata ) {
$slugs = self::get_settings_slugs( $settings, $preset_metadata, $origins );
foreach ( $preset_metadata['classes'] as $class ) {
foreach ( $preset_metadata['classes'] as $class => $property ) {
foreach ( $slugs as $slug ) {
$css_var = self::replace_slug_in_string( $preset_metadata['css_vars'], $slug );
$class_name = self::replace_slug_in_string( $class, $slug );
$stylesheet .= self::to_ruleset(
self::append_to_selector( $selector, '.has-' . $slug . '-' . $class['class_suffix'] ),
self::append_to_selector( $selector, $class_name ),
array(
array(
'name' => $class['property_name'],
'value' => 'var(--wp--preset--' . $preset_metadata['css_var_infix'] . '--' . $slug . ') !important',
'name' => $property,
'value' => 'var(' . $css_var . ') !important',
),
)
);
Expand All @@ -780,6 +777,18 @@ private static function compute_preset_classes( $settings, $selector, $origins )
return $stylesheet;
}

/**
* Transform a slug into a CSS Custom Property.
*
* @param array $input String to replace.
* @param string $slug The slug value to use to generate the custom property.
*
* @return string The CSS Custom Property. Something along the lines of --wp--preset--color--black.
*/
private static function replace_slug_in_string( $input, $slug ) {
return strtr( $input, array( '$slug' => $slug ) );
}

/**
* Given the block settings, it extracts the CSS Custom Properties
* for the presets and adds them to the $declarations array
Expand All @@ -803,7 +812,7 @@ private static function compute_preset_vars( $settings, $origins ) {
$values_by_slug = self::get_settings_values_by_slug( $settings, $preset_metadata, $origins );
foreach ( $values_by_slug as $slug => $value ) {
$declarations[] = array(
'name' => '--wp--preset--' . $preset_metadata['css_var_infix'] . '--' . $slug,
'name' => self::replace_slug_in_string( $preset_metadata['css_vars'], $slug ),
'value' => $value,
);
}
Expand Down Expand Up @@ -1300,10 +1309,9 @@ private static function remove_insecure_settings( $input ) {
) {
$value = $single_preset[ $preset_metadata['value_key'] ];
$single_preset_is_valid = null;
if ( isset( $preset_metadata['classes'] ) && count( $preset_metadata['classes'] ) > 0 ) {
if ( isset( $preset_metadata['properties'] ) && count( $preset_metadata['properties'] ) > 0 ) {
$single_preset_is_valid = true;
foreach ( $preset_metadata['classes'] as $class_meta_data ) {
$property = $class_meta_data['property_name'];
foreach ( $preset_metadata['properties'] as $property ) {
if ( ! self::is_safe_css_declaration( $property, $value ) ) {
$single_preset_is_valid = false;
break;
Expand Down
5 changes: 5 additions & 0 deletions phpunit/class-wp-theme-json-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -978,6 +978,11 @@ function test_remove_insecure_properties_removes_unsafe_styles() {
),
),
'blocks' => array(
'core/image' => array(
'filter' => array(
'duotone' => 'var:preset|duotone|blue-red',
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
),
),
'core/group' => array(
'color' => array(
'gradient' => 'url(\'data:image/svg+xml;base64,PHN2ZyB4bWxucz0naHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmcnIHdpZHRoPScxMCcgaGVpZ2h0PScxMCc+PHNjcmlwdD5hbGVydCgnb2snKTwvc2NyaXB0PjxsaW5lYXJHcmFkaWVudCBpZD0nZ3JhZGllbnQnPjxzdG9wIG9mZnNldD0nMTAlJyBzdG9wLWNvbG9yPScjRjAwJy8+PHN0b3Agb2Zmc2V0PSc5MCUnIHN0b3AtY29sb3I9JyNmY2MnLz4gPC9saW5lYXJHcmFkaWVudD48cmVjdCBmaWxsPSd1cmwoI2dyYWRpZW50KScgeD0nMCcgeT0nMCcgd2lkdGg9JzEwMCUnIGhlaWdodD0nMTAwJScvPjwvc3ZnPg==\')',
Expand Down