Try: Reimplement Block Supports #26111
Conversation
mcsf
added
Framework
|
Size Change: -835 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
mcsf
requested review from
ajitbohra,
Soean,
spacedmonkey,
talldan and
TimothyBJacobs
as code owners
| /** | ||
| * Class encapsulating and implementing Block Supports. | ||
| */ | ||
| class WP_Block_Supports { |
There was a problem hiding this comment.
This will become a public API, we should probably make it private.
|
|
||
| $classes = ''; | ||
| if ( $has_line_height ) { | ||
| $generated_class_name = uniqid( 'wp-block-lineheight-' ); |
There was a problem hiding this comment.
It seems with this PR you're also trying to change how we generate the custom styles in the server. While this is probably a sane approach, IMO, we should stick to the current way for the moment and do this in its own PR. Basically just keep the inline styles for now.
There was a problem hiding this comment.
which also mean wp-block-supports style shouldn't be needed yet.
There was a problem hiding this comment.
Agreed. That’s a remnant from when only class was supported.
There was a problem hiding this comment.
As an aside if we try to generate inline style on the server. I think generating a class name per block instance and then add the styles of that block to that class name is not ideal.
I think we would have the opportunity to use atomic CSS ( each class/selector contains just one rule). Per CSS style rule we e.g: "line-height: 5px" we would have a class e.g: ".Ab { line-height: 5px }". Each block that has a line-height 5px would simply refer to that class. Our preset classes are already atomic so in the case of presets instead of generating a random class, we would simply refer to a human-readable class.
Implementing this is not complex we would have a global dictonaty that maps styles to classes e.g:
array(
'line-height' => array(
'5px' => 'ab'
)
);
and a counter:
$current_class_count = 3;
Each time a style has a class we would simply include that class. If no class was defined we would encode the counter in Base36 (uses A-Z and 0-9) update the mapper to map to that class and increment the counter.
Atomic CSS is deployed in production at Facebook and Twitter and seems to reduce the amount of CSS required.
| /** | ||
| * Constructs the configuration for all supported block-supports features. | ||
| */ | ||
| private function load_config() { |
There was a problem hiding this comment.
I don't really like this giant config here, for me each one of these should be its own file. it doesn't have to be a public API (can use a private one if the class is private) to be the case.
| * | ||
| * @return string String of HTML classes. | ||
| */ | ||
| function gutenberg_block_supports_classes() { |
There was a problem hiding this comment.
I think we should try to get this as close as possible to useBlockProps so what about something like gutenberg_get_block_props or gutenberg_get_block_wrapper_attributes, something generic like that.
There was a problem hiding this comment.
Also, should it takes extra props as arguments and merge theme like useBlockProps does.
There was a problem hiding this comment.
So would it look like this?
// latest-posts/index.php
if ( isset( $attributes ... ) $class .= ' has-dates';
if ( isset( $attributes ... ) $class .= ' has-author';
return sprintf(
'<ul %s>%s</ul>',
gutenberg_get_block_wrapper_attributes( array( 'class' => $class ) ),
$list_items_markup
);I'm not opposed :)
There was a problem hiding this comment.
So essentially it would receive an array-like
gutenberg_get_block_props( array(
'style' => 'border-color: #f00',
'class' => 'has-border'
) );
And return an array with new props that should be added as attributes?
I guess ideally blocks would never add attributes to the block top-level node and always use our function that merges the attributes.
We may also have a function gutenberg_get_block_props_output that returns the attributes HTML definion of the attributes "style="..." class="..." given an array of attributes.
I think a path like that would make sense.
There was a problem hiding this comment.
If we continue with the current approach gutenberg_block_supports_classes what is the plan for the supports that add styles? Is having a gutenberg_block_supports_styles function the next step?
There was a problem hiding this comment.
If we continue with the current approach gutenberg_block_supports_classes what is the plan for the supports that add styles? Is having a gutenberg_block_supports_styles function the next step?
Right, I don't think that's a good solution.
The initial focus on classes came from wanting to see if restricting Block Supports to outputting only classes could be enough, but it's clear we should open up to more attributes.
| } | ||
|
|
||
| $output = array(); | ||
| foreach ( $this->config as $feature_name => $feature_config ) { |
There was a problem hiding this comment.
Maybe we could add some comment here specifying what this loop is doing, at first sight, it is not easy to interpret.
| * | ||
| * @return string String of HTML classes. | ||
| */ | ||
| function gutenberg_block_supports_classes() { |
There was a problem hiding this comment.
So essentially it would receive an array-like
gutenberg_get_block_props( array(
'style' => 'border-color: #f00',
'class' => 'has-border'
) );
And return an array with new props that should be added as attributes?
I guess ideally blocks would never add attributes to the block top-level node and always use our function that merges the attributes.
We may also have a function gutenberg_get_block_props_output that returns the attributes HTML definion of the attributes "style="..." class="..." given an array of attributes.
I think a path like that would make sense.
| @@ -18,7 +18,14 @@ function render_block_core_cover( $attributes, $content ) { | |||
| return str_replace( 'autoplay muted', 'autoplay muted playsinline', $content ); | |||
| } | |||
|
|
|||
| return $content; | |||
| // TODO: this is just a proof of concept, not a proper solution. | |||
| $class = gutenberg_block_supports_classes(); | |||
There was a problem hiding this comment.
I guess we don't need this for cover, as the supports are already applied on the client because on that block we have a proper static markup (the dynamic aspect is just a hack for something very specific that should be removed).
| ), | ||
| ), | ||
| 'callback' => function( $attributes, $block_name ) { | ||
| die( 42 ); |
There was a problem hiding this comment.
Should we remove this die call :)?
|
|
||
| $classes = ''; | ||
| if ( $has_line_height ) { | ||
| $generated_class_name = uniqid( 'wp-block-lineheight-' ); |
There was a problem hiding this comment.
As an aside if we try to generate inline style on the server. I think generating a class name per block instance and then add the styles of that block to that class name is not ideal.
I think we would have the opportunity to use atomic CSS ( each class/selector contains just one rule). Per CSS style rule we e.g: "line-height: 5px" we would have a class e.g: ".Ab { line-height: 5px }". Each block that has a line-height 5px would simply refer to that class. Our preset classes are already atomic so in the case of presets instead of generating a random class, we would simply refer to a human-readable class.
Implementing this is not complex we would have a global dictonaty that maps styles to classes e.g:
array(
'line-height' => array(
'5px' => 'ab'
)
);
and a counter:
$current_class_count = 3;
Each time a style has a class we would simply include that class. If no class was defined we would encode the counter in Base36 (uses A-Z and 0-9) update the mapper to map to that class and increment the counter.
Atomic CSS is deployed in production at Facebook and Twitter and seems to reduce the amount of CSS required.
| @@ -171,6 +171,8 @@ function render_block_core_latest_posts( $attributes ) { | |||
| $class .= ' has-author'; | |||
| } | |||
|
|
|||
| $class .= ' begin-explicitly-added ' . gutenberg_block_supports_classes() . ' end-explicitly-added'; | |||
There was a problem hiding this comment.
begin-explicitly-added and end-explicitly-added are test classes right? Should be removed before the merge.
There was a problem hiding this comment.
Yeah, all changes in block-library were for testing purposes. :)
| @@ -391,6 +399,8 @@ function gutenberg_render_block_with_assigned_block_context( $pre_render, $parse | |||
| return $pre_render; | |||
| } | |||
|
|
|||
| $current_parsed_block = $parsed_block; | |||
There was a problem hiding this comment.
We never know how this global will end up being used, a way to avoid having the global would be having a current_parsed_block private variable as part of WP_Block_Supports instance and a letter that allows setting the variable. Here instead of setting the variable, we would call that setter.
| * | ||
| * @return string String of HTML classes. | ||
| */ | ||
| function gutenberg_block_supports_classes() { |
There was a problem hiding this comment.
If we continue with the current approach gutenberg_block_supports_classes what is the plan for the supports that add styles? Is having a gutenberg_block_supports_styles function the next step?
| @@ -53,6 +53,9 @@ | |||
| "supports": { | |||
| "anchor": true, | |||
| "align": true, | |||
| "color": { | |||
There was a problem hiding this comment.
The cover block does not use the supports mechanism for colors because it has some specificities (overlay) with this change the cover block has two color control UI's.
There was a problem hiding this comment.
Yep yep, not a real change. 👌
|
Superseded by #26192. |
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
Feel free to skip to § Description if you are familiar with the problem.
Superseded by #26192.
Background
From the moment that Block Supports started to parse block content as HTML and manipulate it, there have been issues around encoding and escaping (e.g. missing UTF-8 conversion, lossy HTML entities, double-encoding) and around the HTML handling proper (error handling, DOM traversal, PHP API inconsistencies).
The issues themselves have been fixed, and with enough testing we expect the parsing of HTML to become sufficiently stable.
That said, it has become clear that we need to take a wider look at how Block Supports works on the server, not just as a piece that transforms HTML, but as a sustainable API for core and third-party blocks, and as a part of a larger pipeline that enhances saved blocks alongside Global Styles. This pipeline is critical to the rendering of pages in WordPress.
Goals
What Block Supports is: A means to absorb the complexities of common block tools — e.g. support for alignment, colours controls — such that a block just needs to state “I support alignment” for Gutenberg to provide editing controls and front-end results. It means that core and third-party blocks alike don’t need to individually implement all the pieces that go into offering custom alignment, custom colour or custom typography settings.
What Block Supports isn’t: a system that goes beyond that. It should take a block along with its alignment / colour / whatever attributes and render accordingly. It shouldn’t be capable of executing business logic, reconciling values, inspecting blocks more deeply, or in general have sophisticated awareness of blocks.
Keeping the latter in mind is key to meaningfully analyse current fragilities.
Fragilities
Handling HTML on the server
As hinted in § Background, PHP can deal with HTML, but it’s a delicate operation. It’s no accident that most of WordPress has traditionally handled content as strings throughout its components: the tradeoff is less power in exchange for performance and safety. Thus, our utmost diligence is required when we set out to introduce a system that processes a new HTML document on every
render_block.Handling blocks on the server
Building from the above, we also need to acknowledge the limitations of how much the server can understand blocks. A recent example:
#24300 sought to make Block Supports honour attribute defaults according to a block’s
block.jsonschema. This is reasonable for simple attributes, but breaks as soon as we consider sourced attributes — which the server can’t handle — in which case the server has no idea whether to fetch the default value. The way forward is unclear.(Despite advances brought forward by the
block.jsoninterface, the server’s inability to parse sourced attributes remains a long-standing issue. The closest we got was this experiment of Andrew’s — and even then the solution requires more HTML parsing.)However, even if we had the necessary server-side support, the issue runs deeper: everything dealing with block validation, deprecation and migration is implemented solely on the client. And it should likely remain so: the idea has all along been that the client is meant to perform the heavy lifting of understanding blocks and making decisions based on that understanding; it can then produce the best output to be saved and rendered across environments (front end/editor, Web/Mobile, etc.).
Indirection and excessive looping
Currently, the architecture of Block Supports requires looping over all known block types at
init, and again looping for each block in the content and for each knownsupportsfeature atrender_block, to finally compute the necessary classes and styles to inject in the block (powered byDOMDocumentfor parsing and serialisation).If the server is inherently limited — in ability and resources — in what it can do with blocks, we ought to be explicit about what and how much should be left up to the server.
Meanwhile, the recent adoption of
apiVersion: 2as a signal for “light blocks” means that we could make it a requirement that all blocks wishing to declaresupportsalso conform toapiVersion: 2— in the process also avoiding any issues of backwards compatibility. Edit: #26106 was just opened to purse this idea.Description
This solution rests on the following ideas:
DOMDocumentand instead — by taking advantage of the requirement that blocks conform toapiVersion: 2— make strong assumptions about the shape of block content and limit content manipulation to the injection of classes and inline styles in the root element.gutenberg_block_supports_classes, that block authors can use if they have dynamic blocks and need to explicitly state where their classes should go:To do
wp_add_inline_stylesuseBlockProps.savecolor.backgroundcolor.gradientscolor.linkHow has this been tested?
Screenshots
Types of changes
Checklist: