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

Modifying Class Names on Blocks Breaks Layouts #67134

Open
3 of 6 tasks
pattyok opened this issue Nov 19, 2024 · 14 comments
Open
3 of 6 tasks

Modifying Class Names on Blocks Breaks Layouts #67134

pattyok opened this issue Nov 19, 2024 · 14 comments
Assignees
Labels
[Block] Media & Text Affects the Media & Text Block [Type] Bug An existing feature does not function as intended

Comments

@pattyok
Copy link

pattyok commented Nov 19, 2024

Description

With the release of 6.7, the class for cropped images on the Media Text Block changed from is-image-fill to is-image-fill-element. This totally breaks layouts in all of my sites where I have customized the styles of the media text block.

What makes is worse for a change like this is that the change isn't evident until someone goes in and edits a page. So I make the update, scan the site for issues and three days later the issue shows up on the home page. Changes to classnames on blocks should not be updated casually.

Logging as bug in the process. Class names should not be removed from existing blocks.

Step-by-step reproduction instructions

  1. Create a media text block with Crop Image to Fill selected
  2. Style the block as desired with css
  3. Update your WP version to 6.7
  4. Go back and edit the content of the block. Save the page
  5. See that your styles no longer apply.

Screenshots, screen recording, code snippet

No response

Environment info

Wordpress 6.7

Please confirm that you have searched existing issues in the repo.

  • Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

  • Yes

Please confirm which theme type you used for testing.

  • Block
  • Classic
  • Hybrid (e.g. classic with theme.json)
  • Not sure
@pattyok pattyok added the [Type] Bug An existing feature does not function as intended label Nov 19, 2024
@himanshupathak95
Copy link
Contributor

Hi @pattyok, thank you for reporting this issue.

I tried to reproduce this issue but it seems to be working fine for me. Here are the steps I followed:

  1. Created a test page with a Media Text block
  2. Added image and enabled "Crop image to fill"
  3. Added custom styles including:
    • Text color and background color
    • Typography changes
    • Custom margin and padding
    • Border and background styling
  4. Upgraded to WordPress 6.7
  5. Edited and saved the page

The styles remained intact after the upgrade and editing.

Looking into the code, I found in styles.scss of the media-text block:

/* `is-image-fill` is deprecated and the styles are kept for backward compatibility. */

This indicates that while the class name has been updated to is-image-fill-element, WordPress maintains backward compatibility for the is-image-fill class to prevent layout breaks in older versions.

For additional debugging, could you please provide:

The specific CSS code you're using or any custom theme
A screencast showing the issue if possible

Let me know if you need any clarification or have additional information to share.

@Mamaduka
Copy link
Member

cc @sgomes

@sgomes
Copy link
Contributor

sgomes commented Nov 20, 2024

Hi @pattyok!

I understand your frustration and would like to offer some context, and also whatever help you may need in crafting new CSS.

As @himanshupathak95 mentioned, the previous styles are kept for backwards-compatibility, so it sounds like what happened was that the pages in question were edited, and were therefore upgraded to the new version of the block, which includes the new class name. The editor only "knows" how to save the current version of a block, so any time a page is edited, block markup is automatically upgraded to the new version, through the standard deprecations mechanism.

Pages that were not edited should be working normally, and if they're not then please let us know, since that would definitely be unexpected behaviour.

