-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Editor styles: delete duplicate backwards compat CSS custom properties #60400
Editor styles: delete duplicate backwards compat CSS custom properties #60400
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: -94 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
Thanks for the PR! I may be missing something, but can't we just delete this without injecting backward compatible styles using My understanding is that the CSS defined in common.css for backward compatibility is built as |
Thanks for double checking here. My motivation was to limit the rules to the iframe - I didn't do much digging, but I'll experiment a bit more and get back to you. 🤞🏻 it's what you say - it'd be a much cleaner approach! |
Yeah, thanks for pointing that out 🤔 The styles are effectively doubled up in the editor In the top frame (window) I'm seeing both common and editor styles. Same in the editor iframe. It make me wonder why they're duplicated in editor.scss at all. Maybe for historical reasons that no longer apply? Deleting these duplicate rules in editor.scss seems like the way to go on the surface. I'll do that, but it would be good to double check with @oandregal just in case |
e68b95c
to
eadc8ae
Compare
--wp--preset--font-size--normal: 16px; | ||
--wp--preset--font-size--huge: 42px; | ||
} | ||
|
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.
Could we also delete in this file the following rules that are duplicated in common.scss?
:root .editor-styles-wrapper {
@include background-colors-deprecated();
@include foreground-colors-deprecated();
@include gradient-colors-deprecated();
}
:where(.editor-styles-wrapper) .has-normal-font-size {
font-size: var(--wp--preset--font-size--normal);
}
:where(.editor-styles-wrapper) .has-huge-font-size {
font-size: var(--wp--preset--font-size--huge);
}
?
They are loaded in the top-level and editor frames.
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.
Could we also delete in this file the following rules that are duplicated in common.scss?
From what I've tested, removing these styles should be fine. As I remember, @ellatrix was involved in the process of injecting the document stylesheet into the iframe editor instance. Maybe she has some knowledge.
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.
Thanks again. I'll remove them and ask for forgiveness later
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.
I have no idea. If both stylesheets are present in the iframe, it should be fine
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.
Thanks for checking!
Are we good to merge this do you think @t-hamano ? |
@ramonjd Sorry for the late confirmation! I think it's okay to ship this PR, but I would appreciate it if you could give me a few days, as I would like to test it a little more to make sure everything works as expected. I'm planning to test something like the following:
|
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.
LGTM!
As I investigated in this comment, the style(.min).css
in which the backward compatible styles were defined was already loaded in the editor, and I believe that the backward compatible styles defined in editor(.min).css
should be safe to remove. This PR prefers styles defined in theme.json whether the editor is iframed or not.
Below is an example of a font size with a huge
slug:
Non-iframed Post Editor
/* Inline style */
.editor-styles-wrapper {
--wp--preset--font-size--huge: clamp(1.333rem, 1.333rem + ((1vw - 0.2rem) * 0.555), 1.777rem)
}
:root {
--wp--preset--font-size--huge: clamp(1.333rem, 1.333rem + ((1vw - 0.2rem) * 0.555), 1.777rem);
}
/* style.css */
:root {
--wp--preset--font-size--huge: 42px;
}
Iframed Post Editor and Site Editor
/* Inline style */
:root {
--wp--preset--font-size--huge: clamp(1.333rem, 1.333rem + ((1vw - 0.2rem) * 0.555), 1.777rem);
}
/* style.css */
:root {
--wp--preset--font-size--huge: 42px;
}
Additionally, I tested the following:
- ✅ If font sizes with
huge
andnormal
slugs are not defined in theme.json, the default fallback value (16px/42px) will be applied. - ✅ Works correctly with Twenty Twenty-One. This means that even if font sizes with
huge
andfuga
slug are defined via theeditor-font-sizes
theme support rather than in theme.json, that value will take precedence.
:root .editor-styles-wrapper { | ||
@include background-colors-deprecated(); | ||
@include foreground-colors-deprecated(); | ||
@include gradient-colors-deprecated(); | ||
} |
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.
This style is not relevant to the problem we are trying to solve, so we may want to keep it just in case. It might be possible to remove it in the future. What do you think?
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.
Good call. I'll add it back.
@t-hamano Thanks so much for the thorough testing. Really a big help. |
…to the :root selector, output in the block editor iframe. Doing this effectively reduces specificity of backwards compatibility rules using :where() so they do not override any theme rules in :root, and it's compatible with the hierarchy on the frontend in packages/block-library/src/common.scss
…d in common.scss. It seems safe to delete the editor ones, which have a higher specificity and conflict with theme.json css properties with the same name.
bfa2a16
to
f3a6198
Compare
What and how? And why?
Fixes #59701 #56293
Exploring solutions to:
Alternative to:
Since #42084, theme CSS custom properties reside in
:root
, and notbody
.:root
, however, has less specificity thanbody
and won't overwrite some low-specificity backwards compat CSS custom properties.This PR deals exclusively with backwards compat CSS custom properties in the block editor:
This is a test to delete these properties as they are duplicates of those on common.scss, which appear both in the top and editor frames.
The
:root
selector is more appropriate as it reduces specificity of backwards compatibility rules using:where()
so they do not override any theme rules in:root
, and it's compatible with the hierarchy on the frontend:https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/common.scss#L7-L15
Testing Instructions
Go ahead and define some theme.json font sizes using the "normal" and "huge" slugs:
Now apply these font size presets to text elements:
In the editor, ensure that the theme.json preset values for
--wp--preset--font-size--normal
and--wp--preset--font-size--huge
overwrite the backwards compat values.To test that the backwards compat values still kick in, you can delete one or all of the presets from your theme.json and then refresh the editor.