Fix Block Editor Iframe component to render in standards mode #38855

tomasztunik · 2022-02-16T11:55:46Z

Description

This fixes an issue where iframe component used by Block Editor would
render its contents differently to how they were rendered on the front-end.

Iframe document doctype is not set by default if it is not provided via iframe
src or srcDoc and not having doctype will cause browser to render iframe
contents in quirks mode. Appending doctype to existing document won't change
iframe's rendering mode. Document.write will overwrite the document with
a new one containing a correct doctype and will correctly render in stadards mode.

I've reviewed other areas of Gutenberg and it seems this is the only place
affected. All other places are correctly setting <!DOCTYPE html>.

More details provided in the related issue.

Fixes #38854

Testing Instructions

Go to Template or Template Part editor.
Add block or edit template part containing quirks mode affected CSS/HTML. (Ie. Mini Cart)
Confirm that contents render the same in the editor and in the front-end

Screenshots

Example error:

css	preview

Example valid after applying fix:

css	preview

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
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 (please manually search all *.native.js files for terms that need renaming or removal).
I've updated related schemas if appropriate.

github-actions · 2022-02-16T11:56:12Z

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @tomasztunik! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

ellatrix · 2022-02-16T14:31:01Z

@tomasztunik Did you find any resources about this issue? Is there no other way to achieve it? I tried to use document.write at some point and it caused the contents to load quite a bit slower, so that's worth comparing. Although if there's styling issue, we don't really have much choice. We also need to test in other browsers to confirm that the iframe works, we only have test coverage for Chrome.

tomasztunik · 2022-02-16T14:55:43Z

👋 @ellatrix - thanks for looking into this.

As document.write happens only once on Iframe setup while the iframe document is still empty I did not notice any performance degradation on my end. Tried to run performance profiling on impact but couldn't really notice difference - might be dev machine is just too much for it to make it significant.

Were your findings around using document.write around iframe as well or were they part of DOM rendering as part of page loading flow?

Before I made it to this solution I've tried to populate the initial state using srcDoc, but this conflicted with different areas as it seemed to happen async with react ref setup hook. After that I've learned the hard way that appending doctype to already rendered document doesn't update the document's rendering mode if it was already rendered. So if I tried to use document.implementation.createDocumentType('html', '', '') and prepending it to the document even though you'd be able to see it in inspector it would still remain rendered in quirks mode.

Confirmed the bug to be present on latest Safari (16.1) and Firefox (97) and that the fix works there as well.

tomasztunik · 2022-02-16T17:57:25Z

PS. re links - I've shared some resources on quirks mode on the issue (but I guess they were bit hidden in the description!):

https://quirks.spec.whatwg.org/
https://html.spec.whatwg.org/multipage/rendering.html (page search "In quirks mode")
https://www.quirksmode.org/css/quirksmode.html

and additionally:

https://html.spec.whatwg.org/multipage/syntax.html#the-doctype

ellatrix · 2022-02-17T12:08:59Z

Would it be possible to add an e2e test with some example CSS, then check the result with getComputedStyle?

tomasztunik · 2022-02-17T18:14:47Z

I've added E2E tests that validate that site editor iframe is running in a correct compatMode (ref). I decided to go with this approach as I thought quirks mode can have different impact as mentioned in the docs linked earlier, and some of its side effects could be masked with css reset etc. This way we'll be sure even if all quirks were masked :)

Tested to confirm that prior to code change it would fail the test with BackCompat value set.

Also tried to do it as unit tests with testing-libarary or enzyme rendering but it's super though to test iframes! 😵

(Noticed the tests have failed but it was some kind of timeout on one of the instances in unrelated area of the code)

jsnajdr · 2022-03-07T11:03:29Z

Which service generates the actual document inside the iframe? And can we fix that service to send a correct doctype in the first place?

If we replace the document with a new one (doctype + original) when readyState becomes interactive or complete, doesn't that mean we'll load the document twice? That includes running all the inline scripts again, loading all the resources etc.

tomasztunik · 2022-03-07T11:37:35Z

@jsnajdr by default empty iframe has no doctype (renders in quirks mode). And it's the responsibility of content provider to set it up. It can be done either via src (and it would come from remote content provider) or srcDoc if content provider is local. In theory it should be possible to setup initial document state - including doctype - via srcDoc as mentioned but this caused errors in unrelated areas because of how iframe control is handed over to react via React.Portal. Made some attempts toward that but the surface of the change started getting out of hand and would result in a much more complex code.

