Style Engine: add border to backend

[ramonjd]

Tracking Issue: #38167

What?

Adds border block supports.

Builds upon

• Style Engine: add typography to backend #40082
• Style Engine: add typography and color to backend  #40332

Why?

Borders are gnarly.

How?

As well as generating styles and classnames for border properties, this PR adds support for top, bottom, right and left block style properties, e.g., { "border": { "top": { "color": "#cccccc", ... }, ... } ... }

Testing Instructions

Add a Site Title block and play around with border styles in the post editor.

To make life easier, give the dynamic Site Title block all the supports!

Example Site Title block.json

{
	"$schema": "https://schemas.wp.org/trunk/block.json",
	"apiVersion": 2,
	"name": "core/site-title",
	"title": "Site Title",
	"category": "theme",
	"description": "Displays the name of this site. Update the block, and the changes apply everywhere it’s used. This will also appear in the browser title bar and in search results.",
	"textdomain": "default",
	"attributes": {
		"level": {
			"type": "number",
			"default": 1
		},
		"textAlign": {
			"type": "string"
		},
		"isLink": {
			"type": "boolean",
			"default": true
		},
		"linkTarget": {
			"type": "string",
			"default": "_self"
		}
	},
	"example": {
		"viewportWidth": 500
	},
	"supports": {
		"align": [ "wide", "full" ],
		"html": false,
		"color": {
			"gradients": true,
			"link": true,
			"__experimentalDefaultControls": {
				"background": true,
				"text": true,
				"link": true
			}
		},
		"spacing": {
			"padding": true,
			"margin": true
		},
		"typography": {
			"fontSize": true,
			"lineHeight": true,
			"__experimentalFontFamily": true,
			"__experimentalTextTransform": true,
			"__experimentalFontStyle": true,
			"__experimentalFontWeight": true,
			"__experimentalLetterSpacing": true,
			"__experimentalTextDecoration": true,
			"__experimentalDefaultControls": {
				"fontSize": true,
				"lineHeight": true,
				"fontAppearance": true,
				"letterSpacing": true,
				"textTransform": true
			}
		},
		"__experimentalBorder": {
			"color": true,
			"radius": true,
			"style": true,
			"width": true,
			"__experimentalDefaultControls": {
				"color": true,
				"radius": true,
				"style": true,
				"width": true
			}
		}
	},
	"editorStyle": "wp-block-site-title-editor"
}

And some test block code:

Block code

<!-- wp:site-title {"style":{"color":{"background":"#cf1c1c"},"typography":{"fontStyle":"italic","fontWeight":"800","letterSpacing":"32px","lineHeight":3.9,"textTransform":"uppercase"},"border":{"top":{"color":"var:preset|color|vivid-cyan-blue","width":"9px"},"right":{"color":"var:preset|color|vivid-red","width":"18px"},"bottom":{"color":"var:preset|color|vivid-cyan-blue","width":"12px"},"left":{"color":"#b82b2b","width":"83px"},"radius":{"topLeft":"20px","topRight":"0px","bottomLeft":"20px","bottomRight":"100px"}},"spacing":{"padding":{"top":"82px","right":"82px","bottom":"82px","left":"82px"},"margin":{"top":"67px","right":"67px","bottom":"67px","left":"67px"}}},"textColor":"pale-pink","fontSize":"x-large"} /-->

Unit tests

npm run test-unit-php /var/www/html/wp-content/plugins/gutenberg/phpunit/block-supports/border-test.php

npm run test-unit-php /var/www/html/wp-content/plugins/gutenberg/packages/style-engine/phpunit/class-wp-style-engine-test.php

More to come...

[ramonjd]

I've made some assumptions both here $style_definition['path'][0] and below $style_definition['path'][1].

$style_definition['path'][0] assumes the path array's first value is the style property, e.g., border. It also implies that border contains the necessary definitions (color, width, etc) for the sides.

The use of $style_definition['path'][1] assumes the path array's final item corresponds to the side name.

Both of these are fine in my opinion because nothing will work if the paths are not right, but it does seems a bit "clever" and unreadable.

A possibility is to add explicit properties to the style's metadata such as inherits or something. 🤷

The latter

[andrewserong]

Good question about the assumptions here — just to walk through how I understand it:

• $style_definition['path'][0] will map to the parent level property that we're looking up (in this case border but could theoretically be any of the other root level props of the metadata).
• $style_definition['path'][1] will map to the slug for the side.

It took me a little while to read through and work out what was going on here, but I think the assumptions are good! What I'm wondering is whether we can rename things slightly to improve readability.

In the function signature the $style_definition prop will always be the definition for the side (at least based on the props that have opted in to use this callback). Would it improve clarity to rename this prop to $side_style_definition?

Then, where we have the existing $side_style_definition, what if we renamed this to $style_definition.

