Skip to content

Style engine: switch off optimize by default #53085

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jul 28, 2023
Merged

Style engine: switch off optimize by default #53085

merged 4 commits into from
Jul 28, 2023

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Jul 28, 2023 •
edited
Loading

Resolves #52646

What?

The style engine's default behavior of combining selectors where the CSS declarations are equal results in errors where order matters.

This PR doesn't address the core issue, that is, maintaining the order of general selectors where CSS stylesheet opt-in to optimization.

I'm not convinced that it can given how general elements rules are. Optimization should therefore be "opt-in" only.

For example, many blocks, children of children of parents or whichever, can have the following selector applied to them: .wp-element-1 a. It's impossible for the style engine to know the intended hierarchy when combining such selectors with matching declarations. cc @aristath for thoughts.

If we want the optimizations to work universally, I think elements block supports should create rules specific to the block that needs them. How, I'm not sure. Maybe if we're applying a link color to a paragraph block we could do something like p.wp-element-1 > a. 🤷🏻

Why?

See: https://core.trac.wordpress.org/ticket/58811
Core patch: WordPress/wordpress-develop#4902

The bug is most evident in elements block supports CSS (links), which applies CSS rules to block wrappers that affect all child elements.

If a child element and another parent element (not the child's) share CSS declarations, the CSS selectors will be combined and placed towards of the end of the stylesheet thereby overriding any previous rules.

So far it's only occurring for elements (link colors) but it potentially affects any selector that has general application, e.g., .block-wrapper-class a

The bug is only visible on the frontend (not the editor).

How?

Flipping the optimize flag and updating tests

Testing Instructions

Follow the test instructions in #52646

Here is some test HTML: 
<!-- wp:group {"style":{"spacing":{"padding":{"right":"var:preset|spacing|50","left":"var:preset|spacing|50","top":"var:preset|spacing|50","bottom":"var:preset|spacing|50"}},"elements":{"link":{"color":{"text":"var:preset|color|luminous-vivid-orange"}}}},"backgroundColor":"tertiary","textColor":"luminous-vivid-orange","layout":{"type":"constrained"}} -->
<div class="wp-block-group has-luminous-vivid-orange-color has-tertiary-background-color has-text-color has-background has-link-color" style="padding-top:var(--wp--preset--spacing--50);padding-right:var(--wp--preset--spacing--50);padding-bottom:var(--wp--preset--spacing--50);padding-left:var(--wp--preset--spacing--50)"><!-- wp:group {"style":{"elements":{"link":{"color":{"text":"var:preset|color|vivid-red"}}}},"textColor":"vivid-red","layout":{"type":"constrained"}} -->
<div class="wp-block-group has-vivid-red-color has-text-color has-link-color"><!-- wp:paragraph -->
<p>A paragraph <a href="https://wordpress.org" data-type="link" data-id="https://wordpress.org">inside two group blocks with a link</a></p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->

<!-- wp:group {"style":{"spacing":{"padding":{"right":"var:preset|spacing|50","left":"var:preset|spacing|50","top":"var:preset|spacing|50","bottom":"var:preset|spacing|50"}},"elements":{"link":{"color":{"text":"var:preset|color|vivid-purple"}}}},"backgroundColor":"tertiary","textColor":"vivid-purple","layout":{"type":"constrained"}} -->
<div class="wp-block-group has-vivid-purple-color has-tertiary-background-color has-text-color has-background has-link-color" style="padding-top:var(--wp--preset--spacing--50);padding-right:var(--wp--preset--spacing--50);padding-bottom:var(--wp--preset--spacing--50);padding-left:var(--wp--preset--spacing--50)"><!-- wp:group {"style":{"elements":{"link":{"color":{"text":"var:preset|color|luminous-vivid-orange"}}}},"textColor":"luminous-vivid-orange","layout":{"type":"constrained"}} -->
<div class="wp-block-group has-luminous-vivid-orange-color has-text-color has-link-color"><!-- wp:paragraph -->
<p>A paragraph <a href="https://wordpress.org" data-type="link" data-id="https://wordpress.org">inside two group blocks with a link</a></p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->

Using the above code you should see the following on the frontend

Before After
Screenshot 2023-07-28 at 11 34 50 am Screenshot 2023-07-28 at 11 34 55 am

@ramonjd ramonjd added [Type] Bug An existing feature does not function as intended [Package] Style Engine /packages/style-engine labels Jul 28, 2023
@ramonjd ramonjd requested review from aristath and andrewserong July 28, 2023 01:38
@ramonjd ramonjd self-assigned this Jul 28, 2023
@github-actions
Copy link

github-actions bot commented Jul 28, 2023 •
edited
Loading

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ phpunit/style-engine/class-wp-style-engine-processor-test.php
❔ phpunit/style-engine/style-engine-test.php

@ramonjd ramonjd added the Needs PHP backport Needs PHP backport to Core label Jul 28, 2023
This was referenced Jul 28, 2023
Closed
Closed
andrewserong
andrewserong approved these changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

This tests well for me, and works around the underlying issue, that for optimized styles, we can't know when the order in which they appear in the output is required.

Looking at this issue, I think switching the optimization to default to false is probably a good idea, so that we prioritize the elements API working, over the issue of the size of the CSS output.

I'd be very curious to hear what @aristath thinks, though, since he did the bulk of the work on optimization, but this PR LGTM! ✨

@ramonjd
Copy link
Member Author

ramonjd commented Jul 28, 2023

Thanks for the quick review @andrewserong !

@ramonjd ramonjd enabled auto-merge (squash) July 28, 2023 03:14
@ramonjd ramonjd merged commit 85a4785 into trunk Jul 28, 2023
@ramonjd ramonjd deleted the update/style-engine-disable-optimize-by-default branch July 28, 2023 03:32
@github-actions github-actions bot added this to the Gutenberg 16.4 milestone Jul 28, 2023
@mikachan mikachan added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Aug 2, 2023
@SiobhyB SiobhyB removed the Needs PHP backport Needs PHP backport to Core label Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewserong andrewserong andrewserong approved these changes

@aristath aristath Awaiting requested review from aristath

Assignees

@ramonjd ramonjd

Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Package] Style Engine /packages/style-engine [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
Gutenberg 16.4
Development

Successfully merging this pull request may close these issues.

Gutenberg Bug - Nested WP Element Styles Broken
4 participants
@ramonjd @andrewserong @mikachan @SiobhyB