And regarding your concerns, this wouldn't cause the contents to setup twice as this happens before control is given back to react and before anything is rendered into the iframe, including scripts and styles as the iframe document override is synchronous and happens before these are processed.

Was trying to cpu profile that change prompted by @ellatrix but it wasn't even showing up in the logs and perceptive rendering speed doesn't seem to be affected.

jsnajdr · 2022-03-07T12:12:13Z

Looking at the Iframe component that's being modified, the content of the iframe is managed by React, which renders a React UI into it, using a portal. It doesn't really support a src attribute, because it would overwrite the iframe markup with its own React UI?

So the task to do is to setup the initial empty document with a correct <!DOCTYPE html> tag? Yeah, then it makes sense, although instead of document.write we could write just document.innerHTML = '<!DOCTYPE html>' or init the iframe from a data URL:

<iframe src="data:<!DOCTYPE html>"/>

Appending doctype to existing document won't change iframe's rendering mode.

This sounds like the document.innerHTML solution won't work and we really need to document.write?

I've tried to populate the initial state using srcDoc, but this conflicted with different areas as it seemed to happen async with react ref setup hook.

srcDoc would be preferrable to document.write, and seems very similar to <iframe src="data:..."/>. Yes, it's probably async because document.srcDoc = '...' doesn't load immediately, but behaves as if it was loaded from a "virtual URL". We'd need to wait for some load event before calling setIframeDocument and triggering React render into the portal. But we already do that, because the rendering happens inside setDocumentIfReady?

Where were you calling document.srcDoc = when testing? The load event is probably too late.

tomasztunik · 2022-03-07T12:22:41Z

Modifying existing document was my first instinct as well as mentioned here but unfortunately once document rendering mode is set appending doctype doesn't change it despite doctype being visible in DOM.

I was passing static srcDoc as prop to the iframe element with correct doctype included but as you mention - this caused initial empty document to load and then be asynchronously replaced with the one provided by srcDoc - you've described it better than I did before I think - so document replace happened regardless. This would force us to add complexity in waiting for iframe to be ready by having to skip first initialisation with wrong compatMode and wait for the correct one to be loaded. This replace was causing React which already took over content to act as if someone pulled carpet from under its legs.

The way proposed here seemed cleaner to me from code complexity as it was doing the same just made it synchronous within the iframe binding/setup method as we were already giving the control of the iframe to react and felt like this will keep the concern encapsulated in this method.

If you think this is not the case I could try to look again at the srcDoc approach. Now that we talk about that I'm thinking if waiting for right compatMode rather than interactive state wouldn't be enough... Let me think if we should give it a go :)

jsnajdr · 2022-03-07T12:34:46Z

If you initialize the iframe as <iframe srcDoc="..."/> or <iframe src="data:..."> then it should load that document right away, without loading an empty (about:blank) document first. I think it's worth trying one more time, and if it doesn't work, trying to observe carefully what exactly does not work.

Using document.write is acceptable, I just would write it as:

document.open();
document.write( '<!DOCTYPE html>' );
document.close();

I think this is going to be a little less efficient because the API is synchronous -- like other legacy things like synchronous XHR requests or window.alert. But other than that, it's fine.

The Iframe component should be also modified to not accept a src prop, because from what I see it won't work. The component creates its own document and manages it.

tomasztunik · 2022-03-07T12:36:58Z

I think it's worth a try. Will look into it bit later today - thanks for joining in @jsnajdr and giving a new perspective.

tomasztunik · 2022-03-08T08:15:07Z

I think this looks cleaner now 🧹

Thanks for the review @ellatrix and @jsnajdr. Was quite a trip into the iframe land :)

packages/block-editor/src/components/iframe/index.js

jsnajdr · 2022-03-08T09:40:41Z

packages/block-editor/src/components/iframe/index.js

+			const { readyState, documentElement, compatMode } = contentDocument;
+
+			// As srcDoc loads contents asynchronously this will cause the iframe to
+			// load documents twice. We need to hook react to the correct contentDocument


When you're testing, does the document really load twice for you? With srcDoc, it should only load once.

When I run this little test app:

