Editor Styles: Add a transform for button rules

[MaggieCabrera]

Description

When themes declare styles for form elements (button and inputs) these bleed into the editor's placeholder elements. This PR updates the editor styles transforms to remove placeholder components from the scope of the CSS.

How has this been tested?

Tested in Independent Publisher 2 and Canard. Added a unit test for the two new cases.

Types of changes

Bug fix

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR.

[jffng]

I verified that this also fixes Automattic/wp-calypso#50332.

[scruffian]

@aristath @carolinan you might be interested in this one too...

[youknowriad]

Thanks for the PR, I’m not sure how I feel about this:

• Basically it adds a bit of specificity to the theme button styles (which may work for most use-cases but has a very small potential of unexpected behavior in some)
• It relies on classnames coming from @wordpress/components . So it’s an adhoc fix for several reasons: these classnames are not exhaustive and may even change, the button//input selector (condition) is not exhaustive, and it only address buttons and inputs while in theory, any component can suffer from these style conflicts and it doesn't include any button/input usage that is not coming from @wordpress/components but is also meant to be a "control" and not "content".

That said, it’s the least ugly solution I can think of at the moment. iframing the editor could improve things in some of these situations (not all).

So I'm not sure at this point whether we should land this or not.

[scruffian]

@youknowriad I agree with your concerns but let me put my perspective:

Without this fix we need to make changes to our themes code to avoid it leaking into the editor. There are a few options:

1. Add these :not selectors to our themes code. This is adding a lot more code, and much more maintenance burden. If these selectors do change in the future we have to make the change again in hundreds of themes. This is already happening: Varia: fix button styles leaking into editor Automattic/themes#3326
2. We ship a different bundle of CSS to the theme and the editor. This means maintaining duplicate rules between the two systems with all the repetition that entails. This has also proven to be fragile as the editor css often ends up targeting classes which change.

The list of selectors here is not exhaustive - we may need to add more to them in the future as we find issues.

There's also a chance the class names in these components could change. If they do it's much easier to just update them in this file as well than in thousands of themes!

[jasmussen]

Thanks for the PR. I don't have strong opinions, other than to suggest I have a slight feeling this should be fixed on a per-component level, rather than with code like this. I have a dream that we can wrap all block editor components in Shadow DOM, which seems designed for this very purpose. But in absence of that, we can increase the specificity of component rules when used inside the canvas, so they are less prone to bleed.

Presumably it will also help landing the iframe for the post editor, that will enable editor styles to have normal/low specificity, and therefore be less likely to override the rules of block UI components.

[scruffian]

this should be fixed on a per-component level, rather than with code like this

I don't see how this would be possible. The issue here is that themes can write CSS that targets all elements. The purpose of the PR is to limit the scope of the CSS so that it can't target these placeholders. The other way to achieve the same thing would be to greatly strengthen the CSS for the placeholders to make it harder for themes to override them.

Presumably it will also help landing the iframe for the post editor, that will enable editor styles to have normal/low specificity, and therefore be less likely to override the rules of block UI components.

I don't think so, since placeholders will still sit inside the content of the page and inherit the CSS from the page itself.

[oandregal]

I've looked a bit into this, although I lack prior deep involvement and may miss tons of context. Haven't formed a strong opinion, but wanted to share some thoughts.

I've noticed the issues affect roughly two areas: the classic block toolbar (.block-library-classic__toolbar) and the block's placeholders (.components-placeholder). These two classes seem like could be fairly stable. Could these be used instead of the components' classes, either by the themes or the editor?

For example, if we wanted to do this in the editor, would it work if we made the existing wrapper be .editor-styles-wrapper :not(.components-placeholder) <theme-selector>? I understand this still couples the editor with classes coming from the @wordpress/components. If we wanted to eliminate this coupling, perhaps we can add a .is-editor-canvas class to the placeholders and the classic toolbar in the editor. The wrapper would then be .editor-styles-wrapper :not(.is-editor-canvas) <theem-selector>.

By looking at the example at https://github.com/Automattic/themes/pull/3326/files for themes, it actually looks a nice PR to me: it removes CSS that's general (ID selectors) and leaves the selectors that are specific to the blocks ― the net result is less CSS. Perhaps there're more issues, I'd be glad to look at other examples to educated myself.

[scruffian]

By looking at the example at https://github.com/Automattic/themes/pull/3326/files for themes, it actually looks a nice PR to me: it removes CSS that's general (ID selectors) and leaves the selectors that are specific to the blocks ― the net result is less CSS. Perhaps there're more issues, I'd be glad to look at other examples to educated myself.

The main problem is we have hundreds of themes with generic CSS in. Making these kinds of changes to this many themes is not sustainable.

[scruffian]

The issues with clashes with the block toolbar should be fixed when we move to the iframed post editor, so I'm less concerned about these.

If we wanted to eliminate this coupling, perhaps we can add a .is-editor-canvas class to the placeholders and the classic toolbar in the editor. The wrapper would then be .editor-styles-wrapper :not(.is-editor-canvas) .

This is a nice idea. My only concern is that it requires making more changes to the code, so there might be more unexpected consequences, but I'm happy to go that route if it's going to help us unblock this.

[youknowriad]

The "wrap" transform is a generic transform, and this is very specific, can we extract it to its own transform instead? and add a comment clarifying that yes, it's a bit hacky to target some specific Gutenberg components but that it's a good contained hack.

[MaggieCabrera]

@youknowriad yes, that makes sense to me, I just updated the code, let me know if this looks good to you and I'll go ahead and update the tests and move them to a separate file too.

[youknowriad]

Yep, that's hat I had in mind, thanks for the update.

[MaggieCabrera]

I think this is ready for another review

[youknowriad]

I didn't test this but I'm giving my approval for this to land if tested properly.

[scruffian]

This is looking good. I had to update the test snapshots, but it worked well in my tests.

[scruffian]

LGTM

[youknowriad]

I've been thinking more about this PR and I'm not sure it's the right thing to do anymore unfortunately. Here's an example. Say I have this on my styles:

button {
  color: red;
}

.wp-block-search__button {
  color: blue;
}

Basically, I want buttons to have a red color but the search block button to have a blue color. This style break in the editor with this PR because the "button" selector will get a higher specificity than the class selector due to the extra :not that are added there.

That's just an example, and the same thing can happen with any block using input or button.

[MaggieCabrera]

Basically, I want buttons to have a red color but the search block button to have a blue color. This style break in the editor with this PR because the "button" selector will get a higher specificity than the class selector due to the extra :not that are added there.

That's just an example, and the same thing can happen with any block using input or button.

I see what you mean. Maybe we can do a better job at targetting only the cases where the selector is only for the button or input tag and not any class that contains that word?

[youknowriad]

What if we try to isolate where the style bleed happens. If it's just inside placeholders, maybe we can instead add more specific styles for .components__button and the other similar components when used inside placeholders?

It is a hack as well as it also only fixes a small sub set of issues and introduce some style dependencies between components but it would make sure editor styles work as they should in most cases it seems.

[MaggieCabrera]

What if we try to isolate where the style bleed happens. If it's just inside placeholders, maybe we can instead add more specific styles for .components__button and the other similar components when used inside placeholders?

It is a hack as well as it also only fixes a small sub set of issues and introduce some style dependencies between components but it would make sure editor styles work as they should in most cases it seems.

Mmmh, so far I've found it happens on placeholders and depending on the specific style, it also can bleed into the inserter on the columns block for example or the regular + inserter. I'm not familiar enpugh with the editor components to know if there can be even more cases:

We would have to look into all the possible cases and think about the possible rules that a theme may want to add to a button (like in this test I used the box-shadow that neither of the components are defining at all)

[MaggieCabrera]

I just tested emptytheme with the following css:

button {
	background-color: blue;
	color: red;
	box-shadow:  5px 10px #888888;
}

.wp-block-search__button {
	color: blue;
}

button.myclass {
	color: green;
}

And I did get the blue text on the search button. It's not targetting that selector with the :not:

[youknowriad]

@MaggieCabrera Interesting, I did get some weird results while testing with the search block personally but I'll continue to look and report back if I find anything.

[MaggieCabrera]

@MaggieCabrera Interesting, I did get some weird results while testing with the search block personally but I'll continue to look and report back if I find anything.

Do let me know, I definitely am not familiar with all the possible edge cases. Thanks for taking a look at this!

[youknowriad]

@MaggieCabrera It's definitely not blue in my tests with the same styles above 🤔

[youknowriad]

@MaggieCabrera Oh I know why :) you're testing on trunk where the markup in the editor doesn't match the frontend, it's not "button" there. You should test in this PR instead #30176

[MaggieCabrera]

@MaggieCabrera Oh I know why :) you're testing on trunk where the markup in the editor doesn't match the frontend, it's not "button" there. You should test in this PR instead #30176

Ok, I see what you mean now, it's definitely a specificity battle here... I'm going to think about what could be done here. @scruffian if you could 👀 here too and see if you can think of something too?