We could also store the ['path'][0] and ['path'][1] values in variables, so they've got a concrete name for what they're referring to. Putting it all together, it might look like:

	protected static function get_css_side_rules( $style_value, $side_style_definition ) {
		$rules = array();

		if ( ! is_array( $style_value ) || empty( $style_value ) || empty( $side_style_definition['path'] ) ) {
			return $rules;
		}

		// The first item in the style definition path array tells us the style property, e.g., "border".
		// We use this to get a corresponding CSS style definition such as "color" or "width" from the same group.
		$definition_group_key = $side_style_definition['path'][0];
		$side                 = $side_style_definition['path'][1];

		foreach ( $style_value as $css_property => $value ) {
			if ( empty( $value ) ) {
				continue;
			}

			$style_definition_path = array( $definition_group_key, $css_property );
			$style_definition      = _wp_array_get( self::BLOCK_STYLE_DEFINITIONS_METADATA, $style_definition_path, null );

			if ( $style_definition && isset( $style_definition['properties']['sides'] ) ) {
				// Set a CSS var if there is a valid preset value.
				$slug = isset( $side_style_definition['css_vars'][ $css_property ] ) ? static::get_slug_from_preset_value( $value, $css_property ) : null;
				if ( $slug ) {
					$css_var = strtr(
						$side_style_definition['css_vars'][ $css_property ],
						array( '$slug' => $slug )
					);
					$value   = "var($css_var)";
				}
				// The second item in the style definition path array refers to the side property, e.g., "top".
				$side_css_property           = strtr( $style_definition['properties']['sides'], array( '$side' => $side ) );
				$rules[ $side_css_property ] = $value;
			}
		}
		return $rules;
	}

What do you think? It's probably a fairly small change, so I'm not sure if that improves things very much, so feel free to skip it — but typing some of that out helped me understand what's going on a bit better 😀. At the very least, this is internal behaviour to the style engine, so I think it's reasonably safe for us to go with the general approach here, and we can always refine it in follow-ups if need be, without affecting the exposed API of the engine.

[ramonjd]

This is much better thank you. If it helps folks who are reading it for the first time it's definitely worth it.

[andrewserong]

This is testing really well for me @ramonjd! I left a few comments, and I'll do a little more testing tomorrow, I'll take a closer look at your comments on $style_definition['path'][0] etc then 🙂

[aaronrobertshaw]

Great work on this @ramonjd 🙇

This has tested well for me.

✅ Works well for Site Title and other dynamic blocks with generic border support
✅ No impact on existing blocks given it's limited to dynamic blocks
✅ CSS classes and styles appear the same before/after applying this branch
✅ Unit tests are all passing

Despite my lack of familiarity with the style engine to date, the changes make sense and are easy to follow with only the exception being the "side" terminology.

The "side" functionality is being used for border-radius "corners" already. As @andrewserong points out there are other non-side use cases potentially on the horizon. Can we change this to something more accurate and versatile?

[ramonjd]

I'd argue it's already being used for corners instead of sides for border-radius. Personally, I'd like to see the terminology of "sides" replaced with something that more broadly applies. Perhaps something towards "longhand" given that's sort of what these "sub-properties" are?

Thanks for testing this PR @aaronrobertshaw, and for the tip about corners.

I'll take a look at it and try to come up with a way to accommodate a broader range of properties.

[ramonjd]

I'll take a look at it and try to come up with a way to accommodate a broader range of properties.

I went with 'single' in an attempt the genericize the variable, noting that the method could now be used for any, specific, single CSS property:

/*
"$style_property-$single_feature: $value;", which could represent the following:
"border-{top|right|bottom|left}-{color|width|style}: {value};" or,
"border-image-{outset|source|width|repeat|slice}: {value};"
*/

Very willing to use another term if folks think it's not clear enough, e.g., shorthand and longhand vs default and single. It's a search and replace now 😄

I tried a few alternative data models to store the properties, but couldn't come up with anything more flexible. I guess we'll find out once we have to support more of these kinds of styles.

[glendaviesnz]

Very willing to use another term if folks think it's not clear enough, e.g., shorthand and longhand vs default and single.

I think shorthand & longhand might be clearer, as single could potentially be confused with shorthhand, eg A 'single' property that sets all the sides. shorthand and individual is another option.

[ramonjd]

shorthand and individual is another option.

Thanks for the feedback @glendaviesnz

Fair call about "single".

I also considered "individual". What do you think about that?

Maybe unnecessarily, I kept away from long- and shorthand because, while reading up on common terms, I found them to be interchangeable with longform, shortform and often longhand wasn't used at all with a preference for shorthand to imply grouping of individual properties.

Who said naming things is hard again? 😄

[glendaviesnz]

I also considered "individual". What do you think about that?

individual works - I think it would be much harder to confuse that with the shorthand form than single.

[aaronrobertshaw]

Thanks for wrestling with the terminology used in this one @ramonjd 👍

This is functionally still testing well for me. I've left a few minor tweaks I think we should make but after that this should be good to go 🚢

[andrewserong]

Nice work @ramonjd, this is still testing well for me, too, and nothing to add that Aaron hasn't already mentioned. Once those renamings are fixed up and this has been rebased, it looks good to go to me! 🎉

[mburridge]

Added the Needs Dev Note label in case this needs a dev note (either individual or as part of a "misc" dev note) for WP 6.1 release.