function App() { const elRef = React.useRef(); React.useEffect(() => { function onLoad() { const doc = elRef.current.contentDocument; console.log("load", doc.readyState, doc.compatMode); } elRef.current.addEventListener("load", onLoad); return () => elRef.current.removeEventListener("load", onLoad); }, []); return <iframe ref={elRef} srcDoc="<!doctype html>" />; } ReactDOM.render(<App />, document.getElementById("root"));

I get only one load event logged:

load complete CSS1Compat

Double checked by logging and breaking to inspect the iframe node and this is what we get:

~~I'm thinking if it wasn't because first load fires before ref is set and useEffect executes?~~

Or more likely it's a browser difference - Found this exploration https://stackoverflow.com/questions/10781880/dynamically-created-iframe-triggers-onload-event-twice/38459639#38459639 it seems it might be Chrome/webkit only?

On Firefox I get:

So a little bit different handling but still load fires twice

On my minimal example, I get only one load event even for Firefox: load complete CSS1Compat. I'll try it with the full Gutenberg editor.

I'm thinking if it wasn't because first load fires before ref is set and useEffect executes?

Yes, the load certainly fires right after the element is added to DOM, which is before refs are set and effects executed. But the iframe element has the srcDoc attribute from the very beginning, and therefore should load only once.

Ok got it. First one is initial state not onload event. It is not triggered by the listener but by the manual call to the setDocumentIfReady handler.

https://github.com/WordPress/gutenberg/blob/21e2d2249e8a478fd4c6b49e44c1c170e5c44b2d/packages/block-editor/src/components/iframe/index.js#L213-L220

We have the comment here that load happens only in firefox but now with srcDoc in place it will fire everywhere it seems.

Should we remove L213-215 and leave only the onload event? This way we could remove the comment here and remove or simplify the comment inside the setDocumentIfReady and maybe remove the ready check before we initialise the iframe content.

Should we remove L213-215 and leave only the onload event?

Yes, it's most likely guaranteed that the load is async, and the useRefEffect callback is always called before the load finishes. We can rely on the load event to fire and avoid checking synchronously.

Yes, probably good to only use the load event now. The reason we didn't use the load event for all browsers was because we could start loading the editor much quicker.

I've removed the comment and direct call and removed the compatMode check from ready check. I believe with how much we've simplified it the simple comment around iframe event binding and reason behind srcDoc with E2E tests explaining it a bit more and guarding for ensuring we have the right compatMode should be enough.

ellatrix

Could you change it so we're just using the load event?

jsnajdr

Looks good! I posted two comments that are more about code quality rather than functionality. 🚢

packages/block-editor/src/components/iframe/index.js

jsnajdr · 2022-03-09T12:08:20Z

@tomasztunik Can you rebase the PR branch onto the latest trunk? That could make the React Native e2e tests green again.

Iframe document doctype is not set by default and not having doctype will cause browser to render iframe contents in quirks mode. Appending doctype to existing document won't change iframe's rendering mode. Document.write will overwrite the document with a new one containing a correct doctype and will correctly render in stadard mode. Fixes WordPress#38854

it will visit the editor and ensure that iframe is rendering in standards CSS1Compat rendering mode. If it had BackCompat set it would render in quirks mode.

tomasztunik · 2022-03-09T12:36:10Z

@jsnajdr seems it worked 👍

jsnajdr · 2022-03-09T12:58:45Z

Nice! Now only @ellatrix's 👎 blocks the merge. There used to be a "dismiss merge" action (the requested changes have been done) in the GitHub UI, but I don't see it now.

tomasztunik · 2022-03-09T13:14:56Z

It is under the ellipsis next to reviewer name but if it's available depends on how the repository is setup.

Thanks again for great review session on this to both of you! I feel like I've recapped most of my knowledge on iframes and learned some on top :)

tomasztunik · 2022-03-21T09:53:34Z

Hey @ellatrix, Could you please do the final check? Appreciated!

gziolo · 2022-03-24T11:59:22Z

Everything looks green on CI. I see that all feedback was addressed so let's get this one in.

The site editor was initiating network requests that weren't routed through the service worker. That's a known bug in Chrome, Firefox, and a few other browsers based on these two. The issue is only with the iframes using srcDoc and src="about:blank" as they fail to inherit the root site's service worker. Gutenberg loads the site editor using <iframe srcDoc="<!doctype html"> to force the standards mode and not the quirks mode: WordPress/gutenberg#38855 This commit patches the site editor to achieve the same result via <iframe src="/doctype.html"> and a doctype.html file containing just `<!doctype html>`. This allows the iframe to inherit the service worker and correctly load all the css, js, fonts, images, and other assets. Ideally this issue would be fixed directly in Gutenberg and the patch below would be removed. See #42 for more details

