-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Make editor UI elements immutable to outside styles #15377
Conversation
Temporarily add some styles from `block-library/` that don't get the added specificity but need it.
Thanks so very very much for this PR. The goal of it, and the approach, feels extremely solid and well-thought-out. It gets us that much closer to the virtues of Shadow DOM without actually using it. Impressive. It feels to me, and correct me if I'm wrong, that a good way to test this is to:
I tested TwentyNineteen in a bunch of browsers, and so far this feels solid. Chrome Mac: Firefox Mac: Safari Mac: Edge Windows: IE11 Windows: ☝️the toolbar looking slightly busted is a separate regression and looks the same in master. The bottomline from the above tests is that this feels solid enough to move into code review. Additional sanity checks from @kjellr would also be welcome. |
Thank you for your work on this @m-e-h. I have not tested it, but I have noticed that the permalink inherits from the links styles. Does this PR prevents changes on the permalink? |
It protects the permalink now @benoitchantre . Just added it to the reset. This might be the first time anyone, including the devs, gets to see the title permalink as it was designed to look. 😄 |
Thank you. |
:root .editor-styles-wrapper .dashicon, | ||
:root .editor-styles-wrapper .components-button, | ||
:root .editor-styles-wrapper .components-dropdown-menu__indicator, | ||
:root .editor-styles-wrapper .block-editor-block-icon { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the block library stylesheet used for these resets?
How do we decide which component to reset this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the block library stylesheet used for these resets?
I actually picked this spot kind of randomly, hoping to get some thoughts and suggestions from you and others.
The specificity is calculated so that the reset can be placed anywhere.
It could be loaded in a different editor.scss
, as a separate CSS file, or maybe inlined? Where do you think it belongs?
How do we decide which component to reset this way?
That's a good question. These were picked out of the browser inspector.
Manually is the only way I know of to grab the CSS class of every HTML tag used in an editor component.
Something I've been meaning to add here is a utility class so that 3rd party blocks could use the reset in their blocks. Something like :root .editor-styles-wrapper .editor-protected-tag
.
We could even add the .editor-protected-tag
class to known core elements rather than listing them individually here.
Any suggestions on naming that ".editor-protected-tag" class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is:
-
these components are not the only one that can be used in the editor since any component (block authors are free) can be used in the editor which means it's impossible to know which components to reset. If we were to stay, we limit ourselves to core components, it's not enough as we can add core components without thinking about these resets and it won't be a complete solution.
-
the core components (we have hundreds) do not depend on the editor, they don't know that the editor exist, most of them are in a generic package that can be used outside the editor which means we can't add these styles to the components themselves.
What happens if we remove these specific resets and only use the postcss plugin in a generic way?
'block-editor', | ||
'editor', | ||
'components', | ||
'edit-post', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you decide which package to include here?
Do we want this plugin to be applied to the CSS files shipped in the npm packages or is it meant to only affect the CSS files that ship in the plugin and Core?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you decide which package to include here?
Again, just sort of grabbed the files that I knew needed it here. I'm thinking everything except block-library/
should be super strong in specificity.
One that I missed that should be added here is the format-library/
styles. Also, maybe edit-widgets/
?
Do we want this plugin to be applied to the CSS files shipped in the npm packages or is it meant to only affect the CSS files that ship in the plugin and Core?
Another great question and one I'll need some guidance on. My initial thought is that the added specificity would be helpful in any use-case.
This is really interesting. I like the general idea, but I worry that this is going to overwrite a lot of existing theme styles by mistake. I wonder if it'd be possible to limit this extra specificity to specific components we know shouldn't be messed with: buttons, toolbars, panels, etc. A couple things I noticed for example: This overrides theme font styles for the title field for all themes I tried (Twenty Nineteen, Twenty Seventeen, Twenty Sixteen, and Twenty Fifteen). Here's Twenty Fifteen for example: Before: After: Similarly, this PR overrides all of the custom editor width and alignment styles I found in those themes. Here's Twenty Nineteen for instance: Before: After: Also, this is really minor in comparison, but worth noting: this PR also causes some weirdness with the heading toolbar icons: |
While we didn't solve all these issues yet. I think we've made progress in several ways to address this kind of issues. This PR probably needs to be redone entirely if we ever get to it. Let's close it. Please let me know if you think otherwise. |
Description
Any HTML element like
a
,button
,input
,svg
, etc, used in the Editor UI is currently left wide open for theme styles to accidentally skew it's appearance.Themes could easily override a
.components-icon-button {}
style by accidentally leaving abutton
style in theireditor-style.css
. Thebutton
+ the theme wrapper =.editor-styles-wrapper button {}
which could override anything on a single class component style.Another unpredictable and perhaps more likely scenario is where a theme style isn't overriding an editor style but is a new style property. Specificity on our components wouldn't help in this case.
Something like this would ruin the entire editor experience:
Some discussion about the problem here #10178
This PR hasn't been tested a ton but I wanted to get it up here so that it could be.
Basically it creates a strong reset for all editor UI component which uses an HTML element.
The only way to change any style property, from the initial browser default, on one of these elements is to do it with a more specific selector.
So we need to raise the specificity or our editor styles. But we need to raise all editor component styles equally so the current cascade isn't altered.
I've done this with postCSS during the build process. The resulting editor CSS looks like this:
How has this been tested?
I've tested this by changing the default themes to use
add_editor_style( 'style.css' )
for their editor styles.twentyseventeen
style.css
without the resettwentyseventeen
style.css
with resetTypes of changes
Checklist: