Iframe enqueuing: add editorStyle and warning #50091
Conversation
ellatrix
added
[Type] Bug
|
Size Change: +82 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
ellatrix
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
|
Flaky tests detected in 0522400b4c672ddd0f872df53c7c9b94c7e00e03. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4807099223
|
|
Closes: #37947. |
ellatrix
force-pushed
the
fix/iframe-enqueuing-and-warn
branch
from
0522400 to
083f79f
Compare
|
|
||
| if ( current_theme_supports( 'wp-block-styles' ) ) { | ||
| wp_enqueue_style( 'wp-block-library-theme' ); | ||
| } |
There was a problem hiding this comment.
I discovered that both wp-edit-site and wp-block-library-theme were also not correctly enqueued (compat warning).
There was a problem hiding this comment.
Actually. I don't see wp-block-library-theme enqueued anywhere (post or site editor).
There was a problem hiding this comment.
Upon the init action, gutenberg_reregister_core_block_types is called, which then calls gutenberg_register_core_block_assets, which dequeues wp-block-library-theme under some scenarios. A note for later to investigate. I need to look to another thing first.
There was a problem hiding this comment.
I don't see it listed there as a dependency:
gutenberg/lib/client-assets.php
Lines 315 to 325 in 85704f4
There was a problem hiding this comment.
I tested a bit how wp-block-library-theme works and this is what I see:
- If the theme doesn't declare theme support for
wp-block-stylesit is not loaded. Can be tested using TwentyTwentyThree. - If the theme declares support for
wp-block-stylesbut doesn't load editor styles viaadd_editor_stylesis loaded as a dependency ofwp-edit-blocks. I tested this by using TwentyTwentyTwo and removing support for editor styles. - If the theme declares support for
wp-block-stylesand loads editor styles viaadd_editor_stylesit is loaded, just not as a dependency ofwp-edit-blocks. Can be tested using TwentyTwentyTwo.
As core currently stands, this PR works as expected. The iframe cannot depend on wp-block-styles being enqueued as a dependency because of scenario 3 and checking for wp-block-styles support is fine. The wp-block-library-theme stylesheet loads for the scenarios 2 and 3 but not for 1. We can move forward with this PR.
A question I have that is separate from this PR is: why is there a need for separate scenarios 2 and 3? Isn't it the same as loading wp-block-library-theme stylesheet when the theme declares support for wp-block-styles? Pinging @aristath @gziolo and @ntsekouras as people that I see in the git blames for that area, in case they have thoughts.
There was a problem hiding this comment.
Thanks for investigating, let's continue this conversation in a separate issue or PR. :)
| @@ -197,6 +197,10 @@ Like scripts, you can enqueue your block's styles using the `block.json` file. | |||
|
|
|||
| Use the `editorStyle` property to a CSS file you want to load in the editor view, and use the `style` property for a CSS file you want to load on the frontend when the block is used. | |||
|
|
|||
| It is worth noting that, if the editor content is iframed, both of these will | |||
There was a problem hiding this comment.
Do we have any other documentation that could get linked where folks can learn about the iframed editor concept?
Aside, the included code example doesn't do a great job explaining the purpose of defining editorStyle. Maybe, we could test whether we can use a selector like .wp-block-gutenberg-examples-example-02-stylesheets.is-selected to show that it's directly related to the editing UI.
|
It seems like it's causing some tests to be flaky with the following ouput: I don't know why this is only happening on some attempts but not all though. Will be nice to resolve this before merging 🙂 . |
|
#50122 - I have a fix ready for the |
|
|
ellatrix
force-pushed
the
fix/iframe-enqueuing-and-warn
branch
from
1ffec94 to
6fc6bc4
Compare
ellatrix
force-pushed
the
fix/iframe-enqueuing-and-warn
branch
from
6fc6bc4 to
85704f4
Compare
|
So, one of the goals of this PR is that Once the test plugin was activated and the block added to the editor, I tested that:
I'll look into the other code changes and compare what's the stylesheet footprint (before/after). |
| wp_enqueue_script( 'wp-polyfill' ); | ||
| // Enqueue the `editorStyle` handles for all core block, and dependencies. | ||
| wp_enqueue_style( 'wp-edit-blocks' ); |
There was a problem hiding this comment.
Documenting what I see.
wp-edit-blocks corresponds with the editor styles of the core blocks (core, gutenberg). wp-block-editor-content and wp-block-library are dependencies of it, so no need to enqueue them explicity, hence we remove them.
By marking wp-reset-editor-styles as "done", they won't be enqueue despite also being a dependency.
ellatrix
force-pushed
the
fix/iframe-enqueuing-and-warn
branch
from
85704f4 to
2233c4b
Compare
|
For the warning, I tested by using this plugin which adds styles with and without |
|
This is the stylesheet footprint for this PR compared to
|
ellatrix
force-pushed
the
fix/iframe-enqueuing-and-warn
branch
from
2233c4b to
3eeddb6
Compare
|
Thanks for the review, @oandregal! |
|
I'm getting these warnings.
Also for core styles:
Also for my theme
What about dynamic styles? My requirements are:
1 is very volatile and there can be hundreds of them, so running them through Good solution for me would be to add slots to gutenberg/packages/block-editor/src/components/editor-styles/index.js Lines 93 to 116 in 58d9a1b Or the slots could be even more generic, for the start and end of the
gutenberg/packages/block-editor/src/hooks/style.js Lines 424 to 452 in cd234e8 |
|
Yes, that's a good point. How are you doing these things in a non iframed editor? |
|
Whether the editor is iframed or not, I have a HOC for every block (inefficient but I can't think of other ways) using the Then I have registered a return <>
<PluginSidebarMoreMenuItem ... />
<PluginSidebar ... />
<MyRenderStyles ... /> // this component renders everywhere
</>And in Would be much more simple if in return <>
<Fill name='blockEditor.head.start'>
<style id="my-reset">
{`.editor-styles-wrapper { margin: 0; background: ${defaultBackground}; }`}
</style>
</Fill>
{currentTemplateHasContainer && (
<Fill name='blockEditor.head.end'>
<style id="my-styles">
{`.editor-styles-wrapper { max-width: ${containerWidth}px; margin: 0 auto; }`}
</style>
</Fill>
)}
</> |
|
You add a HoC to every block? And your styles are just some dynamic "global" styles, right? A slot indeed seems to be the best solution. What styles are you resetting? Why do you need to reset styles? |
I have a theme/plugin combo that uses Tailwind, so I use Tailwind preflight CSS reset. Plugin options allow to modify Tailwind config which regenerates the preflight (via a web worker similar to play.tailwindcss.com). There are also configurable Tailwind plugins used that can extend what the preflight contains (forms for example). For example the prelight includes: h1,
h2,
h3,
h4,
h5,
h6 {
font-size: inherit;
font-weight: inherit;
}
blockquote,
dl,
dd,
h1,
h2,
h3,
h4,
h5,
h6,
hr,
figure,
p,
pre {
margin: 0;
}But in my theme h1, h2, h3, h4, h5, h6 {
margin-top: 2rem;
margin-bottom: 1rem;
}and in "styles": {
"elements": {
"h1": {
"typography": {
"fontSize": "3rem",
"lineHeight": "1"
}
},so it's paramount that the reset comes first. I do run that CSS through Along with the preflight I also have dynamic CSS variables there. Not entirely sure if they need to be early in the head, but I think so. What if you want to override one in theme This is why I think it's important to not only have the ability to add dynamic CSS to head, but also have some control about where it's placed.
I have it on the front end, having it in the back end helps iron out any inconsistencies. If it had to be static I could probably make it work but it being dynamic really simplifies things for me. While there is border-width: 0;
border-style: solid;
border-color: theme('borderColor.DEFAULT', currentColor);which is needed for borders to work correctly with Tailwind classes. But talking about dynamic, the example in my previous post: {currentTemplateHasContainer && (
<Fill name='blockEditor.head.end'>
<style id="my-styles">
{`.editor-styles-wrapper { max-width: ${containerWidth}px; margin: 0 auto; }`}
</style>
</Fill>
)}is oversimplified, if I'm only setting max-width why don't I just use a CSS variable? In reality I'm applying the CSS generated from Tailwind class names For blog posts I apply I do hope I've made a good enough case for the dynamic styles/slots/more control. Thanks for reading! |
|
@ska-dev-1 Thinking more about this, you should be using the block editor settings to add styles dynamically. This already has the flexibility of appending or prepending styles. Unfortunately, it's broken right now, but my PR #52767 should fix it. Could you have look and give you feedback? |
|
Styles look promising, how about fonts? Currently I also render dynamic fonts the same way I do styles so I can add/remove/swap fonts with live preview const url = `//fonts.googleapis.com/css2?family=${font.replaceAll(' ', '+')}${variantsQueryString}&display=swap`
return <link id={`ska-font-${id}`} href={url} rel='stylesheet' />There seems to be a method for svgs: Something similar could work for |
|
Could you use CSS imports? |
|
Ah, yes, absolutely, didn't even cross my mind. Thank you! |
What?
Previously: #49655.
Fixes: #37947.
I previously thought
editorStylehandles didn't need to be added to the iframe because I mistakenly thought these would contain styles for editor UI. It turns out this API is almost exclusively used for editor-only styles targeting blocks, so they must be added to the iframe.Note: they are currently enqueuing, but incorrectly through the compat layer.
This PR also logs a warning when styles are added through that compat layer. Now that
enqueue_block_assetscan be used, there should always be a way to add styles correctly.Why?
How?
Testing Instructions
The easiest way, I think, is to cherry-pick the change to
packages/block-editor/src/components/iframe/index.jsinto trunk and load an iframed editor. You'll see warnings logged for incorrectly added styles. Now checkout the whole PR and these warnings should be gone.Testing Instructions for Keyboard
Screenshots or screencast