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

Custom editor styles on element selectors effect block control styles. #10178

Closed
m-e-h opened this issue Sep 25, 2018 · 41 comments · May be fixed by #33494
Closed

Custom editor styles on element selectors effect block control styles. #10178

m-e-h opened this issue Sep 25, 2018 · 41 comments · May be fixed by #33494
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Feature] Custom Editor Styles Functionality for adding custom editor styles [Package] Editor /packages/editor [Type] Enhancement A suggestion for improvement.

Comments

@m-e-h
Copy link
Member

m-e-h commented Sep 25, 2018

Describe the bug
If in my theme-editor-styles.css I have the following:

button {
	box-shadow: 10px 5px 5px red;
	min-height: 2.5rem;
}

then havoc is wreaked on my editor.

gutbutton

Expected behavior
The styles above are exaggerated but I don't think styles on button or input elements are totally edge cases. #10067 (comment)

Possible solution
One fix that would need more testing is to unset styles on the Gutenberg specific class before styles are added.
So, for example, adding the following to the top of components/style.css fixed the button styles for me:

.components-button {
	all: unset;
}

This could be done for each class that Gutenberg applies to an html element other than div. Mostly button and input I would guess.

Desktop (please complete the following information):

  • OS: Ubuntu
  • Browser: chome, firefox
  • Version: latest

Additional context

  • Gutenberg v3.9
@m-e-h
Copy link
Member Author

m-e-h commented Sep 25, 2018

I'm pretty sure autoprefixer doesn't cover all or unset. So an ie11 friendly version would look more like this: 😬

.components-button {
  background-color: initial;
  background-image: initial;
  background-position-x: initial;
  background-position-y: initial;
  background-repeat: initial;
  background-attachment: initial;
  background-clip: initial;
  background-origin: initial;
  background-size: initial;
  border-top-color: initial;
  border-top-style: initial;
  border-top-width: initial;
  border-right-color: initial;
  border-right-style: initial;
  border-right-width: initial;
  border-bottom-color: initial;
  border-bottom-style: initial;
  border-bottom-width: initial;
  border-left-color: initial;
  border-left-style: initial;
  border-left-width: initial;
  border-top-left-radius: initial;
  border-top-right-radius: initial;
  border-bottom-right-radius: initial;
  border-bottom-left-radius: initial;
  border-image-source: initial;
  border-image-outset: initial;
  border-image-repeat: initial;
  border-image-width: initial;
  border-image-slice: initial;
  display: initial;
  position: initial;
  overflow-x: initial;
  overflow-y: initial;
  -moz-appearance: initial;
  color: initial;
  font-size: initial;
  text-indent: initial;
  cursor: initial;
  margin-top: initial;
  margin-right: initial;
  margin-bottom: initial;
  margin-left: initial;
  padding-top: initial;
  padding-right: initial;
  padding-bottom: initial;
  padding-left: initial;
  align-items: initial;
  box-sizing: initial;
  text-decoration-line: initial;
  text-decoration-style: initial;
  text-decoration-color: initial;
...
}

@danielbachhuber danielbachhuber added Needs Technical Feedback Needs testing from a developer perspective. Backwards Compatibility Issues or PRs that impact backwards compatability labels Sep 29, 2018
@chrisvanpatten chrisvanpatten added the [Type] Developer Documentation Documentation for developers label Oct 17, 2018
@m-e-h
Copy link
Member Author

m-e-h commented Oct 31, 2018

Reckon I should go ahead and do a PR on this?

Maybe we cut it down to a reset of only the most commonly used properties of input, select, textarea and button.

By adding the following to the top of packages/block-library/src/style.scss, the editor becomes much more resistant to user styles causing deformed or broken controls.

.components-button,
.editor-post-text-editor,
.edit-post-sidebar__panel-tab,
.editor-block-types-list__item,
.document-outline__button,
.components-checkbox-control__input,
.editor-inserter__search,
.components-select-control__input,
.components-textarea-control__input,
.components-text-control__input {
	background: unset;
	border: unset;
	display: unset;
	position: unset;
	transition: unset;
	color: unset;
	font: unset;
	line-height: unset;
	vertical-align: unset;
	cursor: unset;
	padding: unset;
	margin: unset;
	width: unset;
	min-width: unset;
	max-width: unset;
	box-sizing: unset;
	box-shadow: unset;
	height: unset;
	min-height: unset;
	max-height: unset;
	opacity: unset;
}

