search

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.57k stars 4.22k forks source link

Tracking: Add a Style Engine to manage rendering block styles #38167

Closed andrewserong closed 1 year ago

andrewserong commented 2 years ago

This is a tracking issue for implementing a Style Engine as discussed in improving our saving/rendering of block styles. The goal is to have a consistent API for rendering styling for blocks, across both client-side and server-side applications. The server implementation is preferred for a site's frontend.

Rather than a monolithic refactor, the aim should be to build the smallest implementation possible and land it in the plugin, and iteratively enhance it with small PRs as we go.

In principle, the style engine should be able to receive a style object, and return the rendered styling for that style object, along with any required class names or other data needed to reassemble or use those styles in all required environments. Depending on the implementation, it may be possible for us to avoid or minimise the current reliance on rendering inline styles directly into block markup.

⭐ Current phase - Phase 1: block styles consolidation and refactoring the layout abstraction

Goals:

Planning discussions

To-do

The below to-do list is an assortment of shorter-term to longer-term features that would be great to implement. We should aim to not let the longer-term goals block landing the shorter-term goals — but let's try to use the longer-term goals to inform the design decisions for the shorter-term ones.

The approach will be work iteratively rather than treating this as a project that needs to be complete all at once.

Project board

We've started a project board for managing the above tasks over in: https://github.com/orgs/WordPress/projects/19/views/1

Relevant discussions

ramonjd commented 2 years ago

Add support for de-duping common or repeated styles so that there are no duplicate styles rendered.

We already use cssnano, which can do a lot of this stuff.

From basic testing, it seems like if we stored the styles (in a store?) and then ran them concatenated through cssnano we might get something useful. I can take a look at this further.

youknowriad commented 2 years ago

Refactor layout support to use the styles engine. Refactor blockGap support to use the styles engine.

These two styles are different than the others in the sense that the generated styles shouldn't be applied to the "block wrapper" but instead to the "inner blocks wrapper". The difference is subtle since for most containers blocks, these two wrappers are the same but for something like Cover, they are not the same and that's why it's not possible to enable the current implementation of Layout bock supports for that block. (we need to first refactor the block support to apply to the inner blocks wrapper). I think that's something important to have in my mind while iterating on the style engine to avoid repeating the same mistake we have now.

andrewserong commented 2 years ago

