Skip to content
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

Full site editor: load content in iframe #25775

Merged
merged 1 commit into from
Jan 11, 2021
Merged

Full site editor: load content in iframe #25775

merged 1 commit into from
Jan 11, 2021

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Oct 1, 2020

Description

Same as #21102, but for the full site editor.

See #20797.

  • There would be no admin CSS bleed at all. This is something we’ve been struggling with since the beginning.
  • There would be no need to simulate media queries, which is arguably technically more difficult than using an iframe.
  • Relative units like (r)em and vw/vh just work.
  • For a full site, a theme stylesheet can be just dropped in the editor without any adjustment. I think this is important as it makes the life of theme authors much easier.
  • It's possible to have one selection per window, so one in the admin and one in the content. This is useful for e.g. the link UI where the selection in the content can be kept while the selection is also in an input element (for the URL). Maybe be useful in other cases.

How has this been tested?

The full site editor should work as before.

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@ellatrix ellatrix changed the title Try/fse iframe FSE: load content in iframe Oct 1, 2020
@github-actions
Copy link

github-actions bot commented Oct 1, 2020

Size Change: +2.09 kB (0%)

Total Size: 1.3 MB

Filename Size Change
build/block-editor/index.js 132 kB +1.2 kB (+1%)
build/block-editor/style-rtl.css 11.7 kB +332 B (+3%)
build/block-editor/style.css 11.7 kB +336 B (+3%)
build/block-library/index.js 151 kB -4 B (0%)
build/components/index.js 172 kB +135 B (0%)
build/compose/index.js 11.2 kB +1 B (0%)
build/edit-site/index.js 24 kB +77 B (0%)
build/edit-site/style-rtl.css 4.05 kB +11 B (0%)
build/edit-site/style.css 4.04 kB +9 B (0%)
build/editor/index.js 42.2 kB +2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.8 kB 0 B
build/api-fetch/index.js 3.42 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 664 B 0 B
build/block-directory/index.js 9.04 kB 0 B
build/block-directory/style-rtl.css 1.01 kB 0 B
build/block-directory/style.css 1.01 kB 0 B
build/block-library/blocks/archives/editor-rtl.css 196 B 0 B
build/block-library/blocks/archives/editor.css 196 B 0 B
build/block-library/blocks/audio/editor-rtl.css 194 B 0 B
build/block-library/blocks/audio/editor.css 194 B 0 B
build/block-library/blocks/audio/style-rtl.css 225 B 0 B
build/block-library/blocks/audio/style.css 225 B 0 B
build/block-library/blocks/block/editor-rtl.css 283 B 0 B
build/block-library/blocks/block/editor.css 283 B 0 B
build/block-library/blocks/button/editor-rtl.css 576 B 0 B
build/block-library/blocks/button/editor.css 577 B 0 B
build/block-library/blocks/button/style-rtl.css 552 B 0 B
build/block-library/blocks/button/style.css 552 B 0 B
build/block-library/blocks/buttons/editor-rtl.css 345 B 0 B
build/block-library/blocks/buttons/editor.css 346 B 0 B
build/block-library/blocks/buttons/style-rtl.css 419 B 0 B
build/block-library/blocks/buttons/style.css 419 B 0 B
build/block-library/blocks/calendar/style-rtl.css 319 B 0 B
build/block-library/blocks/calendar/style.css 319 B 0 B
build/block-library/blocks/categories/editor-rtl.css 210 B 0 B
build/block-library/blocks/categories/editor.css 209 B 0 B
build/block-library/blocks/categories/style-rtl.css 208 B 0 B
build/block-library/blocks/categories/style.css 208 B 0 B
build/block-library/blocks/code/style-rtl.css 216 B 0 B
build/block-library/blocks/code/style.css 216 B 0 B
build/block-library/blocks/columns/editor-rtl.css 300 B 0 B
build/block-library/blocks/columns/editor.css 299 B 0 B
build/block-library/blocks/columns/style-rtl.css 529 B 0 B
build/block-library/blocks/columns/style.css 528 B 0 B
build/block-library/blocks/cover/editor-rtl.css 508 B 0 B
build/block-library/blocks/cover/editor.css 506 B 0 B
build/block-library/blocks/cover/style-rtl.css 1.33 kB 0 B
build/block-library/blocks/cover/style.css 1.32 kB 0 B
build/block-library/blocks/embed/editor-rtl.css 594 B 0 B
build/block-library/blocks/embed/editor.css 595 B 0 B
build/block-library/blocks/embed/style-rtl.css 489 B 0 B
build/block-library/blocks/embed/style.css 489 B 0 B
build/block-library/blocks/file/editor-rtl.css 314 B 0 B
build/block-library/blocks/file/editor.css 313 B 0 B
build/block-library/blocks/file/style-rtl.css 352 B 0 B
build/block-library/blocks/file/style.css 352 B 0 B
build/block-library/blocks/freeform/editor-rtl.css 2.55 kB 0 B
build/block-library/blocks/freeform/editor.css 2.55 kB 0 B
build/block-library/blocks/gallery/editor-rtl.css 749 B 0 B
build/block-library/blocks/gallery/editor.css 750 B 0 B
build/block-library/blocks/gallery/style-rtl.css 1.17 kB 0 B
build/block-library/blocks/gallery/style.css 1.17 kB 0 B
build/block-library/blocks/group/editor-rtl.css 433 B 0 B
build/block-library/blocks/group/editor.css 432 B 0 B
build/block-library/blocks/group/style-rtl.css 190 B 0 B
build/block-library/blocks/group/style.css 190 B 0 B
build/block-library/blocks/heading/editor-rtl.css 248 B 0 B
build/block-library/blocks/heading/editor.css 248 B 0 B
build/block-library/blocks/heading/style-rtl.css 212 B 0 B
build/block-library/blocks/heading/style.css 212 B 0 B
build/block-library/blocks/html/editor-rtl.css 384 B 0 B
build/block-library/blocks/html/editor.css 385 B 0 B
build/block-library/blocks/image/editor-rtl.css 801 B 0 B
build/block-library/blocks/image/editor.css 800 B 0 B
build/block-library/blocks/image/style-rtl.css 569 B 0 B
build/block-library/blocks/image/style.css 570 B 0 B
build/block-library/blocks/latest-comments/editor-rtl.css 277 B 0 B
build/block-library/blocks/latest-comments/editor.css 275 B 0 B
build/block-library/blocks/latest-comments/style-rtl.css 382 B 0 B
build/block-library/blocks/latest-comments/style.css 382 B 0 B
build/block-library/blocks/latest-posts/editor-rtl.css 254 B 0 B
build/block-library/blocks/latest-posts/editor.css 254 B 0 B
build/block-library/blocks/latest-posts/style-rtl.css 634 B 0 B
build/block-library/blocks/latest-posts/style.css 634 B 0 B
build/block-library/blocks/list/editor-rtl.css 203 B 0 B
build/block-library/blocks/list/editor.css 203 B 0 B
build/block-library/blocks/list/style-rtl.css 201 B 0 B
build/block-library/blocks/list/style.css 201 B 0 B
build/block-library/blocks/media-text/editor-rtl.css 311 B 0 B
build/block-library/blocks/media-text/editor.css 311 B 0 B
build/block-library/blocks/media-text/style-rtl.css 642 B 0 B
build/block-library/blocks/media-text/style.css 640 B 0 B
build/block-library/blocks/more/editor-rtl.css 545 B 0 B
build/block-library/blocks/more/editor.css 545 B 0 B
build/block-library/blocks/navigation-link/editor-rtl.css 503 B 0 B
build/block-library/blocks/navigation-link/editor.css 504 B 0 B
build/block-library/blocks/navigation-link/style-rtl.css 805 B 0 B
build/block-library/blocks/navigation-link/style.css 803 B 0 B
build/block-library/blocks/navigation/editor-rtl.css 1.38 kB 0 B
build/block-library/blocks/navigation/editor.css 1.38 kB 0 B
build/block-library/blocks/navigation/style-rtl.css 274 B 0 B
build/block-library/blocks/navigation/style.css 274 B 0 B
build/block-library/blocks/nextpage/editor-rtl.css 507 B 0 B
build/block-library/blocks/nextpage/editor.css 507 B 0 B
build/block-library/blocks/paragraph/editor-rtl.css 236 B 0 B
build/block-library/blocks/paragraph/editor.css 236 B 0 B
build/block-library/blocks/paragraph/style-rtl.css 351 B 0 B
build/block-library/blocks/paragraph/style.css 352 B 0 B
build/block-library/blocks/post-author/editor-rtl.css 329 B 0 B
build/block-library/blocks/post-author/editor.css 329 B 0 B
build/block-library/blocks/post-author/style-rtl.css 303 B 0 B
build/block-library/blocks/post-author/style.css 303 B 0 B
build/block-library/blocks/post-comments-form/style-rtl.css 358 B 0 B
build/block-library/blocks/post-comments-form/style.css 358 B 0 B
build/block-library/blocks/post-content/editor-rtl.css 262 B 0 B
build/block-library/blocks/post-content/editor.css 262 B 0 B
build/block-library/blocks/post-excerpt/editor-rtl.css 206 B 0 B
build/block-library/blocks/post-excerpt/editor.css 206 B 0 B
build/block-library/blocks/post-featured-image/editor-rtl.css 453 B 0 B
build/block-library/blocks/post-featured-image/editor.css 453 B 0 B
build/block-library/blocks/post-featured-image/style-rtl.css 223 B 0 B
build/block-library/blocks/post-featured-image/style.css 223 B 0 B
build/block-library/blocks/preformatted/style-rtl.css 193 B 0 B
build/block-library/blocks/preformatted/style.css 193 B 0 B
build/block-library/blocks/pullquote/editor-rtl.css 304 B 0 B
build/block-library/blocks/pullquote/editor.css 304 B 0 B
build/block-library/blocks/pullquote/style-rtl.css 428 B 0 B
build/block-library/blocks/pullquote/style.css 428 B 0 B
build/block-library/blocks/query-loop/editor-rtl.css 217 B 0 B
build/block-library/blocks/query-loop/editor.css 216 B 0 B
build/block-library/blocks/query-loop/style-rtl.css 427 B 0 B
build/block-library/blocks/query-loop/style.css 429 B 0 B
build/block-library/blocks/query/editor-rtl.css 279 B 0 B
build/block-library/blocks/query/editor.css 279 B 0 B
build/block-library/blocks/quote/editor-rtl.css 195 B 0 B
build/block-library/blocks/quote/editor.css 195 B 0 B
build/block-library/blocks/quote/style-rtl.css 284 B 0 B
build/block-library/blocks/quote/style.css 285 B 0 B
build/block-library/blocks/rss/editor-rtl.css 307 B 0 B
build/block-library/blocks/rss/editor.css 309 B 0 B
build/block-library/blocks/rss/style-rtl.css 394 B 0 B
build/block-library/blocks/rss/style.css 393 B 0 B
build/block-library/blocks/search/editor-rtl.css 285 B 0 B
build/block-library/blocks/search/editor.css 285 B 0 B
build/block-library/blocks/search/style-rtl.css 454 B 0 B
build/block-library/blocks/search/style.css 456 B 0 B
build/block-library/blocks/separator/editor-rtl.css 229 B 0 B
build/block-library/blocks/separator/editor.css 229 B 0 B
build/block-library/blocks/separator/style-rtl.css 352 B 0 B
build/block-library/blocks/separator/style.css 352 B 0 B
build/block-library/blocks/shortcode/editor-rtl.css 603 B 0 B
build/block-library/blocks/shortcode/editor.css 603 B 0 B
build/block-library/blocks/site-logo/editor-rtl.css 321 B 0 B
build/block-library/blocks/site-logo/editor.css 321 B 0 B
build/block-library/blocks/site-logo/style-rtl.css 238 B 0 B
build/block-library/blocks/site-logo/style.css 238 B 0 B
build/block-library/blocks/social-link/editor-rtl.css 283 B 0 B
build/block-library/blocks/social-link/editor.css 283 B 0 B
build/block-library/blocks/social-links/editor-rtl.css 811 B 0 B
build/block-library/blocks/social-links/editor.css 810 B 0 B
build/block-library/blocks/social-links/style-rtl.css 1.44 kB 0 B
build/block-library/blocks/social-links/style.css 1.44 kB 0 B
build/block-library/blocks/spacer/editor-rtl.css 390 B 0 B
build/block-library/blocks/spacer/editor.css 390 B 0 B
build/block-library/blocks/spacer/style-rtl.css 184 B 0 B
build/block-library/blocks/spacer/style.css 184 B 0 B
build/block-library/blocks/subhead/editor-rtl.css 223 B 0 B
build/block-library/blocks/subhead/editor.css 223 B 0 B
build/block-library/blocks/subhead/style-rtl.css 210 B 0 B
build/block-library/blocks/subhead/style.css 210 B 0 B
build/block-library/blocks/table/editor-rtl.css 593 B 0 B
build/block-library/blocks/table/editor.css 593 B 0 B
build/block-library/blocks/table/style-rtl.css 501 B 0 B
build/block-library/blocks/table/style.css 501 B 0 B
build/block-library/blocks/tag-cloud/editor-rtl.css 237 B 0 B
build/block-library/blocks/tag-cloud/editor.css 235 B 0 B
build/block-library/blocks/tag-cloud/style-rtl.css 221 B 0 B
build/block-library/blocks/tag-cloud/style.css 221 B 0 B
build/block-library/blocks/template-part/editor-rtl.css 714 B 0 B
build/block-library/blocks/template-part/editor.css 714 B 0 B
build/block-library/blocks/text-columns/editor-rtl.css 220 B 0 B
build/block-library/blocks/text-columns/editor.css 220 B 0 B
build/block-library/blocks/text-columns/style-rtl.css 283 B 0 B
build/block-library/blocks/text-columns/style.css 283 B 0 B
build/block-library/blocks/verse/editor-rtl.css 194 B 0 B
build/block-library/blocks/verse/editor.css 194 B 0 B
build/block-library/blocks/verse/style-rtl.css 214 B 0 B
build/block-library/blocks/verse/style.css 214 B 0 B
build/block-library/blocks/video/editor-rtl.css 617 B 0 B
build/block-library/blocks/video/editor.css 617 B 0 B
build/block-library/blocks/video/style-rtl.css 303 B 0 B
build/block-library/blocks/video/style.css 304 B 0 B
build/block-library/common-rtl.css 1.01 kB 0 B
build/block-library/common.css 1.01 kB 0 B
build/block-library/editor-rtl.css 8.93 kB 0 B
build/block-library/editor.css 8.93 kB 0 B
build/block-library/style-rtl.css 8.53 kB 0 B
build/block-library/style.css 8.53 kB 0 B
build/block-library/theme-rtl.css 860 B 0 B
build/block-library/theme.css 860 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/core-data/index.js 15.2 kB 0 B
build/data-controls/index.js 830 B 0 B
build/data/index.js 8.97 kB 0 B
build/date/index.js 31.8 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.95 kB 0 B
build/edit-navigation/index.js 11.1 kB 0 B
build/edit-navigation/style-rtl.css 938 B 0 B
build/edit-navigation/style.css 944 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.64 kB 0 B
build/edit-post/style.css 6.63 kB 0 B
build/edit-widgets/index.js 26 kB 0 B
build/edit-widgets/style-rtl.css 3.22 kB 0 B
build/edit-widgets/style.css 3.22 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/editor/style-rtl.css 3.89 kB 0 B
build/editor/style.css 3.89 kB 0 B
build/element/index.js 4.63 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 6.75 kB 0 B
build/format-library/style-rtl.css 620 B 0 B
build/format-library/style.css 621 B 0 B
build/hooks/index.js 2.27 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.54 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.15 kB 0 B
build/list-reusable-blocks/style-rtl.css 629 B 0 B
build/list-reusable-blocks/style.css 628 B 0 B
build/media-utils/index.js 5.31 kB 0 B
build/notices/index.js 1.86 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 731 B 0 B
build/nux/style.css 727 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/reusable-blocks/index.js 2.92 kB 0 B
build/rich-text/index.js 13.5 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 3.02 kB 0 B
build/viewport/index.js 1.86 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@ZebulanStanphill ZebulanStanphill added [Feature] Full Site Editing CSS Styling Related to editor and front end styles, CSS-specific issues. labels Oct 1, 2020
@ellatrix ellatrix marked this pull request as ready for review October 2, 2020 12:23
@ellatrix ellatrix changed the title FSE: load content in iframe Full site editor: load content in iframe Oct 2, 2020
@jasmussen
Copy link
Contributor

This works surprisingly well considering the technical achievement it is. It's virtually a non issue.

I noticed two things:

Screenshot 2020-10-05 at 09 16 45

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:

html {
    padding: 24px;
}

Here's a little inspector debugging:

inset

It's a little weird because the iframe element appears to be correctly positioned so it would seem that the iframe focus style should go edge to edge (where by the way we could give it a nicer focus style, once that is consistent with the rest of the interface).

Speaking of the focus style, it's supposed to inherit the spot color from the wp-admin theme, I'm using "Modern":

Screenshot 2020-10-05 at 09 24 32

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:

add_action('admin_head', 'mulish_fonts');

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!

@ellatrix
Copy link
Member Author

ellatrix commented Oct 5, 2020

@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.

@ellatrix
Copy link
Member Author

ellatrix commented Oct 5, 2020

@jasmussen It looks like everything has to go through the add_editor_style API. The problem with that is that you can't add any dependencies? I'm not sure. Alternatively you could use @import but we should definitely make dependencies work.

@jasmussen
Copy link
Contributor

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
Copy link
Member Author

ellatrix commented Oct 5, 2020

@jasmussen The focus padding issues should be fixed now.

@ellatrix ellatrix force-pushed the try/fse-iframe branch 2 times, most recently from 9717694 to cf08cfe Compare December 7, 2020 13:31
@etoledom etoledom removed their request for review December 8, 2020 16:39
@ellatrix ellatrix force-pushed the try/fse-iframe branch 2 times, most recently from fd77c51 to 83c8d1c Compare December 10, 2020 10:34
@jasmussen
Copy link
Contributor

jasmussen commented Dec 10, 2020

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:

Screenshot 2020-12-10 at 13 01 23

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:

Screenshot 2020-12-10 at 13 04 11

As best I can tell what's going on here is this CSS:

.interface-interface-skeleton__content {
    flex-grow: 1;
    display: flex;
    flex-direction: column;
    overflow: auto;
}

that accounts for the outermost scrollbar.

Then there's the iframe itself, which scales to fit with these inline styles that are updated dynamically:

display: block;
width: 100%;
min-height: 100%;
height: 5142px;

After some debugging, I've narrowed it down to this:

.wp-block[data-align="full"], .wp-block.alignfull {
    max-width: 100vw!important;
    width: 100vw!important;
}

That appears to be output inline:

Screenshot 2020-12-10 at 13 20 11

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.

@MaggieCabrera
Copy link
Contributor

Thanks for your work @jasmussen I'm very excited about this!

There's a small resize handle on this field. I don't see it in the master branch, not sure what's up there.

Looks like it's not on master and we are overriding this:

.block-editor-default-block-appender textarea.block-editor-default-block-appender__content {
  resize: none;
}

with inline styles:

Screenshot 2020-12-10 at 13 36 06

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work here, the fact that the code doesn't look that complex where it's obviously a gigantic task says a lot.


$editor_styles = wp_json_encode( array( 'html' => ob_get_clean() ) );

echo "<script>window.__editorStyles = $editor_styles</script>";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better to do that without the new variable. Maybe by just reusing the "styles" property in the editor settings?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can look into that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to do this, but the styles are not available yet at the time when the editor scripts are registered.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
} );
}, [] );
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct! We could do that, yes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still some of our own stylesheets left as well. Maybe we should attempt this in a follow-up?

Copy link
Contributor

@youknowriad youknowriad Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, definitely, what are our own stylesheets that fall here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/block-editor/src/components/iframe/index.js Outdated Show resolved Hide resolved
packages/components/src/popover/index.js Show resolved Hide resolved
packages/e2e-test-utils/src/canvas.js Show resolved Hide resolved
packages/e2e-test-utils/src/show-block-toolbar.js Outdated Show resolved Hide resolved
@ItsJonQ
Copy link

ItsJonQ commented Dec 10, 2020

@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

Screen Shot 2020-12-10 at 10 29 42 AM

There's a "battle of heights" between the iFrame (element), iFrame content, and editor interface that contains the iFrame (and potentially window.document.body as well).

Even with clever dynamic height calculations added to the iFrame (for example, calc(100vh + ${heightOfTopBar})), things aren't coordinating.

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 Cloning

The 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 Editor

It 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 :root { font-weight: 300; }, the editor's UI will be affected.

Screen Shot 2020-12-10 at 10 30 32 AM

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.

@ellatrix
Copy link
Member Author

@ItsJonQ Just wondering, did you test the branch before or after the removal of auto resizing?

@ellatrix
Copy link
Member Author

The 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).

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.

It 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.

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.

@ItsJonQ
Copy link

ItsJonQ commented Dec 10, 2020

Just wondering, did you test the branch before or after the removal of auto resizing?

I'm not sure 🤔 . The latest commit I have for my local branch is Simplify

@ellatrix

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.

Ah! My apologies. My comment was unrelated to the iFrame content work. It was more of an observation 😊

@jasmussen
Copy link
Contributor

Just tested the latest version which removes the auto resizing:

  • There's still a small resize handle on the appender
  • There's still a horizontal scrollbar in some cases

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:

.block-editor-block-list__layout.has-overlay::after {
    content: "";
    position: absolute;
    top: -14px;
    right: -14px;
    bottom: -14px;
    left: -14px;
    z-index: 60;
}

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.

}, [] );

return (
<iframe
{ ...props }
ref={ mergeRefs( [ ref, setRef ] ) }
ref={ useCallback( mergeRefs( [ ref, setRef ] ), [] ) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure, the useCallback here is useless.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not, see #27686.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting :)

@ellatrix ellatrix merged commit e54a9ec into master Jan 11, 2021
@ellatrix ellatrix deleted the try/fse-iframe branch January 11, 2021 18:07
@github-actions github-actions bot added this to the Gutenberg 9.8 milestone Jan 11, 2021
@jasmussen
Copy link
Contributor

💪

/**
* Gets the editor canvas frame.
*/
export function canvas() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure the reason behind naming it this way was worthwhile.

Seeing it used in a test as canvas(), it looks really bizarre.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Needs Accessibility Feedback Need input from accessibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants