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 affect the message inside the Warning component #40536

Closed
luisherranz opened this issue Apr 22, 2022 · 10 comments · Fixed by #40868
Closed

Block styles affect the message inside the Warning component #40536

luisherranz opened this issue Apr 22, 2022 · 10 comments · Fixed by #40868
Assignees
Labels
[Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended

Comments

@luisherranz
Copy link
Member

Description

I expect that UI warning messages (created using the <Warning> component) are not affected by the block styles (Block Supports and Global Styles).

Step-by-step reproduction instructions

  1. Add a block that contains a warning. For example, the current version of Post Comments: <!-- wp:post-comments /-->
  2. Change the typography of the block.

Screenshots, screen recording, code snippet

Screen.Capture.on.2022-04-22.at.10-10-52.mov

Environment info

  • WordPress 5.9, Gutenberg 13 RC1, TwentyTwentyTwo
  • Chrome 100
  • macOS 12.3.1

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

@ockham
Copy link
Contributor

ockham commented May 6, 2022

We now have two alternative PRs to fix this issue:

  1. @DAreRodz' PR (Block Editor: Prevent block styles from leaking into an inner Warning component #40847) tackles the problem at the level of the individual styles that can be modified from the Block Inspector. Folks have voiced the concern that if additional style options are added, we’ll also have to add overrides for those (i.e. requiring maintenance).
  2. @michalczaplinski's draft PR (Fix external styles leaking into Warning component #40868) uses the Shadow DOM to encapsulate that Warning component’s styling; the big question here if GB folks will be open to introduce that for that purpose. Also, it increases the PR surface, which might make it less likely to get backported for 6.0

I just had another idea (and will likely add a third PR to the mix):

Both this issue and #40826 originate from the fact that the Warning is displayed "inside" of the block.

But there’s precedent for Warnings that are displayed outside (such as Invalid Block markup), and they don’t suffer from those issues. So I'd like to try that; I think there's hook for that.

@michalczaplinski
Copy link
Contributor

michalczaplinski commented May 7, 2022

I've investigated a little bit and here's the summary of my findings (this is in addition to what Bernie mentions above)

  1. The third approach that Bernie tried in Post Comments: Add Warning via editor.BlockEdit filter #40898 I think would also suffer from the problem of style leaking if there is another parent block (e.g. Group) that can change styling via Block Supports

    2022-05-06_19-16-34.mp4
  2. Shadow DOM isolates and encapsulates the CSS is not the whole story and there are some exceptions that affect us. This is a good overview of which properties pierce through the Shadow DOM and which ones don't. TL;DR:

    • "inheritable" properties pierce through the shadow DOM
    • CSS custom properties also pierce through the Shadow DOM
  3. Because of the above, in Fix external styles leaking into Warning component #40868, I've realized that I'd need to add all: initial to the shadow host in order to achieve true encapsulation of styles. This same technique is also mentioned in the article (same as linked above).

    However, this approach has a drawback which is that the any styles that are used in the reset.css are not going to be applied.

    In the light of that, I think it might be best to only set initial on the inheritable properties.

  4. Finally, I've tried an approach without using custom elements or Shadow DOM. Just adding a <div style={ { display: 'contents', all: 'initial' } }> that wraps the <Warning/> component, seems to isolate the styles just as well! So, I think the way to go would be to set only the aforementioned inheritable properties to initial on that wrapping <div>.

@DAreRodz
Copy link
Contributor

DAreRodz commented May 7, 2022

I re-read Using Shadow DOM and, AFAIU, using a shadow tree doesn't prevent global styles from being applied inside in any way. What it does is the opposite. CSS styles defined inside the shadow tree are encapsulated there and ignored outside.

That's certainly handy, but not in this case. 😅

I guess the best approach is using all: initial, as @michalczaplinski shared in the comment above. I think that's the way of preventing style inheritance in CSS.

@luisherranz
Copy link
Member Author

using a shadow tree doesn't prevent global styles from being applied inside in any way

It's true that it doesn't prevent inheritance, but it prevents targeting its elements from the outside, though.

Look at how this p rule doesn't affect the paragraph inside the Shadow DOM.

p {
  color: blue;
}

https://codepen.io/luisherranz/pen/OJQNVVG


I didn't know all: initial could be used in the Light DOM. I think you're right; that's probably the best option here 🙂👍️

@DAreRodz
Copy link
Contributor

DAreRodz commented May 9, 2022

Thanks for your example, @luisherranz; pretty useful. 💯 I have to say that I don't know how styles are inherited yet, TBH. It looks like some rules are inherited, some others not?

If you, for example, add color: red; to the .my-element class, you will see that the paragraph inside the shadow tree turns red. 😅

Screenshot 2022-05-09 at 14 31 18

@ockham
Copy link
Contributor

ockham commented May 9, 2022

The third approach that Bernie tried in #40898 I think would also suffer from the problem of style leaking if there is another parent block (e.g. Group) that can change styling via Block Supports.

Good catch, thanks @michalczaplinski!

This could be considered a more generic issue (it might affect existing GB warnings, such as the "Invalid block content" one), but I agree that it kind of gets in the way of #40898 being an exhaustive fix that we can merge in the short term 😬

Edit: Yep, looks like it affects existing warnings. This is a Group with letter-case set to all-uppercase, containing one unsupported block:

image

@michalczaplinski
Copy link
Contributor

It looks like some rules are inherited, some others not?

Exactly. Some rules are inherited and others not. As you pointed out, the Shadow DOM prevents the styles inside of it from leaking out, but the inherited styles still affect the content inside of the Shadow DOM.

but it prevents targeting its elements from the outside, though.

That's also true! If we only use all: initial without the Shadow DOM, then technically some parent component could do something silly like div { background: red; } and mess with the styles of the <Warning />. But I think this is a minor concern.

@michalczaplinski
Copy link
Contributor

I have dug a little deeper still and updated #40868 with the approach of using just all: initial.

There are some caveats that I have documented there.

@luisherranz
Copy link
Member Author

luisherranz commented May 10, 2022

If we only use all: initial without the Shadow DOM, then technically some parent component could do something silly like div { background: red; } and mess with the styles of the . But I think this is a minor concern.

That's also true, although not what I had in mind when I made that comment 😄

There are a lot of tiny details with these APIs, so I've made a video to explain what I meant.

But I think the current solution is great as it is, @michalczaplinski!

EDIT: Now that I read your comment again, I'm not sure anymore if what you meant was what I meant already, or what you meant was that the shadow root can also be styled from the outside and conflict with the all: initial rule 😄🙏

@michalczaplinski
Copy link
Contributor

michalczaplinski commented May 10, 2022

yes, I'm sorry, I think I typed out my previous response too quickly! 😅

What I meant is what you mentioned in the video! That when you use the light DOM, it can still be styled from the outside, even if you add the inital: all. But that's not a big issue in practice because probably nobody is going to target the specific classes that are applied in the Warning component nor would they usually write styles targeting all p :

/* Don't think anyone's gonna write something like that outside of the Warning component */
.block-editor-warning__message {
  text-transform: uppercase;
}

/* And if you add styles like that in your site, you probably have a bigger 
   problem or you know what you're doing :)  */
p { 
  text-transform: uppercase;
}

@priethor priethor removed the [Status] In Progress Tracking issues with work in progress label May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended
Projects
None yet
5 participants