I believe the initial version of the style engine PR (https://github.com/WordPress/gutenberg/pull/37978) is in a good place for wider feedback / review now, and to confidence check the approach. I've added a tiny comment/update to the PR.

andrewserong commented 2 years ago

Just adding that there's been some discussion in https://github.com/WordPress/gutenberg/issues/38277 and https://github.com/WordPress/gutenberg/issues/27075 about exploring a standardized way to modify hover/focus/active states for blocks — it'd be good to keep an eye on these discussions to see if/when/how the style engine might slot into those explorations.

gziolo commented 2 years ago

It looks like the automatically generated class names to move granular class names and styles outside of the block's HTML to the style tag introduce some challenges. See #38719 and #38694. It would be great to keep that on the radar while working on the Style Engine implementation.

There are vast benefits of the single automatically generated class name for a given block:

However, it would be great to keep the flexibility of the current blocks that use several discrete class names to indicate selected block customizations like colors, fonts, etc.

andrewserong commented 2 years ago

Update: the initial version of the style engine package has been merged in #37978, so we're now in a good place to start opening up smaller PRs to explore some of the ideas raised in discussions. I'm keen to have a play with how the server-rendering implementation might look.

Thanks @gziolo for linking in those related discussions. And good points about pollution of the HTML and targeting inner HTML elements. Since we're moving in a direction where most of the styles will ultimately be rendered server-side, hopefully that means if we expose discrete class names, it'll be fairly safe, because we'll be bypassing the issues of block validation and deprecations in post content.

Let's look at these issues in more depth in the discussions and related PRs.

fwazeter commented 2 years ago

Glad to have found this tracking issue! Thanks @ramonjd for linking it! Which channel in slack is this stuff discussed in? Been looking for months for discussion around these issues!

Developed a CSS API via plugin to handle this exactly - taking sources from global styles, a CSS file (re-tooling some old PEAR PHP package for parsing CSS from css files), CPT etc, universalizes it then renders the styles inline, which was originally meant as a way to override the implementation of Layout Support (since we've been using FSE in production sites since 5.8 launched with the Gutenberg plugin).

I'd be happy to migrate the PHP classes to WP's coding standard (I've been writing in PSR style and tend to make smaller classes split up among files using interfaces & setter injection via containers) to get the ball rolling on a PHP side implementation.

In the plugin we made, we've been using a CPT as a data store for values, following the logic behind block-templates & navigation block. The flow is a little like this:

  1. files from a repository class are sent to the CSS parser (e.g. from core blocks, fromAPI (wp_get_global_stylesheet), from CPT (e.g. internal), from JSON.

  2. Parser outputs CSS into declaration block associative arrays, e.g. 

    "declaration_block" => array(
    "property" => "value"
    "property" => "value"
    ...

    Where the declaration block acts as the selector name.

  3. Copy of these are saved as a stylesheet CPT with the corresponding declaration blocks

  4. Stylesheet(s) are enqueued and rendered as inline styles.

  5. for Layout Support, if block attr doesn't exist for (contentSize, wideSize, flex props, blockGap etc), then it creates a new CSS declaration block matching user input with a name suffix like __600px. If the style already exists in the stylesheet(s), rather than creating a new property, it simply uses the existing property.

  6. adds the class to the block that has the support flag.

This is also done for overriding core block styles to deregister and reregister new default block styles based on user / developer preference, which is the main reason for the CPT - the CPT provides an easy front end interface to 'hot swap' values on the defaults.

So, for example, if I wanted to change all 'margin-left' properties of all blocks & auto generated styles (e.g. from layout support), to be margin-inline-start, then I can just change a single setting and the above API does all the work for me, replacing the corresponding properties, but not their values - or vice versa.

What we're also experimenting with is, rather than using a new class name for each user defined custom value (which doesn't happen often - 99% of the time a block using contentSize and wideSize is just using the theme default, we've literally overrode this with a custom value once across ~13 production sites in the wild.

For instance, if you assign contentSize and wideSize (or a flex property or whatever) to a custom property, and then add a css style that says "#make200" that sets the value of the customproperty to something different, then it'll change the custom prop just for that unique id - eliminating the need for a new class altogether.

<style>
--wp--style--content-size: 800px;

.wp-container-default {
   max-inline-size: var(--wp--style--content-size);
}

#make200 {
--wp--style--content-size: 200px;
}
</style>

<div class="wp-container-default">
     <p>My container is 800px wide</p>
</div>

<div class="wp-container-default" id="#make200">
     <p>My container is 200px wide</p>
</div>

Ultimately, the trickiest / most verbose part comes in expanding flex properties and a custom user setting, and so far the cleanest solution has just been to break up the overall declaration block into smaller declarations, but that does make for a long class="" line. In the end, that one just gets tricky if a box has an option to be both say, horizontal and vertical and have a variety of different options - you end up with a large matrix pretty fast.

Adding global / common alignment options for e.g. items-justified-center, etc as already exists seems to be the most prudent there - we just tap into those already declared classes rather than re-rendering them in a separate call.

For cases like row vs column, it's probably just more prudent to keep handling that via block style variants, such as is already done with row block & group block.

We also reduce the overall styling duplication with a couple smart global defaults, like: * { box-sizing: border-box; } and then a hook into entry-content / main the_content tag for something like: 

main > * {
box-sizing: content-box;
padding-inline-start: var(--wp--style--block-gap);
padding-inline-end: var(--wp--style--block-gap);
}

The above allows us to remove any box-sizing property in core blocks, because they by default inherit border-box and inline elements aren't effected. Then the direct children only of the post_content area have padding visibly only on mobile completely preventing the problem of a paragraph block's text being flush with the side of the mobile device.

The slight tweak to .has-background on a paragraph element to make it still flush is like this:

/* in this implementation we were assigning the box-sizing prop to a custom prop in theme.json, with a default to border-box */
p.has-background {
    --wp--custom--box-sizing: content-box;
    box-sizing: var(--wp--custom--box-sizing);
}

This keeps the element in alignment as expected and not doubled up on padding via inheritance.

fwazeter commented 2 years ago

Refactor layout support to use the styles engine. Refactor blockGap support to use the styles engine.

These two styles are different than the others in the sense that the generated styles shouldn't be applied to the "block wrapper" but instead to the "inner blocks wrapper". The difference is subtle since for most containers blocks, these two wrappers are the same but for something like Cover, they are not the same and that's why it's not possible to enable the current implementation of Layout bock supports for that block. (we need to first refactor the block support to apply to the inner blocks wrapper). I think that's something important to have in my mind while iterating on the style engine to avoid repeating the same mistake we have now.

Can personally attest to how finicky this subtly is to work with. The cover block overall is simultaneously one of the most useful blocks in the arsenal, but also the hardest to work with and tweak because of it's structure. Part of it's finicky nature is the fact that it's trying to handle so much at once - it's pretty much got n possibilities of configuration.

Current manipulation to specific sizing usually involves stacking it within other container blocks or other container blocks within the cover block to achieve desired widths inside/outside (even stacking covers inside covers!). In the end we made a few custom blocks that separated out that logic, rather than try to contain it all within a single block, and used block patterns to handle the re-use.

I've got thoughts on a few potential work arounds, but will have to consolidate thoughts on them later.

andrewserong commented 2 years ago

Which channel in slack is this stuff discussed in?

Thanks for sharing your thoughts here! Just adding that most of the discussion is happening async in Github, which works nicely for a lot of us since there's such a wide timezone spread of people contributing to the discussions. (Many of which began in this discussion thread). I've linked a few of the discussions and issues in the issue description. If there are issues or discussions that anyone thinks should be included here, please feel free to link to this tracking issue and we can update the description so that they're all gathered together.

mrwweb commented 2 years ago

I just opened a ticket that overlaps with many of the things being discussed here on #38998. As it relates to this ticket, I hope this can call attention to the needs of theme and plugin developers for core markups and styles to be consistent and easy to customize, both through standardized conventions in WP and with custom CSS.

gziolo commented 2 years ago

As reported in #38917 by @oldrup, after upgrading to WordPress 5.9, sites have issues with HTML validation: "Error: Element style not allowed as child of element body in this context". These errors seem to be related to the styling of blocks when .wp-container-* class is injected in the HTML. It's something that should be included in the planning of the tasks for the style engine.

ramonjd commented 2 years ago

after upgrading to WordPress 5.9, sites have issues with HTML validation: "Error: Element style not allowed as child of element body in this context".

Thanks for the linking that issue. I think it's something we should prioritize, not only to have the satisfaction of receiving digital praise from online validators, but to avoid errors in browsers we haven't tested.

andrewserong commented 2 years ago

Update (2022/03/22):

A quick status update: we're currently exploring options for how the style engine might be implemented on the server. While the initial style engine PR landed a simple approach focusing on just one of the existing block supports within the editor (padding), we've been keen to explore some ideas at the other end of the spectrum and look at how the layout support might be implemented, supported by a PHP implementation of the style engine. The goal is to learn a bit more about the direction we'd ultimately like to go in for a more complex use case, to ensure that what we build will get us to the improvements we've been hoping to make.

Relevant PRs (these are still very much WIP and likely to change quite a bit as we continue to experiment):

The goal is to settle on a structure for the PHP implementation that we feel comfortable we can continue to extend and build on in subsequent PRs. It's still quite exploratory, but at this stage it looks like #39446 is the PR that will most likely be first to be in a mergeable state!

cbirdsong commented 2 years ago

Question – is the end goal here to generate all styles dynamically and apply them using inline <style> blocks instead of shipping any normal .css files?

slr1979 commented 2 years ago

@cbirdsong @andrewserong

Question – is the end goal here to generate all styles dynamically and apply them using inline <style> blocks instead of shipping any normal .css files?

This is also something I would like to know as this would most likely satisfy asynchronous page loading as per my issue https://github.com/WordPress/gutenberg/issues/39054. Duotone in WP >= 5.8 worked fine with my asynchronous dev site when styles were applied inline. I had to backport wp_render_duotone_support() in 5.8 to 5.9 to get duotone to work in my asynchronous dev site.

ramonjd commented 2 years ago

is the end goal here to generate all styles dynamically and apply them using inline