I've see zero side effects in my tests.

@youknowriad
Copy link
Contributor

youknowriad commented Nov 1, 2018

This is indeed a good idea, but I don't like the fact that we need to polyfill it for IE11. Can we just start with the other browsers for now? I'm worried about the extra bytes in CSS this will create.

That said, I'm wondering if you considered using the editor styles mechanism to provide your editor styles, it shouldn't interfere that much using this technique. https://wordpress.org/gutenberg/handbook/extensibility/theme-support/#editor-styles

(The editor styles work better in 4.2 RC1)

@m-e-h
Copy link
Member Author

m-e-h commented Nov 1, 2018

Supporting all browsers including IE11 wouldn't be a problem. unset just chooses whether inherit or initial is used.
We'd just need to be more selective and thoughtful in the properties chosen and the reset used.
For example,

color: inherit;
padding: initial;

That being said, I forgot about the editor-styles mechanism.
It actually works against this solution. 😯

.editor-block-list__block input, 
.editor-block-list__block select, 
.editor-block-list__block textarea, 
.editor-block-list__block button

This targets the block controls even more specifically. To the point where unsetting .components-button wouldn't be specific enough to protect the insertion-point-button from user's button styles.
We can't increase the specificity of the unset or it'll override core styles added to single class selectors.

I'm going to think about it some more but right now I don't know how this initial proposal could work.

To be clear, my whole point here is that I want my editor buttons and inputs to look as they do on the front-end.
With these things largely coming from plugins and shortcodes, the [class], if there is one, can't be known at the time the theme is built. Styling the elements is the only way.

@m-e-h
Copy link
Member Author

m-e-h commented Nov 5, 2018

Related:
#11503
#11505
#11132

@m-e-h
Copy link
Member Author

m-e-h commented Nov 6, 2018

One possible solution here would be to modify the way the style wrapper #10956 works for non-"root" selectors. In addition to wrapping selectors, we add a :not() pseudo-class to them, negating a class or attribute assigned to editor components and controls using elements.

An example of a modified user's editor-style would look something like this:

.editor-styles-wrapper button:not([class^="components-"]) {

The [class^="components-"] in the above example seems to cover it for the buttons but it may be better to add a new class or attribute, for targeting, to all non-div editor elements.

@chrisvanpatten
Copy link
Contributor

@m-e-h I'm curious — how much of this could be solved by applying a generic reset in CSS to our components and making their selectors more specific, so our component styles are less likely to be impacted? I think I'd personally feel more comfortable with that, vs. a CSS rewriting hack which seems more fragile.

@m-e-h
Copy link
Member Author

m-e-h commented Nov 9, 2018

Yes @chrisvanpatten a CSS reset like I originally proposed in this issue would work perfectly for those that are enqueuing their styles without using the editor-styles theme_support method.

However when using the current CSS rewriting hack, the themes styles become more specific than our reset: .editor-styles-wrapper button > .components-button so in this case the theme's button styles will override the block button controls.

Perhaps it's worth throwing a reset at the top of the components anyway to cover those not using the editor-styles?
For those that are using editor-styles, I'm not particularly a fan of the current CSS rewriting either but I don't know that modifying it would make it any more fragile than it already is.

@m-e-h
Copy link
Member Author

m-e-h commented Nov 9, 2018

An example

button {
	min-height: 3rem;
}

Since the controls on the blocks don't specify min-height all of them will now be 3rem tall and appear as ovals instead of circles.

The following reset, placed before core defines any styles will ensure any properties not specified(by at least a single class selector) later in the stylesheet will retain their initial or inherited values.
(actual would be more verbose for browser compat)

.components-button {
	all: unset;
}

Now we add the wrapper to our theme's button style:

.editor-styles-wrapper button {
	min-height: 3rem;
}

It overrides our reset and our button control.

We can increase our reset specificity:

.components-button.components-button {
	all: unset;
}

But now our reset is overriding our own control styles.
Quite the can of worms.

@laurelfulford
Copy link
Contributor

#11505 is a duplicate report I made of the above issue: button styles in Twenty Seventeen's editor CSS, pulled into the editor with editor-styles, can change the font sizes in some Gutenberg buttons. Affected buttons include media upload buttons, and the edit buttons in reusable blocks:

image

The culprit is the .editor-styles-wrapper button selector @m-e-h mentions above, which lets the editor styles override Gutenberg's own styles, in part because they're embedded.

It makes more sense to move that discussion here, so closing #11505 in favour of this issue!

@chrisvanpatten
Copy link
Contributor

Hey folks -

I'm going to remove the documentation label on this, as I'm not entirely sure documentation is appropriate here. I was prepared to write this off as an edge case but if it's happening in the twenty(.*) themes, it probably warrants a closer look into how we can truly fix the problem (at least for buttons and possibly for other elements as well).

@chrisvanpatten chrisvanpatten removed the [Type] Developer Documentation Documentation for developers label Nov 13, 2018
@designsimply designsimply added the [Feature] Custom Editor Styles Functionality for adding custom editor styles label Nov 16, 2018
@designsimply
Copy link
Member

Tested and confirmed that using button as a selector when adding custom editor styles results in buttons in the editor UI being affected.

Steps to reproduce:

  1. Add the following lines to a theme's functions.php file.
add_theme_support( 'editor-styles' );
add_editor_style( 'editor-style.css' );
  1. Add the following sample CSS to the theme's editor-style.css file.
button {
	box-shadow: 10px 5px 5px red;
	min-height: 2.5rem;
}
  1. View the editor.

Result: buttons in the block toolbar and the empty/starter block are affected by the custom editor styles, but the expectation was to be able to use style elements such as h2 and button in custom editor styles without it affecting editor UI elements.

screen shot 2018-11-16 at 12 17 00 pm
Seen at http://localhost:8888/wp-admin/post-new.php running WordPress 4.9.8 and Gutenberg 4.4 f9ddd6882 and adding custom editor styles to the Twenty Seventeen 1.7 theme and tested with Firefox 63.0.1 on macOS 10.13.6.

@designsimply designsimply removed the Needs Technical Feedback Needs testing from a developer perspective. label Nov 16, 2018
@m-e-h
Copy link
Member Author

m-e-h commented Nov 20, 2018

I really think this is or will become a bigger issue than it seems.

When it comes to current themes, my guess is that many (most?) will simply add_theme_support and use the same editor stylesheet that they're currently using with the classic editor.

All of these themes will be pushing their button, input, textarea, label, select, and a styles into Gutenberg.

What kind of affect will that have on the UI?
I don't think it's safe to assume themes will all use sane defaults for element styles. Gutenberg functionality could easily be broken by one rogue float or display property.

So how do we protect the core components?
There are 2 options that I can think of:

Both of these options would be made much simpler and more scalable if we can add a uniform .class or data-att** on each element we need to protect.

  1. Modify the style wrapper to include a negation .editor-styles-wrapper button:not([data-elem])
    or
  2. Use a reset all: unset on the class or data-att at the beginning of the stylesheet. You could even add the class on the body and html elements to protect those.
    We'd probably want to do away with the Wrapper if we do this option since the specificity it adds makes the reset useless. Could still do the body and html mapping I guess.

I'm happy to test some things and create a PR but tracking down each editor element to target is a tedious task. I want to first make sure this is worth spending time on.

@youknowriad Do you think any of these options are worth exploring?
@jasmussen @chrisvanpatten ?

@chrisvanpatten
Copy link
Contributor

I really think this is or will become a bigger issue than it seems.

When it comes to current themes, my guess is that many (most?) will simply add_theme_support and use the same editor stylesheet that they're currently using with the classic editor.

I'm open to the idea that there's more we can do here, but it would be helpful to have more hard evidence that this will be a problem. It's definitely something we've seen happen but the reports have not been overwhelming.

As far as I can tell the main problematic elements are <button>s; and text inputs/selects to a lesser degree. I haven't personally seen any issues with text links or labels.

I'm just keen to make sure we don't over-correct here — consider me a devil's advocate for the purpose of getting to the root of the problem :)

@youknowriad
Copy link
Contributor

I'm open to improvements here but I agree that it remains to be seen if it's a big issue or not. Most themes are handling this pretty well for the moment.

Both of these options would be made much simpler and more scalable if we can add a uniform .class or data-att** on each element we need to protect.

My concern here is that we're building something context agnostic, which means our components are not WP only, people can also add third-party components in the mix. So this won't scale properly.

Use a reset all: unset on the class or data-att at the beginning of the stylesheet. You could even add the class on the body and html elements to protect those.
We'd probably want to do away with the Wrapper if we do this option since the specificity it adds makes the reset useless. Could still do the body and html wrapping I guess.

I think this is probably the most promising way to solve it but I think we should add it case by case to the components we want to "protect". like .components-button for instance.

@m-e-h
Copy link
Member Author

m-e-h commented Nov 20, 2018

Thanks for the responses!
You're both correct now that I think about it, there really hasn't been disastrous reports as a result of this. 😁
Maybe it isn't as big of an issue as I'm thinking.

It could always just be a small issue where themes nudge the UI elements enough to make things feel a little unpolished but not broken.
A different font-weight here or a line-height there. https://github.com/WordPress/WordPress/blob/5.0-branch/wp-content/themes/twentyseventeen/assets/css/editor-style.css#L35-L44
The Twenty themes do use sane defaults but just showing an example of these being in editor stylesheets

Maybe I'll start with the buttons and see if there's a simple side-effect free reset.

@jasmussen
Copy link
Contributor

jasmussen commented Nov 20, 2018

I think it's important to have a long term view, and a near-term view.

Long term, the utopian and possible not fully achievable goal should be that the front and backend look exactly the same and it should be trivial for the developer to make it happen, perhaps even just load the theme stylesheet itself into the editor.

Near term, we should strive to make the backend look like the frontend, but I think we should also be okay with a few inconsistencies here and there. This is quite advanced stuff if you think about it, and even just setting a font and a background color goes a long way.

Honestly in my opinion the Twenty themes, though perhaps especially TwentyNineteen go way farther than I expected possible or even necessary. I mean that's delightful, and it's a wonderful way of explaining the point of the whole thing in the first place.

@MichaelArestad
Copy link
Contributor

I really think this is or will become a bigger issue than it seems.

In more ways than are covered here. Blocks are going to use things like buttons and list elements as part of the interface or "edit mode" of a block instead of just for display. We're already seeing conflict between styles in Twenty Seventeen and Jetpack. I expect we will see broken block interface regularly until there are in place a clear set of guidelines for themes to follow. Even better would be some way to sandbox the edit mode of a block or specific classes.

Right now, the best options I can think of from the block-maker's perspective is to add !important to all of the styles. I really don't want to do this, but I have no way to predict what a theme will or wont style and how it will impact the interface of the block.

Here is the Twenty Seventeen example of colliding list element styles causing the layout to break as well as adding numbers:
image

To reiterate, I think this is a big problem. Folks aren't going to realize the theme is causing problems. They are going to make the jump that either Gutenberg or the block/plugin just don't work well.

@laurelfulford
Copy link
Contributor

Blocks are going to use things like buttons and list elements as part of the interface or "edit mode" of a block instead of just for display.

@MichaelArestad I'm sure this will introduce issues of its own (loss of semantic meaning being one of them!), but in this case, would it work to swap lists for something more generic like div? Lists are a lot more likely to have styles than buttons in existing editor CSS, but it seems really unlikely that a theme to touch div there.

@MichaelArestad
Copy link
Contributor

but in this case, would it work to swap lists for something more generic like div?

It could if we went heavy on aria attributes. This could be something to keep in mind.

@benoitchantre
Copy link
Contributor

svg icons inside the component toolbar inherit from body color.

@m-e-h
Copy link
Member Author

m-e-h commented Apr 23, 2019

@benoitchantre Are you using the the add_theme_support( 'editor-styles' );?
Do you have a button style in your CSS?

If so, my guess would be the svg icons are inheriting your button color, which may be set to inherit your body color.

The svg icons have this:

.components-icon-button svg {
    fill: currentColor;
    outline: none;
}

If you have a button style in your CSS it would be changed to this:

.editor-styles-wrapper button {
    color: inherit;
}

Which would override

.components-icon-button {
    color: #555d66;
}

And then yep, the svg icons would inherit your body/.editor-styles-wrapper color.

That's just one possible scenario. I'm sure there are other ways it could happen.

I'm sure there are plenty more cases like this where folks don't report it and just assume the UI is unpolished.

I'm hoping to put together a PR soon that would better protect the core components from things like this.

@MichaelArestad
Copy link
Contributor

I'm hoping to put together a PR soon that would better protect the core components from things like this.

Heck yes. How so?

@benoitchantre
Copy link
Contributor

@m-e-h Thank you for your detailed answer. Yes, I’m using add_theme_support( 'editor-styles' );. button styles are defined in the editor stylesheet because some styles are imported to reduce the work.

For information, this is in the context of WP rig v2.0

I have set some overrides to limit the consequences, but not every case is covered and a solution at the editor level would be better.

button,
select,
option {
	color: inherit;
	font-family: inherit;
	font-size: inherit;
	line-height: inherit;
	border-radius: inherit;
}

@m-e-h
Copy link
Member Author

m-e-h commented Apr 24, 2019

@MichaelArestad I've had a couple ideas that should work, ..in theory 😕 🤔
Haven't actually mapped anything out or tested anything yet. Maybe I'll work through it here. 😃

The trick is getting around the .editor-styles-wrapper added to user styles.

.editor-styles-wrapper <element> = 11 in specificity:
So our reset will need to be no less than a 12 in specificity and our component classes will need to be 13 or greater.

Most of the component classes are single class selectors so currently they're a 10 in specificity.
If we wrap the component SCSS @imports in :root { @import "./button/style.scss"; } specificity will bump up equally across the board and the single class selectors now have a 20 specificity.

Something like this:

reset

/* specificity 12 */
button.components-button:not(div) { 
    all: unset;
}

component

/* specificity 20 */
:root .components-button { 
    color: #222;
}

user style

/* specificity 11 */
.editor-styles-wrapper button { 
    color: green;
    box-shadow: 10px 5px 5px red;
}

color: green doesn't take because the component is higher.
box-shadow doesn't stick because the reset is higher.

With specificity this precise the order won't matter but ideally, I guess, the reset should come first.
The tedious part will be pulling out all the component classes that we have on html elements.

Anything look off with this? Anyone?

@m-e-h
Copy link
Member Author

m-e-h commented Apr 24, 2019

@benoitchantre it may or may not be helpful in your situation but I created a postcss plugin to get me by until core gets a fix for the element style leakage. https://github.com/m-e-h/postcss-editor-styles

@jasmussen
Copy link
Contributor

This is a really interesting idea, @m-e-h. Basically, provide a very strong reset, then raise the specificity on every subsequent editor style. Is that a correct interpretation?

If yes, what are our current blockers to implementing that with the current add_editor_style rewriting system we have? What magic do you have in your postcss plugin that we might want to canonize into the rewriting engine? What limitations are there on the current rewriting system you'd like to see addressed, and lastly, do we have any browser challenges to this issue?

Windows 7 is going out of support soon, and while I believe IE11 will still ship with Windows 10 for a while, there's an argument to be made that the new Chromium based Edge counts as a new version. Given a historical "support the last two browser versions", we COULD be looking at supporting Chromium Edge and Edge in the not too distant future, especially if there are solid arguments to doing so. (Note that I say that in the most unofficial capacity possible, this is mostly wishful thinking :).)

@m-e-h
Copy link
Member Author

m-e-h commented Apr 25, 2019

Basically, provide a very strong reset, then raise the specificity on every subsequent editor style. Is that a correct interpretation?

More or less.
The reset is only as strong as it needs to be in order to override the add_editor_style thing. And the specificity on the editor components needs raised in order to override the reset. Sort of a snowball effect.
The editor_style rewrite throws the whole cascade off balance. This, in some ways, is an attempt to bring balance back.

what are our current blockers to implementing that with the current add_editor_style rewriting system we have?

The current editor style rewrite only deals with theme styles. This doesn't touch theme styles.
I suppose we could point that rewrite script at the editor CSS and raise the specificity that way. That's actually not a bad idea. Since that way we know we're in WP so instead of using :root we could prefix with a class like wp-admin.

What magic do you have in your postcss plugin that we might want to canonize into the rewriting engine?

No magic. It uses :not() to negate editor component classes. It was the best I could come up with and control from a theme. A reset in the actual editor would be better.

What limitations are there on the current rewriting system you'd like to see addressed

Can't think of any "limitations" really. I wasn't around much when the rewrite thing was introduced so I don't know all the problems it was meant to solve. It does a good job of keeping theme CSS out of the admin sidebars (and throwing off the cascade 😉 ).

I realize we have a lot of excellent JS devs in here and it's a good thing we do!
But just because we have a hammer, every problem doesn't need to become a nail. 😄

do we have any browser challenges to this issue?

Isn't there always? unset doesn't work in IE but we could hand pick some common CSS properties and give them default values.

@jasmussen
Copy link
Contributor

The current editor style rewrite only deals with theme styles. This doesn't touch theme styles.
I suppose we could point that rewrite script at the editor CSS and raise the specificity that way. That's actually not a bad idea. Since that way we know we're in WP so instead of using :root we could prefix with a class like wp-admin.

This sounds good to me.

I'm not necessarily opposed to a theming system that goes outside the editing canvas (though I'm struggling to understand the use cases?), but yes I'm definitely looking at making it easier to make editor styles.

Can't think of any "limitations" really. I wasn't around much when the rewrite thing was introduced so I don't know all the problems it was meant to solve. It does a good job of keeping theme CSS out of the admin sidebars (and throwing off the cascade 😉 ).

The rewriting engine intends to solve these problems:

  1. CSS styles from wp-admin bleed into the editor. Anything from trivial things like the li having a bottom margin, to the h2 having a font size. By increasing the editor style specificity, your editor styles can override this bleed without you having to know about it. (I.e. you shouldn't have to write .editor-styles-wrapper in your editor style in order to both target the right canvas, and to manually increase specificity).
  2. Related to 1, it is a way for making existing editor styles potentially compatible with the new editor, or at least much easier to update.
  3. The editor-styles-wrapper class is also applied to block previews. This allows the same editor style to actually style things that are outside the editing canvas, without you having to know about it.

Screenshot of 3 in action:

Screenshot 2019-04-26 at 08 41 54

@m-e-h
Copy link
Member Author

m-e-h commented Apr 30, 2019

@jasmussen Thanks, as always for the thorough answer! That all makes sense

I'm not necessarily opposed to a theming system that goes outside the editing canvas (though I'm struggling to understand the use cases?)

I'm not talking about doing anything that we're not already doing. Many of the UI components, that this is meant to protect, aren't limited to the editing canvas.

