Editor canvas iframe: use load event and default body element#76314
Merged
Editor canvas iframe: use load event and default body element#76314
Conversation
|
Size Change: -32 B (0%) Total Size: 6.89 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 35d1fa4. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/22855152734
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ciampo
reviewed
Mar 9, 2026
| const { contentDocument } = node; | ||
| const { documentElement } = contentDocument; | ||
| iFrameDocument = contentDocument; | ||
| setIframeDocument( contentDocument ); |
Contributor
There was a problem hiding this comment.
If I understand correctly, the iframe will now load after the load event fires, while before it was loading earlier (during parsing). That feels like a more correct behaviour to me, although I wonder if there are any edge case scenarios where it could create a race condition?
Member
Author
There was a problem hiding this comment.
Yes, the iframe content is now rendered (using a portal) only after load. At that time the document is ready: the body element exists (React 19 will correctly hydrate it) and theme styles are fully loaded (no flash of unstyled content).
When the render was happening during parsing, we had to create our own body element because the parsed one didn't exist yet--the parser didn't get that far in the document.
Comment on lines
+293
to
+302
| const bodyRef = useRefEffect( | ||
| ( node ) => { | ||
| if ( node.ownerDocument.defaultView ) { | ||
| unguardedBodyRef( node ); | ||
| return () => unguardedBodyRef( null ); | ||
| } | ||
| return () => {}; | ||
| }, | ||
| [ unguardedBodyRef ] | ||
| ); |
Contributor
There was a problem hiding this comment.
This guard implies that there is a scenario in which node.ownerDocument.defaultView is not available yet, but if that is true, the refs are never attached?
Member
Author
There was a problem hiding this comment.
Yes, this scenario happens when the iframe element is moved from one location to another. Typically, when displaying a list of patterns of templates with previews, the previews are iframes, and they are all in a list. When the sort order of the list is changed, the existing iframe is removed from DOM and re-inserted at another place.
This DOM removal unloads the iframe and forces it to load again. The ownerDocument.defaultView window object is destroyed and re-created. And it's briefly null. At the same time, React dev mode umounts and remounts refs and effects. Here you'll see the null defaultView. However, the final mount of the refs and effects happens at a time where defaultView is initialized again.
In short, this guard ensures that all the repeated extra re-mounts that React does in dev mode don't fail.
Contributor
There was a problem hiding this comment.
Got it. We should probably add more context to the code comment, since future maintainers (and AI agents) may get confused by this snippet
Maybe something like
// Guard against attaching refs when defaultView is briefly null,
// which happens during React strict mode re-mounts when an iframe
// is moved in the DOM (e.g., re-sorting pattern/template previews).
Member
There was a problem hiding this comment.
Are we going to use the same ref callback after React 19? When we switch useRefEffect to useCallback, it runs twice in StrictMode, which can cause side effects.
Member
Author
There was a problem hiding this comment.
With React 19, useRefEffect will be implemented as alias to useCallback. What useRefEffect does is essentially to implement ref cleanup functions in user space, but that is now supported natively.
it runs twice in StrictMode
I think the number of calls will be the same with both useRefEffect and useCallback. The useRefEffect implementation also uses useCallback internally.
Member
Author
There was a problem hiding this comment.
We should probably add more context to the code comment
OK, pushed a commit with an expanded comment 👍
ciampo
approved these changes
Mar 10, 2026
Contributor
ciampo
left a comment
There was a problem hiding this comment.
Changes look good. And the timing (early in the 7.1 dev cycle) will also give us che chance of iterate on any regressions that we may have missed.
🚀
ciampo
reviewed
Mar 10, 2026
| // before evaluating whether typing is to be stopped. Otherwise, | ||
| // typing will re-start. | ||
| timerId = defaultView.setTimeout( () => { | ||
| timerId = node.ownerDocument.defaultView.setTimeout( () => { |
Contributor
There was a problem hiding this comment.
If ownerDocument.defaultView can be null (especially durint react strict mode when iframe is detached/reinserted), is there a chance that this line (and other function calls on defaultView in the useTypingObserver hook) will throw an error? Maybe we should add an initial guard or use optional chaining?
Otherwise, we should consider adding a comment explaining why that's not a risk
Member
Author
There was a problem hiding this comment.
This guard is now centralized in the unguardedBodyRef/bodyRef logic. The inner hooks like useTypingObserver will never be called unless there is valid defaultView.
The alternative is to distribute the guards in each individual hook, with very unclear rules about where the guard is needed and where it's not.
Member
|
I think this is also a good opportunity to run perf tests a couple of times and confirm if iframe changes are responsible for the inserter hover metrics regression. IIRC, that was one of the theories in the main PR. |
35d1fa4 to
7a5157a
Compare
Member
Author
There will be two runs in the PR branch (initial and rebased), and then we can watch the results in |
tyxla
reviewed
Mar 10, 2026
Member
tyxla
left a comment
There was a problem hiding this comment.
Glad we managed to move this out of the React 19 migration 👏
I think it's looking good. Just want to be sure that we've documented all the subtle changes well enough.
Comment on lines
-129
to
-131
| const { ownerDocument } = node; | ||
| const { defaultView } = ownerDocument; | ||
| const selection = defaultView.getSelection(); |
Member
There was a problem hiding this comment.
Is it worth adding a comment at the top of the effect explaining why we don't destructure these? It's a subtle correctness detail that someone could "cleanup" later without realizing.
| @@ -1,4 +1,4 @@ | |||
| window.addEventListener( 'load', () => { | |||
| document.body.dataset.iframedEnqueueBlockAssetsL10n = | |||
| document.documentElement.dataset.iframedEnqueueBlockAssetsL10n = | |||
Member
There was a problem hiding this comment.
Is it worth adding a brief comment in the test plugin script explaining why this targets documentElement instead of body? May not be clear to future readers.
Member
Author
There was a problem hiding this comment.
I noticed these comments (asking for docs) only after hitting "auto-merge", apologies 🙂 I'll incorporate them into the main PR or a followup.
Member
There was a problem hiding this comment.
No problem at all, I appreciate the incremental approach we're taking here 👍
Closed
6 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Trying to isolate the iframe loading changes from #61521. Turns out it works also with React 18. The only additional change I had to make is to keep the auto-deletion of the HTML-parsed
bodyelement. React 18 portal will create a duplicatebodyelement. React 19 portal will (correctly) take over ownership of the existing one.