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

Use the style-engine in global styles #42893

Open
wants to merge 13 commits into
base: trunk
Choose a base branch
from

Conversation

aristath
Copy link
Member

@aristath aristath commented Aug 2, 2022

What?

Runs the global styles through the style-engine's processor to optimize styles.

Why?

Optimizes the CSS: Removes duplicates, combines selectors where appropriate, sanitizes styles.
The result is a reduction in the size of global-styles without losing anything ♻️ 🌍 🚀
Also implements prettifying the styles when SCRIPT_DEBUG is true to facilitate debugging 🎉

This PR is a simple method to get started with the style-engine inside global styles, without refactoring global-styles which is something that will be done in the future.

How?

This is just the 1st step towards refactoring global-styles to use the style-engine.
Instead of refactoring the whole thing, this PR introduces a method to parse CSS in the WP_Style_Engine_Processor class, and optimize it.

@ramonjd ramonjd added [Type] Experimental Experimental feature or API. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Package] Style Engine /packages/style-engine labels Aug 3, 2022
@aristath aristath force-pushed the try/global-styles-style-engine-2 branch from e8c7e0b to 2ccd54e Compare August 3, 2022 06:48
@aristath aristath marked this pull request as ready for review August 3, 2022 07:49
@aristath aristath self-assigned this Aug 3, 2022
@aristath aristath force-pushed the try/global-styles-style-engine-2 branch from 44eec70 to 6ae51f1 Compare August 4, 2022 06:12
@aristath aristath force-pushed the try/global-styles-style-engine-2 branch from 9f71cae to b8ff47c Compare August 4, 2022 07:46
@aristath aristath force-pushed the try/global-styles-style-engine-2 branch 2 times, most recently from c498a6d to cf6f4e6 Compare August 5, 2022 07:37
@ramonjd
Copy link
Member

ramonjd commented Aug 10, 2022

Thanks for getting this PR up, and apologies for not taking a look sooner.

Global styles is definitely on the roadmap, and we'll have to dive into how we tackle each area in the medium term, so it's great to see ideas already about how we'll move forward.

I can see how it might be useful to be able to run already-compiled CSS through the style engine in general, but I can't shake the impression that we might be putting the "cart before the horse" in this instance.

What benefit does compiling global styles as per normal, exploding the string, then recompiling through the style engine bring?

Running the branch I see that we’re prettifying the output, and in smoke testing I haven't found any regressions between the editor and frontend, but what other benefits are we seeing for existing themes?

Are there any output comparisons or test cases?

Since we processing the entire output through one funnel, I'm concerned that we'd have to also maintain the added abstraction layer as we work through global styles.

On the plus side, it corrals all styles generated for global styles through the style engine, so future enhancements will be nudged in that direction.

I'm not 100% convinced that it will prevent the sorts of changes and customizations we've been seeing in global styles however.

I think it's well accepted that global styles contains too many disparate CSS implementations, and the styles are many and bug-prone.

I can't help but believe that if we address the underlying components of global styles first, we'll be able to better deal with refactoring, bugs and edge cases as they arise.

In addition to the experiments we've done already (I've tried to store them all on the project board), there's scope to:

  • look at theme.json holistically, then identify the various style categories/features, e.g., top-level styles/referencing values via ref, we need to tackle, and
  • isolate these style categories one at a time so that we can generate the CSS using the style engine.

In my opinion, this would be a great place to start once we're satisfied that the work so far is stable and won't make 6.1 explode.

I'd be especially interesting in also using the period after the 6.1 release to take a breath, wait for bug reports 😄 and, more importantly, listen to any feedback that might guide the next phases.

We've come so far 🎉 , and already our changes will have a big impact on the way WordPress delivers block supports styles.

That's just my 2️⃣ 🪙s.

What do folks think?

@andrewserong
Copy link
Contributor

Thanks for getting this PR up, and apologies for not taking a look sooner.

+1, thanks for exploring how the global styles processing might work, I'd been meaning to look at this sooner, and it's a great idea to kick the tyres on some of these ideas! In this case, I agree with seeing if we can take a slower approach to implementing for global styles. We've already run into a fair few edge cases with just the Layout support, and I think it'll be easier to debug and fine-tune the implementation when we're working with a known set of block / theme.json style attributes, and a finite set of features, rather than arbitrary strings of CSS, which can be difficult to predict. My main question when looking at the change to the output in gutenberg_get_global_stylesheet is how might we determine the correctness of the output, or whether or not this PR is safe to land, with all the current permutations of features in global styles? Some of the recent bugs we've had to deal with lately have been quite subtle, so I think I'm feeling a little cautious about the changes we're introducing in the lead-up to the 6.1 feature freeze next month.

isolate these style categories one at a time so that we can generate the CSS using the style engine.

This would be my preferred approach, where we can test and determine the right approach for each feature, one at a time. There's already quite a lot proposed to land in 6.1, and I imagine there'll be lots of tweaking through the beta and release candidate stage in order to get a viable version of the style engine in core, and I'd be concerned that attempting to parse arbitrary CSS could wind up being a blocker.

If it looks like implementing support for global styles is too complex to do in time for 6.1, it might be worth considering whether we should bump it for this release (or determine how much / which features we think we can implement in time)? I think we've already made great progress on consolidating most of the individual block supports along with improving output of the Layout block support which was a big pain point. So it looks like we're most of the way there in terms of the Phase 1 tasks from the tracking issue.

@aristath aristath force-pushed the try/global-styles-style-engine-2 branch from 967d5c2 to 7952c2e Compare March 8, 2023 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Package] Style Engine /packages/style-engine [Type] Experimental Experimental feature or API.
Projects
Status: 🎾 In progress
Development

Successfully merging this pull request may close these issues.

3 participants