Move defining style-engine definitions meta to block-supports

[aristath]

What?

Moves defining the BLOCK_STYLE_DEFINITIONS_METADATA outside the styles engine, and in the respective block-supports.

Why?

• Separation of concerns: Each block-support is responsible for adding its own definitions
• Avoid noise in the styles-engine object
• Extensibility

How?

• Add a new register_block_style_definitions_metadata method in the WP_Style_Engine class.
• Move the arrays of data to their respective block-supports.

Testing Instructions

Screenshots or screencast

[ramonjd]

Hi @aristath

Thanks for putting this PR up, and exploring assumptions about the style engine. 👍

A bit context about the original ideas behind BLOCK_STYLE_DEFINITIONS_METADATA: it was originally intended to be a central library of immutable block style definitions to offer block supports a declarative way to generate block styles.

The idea was to separate and centralize style generation rules so that all the system needed to do was throw a block styles object at it.

A good example of where this is also effective is global styles. In this test PR, we're passing all possible styles at the style engine and getting back what we want. It might be cumbersome if we had to first re-register all the definitions again I think.

True, it's tightly-coupled to Gutenberg for now, but that's intentional while we work on required functionality. Dencentralizing things too much at this point might be counter-productive.

I think I get the motivation behind extracting them, and one day we'd probably want to think about applications outside Gutenberg, but maybe we're optimizing too soon?

I see a couple of alternatives:

