Better CSS reset style loader order #30034
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
requested review from
ajitbohra,
nerrad,
ntwb,
spacedmonkey and
TimothyBJacobs
as code owners
youknowriad
requested review from
jasmussen,
kjellr,
scruffian,
aristath and
gziolo
|
Size Change: +1.76 kB (0%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
|
possibly related or solves: #21785 |
youknowriad
added
[Feature] Custom Editor Styles
|
Amazing PR. This test well for me, and I'm excited to land it and the followup you mention. Are there any specific test instructions you'd like me to follow? Just in general testing, I see no before/after difference. |
|
@jasmussen well, it's too broad to have some specific instructions but I'd say just test several themes editor styles and check that think work at least as good as "trunk" For the follow-up, I'd need some help from @gziolo and @aristath |
|
This looks good!
What if on the editor, we add the reset as a dependency on all stylesheets enqueued? |
it would solve the issue but it means we register the style differently between frontend and backend which is a bit weird. I think the enqueuing is rendered is something that might be problematic in the long run but I wouldn't mind that approach though |
aristath
approved these changes
Mar 22, 2021
7 tasks
|
Just testing this in the navigation and widgets editors, this seems to cause some styling issues. trunk
this branch
|
|
The effect this has on the navigation screen seems okay insofar as it does render browser defaults. Perhaps the followup is to add the system font definition to just that screen? Happy to do so in a followup. |
|
So are we ok with this impact? I'm not sure I know the navigation screen enough to be able to say how best to address this. I guess before this PR, the navigation screen didn't use the "reset" at all and was just inhering wp-admin defaults which doesn't seem like a great thing either? |
|
I'm happy to reintroduce systemfonts to the navigation screen. That is what we're using now, and there is CSS just for the screen anyway. |
|
Ok Let's do this then. |
|
The main impact seems to be the block toolbar positioning and the font, so I'm sure we can fix those things. cc @noisysocks @kevin940726 as this also affects the widget editor for non legacy widget blocks. |
|
I'll try to push a PR for the widgets screen. |
7 tasks
|
Followup in #30085. |
This is the related ticket in WordPress core: We should prioritize it for WordPress 5.8 cycle. It looks like explored several things in https://core.trac.wordpress.org/attachment/ticket/50328/50328-skip-assets.diff, but eventually switched to other tasks. |
7 tasks
7 tasks
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related #29976
closes #21785
This PR remove the "editor-styles.css" file which was being used as "Admin Reset" and move it to a dedicated style that is enqueued explicitly instead of considering this style as a regular "theme editor style".
To clarify here, let's take a look at a paragraph style: We know that WP Admin defines a default size for the paragraph like this:
To fix that issue, we wrap the editor in a
editor-styles-wrapperclassname and we use this class to reset wp-admin styles. So we do this:Some blocks do define precise sizes for some paragraphs for instance:
Sometimes themes override default block styles for instance by doing:
In trunk, there's no difference between the "reset style" and the "theme editor style" , both these two stylesheets are considered "editor styles" and loaded as an injected style tag (so latest order), so the styles are being loaded in the following order:
(in the example above, it means the reset of the paragraph size will override the style of the comment block)
It is clear here that the "reset" style could inadvertently override some legit block styles from
block-library-style.css. So what I'm doing in this PR is that I'm changing the way this "reset" get enqueued, instead of considering it an editor style, it's just a regular stylesheet file that get inserted in the editor before the blocks stylesheet, so we get the following order:Notes
This PR doesn't solve everything :), there are two remaining things:
it would have overriden the latest comments block styles because the specificity is equal and the theme overrides are always loaded last. I believe there's no easy fix for this (aside the iframe). In these cases, the theme-author needs to provide an override for the comments block as well. (of course using theme.json config solve this too instead of providing a CSS file).