Style Engine: add typography to backend

[ramonjd]

Tracking Issue: #38167

What?

Following on from #39446 that introduced spacing to the Style Engine backend, this PR takes on typography ⚔️

Why?

To introduce the Style Engine incrementally at first, and not change any output for now.

How?

1. Using an existing method to output inline styles generate
2. Creating a new method get_classnames to output classnames inc. slugs.

Testing Instructions

For dynamic blocks (Site Title, Navigation, Post Title, Post Date for example), check that the typography is output correctly on the frontend.

I've been testing so far with the Site Title block. To test with all controls you can update that block's JSON supports with the following:

		"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
			}
		}

Test:

• Preset color classnames and font sizez
• Inline styles

Some test code (using TwentyTwentyTwo theme)

<!-- wp:site-title {"style":{"typography":{"lineHeight":1.7,"textTransform":"uppercase","letterSpacing":"113px","textDecoration":"line-through"},"color":{"background":"#c2c9d0"}},"fontSize":"large","fontFamily":"source-serif-pro"} /-->

Deprecated HTML with presets

<!-- wp:navigation {"orientation":"horizontal","style":{"typography":{"textTransform":"var:preset|text-transform|lowercase","textDecoration":"var:preset|text-decoration|strikethrough","fontStyle":"var:preset|font-style|italic","fontFamily":"var:preset|font-family|someAwesomeFont-right-here","fontWeight":"var:preset|font-weight|100"}}} -->
<!-- wp:navigation-link {"label":"WordPress","url":"https://www.wordpress.org/"} /-->
<!-- /wp:navigation -->

Screenshots or screencast

[ramonjd]

We don't need to check for a preset when it lands in core

TODO: check what this means

cc @ockham Do you know if this comment is still valid? Grabbed your name from 1e17b5a#diff-2c54d3c8305590cf826f83511a9fd85d5b405470addd62edc3183ff9118177fbR115

Or would we need the code permanently for backwards compatibility?

[ramonjd]

We don't actually use these feature, so I'm removing it.

[ramonjd]

To match the clientside naming.

[ramonjd]

Updated to use spaces to improve readability.

[andrewserong]

Great work @ramonjd, this is testing very nicely!

✅ Each of the Typography styles are rendered correctly on the front end
✅ The generated classnames appear to be rendered and injected correctly into the markup

I've left a few comments, I hope you don't mind — mostly poking at the idea of whether it'd be feasible for us to consolidate into a single call to generate, move the logic of inferring classnames from attribute values to the style engine, and return an array with keys for the inline styles and classes.

It probably doesn't make a huge difference to do that at this stage, but I wondered if it'd be easier to lay the groundwork for that now, in prep for other block supports that will likely use classnames (and generated / constructed or random id classnames) more verbosely or frequently (mostly thinking of elements and layout supports).

What do you think? I can imagine there's a good argument either way for keeping separate for now / versus consolidating now.

Happy to do more testing tomorrow!

[andrewserong]

Seeing these two calls side-by-side, I was wondering if we should consolidate these calls so that get_classnames becomes an implementation detail of generate within the style engine? Then, we could adjust the return signature of generate to be an array with keys for classes and a key for inline_styles (or whatever we want to name the keys).

By the time we get to refactoring the elements block support to use the style engine, we'll need to handle returning generated class names, so I wondered if it's worth factoring that in now?

[ramonjd]

Seeing these two calls side-by-side, I was wondering if we should consolidate these calls so that get_classnames becomes an implementation detail of generate within the style engine?

I agree it's something we probably should do.

I spent some time thinking about this before I landed on two methods, and I got stuck on the structure of the arguments signature more than the return. That is, how show we should tell the style engine to deal with the different values?

The $block_styles arg matches Gutenberg's style object and has a very particular structure, and I couldn't see how classname slugs would fit into that.

We could always expand the options array. I did try that but wasn't satisfied with my approach as it appeared haphazard and unintuitive, and not related at all to the main argument $block_styles.

generate( 
     $block_styles,
      [ classnames => $array_of_classnames ]
)

The first argument could be a collection of [ $block_styles, $block_classnames ] I suppose.

Another thought I had was to extend the style value with metadata, which would mean the style engine had its own implementation of $block_styles. For example:

// Classname/slug
$style['color'] = [ 'classname' => 'crimson'  ]; 

// Regular style
$style['color'] = '#fff'; // or  [ 'value' => '#fff'  ];

The reason why I left it at two methods was so we could indeed have separate methods that we could ultimately call in a single generate.

So maybe the current generate should rather be named get_styles or whatever, and we create a new generate that calls get_styles or get_classnames accordingly. 🤷

What do you reckon 😄 ?

[andrewserong]

Oh, that is an interesting challenge! Thanks for explaining the logic there, and apologies when I was reading this yesterday, I totally missed that the following lines are grabbing root level attributes, not values within the style object 😅:

$classes['fontSize'] = _wp_to_kebab_case( $block_attributes['fontSize'] );
$classes['fontFamily'] = _wp_to_kebab_case( $block_attributes['fontFamily'] );

I think we're now bumping into one of the tricky areas where there's a part of the feature here that we probably need to decide between "do we change how it works now, or later?".

As in the border PR (#37770) I think there's been discussion in various places about whether these root level attributes will (long-term) better belong within the style object. Ideally, we'd have everything we need right there in the style object, whether that style object is coming from block attributes, or global styles.

However, we're not there yet!

So in the spirit of the approach we've been taking thus far, which is — let's see if we can refactor what's currently working (pretty much as-is) and then iteratively switch over to new approaches — I reckon we go in the direction you've been exploring 👍

We could always expand the options array. I did try that but wasn't satisfied with my approach as it appeared haphazard and unintuitive, and not related at all to the main argument $block_styles.

You're right, it's tricky to come up with something that feels intuitive. I was kinda wondering if we could nearly have a value in the options array called attributes or additionalAttributes or something, which is a way to pass in attributes that are not part of the style object — but that could feel a bit awkward, too...

Basically, I was thinking of something like:

$style_engine->generate(
		array( 'typography' => $styles ),
		array(
			'inline' => true,
			'attributes' => array(
				'fontSize' => 'large', // just the raw attribute, let the style engine do the kebab-ing?
				'fontFamily' => 'papyrus'
			)
		)
	);

But I agree, it does feel awkward including those attributes separately in the list of options, so it might not be the right approach...

Another thought I had was to extend the style value with metadata, which would mean the style engine had its own implementation of $block_styles

That's actually not such a bad idea 🤔😀 — if we think of this as an opportunity to come up with the structure of how we long-term hope to structure the style object so that it contains those root level attributes, if we go with this approach, it might be a good way to confidence check what that shape will eventually look like.

It could also mean we kinda have an implementation which is "if the values are all in the style object, we don't need to look for anything else, otherwise, migrate over those other attributes to the style object for backwards compat" — that way we might be able to handle the desired location in the style object without ever having to worry about migrating old attributes? Just a thought....

The reason why I left it at two methods was so we could indeed have separate methods that we could ultimately call in a single generate.
So maybe the current generate should rather be named get_styles or whatever, and we create a new generate that calls get_styles or get_classnames accordingly.

I really like this idea, it means we've got two inner methods that are doing different things, and we have generate as an abstraction that sits just above, so we can have a single entry point to generating styles 👍

[andrewserong]

Does this need to be an option, or could we infer it based on the preference within BLOCK_STYLE_DEFINITIONS_METADATA? Because $options is set for the overall call to generate (or get_classnames in this case) I was wondering if use_schema is a little too broad, in that each property / support might need a different approach?

Or did you have in mind a use case where we really do want to be able to globally switch between the two approaches when calling the style engine? 🤔

[ramonjd]

Good question.

We don't need this right now, true. I was just playing with an idea of being able to pass a custom schema to the style engine for styles that might not be defined in BLOCK_STYLE_DEFINITIONS_METADATA , like layout container classes wp-%s.

$options['use_schema'] is fairly generic, but it was supposed to mean "use the pattern/schema stored in the style engine if there is one".

I'm not really sure to be honest.

It's not needed for this PR, so I'll probably dump it until we have a real-world use case.

[andrewserong]

I was wondering if it'd be worth moving this call to convert the value to kebab case to the style engine, too, so that the caller doesn't have to worry about the detail of how the class name is ultimately constructed, and the style engine can be responsible for that?

What I have in mind, is whether it'd be feasible for the style engine to infer the class names based on the attributes passed in via the style object handed to generate, instead of having to call get_classnames separately with the $classes value 🤔

[ramonjd]

convert the value to kebab case to the style engine, too, so that the caller doesn't have to worry about the detail of how the class name is ultimately constructed

It probably should, but would it need to if it's inferring classnames and building them "in house" based on attributes alone?

The tricky part of the puzzle here is building classnames based on existing patterns. For typography only font size received the kebab case treatment. That's the reason why I left it to the block support to pass the right slug, and kept the style engine "dumb" in that regard.

I can't think of any reason not to do this - in fact I agree that we definitely should ensure that all classnames are consistent in that way - however if ever a block/block support didn't want kebab case (an extreme edge case) and something else, we might have painted ourselves into a corner too early.

Once again, I'm not sure. Maybe we'll know as we add more supports to the mix.

[andrewserong]

Once again, I'm not sure. Maybe we'll know as we add more supports to the mix.

Good points! I think if we're not sure, then perhaps let's leave it in the block support so that we haven't painted ourselves into a corner, like you say. We could always move it over in a follow-up, if need be.

[ramonjd]

Alternative that includes typography, but takes into account classname generation for colors here:

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

[ramonjd]

I think I'll close this in favour of

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

Since that PR takes classnames/presets into account.