Full site editor: load content in iframe #25775
Conversation
|
Size Change: +2.09 kB (0%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
ZebulanStanphill
added
[Feature] Full Site Editing
CSS Styling
ellatrix
force-pushed
the
try/fse-iframe
branch
from
527154e to
3986976
Compare
ellatrix
requested review from
ajitbohra,
chrisvanpatten,
gziolo,
nerrad,
ntwb,
spacedmonkey,
talldan,
TimothyBJacobs and
youknowriad
as code owners
ellatrix
requested review from
cameronvoell,
etoledom and
guarani
as code owners
ellatrix
force-pushed
the
try/fse-iframe
branch
from
6875e10 to
479d6a3
Compare
ellatrix
changed the title
|
This works surprisingly well considering the technical achievement it is. It's virtually a non issue. I noticed two things:
That shows a stock Chrome focus style around the iframe itself (.edit-site-visual-editor). It's inset 24px, which happens to match what I have in my editor style: Here's a little inspector debugging:
It's a little weird because the Speaking of the focus style, it's supposed to inherit the spot color from the wp-admin theme, I'm using "Modern":
CC: @youknowriad Finally perhaps more of a support question: I'm using a Google font, called "Mulish" in my theme and editor style. I've been enqueueing it like so: That's almost certainly the wrong way to do it. However what would be the right way to do it? Really astoundingly nice work here Ella! |
|
@jasmussen Thanks for testing! Good question about the fonts. The font should be a dependency of the editor styles, and then it should (or should be able to) be added properly in the iframe too. |
|
@jasmussen It looks like everything has to go through the |
|
I haven't had a chance to try quite yet. And I'd also assume it's something we can fix — I'm fine with having to do a little theme rejiggering to get my Google fonts to work. |
ellatrix
force-pushed
the
try/fse-iframe
branch
from
5fd560c to
2ab58ff
Compare
|
@jasmussen The focus padding issues should be fixed now. |
ellatrix
force-pushed
the
try/fse-iframe
branch
2 times, most recently
from
9717694 to
cf08cfe
Compare
ellatrix
force-pushed
the
try/fse-iframe
branch
2 times, most recently
from
fd77c51 to
83c8d1c
Compare
|
Took this for a spin today. It's REALLY close. And it's going to be so sweet to get this down. @MaggieCabrera and @scruffian you'll want to see this :) Most of the stuff I'd expect to break just works. It's just a really good experience. I did notice a few small things, though:
There's a small resize handle on this field. I don't see it in the master branch, not sure what's up there. As a potentially slightly bigger issue, we've got some scrollbar shenanigans going on with some particularly complicated templates:
As best I can tell what's going on here is this CSS: that accounts for the outermost scrollbar. Then there's the iframe itself, which scales to fit with these inline styles that are updated dynamically: After some debugging, I've narrowed it down to this: That appears to be output inline:
I'm not sure what added it, whether it was my own theme, TwentyTwentyOne Blocks (which I also tested), or something else. But the tricky thing is that 100vw does not include scrollbars in their math, which means anything that's this wide ironically causes scrollbars to appear. It looks like the iframe isn't actually the container that scrolls the content, but that a parent container that holds the iframe actually scrolls the content and the iframe is then resized with the browser or content. I'm going to assume there's a really good reason for that, but it also seems to be causing issues when you don't have auto-hiding scrollbars. |
|
Thanks for your work @jasmussen I'm very excited about this!
Looks like it's not on master and we are overriding this: with inline styles:
|
|
|
||
| $editor_styles = wp_json_encode( array( 'html' => ob_get_clean() ) ); | ||
|
|
||
| echo "<script>window.__editorStyles = $editor_styles</script>"; |
There was a problem hiding this comment.
Is there a better to do that without the new variable. Maybe by just reusing the "styles" property in the editor settings?
There was a problem hiding this comment.
Yeah, I can look into that
There was a problem hiding this comment.
I tried to do this, but the styles are not available yet at the time when the editor scripts are registered.
There was a problem hiding this comment.
I was not suggesting at the script registration, I was more suggesting block_editor_settings filter for post editor and gutenberg_edit_site_init for the site editor.
There was a problem hiding this comment.
That's what I mean. The block_editor_settings filter is run at script registration, which is too early. The settings are passed as an inline script initialising the editor.
packages/block-editor/src/components/block-list/block-selection-button.js
Show resolved
Hide resolved
| } | ||
| } ); | ||
| }, [] ); | ||
| } |
There was a problem hiding this comment.
Is this meant for stylesheets from themes and plugins not using the "official" editor styles way of enqueuing things? Should we add a deprecated call when we find one of these?
There was a problem hiding this comment.
That's correct! We could do that, yes.
There was a problem hiding this comment.
There's still some of our own stylesheets left as well. Maybe we should attempt this in a follow-up?
There was a problem hiding this comment.
yes, definitely, what are our own stylesheets that fall here?
There was a problem hiding this comment.
In the case of the site editor (I need to check for the post editor, but I think there are several), it's the edit-site package setting some default styles:
.wp-block {
max-width: 840px;
}
.wp-block[data-align=wide] {
max-width: 1100px;
}
.wp-block[data-align=full] {
max-width: none;
}It seems that there styles need to more in some sort of default stylesheet. They don't belong in the package I think.
packages/e2e-tests/specs/experiments/multi-entity-editing.test.js
Outdated
Show resolved
Hide resolved
|
@ellatrix This is absolutely amazing 😍 ! I can't wait for this to land. I took it for a spin locally. Some of the issues I noticed were reported above - apologies for duplicates. 📏 Heights are fighting
There's a "battle of heights" between the Even with clever dynamic height calculations added to the iFrame (for example, Layout styles, especially height related stuff, is always really tricky. This setup is basically attempting to do this is "Very Hard mode". I've noticed "jumpiness" when interacting with the editor. I think it comes back to the mismatched heights. I'm not sure how to resolve this yet. It will take some time poking and playing with various height styles (and perhaps dynamic calculation) to make all of the layouts happy. 🐑 Stylesheet CloningThe technique you have is 👍 . It's something I've done in the past. It's surprising how simple (and effective) that technique is (the CSS engine is pretty great). I don't think this would be a problem (at all). But... if... IF... there are dynamic styles (either stylesheets or inline styles) that are injected AFTER the fact (during the lifecycle of the editor's session), those won't be cloned over. The resolve this, we can create a MutationObserver to transfer any newly injected stylesheets (Again, this is only if there is a problem). 🎨 Theme vs EditorIt looks like the iFrame only encompasses the Canvas. All the things in the sidebar, topbar, etc... are outside of the iFrame, which is okay. Unfortunately, this subjects them to whatever stylesheets that may load. For example, if a theme has global styles, like
Fortunately, this won't be a problem when we successfully roll-out the upgraded Component System, as the style system is designed to be as stable as possible, regardless of environment (with zero side effects). That's all I've got! Hopefully this is helpful. |
|
@ItsJonQ Just wondering, did you test the branch before or after the removal of auto resizing? |
This is only meant to be a compatibility layer of stylesheets loaded on the server side. Ideally editor style sheets need to be set the official way, not just enqueuing the styles on the page.
I'm not sure I'm following. The iframe is meant to only contain content, not the rest of the editor. The styles for the content and editor UI are different. |
I'm not sure 🤔 . The latest commit I have for my local branch is Simplify
Ah! My apologies. My comment was unrelated to the iFrame content work. It was more of an observation 😊 |
|
Just tested the latest version which removes the auto resizing:
But both of those are existing issues that are only uncovered by the iframe. The resize handle removal was inherited from somewhere and that CSS should be moved to the appender instead. And the horizontal scrollbar is created by this: those -14px are responsible. But the removal of the autosizing makes this go from feeling sensitive, to feeling totally solid. I'm over the moon about this, and I think we can land it 🎉 — it's my strong impression that all the remaining issues are best solved as small separate PRs, and that the entire block editor will be better for it. Amazing work. |
ellatrix
force-pushed
the
try/fse-iframe
branch
from
2d87a7b to
a1616bd
Compare
| }, [] ); | ||
|
|
||
| return ( | ||
| <iframe | ||
| { ...props } | ||
| ref={ mergeRefs( [ ref, setRef ] ) } | ||
| ref={ useCallback( mergeRefs( [ ref, setRef ] ), [] ) } |
There was a problem hiding this comment.
I'm pretty sure, the useCallback here is useless.
ellatrix
force-pushed
the
try/fse-iframe
branch
from
72f3936 to
8ce248c
Compare
|
💪 |
| /** | ||
| * Gets the editor canvas frame. | ||
| */ | ||
| export function canvas() { |
There was a problem hiding this comment.
Not sure the reason behind naming it this way was worthwhile.
Seeing it used in a test as canvas(), it looks really bizarre.
There was a problem hiding this comment.
Any suggestions? This will be used a lot in all tests when the iframe will be used for the post editor, so it would be good if it's short like page so it doesn't trigger code reformatting for thousands of lines.
Description
Same as #21102, but for the full site editor.
See #20797.
How has this been tested?
The full site editor should work as before.
Screenshots
Types of changes
Checklist: