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 for global-styles #42592

Closed
wants to merge 31 commits into from

Conversation

aristath
Copy link
Member

@aristath aristath commented Jul 21, 2022

What?

Uses the Style Engine objects in global-styles.

Why?

  • Consistent formating for all styles
  • An opportunity to optimize the styles and minify them as needed in the future by directly editing the Style-Engine instead of each instance where we add styles.
  • Optimizing styles: Combining selectors. body { margin: 0; } body { color: #000; } becomes body { margin: 0; color: #000; }. Similarly, if 2 selectors have the same styles, they get combined. So .foo { color: red; } .bar { color: red; } becomes .foo,.bar { color: red; }.
  • Optimizing styles: No duplicate properties in selectors. So if we have body { color: red; } body { color: black; }, the new output will be body { color: black; } keeping only the last item which overrides the previous ones.
  • Consistent sanitization for properties & values in CSS

How?

Converted the string-concatenation method in global styles with style-engine objects and methods. This required adding new methods to get the stores. For example, we previously had a method called get_css_variables, which was generating the styles for CSS variables. This PR added a new get_css_variables_store method, moved & converted the previous code in there, and then the get_css_variables method simply gets the store from get_css_variables_store and returns the CSS from said store. This was done for all methods that required it.

Some tweaks were also made to the Style-Engine objects to ensure sanitization & all props work properly.
There were a few failing tests because the generated CSS is different. I went through all instances of failing tests, manually compared the before/after results and confirmed that they are identical - the only difference being whitespace changes and the optimizations outlined above (combining selectors & deduping styles).

Testing Instructions

I manually tested it thoroughly... Used a few block themes, opened a site on trunk, copied the generated styles on trunk to my editor. Then switched to this branch, repeated the process, and finally compared the 2 CSS snippets.
There were no visual differences in the browser, and the CSS diff was the reasonable optimizations that I would have made if I were manually trying to optimize the styles.

@aristath aristath requested review from ramonjd and andrewserong July 21, 2022 11:41
$layout_selector,
array(
array(
'name' => 'display',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think of it, one potential issue with this one is that display is being manually injected here because it is not included in the allow-listed list of CSS properties (here's the core PR: WordPress/wordpress-develop#2928 where display was discussed), so it has special handling here to determine that the value is from a hard-coded list of valid display modes.

Do we need to have a mechanism to allow potentially unsafe declarations to bypass calling safecss_filter_attr? I'm imagining some kind of approach where we can say to the style engine (at some point), here is an already validated declaration, please trust it 😅

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to have a mechanism to allow potentially unsafe declarations to bypass calling safecss_filter_attr?

We kind of already have that... 😉 I was allowing CSS variables in the Declarations object in this PR, so I added display when the value is not none (I believe display:none is the reason we don't allow display in safe-css, so this implementation accounts for that scenario)

Copy link
Member Author

@aristath aristath Jul 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context, check out the tweaks in packages/style-engine/class-wp-style-engine-css-declarations.php in this PR. If the property starts with -- it skips safecss_filter_attr() (to account for CSS variables), and the same thing happens for display if the value is not none 👍

@aristath aristath force-pushed the try/global-styles-use-style-engine branch from e29310a to 28342e4 Compare July 22, 2022 09:18
@aristath aristath changed the title WIP: Use the Style Engine for global-styles Use the Style Engine for global-styles Jul 22, 2022
@aristath aristath marked this pull request as ready for review July 22, 2022 09:45
@aristath aristath requested a review from spacedmonkey as a code owner July 22, 2022 09:45
@aristath aristath added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Jul 22, 2022
@aristath aristath force-pushed the try/global-styles-use-style-engine branch from 8ebaf85 to 326aca1 Compare July 25, 2022 08:02
@aristath
Copy link
Member Author

aristath commented Aug 2, 2022

After #42452 was merged, it's impossible to get this PR to a working state (well actually not exactly impossible, but it would take me days to refactor).
I'll go ahead and close this PR, then submit other PRs to attempt and achieve the same result.

@ramonjd
Copy link
Member

ramonjd commented Aug 2, 2022

After #42452 was merged, it's impossible to get this PR to a working state (well actually not exactly impossible, but it would take me days to refactor).

Sorry! If you want to save yourself some refactoring grief, I suspect that migrating global styles and the theme json class to the style engine isn't targeted for 6.1 anyway, so we can revisit this work after September, or whenever the code freeze is.

@aristath aristath self-assigned this Aug 3, 2022
@aristath aristath deleted the try/global-styles-use-style-engine branch August 3, 2022 08:56
@aristath
Copy link
Member Author

aristath commented Aug 3, 2022

I suspect that migrating global styles and the theme json class to the style engine isn't targeted for 6.1 anyway

Well, we can definitely take the first step, without completely refactoring global styles! #42893 makes use of the style engine in a simple way, taking advantage of some improvements that style-engine implements but without all the hassle of a refactor 😉

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
Projects
Status: 🗄 Closed / not merged
Development

Successfully merging this pull request may close these issues.

3 participants