I understand your concerns regarding introducing new class names, and can assure you this was not done lightly. Changes were needed in order to fix performance issues with the block (see #52789 for the bug report), and adding a new class was necessary in order to avoid breaking existing pages, since the markup is fundamentally different between both versions of the block. If you're curious, you can follow this thread where the issue was discussed in more detail.

As for this being done more or less invisibly, I understand where you're coming from, and agree it would be useful to be aware of these kinds of changes. I'll do some research into whether any mechanism already exists to make these block upgrades (and the need for validation) more obvious. If that mechanism doesn't exist, it sounds like something for which we could file a feature request!

@sgomes
Copy link
Contributor

sgomes commented Nov 20, 2024

@pattyok : Is there any chance you could share what your goals were with customising the styles for the Media & Text block, or even share some sample CSS? I mention this because another approach we could also look at would be providing the customisation you need directly as part of the block options, so that CSS overrides wouldn't be necessary in the first place.

@pattyok
Copy link
Author

pattyok commented Nov 20, 2024

@sgomes I build custom themes for my sites. We use the media text block extensively and I often use the various options provided on the block to build different style options.

With the image-fill class I usually customize the padding/margins on each side separately and set my own default minimum height. One of the limitations of the media text block is that the padding/margin settings apply to the whole block and not to either side, which is what I want.

Here is an example: https://eatlocalfirst.org/holiday-food-farm-finder/
This page shows the differentiation of the two styles of media text block, one with the 'W' logo has padding on the whole block, while the one with the image-fill only has the padding set on the content side.
https://eatlocalfirst.org/

I appreciate the idea in trying to offer everything in the block settings, but I also appreciate the consistency that I can get with setting the styles in the css vs the block settings. In building sites with the client editing experience in mind, I aim to make it easy for them to put blocks on the page and have them behave consistently.

I should have learned my lesson by now that I can't rely on the markup from core blocks, but adding my own attributes/styles seems redundant especially in this case.

@sgomes
Copy link
Contributor

sgomes commented Nov 21, 2024

Thank you for taking the time to explain your requirements so thoroughly, @pattyok!

It sounds like the fundamental issue here is that is-image-fill / is-image-fill-element is the only signal available on markup to indicate that the image fill option is enabled, and so by necessity your CSS had to rely on it. When we changed the class name out of a need to preserve backwards compatibility in unedited pages, we removed the signal you relied on in new and edited pages. I'm not sure it would have been possible to avoid this breakage, since the markup fundamentally changed, but the main issue was that, as you mentioned, all of this happened transparently and behind the scenes without anything immediately visible to you.

It might be interesting to explore ways of communicating block options in markup in a more stable manner, although I'm not sure how often these class changes take place. Your reply makes it sound like this isn't the first time class changes have pulled the rug from under your feet?

As for your requirements, thank you again for all the info! It does sound like we could consider some options around providing more fine-grained padding options for each side of the block.

I do also understand your point regarding consistency without user overhead (i.e., not requiring users to go in and modify options on every block), and noticed your use of CSS custom properties to keep things consistent across your theme. If we were to enable the options you need at the block level at some point, then one possibility to consider would be setting up some patterns with your preferred settings, and guiding authors to that higher-level building block when building their pages. Or perhaps even better, setting up global styles for your theme, to have them applied automatically to all instances of the block.

In any case, I'll share your feedback with the rest of the team, and see where we can go from there!

@mrwweb
Copy link

mrwweb commented Nov 21, 2024

I just want to chime in that this is definitely not the first time a core block class change has gone largely unmentioned in dev notes and it's very hard.

For context, there are 13 plugins and 30 themes in the repository using these classes.

I also use this in one plugin of my own and in my standard starter theme.

Is there any chance you could share what your goals were with customising the styles for the Media & Text block, or even share some sample CSS? I mention this because another approach we could also look at would be providing the customisation you need directly as part of the block options, so that CSS overrides wouldn't be necessary in the first place.

@sgomes In nearly all of my cases, customization options would not be helpful for me, because the point of the styles is to enforce various style changes that are baked into the site design.

@sgomes
Copy link
Contributor

sgomes commented Nov 22, 2024

Thank you, @mrwweb, that's good feedback! I'm not sure how best to handle CSS class name changes going forward, but you're right that they weren't even mentioned in the dev notes in this case.

In nearly all of my cases, customization options would not be helpful for me, because the point of the styles is to enforce various style changes that are baked into the site design.

This is really interesting to know, and something I'd like to dig into a bit deeper, as being able to configure styling for all blocks of a given type is the main idea behind global styles. Is there some way we could make those better, to reduce the need to write custom CSS?

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Nov 22, 2024
@sgomes
Copy link
Contributor

sgomes commented Nov 22, 2024

Apologies for the confusion, I mistakenly linked an unrelated PR to this issue.

@sgomes sgomes removed the [Status] In Progress Tracking issues with work in progress label Nov 22, 2024
@cbirdsong
Copy link

cbirdsong commented Nov 22, 2024

This is really interesting to know, and something I'd like to dig into a bit deeper, as being able to configure styling for all blocks of a given type is the main idea behind global styles. Is there some way we could make those better, to reduce the need to write custom CSS?

Not speaking for @mrwweb, but in our case, adjustment of styling on blocks is treated as a bonus if it's allowed at all. We don't want our editors to fiddle around with the design in the editor, we want them to just use the pieces we've provided.


In the specific case of this change, you could have avoided changing the class by adjusting the selector for the legacy styles to look for the background image:

// Legacy versions of the block markup that use a background image
.wp-block-media-text.is-image-fill > .wp-block-media-text__media:where([style*="background-image"]) {
	background-size: cover;
}
.wp-block-media-text.is-image-fill > .wp-block-media-text__media:where([style*="background-image"]) img {
	// The image is visually hidden but accessible to assistive technologies.
	position: absolute;
	width: 1px;
	height: 1px;
	padding: 0;
	margin: -1px;
	overflow: hidden;
 	clip: rect(0, 0, 0, 0);
 	border: 0;
 }

// The new object-fit styles are unaffected by the above and will work exactly as written

I'm not sure if it's too late to switch back to just using is-image-fill, but if .is-image-fill-element must also now be supported going forward then you could unify around a single class by changing the selectors to .wp-block-media-text:is(.is-image-fill, .is-image-fill-element).

(Importantly, .wp-block-media-text.is-image-fill > .wp-block-media-text__media:where([style*="background-image"]) and .wp-block-media-text:is(.is-image-fill, .is-image-fill-element). both have the same specificity as their previous equivalents.)

@Mamaduka Mamaduka added the [Block] Media & Text Affects the Media & Text Block label Nov 24, 2024
@sgomes
Copy link
Contributor

sgomes commented Nov 25, 2024

Very interesting, thank you @cbirdsong! Using :where([style*="background-image"]) is an inspired solution that didn't occur to me at all, and doesn't seem to run afoul of our browser support policy (everything seems to support :where).

I'm also not sure whether it's something we could switch to at this point. Do you have any idea, @Mamaduka?

Not speaking for @mrwweb, but in our case, adjustment of styling on blocks is treated as a bonus if it's allowed at all. We don't want our editors to fiddle around with the design in the editor, we want them to just use the pieces we've provided.

I guess my specific question is: where adjustment is available (of course the point is moot if not), why do you prefer providing those pieces as CSS, rather than e.g. theme-level global styles, that editors could even easily override for a specific instance of a block if they so chose?

Is there something lacking from global styles that we could improve to reduce the amount of CSS it takes to customise a theme, and particularly the need to rely on block implementation aspects such as option-related CSS class names?

@cbirdsong
Copy link

cbirdsong commented Nov 25, 2024

I guess my specific question is: where adjustment is available (of course the point is moot if not), why do you prefer providing those pieces as CSS, rather than e.g. theme-level global styles, that editors could even easily override for a specific instance of a block if they so chose?

Is there something lacking from global styles that we could improve to reduce the amount of CSS it takes to customise a theme, and particularly the need to rely on block implementation aspects such as option-related CSS class names?

There are several reasons:

  • Figuring out how to express the CSS we want to write in JSON is awkward, and the system will never be able to cover all the possibilities of CSS as a language. (For instance, on a recent project we increased the font size and padding of button blocks for touch screens using the pointer media query.)
  • We support more than just Wordpress, and don't want our developers to have to think about styling in a totally different way to work on a Wordpress theme.

@mrwweb
Copy link

mrwweb commented Nov 25, 2024

Is there something lacking from global styles that we could improve to reduce the amount of CSS it takes to customise a theme, and particularly the need to rely on block implementation aspects such as option-related CSS class names?

Similar to @cbirdsong's 1st point, when we get down to reasons I'm still writing CSS—while still embracing the block editor—it usually comes down to needing conditional styles.

  • My one boilerplate CSS rule that includes .is-image-fill is to set a larger minimum height for Media & Text blocks when the image crop setting is enabled.
  • In a plugin of mine, I'm using the selector set the aspect ratio of the image as controlled by a custom editor control.
  • I can't remember where, but I'm pretty sure I've needed to target the combination of .is-image-fill with a block style (e.g. is-style-XYZ) to make it work.
  • I write other CSS rules for things like, when the background color is X, automatically set the text color to Y, etc. Things that I can't imagine theme.json ever universally supporting because it can never fully cover all of CSS.

Looking at the list of themes using it, I think it's similar:

  • Go appears to only style the class when the block also has .alignfull
  • Michelle sets a minimum height, just like me.
  • Aeonium has a bunch of custom block styles that need adjustment when .is-image-fill is present

Hope that's some useful context.

@sgomes
Copy link
Contributor

sgomes commented Nov 26, 2024

Thank you both, @cbirdsong and @mrwweb, that's very useful information indeed! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Media & Text Affects the Media & Text Block [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

6 participants