Gutenberg loads the site editor using <iframe srcDoc="<!doctype html>" to force the standards mode and not the quirks mode: WordPress/gutenberg#38855 This commit patches the site editor to achieve the same result via <iframe src="/doctype.html"> and a doctype.html file containing just `<!doctype html>`. This allows the iframe to inherit the service worker and correctly load all the css, js, fonts, images, and other assets. More details: #91 (comment)

The site editor was initiating network requests that weren't routed through the service worker. That's a known bug in Chrome, Firefox, and a few other browsers based on these two. The issue is only with the iframes using srcDoc and src="about:blank" as they fail to inherit the root site's service worker. Gutenberg loads the site editor using <iframe srcDoc="<!doctype html"> to force the standards mode and not the quirks mode: WordPress/gutenberg#38855 This commit patches the site editor to achieve the same result via <iframe src="/doctype.html"> and a doctype.html file containing just `<!doctype html>`. This allows the iframe to inherit the service worker and correctly load all the css, js, fonts, images, and other assets. Ideally this issue would be fixed directly in Gutenberg and the patch below would be removed. See WordPress/wordpress-playground#42 for more details

Gutenberg loads the site editor using <iframe srcDoc="<!doctype html>" to force the standards mode and not the quirks mode: WordPress/gutenberg#38855 This commit patches the site editor to achieve the same result via <iframe src="/doctype.html"> and a doctype.html file containing just `<!doctype html>`. This allows the iframe to inherit the service worker and correctly load all the css, js, fonts, images, and other assets. More details: WordPress/wordpress-playground#91 (comment)

tomasztunik requested a review from ellatrix as a code owner February 16, 2022 11:55

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Feb 16, 2022

gziolo added the [Package] Block editor /packages/block-editor label Feb 16, 2022

tomasztunik requested review from ntwb, nerrad and ajitbohra as code owners February 17, 2022 18:03

tomasztunik force-pushed the fix/block-editor-iframe-used-quirks-mode branch from 75deed4 to efa7458 Compare February 25, 2022 13:58

jsnajdr reviewed Mar 8, 2022

View reviewed changes

ellatrix mentioned this pull request Mar 8, 2022

Block editor: fix iframe rendering mode #39289

Closed

ellatrix requested changes Mar 8, 2022

View reviewed changes

jsnajdr approved these changes Mar 9, 2022

View reviewed changes

packages/block-editor/src/components/iframe/index.js Outdated Show resolved Hide resolved

packages/block-editor/src/components/iframe/index.js Outdated Show resolved Hide resolved

tomasztunik requested a review from ellatrix March 9, 2022 10:20

tomasztunik added 4 commits March 9, 2022 13:11

Tests validating site editor's iframe rendering mode

c74edb6

it will visit the editor and ensure that iframe is rendering in standards CSS1Compat rendering mode. If it had BackCompat set it would render in quirks mode.

restart CI after flaky test failure

512fa24

Solve the issue with srcDoc rather than document.write

d953032

tomasztunik added 3 commits March 9, 2022 13:11

Clean up the iframe srcDoc

271485d

Simplify the code based on findings

552ac83

cleanup + ensure we unbind the event

1473350

tomasztunik force-pushed the fix/block-editor-iframe-used-quirks-mode branch from 12287ca to 1473350 Compare March 9, 2022 12:11

gziolo added the [Type] Bug An existing feature does not function as intended label Mar 24, 2022

gziolo merged commit 02404fa into WordPress:trunk Mar 24, 2022

github-actions bot added this to the Gutenberg 13.0 milestone Mar 24, 2022

Mamaduka mentioned this pull request Mar 24, 2022

Tests: Apply style variations using labels #39725

Merged

talldan mentioned this pull request Mar 25, 2022

Fix global style variations previews not showing correct height #39737

Merged

youknowriad mentioned this pull request May 5, 2022

Fix the styles compatibility hook for the editor iframes #40842

Merged

Mamaduka mentioned this pull request Aug 2, 2022

Canvas height in focus mode should wrap document contents #42226

Closed

talldan mentioned this pull request Aug 19, 2022

Fix template part focus mode resizable editor height #43408

Merged

adamziel mentioned this pull request May 22, 2023