1. Maybe we could extract the BLOCK_STYLE_DEFINITIONS_METADATA, and keep it in the style engine package, but still make it extensible using something like register_block_style_definitions_metadata Is there a current use case for this yet though?
2. Or, if we think registering block definitions is really needed right now (I'm not convinced yet), we register them all at once (and only once) somewhere in Gutenberg so that the rules persist for block support styles and global styles and anywhere else we need to generate styles.

Still, I think we could look at these types of optimizations later when the need is there, and after we have something stable.

Insofar as priorities for the style engine, this post outlines what I think we should be working towards right now: https://make.wordpress.org/core/2022/06/24/block-editor-styles-initiatives-and-goals/

So the work we're doing on consolidating, rendering and then registering styles to reduce inline style tags will have a greater impact in the short term.

This is all just me running my mouth! 😄 I'm interested to hear what you and @andrewserong think.

[andrewserong]

Very cool idea @aristath! I really like that the change in this PR gets the style engine a little closer to the JS implementation, where instead of having one big list of metadata, the declarations are situated in a logical place along with the rest of the individual block support. One of the things @ramonjd explored in earlier PRs was the question "With the style engine in place, what's the role of the block support itself?" One of the answers was to validate and pass along the correct piece of the style object, so that the style engine didn't need to worry about that. Expanding that idea to also include defining the metadata structure for that slice of the style object also makes pretty good sense to me.

I think I get the motivation behind extracting them, and one day we'd probably want to think about applications outside Gutenberg, but maybe we're optimizing too soon?

My main hesitancy about doing this refactor at this time is that the block style definitions is still changing shape with each PR that rolls new block supports into the style engine. While the shape of the data is still in flux, I think it might be worth keeping the metadata in the style engine class so that it's easier to change with each subsequent PR? We've still got a couple additional block supports to implement in the style engine before things are consolidated (and then also dealing with how we should hook it into global styles).

Once the Layout refactor PR #40875 lands, and we explore using semantic classnames for most of the remaining attributes of layout, we should be in a good place to work out where blockGap fits for the style engine (and how much the style engine needs to know about layout definitions). Once we've figured out how the Layout support works with the style engine, I think the last block support to figure out is how we want to handle Duotone support? I took a quick look at an outstanding Duotone issue yesterday (#39054) and it doesn't seem immediately obvious to me how the style engine should work with it, so that one might take a bit of exploration to see how it should fit in.

True, it's tightly-coupled to Gutenberg for now, but that's intentional while we work on required functionality. Dencentralizing things too much at this point might be counter-productive.

Yes, I think the primary goal for the project so far has been to consolidate Gutenberg's styles rather than to write a general purpose style engine that could be made extensible. I think it'd be very cool if we can get there, and I think this PR does neaten up the style engine class quite nicely, but my hesitancy is mostly that we've still got a lot of work to do on the initial consolidation.

[github-actions]

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ lib/block-supports/background.php
❔ lib/block-supports/border.php
❔ lib/block-supports/colors.php
❔ lib/block-supports/dimensions.php
❔ lib/block-supports/shadow.php
❔ lib/block-supports/spacing.php
❔ lib/block-supports/typography.php

[aristath]

@andrewserong @ramonjd it's been a long time since we discussed this, and since then some things have changed 😄
I rebased the PR and brought it up to speed with the latest changes in Gutenberg.
Let's see if now is a better time to do this... 👍

[aristath]

Hmmm looks like the style-engine loads from Core instead of the Gutenberg package, and that causes PHP fatalities.
We can't extend the class since it's final, and we can't overload it either.

Does that mean that we can no longer do any kind of work to the style engine in Gutenberg? 🤔

[ramonjd]

Thanks for resuscitating this one @aristath

Hmmm looks like the style-engine loads from Core instead of the Gutenberg package, and that causes PHP fatalities.

That's strange. Maybe something has changed. We set webpack up to rename the classes/functions to _gutenberg etc as they were copied over to the build folder.

I'll take a look later.

[ramonjd]

This branch is building for me, but only when I replace the value_func values in the blocks supports files with 'value_func' => 'WP_Style_Engine_Gutenberg::get_individual_property_css_declarations', or 'value_func' => 'static::get_individual_property_css_declarations', (since self can't be referenced from inside the block support itself)

I like the idea behind this, would it be a lot of work for the consumer to have to keep feeding in the definitions?

The other thing I can think of is when we change the internals (keys etc), we'd have to deal with backwards compat. The style engine is pretty stale and need some love - I'd expect we'd want to leave some elbow room in case supporting globals styles requires an internal refactor. I've been meaning to turn my attention to it this year but other stuff came up 😅

Related to this PR, I've had a long-running idea to allow folks to extend the base style definitions in:

• Style engine: extend block support style definitions #45296

it doesn't allow introducing value_func values though - I'm not sure whether WP should allow running unchecked functions?

[andrewserong]

I like the idea behind this, would it be a lot of work for the consumer to have to keep feeding in the definitions?

I like the idea behind it, too! It does feel nice to me that the registration for the definitions lives alongside the block support. One potential wrinkle in exposing it as an API is that there's no connection between the PHP implementation and the JS one for the style engine. So if folks were to add another style definition, or if we were to allow plugins to unregister style definitions, that would only affect the PHP / server-rendering, and not the output within the block editor. To achieve the goal of extensibility, how might we create consistency there? Or is it okay that the PHP side of things might be a little separate to the JS implementation 🤔

One other question in regards to adding in a public API for registering the style definitions: would it be a problem that the list of properties we can add in needs to be one that's supported by the list of available CSS properties supported by safe_style_css in kses.php (here)?

[aristath]

Thank you @ramonjd for catching that issue with the self - I forgot to change it when copy-pasting 🤦

From what I can tell, these value_func callbacks are only used in those specific block-supports. So we could even move these outside the WP_Style_Engine_Gutenberg and in their respective block-supports files 🤔

The style engine is pretty stale and need some love - I'd expect we'd want to leave some elbow room in case supporting globals styles requires an internal refactor. I've been meaning to turn my attention to it this year but other stuff came up 😅

Yeah, the style engine hasn't reached its full potential and I agree that it needs a lot of love... It does need an internal refactor!

it doesn't allow introducing value_func values though - I'm not sure whether WP should allow running unchecked functions?

WordPress allows defining callbacks in all sorts of places... It wouldn't be the first time we allow extensibility like that. Ultimately the responsibility falls to implementers. If they do bad things, they'll get bad results and there's no way to protect against that. It's better to leave the implementation a bit more open.. 👍

One potential wrinkle in exposing it as an API is that there's no connection between the PHP implementation and the JS one for the style engine. So if folks were to add another style definition, or if we were to allow plugins to unregister style definitions, that would only affect the PHP / server-rendering, and not the output within the block editor. To achieve the goal of extensibility, how might we create consistency there? Or is it okay that the PHP side of things might be a little separate to the JS implementation 🤔

IMO it's fine that they are separate. However, if we want to bring consistency and define these in only 1 place, we could define them in PHP and then pass that data, json-formatted, to the editor.

would it be a problem that the list of properties we can add in needs to be one that's supported by the list of available CSS properties supported by safe_style_css in kses.php

I don't think that would be an issue... The same is true in many other areas in Core - and besides, historically we keep adding properties to the safe_style_css filter whenever it's needed. That list is not set in stone and it keeps evolving with our needs.

---

There are still some failing tests here, but before I commit to fixing them, debugging and making this PR production-ready, I want to make sure we're not wasting our time here. So for now we can ignore them and assume things will be fixed when we make a decision about what we want to do 😅

[andrewserong]

Thanks for the explanations @aristath! I'm curious to hear what @ramonjd thinks, too, but overall I quite like the direction this is going in. Looking at the code change, it feels pretty natural to me that the registrations would all appear at the end of each block support, and other than that the big change appears to be exposing the methods for get_url_or_value_css_declaration and get_individual_property_css_declarations publicly. I think that's likely preferable to moving those callbacks to the block supports files, as there could conceivably be future block supports that might want to make use of those callbacks.

IMO it's fine that they are separate. However, if we want to bring consistency and define these in only 1 place, we could define them in PHP and then pass that data, json-formatted, to the editor.

Good point — I think leaving them as separate for now sounds fine to me, too 🙂

Thanks for persisting with this!

[ramonjd]

Great discussion. I hope 2024 will be the year of the style engine. There are a lot of world problems to solve here 😆

The other thing I can think of is when we change the internals (keys etc), we'd have to deal with backwards compat.

This is the main thing I'd be worried about to be honest. Because we don't yet expose these internals, changing them is trivial while the style engine is still not "feature complete". 🤔

One potential wrinkle in exposing it as an API is that there's no connection between the PHP implementation and the JS one for the style engine. So if folks were to add another style definition, or if we were to allow plugins to unregister style definitions, that would only affect the PHP / server-rendering, and not the output within the block editor

This is a good point. Currently there's no bridge between the JS and PHP implementations, and so far we've been fine with that. If we open things up to hooks/filtering however, I'd suppose consumers would expect them to have universal effect?

I actually really like the following idea:

if we want to bring consistency and define these in only 1 place, we could define them in PHP and then pass that data, json-formatted, to the editor.

Let's say the definitions were pulled out of the WP_Style_Engine class into a stand-alone PHP definitions module/class, and even broken up into separate block supports:

1. The main style engine class wouldn't necessarily need to know about it and the idea behind this PR could be achieved to some extent. For example, the definitions class/whatever would be available to individuals block supports files to be "registered", only that the definitions themselves would still be centralized (and therefore filterable) and easier to maintain.
2. The definitions class/whatever might even enqueue/print a JS object to the browser so the style engine JS could consume the definitions
3. The definitions class/whatever could offer a way to extend the definitions via an API, and take care of its own extensions rather than internally (like Style engine: extend block support style definitions #45296 tries to do)

I'm thinking out my 👖 here. I just don't want to get ahead too much - as @aristath says, there's still a lot of catching up to do to get things to their potential.