I suppose for the purpose of this experiment we can start out assuming that everyone is using the editor-styles rewrite feature. I'll limit the reset to UI components within .editor-styles-wrapper.

Testing with just this:

:root .editor-styles-wrapper .dashicon:not(div),
:root .editor-styles-wrapper .components-button:not(div),
:root .editor-styles-wrapper .components-dropdown-menu__indicator:not(div),
:root .editor-styles-wrapper .block-editor-block-icon:not(div) {
	all: unset;
	box-sizing: inherit;
}

and I've bumped up the specificity evenly across 'block-editor', 'editor', 'components', and 'edit-post' stylesheets using postcss during the build process.

Changing the default themes to add_editor_style( 'style.css' ) and everything is looking pretty close to perfect.

twentyseventeen style.css without the reset
2017old

twentyseventeen style.css with reset
2017new

I'll put my rough first pass together in a PR for some feedback and testing. I'd never even looked at Gutenberg's build process till last night so I'm definitely going to need some direction on any conventions used there.

@jasmussen
Copy link
Contributor

That's very cool!

I'd never even looked at Gutenberg's build process till last night so I'm definitely going to need some direction on any conventions used there.

Please if there's ever any blockage here, you know where to find me. You can ask openly for help in the core slack, #core-editor, or you can ping me directly (@joen in core slack), or just here. Happy to help, so we can get your expert input!

@m-e-h
Copy link
Member Author

m-e-h commented May 1, 2019

Thanks @jasmussen !

PR created here #15377
Any testing anyone can do would be awesome!

It's still an early draft. I know I need to move some of the files around but it seems to work well.

@felixarntz
Copy link
Member

I've encountered similar issues with the components buttons as well as the permalink. As noticed before, it's problematic that Gutenberg's styles for editor controls are not wrapped in .editor-styles-wrapper, while the theme's editor styles are.

I like some of the suggestions made here, including the PR by @m-e-h, which seems hard to get right though. I know it's not so great from a pure CSS standpoint, but could we not wrap Gutenberg's styles in the .editor-styles-wrapper class as well (with a build step tool)? If the built stylesheet contained rules like .components-button, .editor-styles-wrapper .components-button { ... }, I think that would resolve most of the issues where generic theme editor styles (e.g. button, input, textarea, select { ... } override Gutenberg core component styles.

@jasmussen
Copy link
Contributor

My plate is all kinds of full, but this is still on my radar. I hope to resurrect the ongoing efforts here that were close, if I recall correctly.

@m-e-h
Copy link
Member Author

m-e-h commented Oct 1, 2019

I'd really like to explore some options here and hope I can find some time in the near future to do so. My extra time unfortunately disappeared around the time I started experimenting with solutions for this.

An editor-styles-wrapper would help some by increasing the specificity of the already declared style properties.
But I think the more vulnerable area here is when a theme or plugin uses style properties that haven't been declared on button, input, textarea, select. It's also a harder bug to track down since it has nothing to do with specificity like most would think.

For example, I don't think a wrapper would help at all with the case used at the start of this issue thread. #10178 (comment)

button {
	box-shadow: 10px 5px 5px red;
	min-height: 2.5rem;
}

@jasmussen
Copy link
Contributor

jasmussen commented Oct 8, 2019

Taking a look at this with slightly fresher eyes, it seems clear that while theme developers can work within the limitations currently present due to the CSS bleed, it's still onerous and they shouldn't have to. So while it may not be urgent to address this issue, addressing this issue should make virtually all developers lives easier.

In my experience, it's always useful to look at the utopian solution first, and then plot a course towards that. In my estimation, Shadow DOM was created for this exact purpose; to encapsulate an element from inheriting styles. It seems like if we were able to encapsulate the most in-editor used components, such as the Block Toolbar, the Placeholder, and the Mover control, we'd be mostly there.

I know Shadow DOM has been explored and abandoned, but I can't recall what was holding us back — was it the lack of support in IE11/Classic Edge? Given the new Chromium based Edge supports Shadow DOM, could we consider editor styles to be a progressive enhancement for modern browsers?

Similar to how Shadow DOM protects the styles of an element, the most promising approach from this thread seemed to involve strengthening the UI styles to insulate them from being overridden by editor styles. The main point of feedback against this approach, cc @youknowriad, was that ideally we should be be content agnostic: since we can't know what additional components a plugin developer might add to the UI, it seems confusing to just protect a few elements.

But would this not also be the case with Shadow DOM? And is there a way to simplify the API surface so that controls you do not want to inherit theme styles can be wrapped? I believe this is what @m-e-h is referring to here.

So it seems like defining what content agnosticism we want is key to unsticking this issue. Let's explore that a bit:

  1. There are UI elements that should never inherit CSS. Things like the <Button /> component, and many of the other — sliders, radio buttons, toggles checkboxes. Common to most of those is that there's a finite amount, and when plugin developers use them their experiences are consistent and arguably follow best practices.
  2. There are also UI elements where you DO want for styles to be inherited. For example, selecting the Image block pops out a caption field. This caption field should inherit the font specified by the editor style.

Based on the above two take-aways, it seems like a pragmatic appraoch would be to:

  • Create an allowlist of Block Editor UI components to protect
  • Create instructions for developers on how to protect their own custom elements

Potentially this would solve 80% of our problems.

And it might even plot us a path towards Shadow DOM. Long term, Shadow DOM would protect elements, but near term, higher element specificity would do so. Specifically this idea seems like it could be compatible with an allowlist of elements to protect:

Both of these options would be made much simpler and more scalable if we can add a uniform .class or data-att** on each element we need to protect.

To throw it out there, this could be an .is-protected class you add to components, or even data-protected-styles. For now we'd use those classes or attributes to strengthen their styles as @m-e-h suggested, but long term could we not also use those to target what to Shadow DOM encapsulate?

In the end this would provide developers with a set of components they know won't inherit CSS from theme styles, and instructions for how to protect their own elements in a similar way. Would love your thoughts, and would love to unstick this.

@paaljoachim
Copy link
Contributor

Can we get an update from someone on this very old issue?
Thank you!

@fabiankaegy
Copy link
Member

The current PR that seems to work in this issue is #33494

@mtias
Copy link
Member

mtias commented Jul 2, 2024

Unless I'm missing something, the introduction of the iframe for the editor content should be preventing the most common of these issues.

@mtias mtias closed this as completed Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backwards Compatibility Issues or PRs that impact backwards compatability [Feature] Custom Editor Styles Functionality for adding custom editor styles [Package] Editor /packages/editor [Type] Enhancement A suggestion for improvement.
Projects
None yet