Iframe: avoid asset parsing & fix script localisation #52405
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
ellatrix
added
[Type] Plugin Interoperability
|
Now we will see what the tests are saying. |
|
Size Change: +30 B (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 244de6e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5484365015
|
bph
added
the
Backport to WP 6.7 Beta/RC
|
Tested in: Firefox All basic editor functionality works!
wp_enqueue_script( 'custom-scripty', get_template_directory_uri() . '/test.js' , array(), '1.0.0', true );
wp_localize_script( 'custom-scripty', 'test_test_test', array( 'dude' => 123 ) );
|
ramonjd
approved these changes
Jul 10, 2023
ockham
added
the
Backport to Gutenberg RC
I believe that is intentional, no? See: gutenberg/lib/compat/wordpress-6.3/script-loader.php Lines 64 to 68 in 7b6c184 |
fullofcaffeine
pushed a commit
that referenced
this pull request
Jul 10, 2023
* Iframe: avoid asset parsing & fix script localisation * Add e2e test for script localisation
Glad I asked then. Thanks for surfacing the relevant comment. 👍🏻 |
tellthemachines
pushed a commit
that referenced
this pull request
Jul 11, 2023
* Iframe: avoid asset parsing & fix script localisation * Add e2e test for script localisation
|
I just cherry-picked this PR to the update/beta-4-second-round branch to get it included in the next release: 730f7f1 |
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
tellthemachines
added a commit
that referenced
this pull request
Jul 11, 2023
* Post and Comment Template blocks: Change render_block_context priority to 1 (#52364) * Footnotes: fix lingering format boundary attr (#52439) * Footnotes: Fix incorrect anchor position in Firefox (#52425) * Scope CSS rules for the wp admin reset to js support only. (#52376) * Fix: Patterns & template parts: remove "apply globally" option from block settings (#52160) * Advanced styles panel: add an early return * Update index.js * Minor styling changes * Use array of features --------- Co-authored-by: George Mamadashvili <[email protected]> * make the body of the editor minimmum viewport height so that smaller contents maintain clickability (#52406) * Patterns: Add renaming, duplication, and deletion options (#52270) * Patterns: Update manage pattern links to go to site editor if available (#52403) * [Patterns] Separate sync status into a filter control (#52303) Co-authored-by: Saxon Fletcher <[email protected]> Co-authored-by: Glen Davies <[email protected]> * Patterns: Add missing decoding entities processing in Patterns and Template/Parts pages (#52449) * Fix document title icon appearance (#52424) * Quote block: Add transform to paragraph (#51809) * Add ungroup transform as transform to p * Lint * Update test and snapshot. --------- Co-authored-by: Juan Aldasoro <[email protected]> * remove sidebar group descriptions (#52453) * Patterns: alternative grid layout to improve keyboard accessibility (#52357) * add sync tooltip (#52458) * Patterns: Update section heading levels (#52273) * Ensure that the unsaved title is not persisted when reopening the modal (#52473) * Iframe: avoid asset parsing & fix script localisation (#52405) * Iframe: avoid asset parsing & fix script localisation * Add e2e test for script localisation * Update descriptions (#52468) * Footnotes: show in inserter and placehold (#52445) * Footnotes: show in inserter and placehold * Fix placeholder block membrane; fix copy; add icon, label --------- Co-authored-by: Miguel Fonseca <[email protected]> * Fix: Focus loss on navigation link label editing on Firefox. (#52428) * Update tooltip (#52465) * Refactor, document, and fix image block deprecations (#52081) * Refactor, document, and fix image block deprecations * Fix v5 attributes & supports * Fix v1 & v2 deprecation tests * Rename deprecated test descriptions * Respect custom aspect ratio (#52286) * add image width and height via css inline style, update width and height attrs to be string * keep width and height as attributes too, keep the attributes as numbers * updates image fixtures * RichText/Footnotes: make getRichTextValues work with InnerBlocks.Content (#52241) * RichText/Footnotes: make getRichTextValues work with InnerBlocks.Content --------- Co-authored-by: Miguel Fonseca <[email protected]> * Footnotes: save numbering through the entity provider (#52423) * Footnotes: save numbering through the entity provider * Add sup so no styling is needed at all * Migrate old format * Restore old styles, fix nested attribute queries * Fix anchor selection * Migrate markup in entity provider instead * Fix tests * Fix typo * Fix comment --------- Co-authored-by: Miguel Fonseca <[email protected]> * Revert "Post editor: Require confirmation before removing Footnotes (#52277)" (#52486) This reverts commit e6426ea. * List block: Fix selected type option (#52472) * Library - make pattern title clickable (#51898) * Use button inside title * Remove href * Preserve roving tab index * Fix link colors to match trunk $gray-600 * Remove redundant var * Amend colors as per review * remove old files again --------- Co-authored-by: scruffian <[email protected]> * remove status icon (#52457) * Rename block theme activation nonce variable. (#52398) --------- Co-authored-by: Bernie Reiter <[email protected]> Co-authored-by: Ella <[email protected]> Co-authored-by: Aki Hamano <[email protected]> Co-authored-by: Andrea Fercia <[email protected]> Co-authored-by: Carolina Nymark <[email protected]> Co-authored-by: George Mamadashvili <[email protected]> Co-authored-by: Andrei Draganescu <[email protected]> Co-authored-by: Aaron Robertshaw <[email protected]> Co-authored-by: Kai Hao <[email protected]> Co-authored-by: Saxon Fletcher <[email protected]> Co-authored-by: Glen Davies <[email protected]> Co-authored-by: James Koster <[email protected]> Co-authored-by: Rich Tabor <[email protected]> Co-authored-by: Juan Aldasoro <[email protected]> Co-authored-by: Miguel Fonseca <[email protected]> Co-authored-by: Jorge Costa <[email protected]> Co-authored-by: Alex Lende <[email protected]> Co-authored-by: Héctor <[email protected]> Co-authored-by: Petter Walbø Johnsgård <[email protected]> Co-authored-by: Dave Smith <[email protected]> Co-authored-by: scruffian <[email protected]> Co-authored-by: Peter Wilson <[email protected]>
|
This was also cherry-picked to the |
ockham
removed
the
Backport to Gutenberg RC
This was referenced Jul 19, 2023
This was referenced Aug 17, 2023
Draft
Mamaduka
reviewed
Dec 18, 2024
Comment on lines
+196
to
+198
| <body> | ||
| <script>document.currentScript.parentElement.remove()</script> | ||
| </body> |
There was a problem hiding this comment.
@ellatrix, can you provide some additional details about this hack? Why is it needed, and what does it solve?
It looks like some internal changes in React 19 affect it: #61521 (comment).
There was a problem hiding this comment.
What this does: the styles and scripts are HTML literal values that contain styles and scripts to load. Their source is the __unstableResolvedAssets setting: a HTML chunk generated by the PHP _wp_get_iframed_editor_assets function.
The initial document in the iframe is this html that does the preloading. It:
- Notifies the parent frame that it has loaded (the
_load()call) - Starts loading the styles and scripts
- After all the loading is finished, the
bodyelement is removed by thescriptinside it.
At the same time, React will render a portal markup inside this iframe once the _load callback is called. This is the code that does this:
const [ iframeDocument, setIframeDocument ] = useState();
const ref = useRefEffect( ( node ) => {
node._load = () => {
setIframeDocument( node.contentDocument );
};
} );
return <iframe ref={ ref }>{ iframeDocument && createPortal(
<body><Editor /></body>,
iframeDocument.documentElement )
}</iframe>;The rendering of the portal is triggered by the _load call. That means that the portal is rendered, and its body is rendered, while the scripts and styles in the original document are still loading.
Whether this all works depends on the subtle details of what exactly React does when rendering the portal. The original content of the iframe is:
<html>
<head>
<script><link>
</head>
<body>
<script>removeYourself();</script>
</body>
</html>
The portal is rendered into the <html> element, that's what iframeDocument.documentElement points to. What happens to the existing <head> and <body> markup? I don't know. Usually the elements into which portals are rendered are supposed to be empty.
I don’t fully understand how this hack was solving script localization bug
Script localization is done with inline script tags. But if you look at the original code before this patch, namely the loadScript function, it can handle only <script src="..."> tags that load from a URL. Now the HTML chunks with the script tags are directly pasted as strings into the html content. And inline script tags are working.
There was a problem hiding this comment.
In #61521 @tyxla changed the portal-rendering condition to:
<iframe>{ iframeDocument?.body && createPortal() }</iframe>adding a check if .body is there. But this is problematic. The node._load() call which triggers setState( iframeDocument ) happens in the iframe document's head. When that script is executed, however, the body element doesn't exist yet. The portal is not rendered. Later, when the body element is created, nobody tells React about that, there is no state update. The portal remains not rendered. Until some other unrelated state update triggers the component rerender and the .body check finally succeeds.
Try saving this into a .html file and open it in a browser:
<html>
<script>console.log('head body:',document.body)</script>
<body>hello</body>
<script>console.log('tail body:', document.body)</script>
</html>
The first console.log will log null, the second one will log a body element.
One very important thing about this is that between the ._load() call and the <body> element the HTML contains several scripts and styles loaded over the network. There is significant delay between them.
@Mamaduka created a Codesanbox toy example that doesn't have this property. There is nothing async there between the _load() and the <body>. The iframeDocument?.body code will reliably see the already existing body.
There was a problem hiding this comment.
I just verified on a toy app how React 18 and React 19 differ when rendering portals.
Load an iframe with this HTML as src in a blob URL:
<html>
<head>
<meta name=hello value=world>
<script>window.frameElement._load()</script>
</head>
<body>
<div>original</div>
</body>
</html>
and then render a portal into that iframe's html element:
createPortal(
<body>
<div>portal</div>
</body>,
iframeElement.contentDocument.documentElement
);React 18 creates two body elements, one with original content, other with portal content.
React 19 removes the original element and replaces it with body with portal content.
The head element is left intact, the meta and the script tag are still there. In both React 18 and 19.
So far this seems that React 19 fixed a bug with the double body element. And that we no longer need the self-destructing body element.
I still don't understand how this change could break some behavior and e2e tests. I'll continue looking into this.
There was a problem hiding this comment.
Thanks, @jsnajdr!
Btw, I also noted that difference in #61521 (comment). Sorry, I should've highlighted it.
There was a problem hiding this comment.
It's necessary to delete the body element, not just to avoid having two body elements, but also to prevent other scripts from attaching event handlers to the wrong body element.
There was a problem hiding this comment.
FWIW this wasn't breaking E2E tests, it was literally breaking the editor. I did change it a few months ago when testing with one of React 19 betas, so I'll need to come back to it and see if it's still necessary, and if yes, if there is a better solution. Hoping to come back to this early next year.
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.
What?
Same as #50913, but keeping the body as a React element so events on the body bubble up as React events.
Since we started using srcDoc and now a src with a blob, we can simply pass the script dependencies we get as HTML. We don't need to parse the HTML and then load every script one by one manually. This also fixes script localisation and generally inline scripts.
The other benefits is that script using the
loadandDOMContentLoadedevents now work normally.Why?
How?
While we require the body element to be replaced, we can do so immediately after it was created so it's not used by scripts to attach events to.
Testing Instructions
Check if the iframe works in all browsers. I will add an e2e test for script localisation.
Testing Instructions for Keyboard
Screenshots or screencast