WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.58k stars 4.23k forks source link

Global styles custom CSS bleed into interface elements #46568

Open carolinan opened 1 year ago

carolinan commented 1 year ago

Description

When testing the new feature, I added the following CSS in the new "Additional CSS" field in the global styles sidebar in the Site Editor: * { color: red; }

This meant that the "add pattern" button got a red text color. We may need to increase the specificity of the styles for some interface elements.

Step-by-step reproduction instructions

Add the CSS as described above.

Screenshots, screen recording, code snippet

Screenshot 2022-12-15 at 06 25 43

Environment info

WordPress 6.1.1 Gutenberg trunk

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

glendaviesnz commented 1 year ago

Because this css causes the color to be applied direct to the SVG path it overrides the fill color that is currently applied to the parent:

svg path

the only way I could see to fix it would be to add an explicit color setting to the path in the editor styles. I am not sure what the implications are of apply the color direct to the path. @jasmussen, @jameskoster do you have any thoughts on potential implications of applying the block inserter color direct to the SVG path?

At a glance I couldn't see any other interface elements that were impacted.

jasmussen commented 1 year ago

Oh, good issue and thank you for the ping. Lots going on here. From what I can tell, the "editor styles rewriting" feature is already in play, rewriting body to .editor-styles-wrapper, whic means it's mainly in-canvas UI like the pictured inserter, but also theoretically placeholders and buttons inside would be susceptive to this.

While components could arguably have more specific CSS to account for this, especially those used in-canvas, ultimately so long as we allow editor styles and custom CSS (these both seem to fall into the same bucket as far as CSS bleed, wouldn't you agree?), the only alternative to more specific components CSS would be to embrace Shadow DOM on a per-component basis. Not trivial, but potentially worth it longer term. What do you think?

glendaviesnz commented 1 year ago

the only alternative to more specific components CSS would be to embrace Shadow DOM on a per-component basis. Not trivial, but potentially worth it longer term. What do you think?

That sounds like a 2023 project 😄 The custom CSS option has been moved under an experimental flag, so we have a bit more time to sort this.

jasmussen commented 1 year ago

Shadow dom definitely needs careful consideration. But I wonder if stronger CSS properties for in-canvas components might work as a simpler fix in the mean time? I think potentially @scruffian tinkered with this? Also maybe CC: @mirka

scruffian commented 1 year ago

Yes, I've encountered this a few times (buttons in placeholders is the example that springs to mind). Shadow DOM seems to be the ideal solution, but it's not trivial. IMO adding more specific CSS to the component seems to be a much simpler solution and I don't personally have any objections to it, but I seem to remember others not being on board in the past, though I don't recall the reasons.

mirka commented 1 year ago

I'm trying to understand the common scenarios that will arise. The example given is .editor-styles-wrapper * { color: red; }, which I don't quite get for a couple reasons, like why the universal * selector needs to be used (which is increasing the specificity needlessly), or why the normal Global Styles color settings aren't used instead.

I feel like it would usually be a bad idea for users to add broadly-scoped custom CSS (e.g. button { color: red; } instead of .some-block .my-button { color: red; }) in this kind of complex content environment to begin with, regardless of whether that would affect admin UI elements. So what are some kinds of legitimate broadly-scoped custom CSS that a user would want to add, that would negatively affect a UI element?

Then perhaps we can look into how components could defend against those common scenarios. This will likely only involve "setting an explicit CSS declaration" to the component, rather than "artificially increasing specificity of existing declarations" which will just start specificity wars everywhere.

For example, I think it's a good idea for certain UI components to set an explicit color to themselves, rather than just rely on inheritance. As long as it's a good idea for the component to have that style in the general case and not just in this specific Gutenberg use case, I'm all for that kind of improvement.

glendaviesnz commented 1 year ago

.editor-styles-wrapper { color: red; }, which I don't quite get for a couple reasons, like why the universal selector needs to be used

It doesn't have to be used at all, it is just an extreme example of what a user could do, rather than should do ... but without a doubt, some user somewhere will do it 😄. I guess the question is what lengths is it worth going to in order to prevent a user from messing up their own admin interface?

In my testing the inserter button was the only thing affected in this extreme example, so I don't think we need to make any major changes to prevent this at this point.

jasmussen commented 1 year ago

I guess the question is what lengths is it worth going to in order to prevent a user from messing up their own admin interface?

I think this is a key aspect, which also says to me that this concern shouldn't block the Custom CSS solution from landing. But we can speculate in common CSS that people might want to throw in there, and then harden our components in response. Just off the top of my head, here's what I imagine people could think up:

* {
    box-sizing: border-box;
}
body {
    margin: 0;
    padding: 40px;
}
svg {
    fill: blue;
}
figure {
    margin: 0;
}
button, input, textarea {
    border: 1px solid;
    border-radius: 10px;
}
button {
    background: black;
    color: white;
}

Of course that's entirely random, but what I'm saying is, if our in-canvas components (placeholders mainly) are able to handle such styles, I would think we're in a pretty good place. Without Shadow DOM we can't protect against fun things like these:

* {
    font-family: "Comic Sans MS" !important;
    font-size: 200px !important;
}
sabernhardt commented 1 year ago

As reported in Trac 57414, Twenty Ten sets a color on any (*) element in its editor-styles.css. At least four other themes have the same gray block-editor-inserter__toggle button: Nirvana, Atahualpa, Tempera and Parabola.

For the color, I suggested adding something like this below the :hover state in the button's styles:

        & * {
            color: $white;
        }
roborourke commented 3 months ago

I'm not sure I follow here why a Shadow DOM implementation would need to be done per block?

You could have a single Shadow DOM at the .editor-styles-wrapper level or one higher, where the stylesheets are injected to replicate the iframed editor behaviour.

Additionally you could set the .editor-styles-wrapper container as a containment context and rewrite @media ... ( to @container ( and get a true (or at least better) reflection of iframed responsive editor content when there are sidebars open.

jasmussen commented 3 months ago

I'm not sure I follow here why a Shadow DOM implementation would need to be done per block?

One reason, theme styles that are applied in the canvas can bleed into block UI that's also in that same DOM, such as placeholders.

roborourke commented 3 months ago

I see, so UI that's inline rather than in toolbars etc... I suppose that happens already though. You would need a 2nd level of Shadow DOM for editor UI specifically, and maybe introducing a new type of stylesheet for that :/ So, not that trivial, but I think it would be a step in the right direction at least, solving one problem if not the block UI.

jasmussen commented 3 months ago

Yep, it's non trivial, and partially solved using !important's, so not that urgent.