Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an inline styles handler #41896

Closed
wants to merge 8 commits into from
Closed

Conversation

aristath
Copy link
Member

@aristath aristath commented Jun 23, 2022

What?

Adds an inline-styles handler to improve & optimize the way we handle our styles.

Why?

There has been an increase in the inline styles we add on the frontend; We add styles for global-styles, layouts, blocks styles, the list goes on.
This PR aims to improve the styles we add, by combining them and optimizing them. This can help improve the performance, as well as sustainability of our implementation on the frontend. 🌍 🌳

How?

Adds a WP_Inline_Styles_Handler class. Instead of printing a bunch of different <style> elements, we can use that object to add all styles to a "pool" of styles that can then be combined and printed all at once.
The class currently includes the following optimizations:

  • Styles are added as an array. The result of that is that we don't have duplicate rules, since the array has the form [selector => [ rule => value ] ].
  • If multiple selectors have identical styles, then they are combined. So this: .selector1 { color: red; } .selector2 { color: red; } gets transformed to this: .selector1,.selector2{color:red;}.

The PR in its current form adds the PHP class, and uses it in the functions for layout styles. The result is extremely positive: Before the PR, the twentytwentyone theme adds ~12k of inline styles for layouts. After the PR, the size of those styles gets reduced to ~6k. That means that we can shave multiple kilobytes of data loaded for 40% of the web... well worth it.

Though this PR only uses the class in the layouts functions, the more we use it the more it can optimize the styles. So we can in a future PR implement it for global styles as well as other block-supports that add inline styles, as part of our global-styles engine.

Testing Instructions

Use any block theme and ensure that the frontend looks the same, while the amount of inlined styles is reduced.

@aristath aristath requested review from mtias, andrewserong and a team June 23, 2022 08:51
@aristath aristath requested a review from geriux as a code owner June 23, 2022 10:27
@aristath
Copy link
Member Author

Update: Added methods to parse CSS, implemented it in the gutenberg_render_elements_support function, and the gallery block which was adding some styles.

@Mamaduka Mamaduka requested a review from ramonjd June 23, 2022 10:32
@Mamaduka
Copy link
Member

@aristath, is this new class part of the Styles Engine?

@aristath
Copy link
Member Author

@aristath, is this new class part of the Styles Engine?

I'm hoping it can be 😄 IMO it would make sense to have an object that collects and then prints styles on the frontend... I don't think it's currently on the board, but it can simplify our processes a lot 👍

@andrewserong
Copy link
Contributor

This is very cool @aristath, nice exploration! I particularly like the optimisation of grouping rules by selector. I'll try to group my feedback / thoughts into separate sections, as this touches on a few different areas.

Layout support

Over in #40875, I'm working on a refactor for the Layout support that centralises Layout definitions and base styles that are output for the different layout types. This moves rendering for common styles like display: flex to the global styles (theme JSON) class, so that we only have a single display: flex rule attached to .is-layout-flex instead of having to output it for each container class name.

My hope, if that PR looks viable (there's still a bit more work to do), is that we can then do a follow-up to replace the remaining attributes of Layout that are of a controlled set (e.g. content justification) with a presets-like approach (#36944) that similarly involves only a single rule being output in global styles. Then we can use semantic classnames to link these styles to the individual block at render time.

The result would be that we'd drastically reduce the need to output id-based container classnames for the individual blocks that opt-in to the layout support at render time. However, we'd still need to output those styles for the case of a unique blockGap or content size value, so in that instance, using a mechanism like in this PR would still be very useful to ensure that the styles that are output are optimised.

My preference (and I know I'm quite biased here!) would be to avoid refactoring the output of the styles inside the Layout support until we have the centralised and semantic classname based layouts in place, since it's likely we'll be stripping out a fair bit of that style generation anyway. Please let me know if anything here doesn't make sense — the PR I'm working in is quite complex.

In the main, though, I think the ideas are very compatible. It'd be something like:

  • Centralise Layout style output and use semantic classnames.
  • For those styles that are not already covered in the above step, ensure they're optimised with the approach in this PR

@ramonjd just published a blog post going into a bit more of the thinking behind how we can prioritise some of these tasks: https://make.wordpress.org/core/2022/06/24/block-editor-styles-initiatives-and-goals/

Style engine

I think ideally, the logic here could form part of the style engine class, rather than being a separate class on its own? Part of the feedback that @youknowriad gave on many of the style engine PRs was to try to ensure that we have a single point of entry / simple API that we use outside of the style engine itself, so I was wondering if we can potentially consolidate the logic in methods of the Style engine instead of using a separate class?

Naming

At first I thought "inline styles" was referring to inline style attributes of blocks and HTML elements, which is another issue that's been flagged. A future task is to look into refactoring how individual block supports like color, border, etc work, and to potentially output their styles in a style tag instead of being inlined.

The proposal in this PR refers to style tags that are inlined into the HTML output of the page at render time, which is still a valid use of the word "inline" 😄, but caused me a tiny bit of confusion when I first started looking at the PR to work out what its intent was. Is there another name that more closely targets what we're aiming to do here, that focuses more on style rule grouping/optimisation?


Overall, though, I think the grouping and optimisations here are a terrific idea — most of my concerns are surrounding 1) where do we put the optimisation logic in relation to the style engine class (and how is it called), and 2) in what order should be attempt to refactor how styles are output in the Layout block support. 🙂

@ramonjd
Copy link
Member

ramonjd commented Jun 24, 2022

Thanks for dedicating some 🧠 time to this @aristath and for the comprehensive explanations @andrewserong

I definitely 💟 where you're going with https://github.com/WordPress/gutenberg/pull/41896/files#diff-33a205b1a9a2d2f4e7668887fe48010d35fd8201d0e6962c93950136036d1198

As Andrew mentions, it's be fab to have the render functionality as part of the style engine1 suite of methods. The improvements you've made here are on the radar so it'd be great to have things centralized. I've been splashing about the kiddy pool over in #41424, which still needs a lot of architectural improvements.

What I'm trying to do is, I think, similar to your line of reasoning:

  • storing styles in a "pool" so we can combine, process and print them together rather than have CSS in separate style tags in the HTML
  • abstracting rendering methods (e.g., generating inline styles destined for a element attribute, or an entire CSS rule) that can be used anywhere in the application (but mostly by the style engine)

The combine selectors idea is 100% something we'd want to do, so I'm excited to see the example you have here.

Another consideration is how we can store separate "layers" of the style hierarchy in order to have control over how and where we render styles to the page. For example, ultimately it'd be great to organize global-styles, layouts, blocks styles into groups so we can control the order in which they're processed and rendered to the page etc.

Initially the target is block supports, so consolidating layouts, elements and other sources of multiple style tags. I imagine we could branch out to global styles and others once we have something stable.

What do you think the best way forward would be? I'd like to keep up the momentum on this one.

I'm not wedded to #41424 - it's an experiment so far - so if we could somehow combine the good parts of each...

Footnotes

  1. The term "style engine" has become an unfortunate catch all phrase, but in this instance I think it makes sense for it to assume the responsibility of generating and enqueuing common styles

@aristath
Copy link
Member Author

aristath commented Jun 24, 2022

Thank you all for the feedback!

@andrewserong

My preference (and I know I'm quite biased here!) would be to avoid refactoring the output of the styles inside the Layout support until we have the centralised and semantic classname based layouts in place, since it's likely we'll be stripping out a fair bit of that style generation anyway. Please let me know if anything here doesn't make sense — the PR I'm working in is quite complex.

Regarding the changes in the layout supports: In this PR I used more as an example of how we can use the object, and a "study case" to determine if it improves things and what kind of impact it can have.
I love what you're doing over at #40875, and I agree... The 2 concepts are very compatible, and complementary.

I think ideally, the logic here could form part of the style engine class, rather than being a separate class on its own? Part of the feedback that @youknowriad gave on many of the style engine PRs was to try to ensure that we have a single point of entry / simple API that we use outside of the style engine itself, so I was wondering if we can potentially consolidate the logic in methods of the Style engine instead of using a separate class?

Makes perfect sense to me 👍

@ramonjd the main issue I currently see with the style engine, is the way we add data: We add strings like for example

.wp-elements-6d26dce62e274fbec11380003187995e a {
    color: var(--wp--preset--color--pale-pink);
}

Adding styles in this form, makes it difficult to process, combine and optimise styles...
Ideally, we'd be adding the same data as an array:

[
    '.wp-elements-6d26dce62e274fbec11380003187995e a' => [
        'color' => 'var(--wp--preset--color--pale-pink)',
    ]
]

If we add data this way, then we can combine selectors, avoid duplicate rules (in an array you can't have color defined twice), process and optimise styles however we want. Of course we can add plain CSS and then parse/convert it to an array so we can perform our optimisations (which is something I'm also doing in the class I added here), but that's wasteful and in most cases unnecessary if we add the data right to begin with.

I like where #41424 is going... the idea of a separate rendered and a store for styles is great!
We can probably port all the tweaks in this PR over to #41424 (or make a subsequent PR) inside the store & renderer classes 🤔
I'll take a look at the implementation and see if we can combine our efforts.

@ramonjd
Copy link
Member

ramonjd commented Jun 24, 2022

Adding styles in this form, makes it difficult to process, combine and optimise styles... Ideally, we'd be adding the same data as an array:

Totally!

I actually think we're on the same page about how to register styles. It's similar to older experiments, and what I was trying to do over in that other PR.

As you say, the objective is to combine and optimize.

The style engine returns a styles string now because it's first job is to take over the work that current block support files etc are doing: essentially replicating existing functionality until we can centralize it all and do precisely what you're proposing: register, store, process, win 🎉 !

@aristath
Copy link
Member Author

Closing this one since it will have to be combined and embedded in the Renderer class we'll be adding to the Style Engine (see #42463)

@aristath aristath closed this Jul 15, 2022
@ramonjd ramonjd added the [Package] Style Engine /packages/style-engine label Jul 15, 2022
@aristath aristath self-assigned this Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Style Engine /packages/style-engine
Projects
Status: 🗄 Closed / not merged
Development

Successfully merging this pull request may close these issues.

4 participants