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

Block Styles: Fix block style variation styles for blocks with complex selectors #62125

Merged

Conversation

aaronrobertshaw
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw commented May 30, 2024

Fixes: #62122

What?

Fixes the style generation for instanced block style variations for blocks that use complex selectors. For example:

  • Button: .wp-block-button .wp-block-button__link
  • Image:
    • .wp-block-image
    • .wp-block-image img, .wp-block-image .wp-block-image__crop-area, .wp-block-image .components-placeholder

Why?

Without this fix, variation styles when applied to an instance would only be applied to the block's wrapper. This means styles like the Button block's outline block style would get double borders, padding et.c

How?

Reworks the generation of the instanced block style variation stylesheets so that the result has selectors that apply different types of styles to the block's desired elements. For example, the Button block styles go to the inner .wp-block-button__link element or an Image's border goes to the inner img

The style load order was also tweaked to ensure the block style variations styles were loaded after the block library styles.

Testing Instructions

  1. Register some custom block style variations following #57908's instructions
  2. Apply these in a nested fashion and ensure they still continue to work
  3. Make sure you have a button block in your post or page
  4. Apply the outline block style to the button
  5. Make sure all the blocks display correctly in both editors and the frontend
  6. Disable separate core block assets via snippet below or activate a theme which does such as Powder
  7. Confirm block library styles do not override the outline block style variation styles for Button blocks

add_filter( 'should_load_separate_core_block_assets', '__return_false' );

Screenshots or screencast

Before After
Screenshot 2024-05-30 at 4 25 25 PM Screenshot 2024-05-30 at 4 25 08 PM

@aaronrobertshaw aaronrobertshaw added [Type] Bug An existing feature does not function as intended [Feature] Block Style Variations Issues or PRs that are related to the style variations for blocks labels May 30, 2024
@aaronrobertshaw aaronrobertshaw self-assigned this May 30, 2024
Copy link

github-actions bot commented May 30, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: talldan <talldanwp@git.wordpress.org>
Co-authored-by: bgardner <bgardner@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

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
❔ lib/block-supports/block-style-variations.php

@aaronrobertshaw aaronrobertshaw force-pushed the fix/block-style-variations-for-complex-selectors branch from 171b614 to 61aeef9 Compare May 30, 2024 06:35
@aaronrobertshaw
Copy link
Contributor Author

The changes here have been included within the overall backport for the section styles feature: WordPress/wordpress-develop#6662.

If this PR requires further updates, that backport will also need updating.

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

It does fix the issue, but I don't understand the fix, so I'd have a hard time approving it.

I do notice there are two sets of identical styles:
Screenshot 2024-05-30 at 4 31 04 pm

If there's any way to avoid that it might be good, but it could be considered a separate change.

lib/block-supports/block-style-variations.php Outdated Show resolved Hide resolved
lib/block-supports/block-style-variations.php Outdated Show resolved Hide resolved
@aaronrobertshaw
Copy link
Contributor Author

aaronrobertshaw commented May 30, 2024

Appreciate the thorough review @talldan 🙇

I believe I've addressed all the feedback so far. Should be good to go for another spin now.

If there's any way to avoid that it might be good, but it could be considered a separate change.

I think that is definitely best done via a separate follow-up.

For the record, one set of styles is from the standard theme.json stylesheet but can't be relied upon as if variations are nested the load order needs to be different for the correct variation's styles to take precedence. That's where the second set of styles comes in as they are the styles specific to this individual application or instance of the block style variation.

The code creating the standard set of styles is actually leveraged to generate the individual instance styles.

It is 100% something that would be good to clean up. Right now though, I think we need to land a fix for the issue affecting outline buttons etc. then I can circle back to this optimisation and clean up.

Copy link

Flaky tests detected in 980ca36.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9300616448
📝 Reported issues:

@bgardner
Copy link

@aaronrobertshaw

There seems to still be an issue with specificity and styles. My Powder theme which uses theme.json to style the outline variation (https://github.com/bgardner/powder/blob/main/theme.json#L288) is being overridden by the following:

:root :where(.wp-block-button .wp-block-button__link.is-style-outline), :root :where(.wp-block-button.is-style-outline>.wp-block-button__link) {
    border: 2px solid;
    padding: .667em 1.333em;
}

@bgardner
Copy link

The button should have a border of 1px thick, but looks like this (note the lower right side styles being overridden:

image

@aaronrobertshaw
Copy link
Contributor Author

aaronrobertshaw commented May 30, 2024

Thanks for the extra information @bgardner, it definitely helps 👍

After downloading your theme and poking around a bit, it looks like the style load order isn't quite right. The key difference I noted while testing was that Powder disables the separate block assets via the should_load_separate_core_block_assets filter.

The wp-block-library-inline-css styles are added after the global styles and block style variation styles. When the block styles are separated however their individual stylesheets are loaded before the global styles and variation styles.

I'll work on a fix for this first thing tomorrow. It might also be better as a separate PR.

Separate Block Assets All Together
Screenshot 2024-05-30 at 11 39 24 PM Screenshot 2024-05-30 at 11 29 52 PM

Note: The separate assets screenshot is actually TT4 but illustrates the point.

@bgardner
Copy link

@aaronrobertshaw That makes sense, thanks for spelling that out. If there's an alternative (more optimal) way for me to handle things in Powder, I'm all ears.

@aaronrobertshaw
Copy link
Contributor Author

If there's an alternative (more optimal) way for me to handle things in Powder, I'm all ears.

I don't think you should need to make any changes here. Hopefully, a bit of tweaking to the order styles load in should see this working as expected.

@aaronrobertshaw
Copy link
Contributor Author

@bgardner with fresh eyes this morning it looks like the tweak the load order is rather trivial so I've rolled it into this fix.

Before After
Screenshot 2024-05-31 at 9 55 29 AM Screenshot 2024-05-31 at 9 55 09 AM

@bgardner
Copy link

@aaronrobertshaw Can confirm, per the below. LGTM!

image

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Looks good now, thanks for adding the improved explanations in the code, that helped me understand the issue and fix.

@aaronrobertshaw
Copy link
Contributor Author

Appreciate all the reviews and testing. Thanks @talldan and @bgardner 🙇

@aaronrobertshaw aaronrobertshaw merged commit cc7dbab into trunk May 31, 2024
63 checks passed
@aaronrobertshaw aaronrobertshaw deleted the fix/block-style-variations-for-complex-selectors branch May 31, 2024 02:34
@github-actions github-actions bot added this to the Gutenberg 18.5 milestone May 31, 2024
@aaronrobertshaw aaronrobertshaw added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Needs PHP backport Needs PHP backport to Core labels May 31, 2024
carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Jun 4, 2024
…x selectors (WordPress#62125)

Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: talldan <talldanwp@git.wordpress.org>
Co-authored-by: bgardner <bgardner@git.wordpress.org>
patil-vipul pushed a commit to patil-vipul/gutenberg that referenced this pull request Jun 17, 2024
…x selectors (WordPress#62125)

Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: talldan <talldanwp@git.wordpress.org>
Co-authored-by: bgardner <bgardner@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Feature] Block Style Variations Issues or PRs that are related to the style variations for blocks [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Double border on buttons with outline variation applied
3 participants