WordPress: Fix the srcDoc patch in WordPress block-editor.js WordPress/wordpress-playground#408

Merged

ellatrix mentioned this pull request May 26, 2023

Iframe: remove obsolete readyState checks #51003

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Block Editor Iframe component to render in standards mode #38855

Fix Block Editor Iframe component to render in standards mode #38855

tomasztunik commented Feb 16, 2022 •

edited

Loading

github-actions bot commented Feb 16, 2022

ellatrix commented Feb 16, 2022

tomasztunik commented Feb 16, 2022

tomasztunik commented Feb 16, 2022

ellatrix commented Feb 17, 2022

tomasztunik commented Feb 17, 2022 •

edited

Loading

jsnajdr commented Mar 7, 2022

tomasztunik commented Mar 7, 2022 •

edited

Loading

jsnajdr commented Mar 7, 2022

tomasztunik commented Mar 7, 2022 •

edited

Loading

jsnajdr commented Mar 7, 2022

tomasztunik commented Mar 7, 2022

tomasztunik commented Mar 8, 2022

jsnajdr Mar 8, 2022

tomasztunik Mar 8, 2022

tomasztunik Mar 8, 2022 •

edited

Loading

tomasztunik Mar 8, 2022 •

edited

Loading

tomasztunik Mar 8, 2022 •

edited

Loading

jsnajdr Mar 8, 2022

tomasztunik Mar 8, 2022 •

edited

Loading

jsnajdr Mar 8, 2022

ellatrix Mar 8, 2022

tomasztunik Mar 8, 2022

ellatrix left a comment

jsnajdr left a comment

jsnajdr commented Mar 9, 2022

tomasztunik commented Mar 9, 2022

jsnajdr commented Mar 9, 2022

tomasztunik commented Mar 9, 2022 •

edited

Loading

tomasztunik commented Mar 21, 2022

gziolo commented Mar 24, 2022

Fix Block Editor Iframe component to render in standards mode #38855

Fix Block Editor Iframe component to render in standards mode #38855

Conversation

tomasztunik commented Feb 16, 2022 • edited Loading

Description

Testing Instructions

Screenshots

Example error:

Example valid after applying fix:

Types of changes

Checklist:

github-actions bot commented Feb 16, 2022

ellatrix commented Feb 16, 2022

tomasztunik commented Feb 16, 2022

tomasztunik commented Feb 16, 2022

ellatrix commented Feb 17, 2022

tomasztunik commented Feb 17, 2022 • edited Loading

jsnajdr commented Mar 7, 2022

tomasztunik commented Mar 7, 2022 • edited Loading

jsnajdr commented Mar 7, 2022

tomasztunik commented Mar 7, 2022 • edited Loading

jsnajdr commented Mar 7, 2022

tomasztunik commented Mar 7, 2022

tomasztunik commented Mar 8, 2022

jsnajdr Mar 8, 2022

Choose a reason for hiding this comment

tomasztunik Mar 8, 2022

Choose a reason for hiding this comment

tomasztunik Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

tomasztunik Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

tomasztunik Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

jsnajdr Mar 8, 2022

Choose a reason for hiding this comment

tomasztunik Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

jsnajdr Mar 8, 2022

Choose a reason for hiding this comment

ellatrix Mar 8, 2022

Choose a reason for hiding this comment

tomasztunik Mar 8, 2022

Choose a reason for hiding this comment

ellatrix left a comment

Choose a reason for hiding this comment

jsnajdr left a comment

Choose a reason for hiding this comment

jsnajdr commented Mar 9, 2022

tomasztunik commented Mar 9, 2022

jsnajdr commented Mar 9, 2022

tomasztunik commented Mar 9, 2022 • edited Loading

tomasztunik commented Mar 21, 2022

gziolo commented Mar 24, 2022

tomasztunik commented Feb 16, 2022 •

edited

Loading

tomasztunik commented Feb 17, 2022 •

edited

Loading

tomasztunik commented Mar 7, 2022 •

edited

Loading

tomasztunik commented Mar 7, 2022 •

edited

Loading

tomasztunik Mar 8, 2022 •

edited

Loading

tomasztunik Mar 8, 2022 •

edited

Loading

tomasztunik Mar 8, 2022 •

edited

Loading

tomasztunik Mar 8, 2022 •

edited

Loading

tomasztunik commented Mar 9, 2022 •

edited

Loading