Fix flash of unstyled content when loading post editor #47808
Closed
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
|
Size Change: -6 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in e1367b1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4279647527
|
talldan
commented
Feb 7, 2023
| Promise.resolve() | ||
| ) | ||
| .finally( () => { | ||
| // When script are loaded, re-render blocks to allow them | ||
| // to initialise. | ||
| forceRender(); | ||
| } ); | ||
| }, [] ); | ||
| }, [ head, scripts ] ); |
There was a problem hiding this comment.
This is possibly a bit risky, as if scripts changes, it'd mean duplicate scripts being attached. It might be safer to remove the scripts dep, or keep track of already loaded scripts.
There was a problem hiding this comment.
Since we have the id attribute of the script, could we remove the existing script before creating a new <script> element? Or just skip creating that script, whichever makes more sense.
There was a problem hiding this comment.
I changed it back to work the way it did in trunk.
Your suggestion sounds good, but I think it's best to address the script loading in a separate PR, as it's unrelated to the bug.
0bba997
to
b814b93
Compare
talldan
commented
Feb 27, 2023
| <StyleProvider document={ iframeDocument }> | ||
| { children } | ||
| </StyleProvider> | ||
| </body>, | ||
| iframeDocument.documentElement |
There was a problem hiding this comment.
I think it would be good to use the same approach on the body:
- Avoid removing the existing iframe body
- Portal to the
bodyinstead of thedocumentElement.
With the changes in this PR, documentElement already contains a head child, and the portal is appending a body, which is something that I didn't know would work. There's no reference in the react docs to what happens if you create a portal on an element that already has children.
There was a problem hiding this comment.
Is there any way we could make react use the existing head and body?
There was a problem hiding this comment.
It does already use the existing head in this PR, but I haven't made it use the existing body. I'd be happy to do that.
There was a problem hiding this comment.
@ellatrix I've pushed a commit (7d3a6cb) that keeps the existing <body>. The main issue is that there were a number of refs being bound to the body, and now that its no longer rendered as a react element, that's harder to do.
The quick solution I came up with is to add an extra div around the content and bind the refs to that div. I know you hate divs 😄
There was a problem hiding this comment.
Oh no, divs!
What I meant with my comment is if we could perhaps use React hydration. I have no idea how it works exactly, but if it work for server side rendered HTML, couldn't it work for the initial HTML of an iframe?
There was a problem hiding this comment.
Ah, I see. I have no idea how it works either, but that's an interesting idea. I'll do some research.
There was a problem hiding this comment.
I tested it out, but I'm not sure it will be possible. hydrateRoot really seems like the equivalent of createRoot and is expected to be called once to create a completely isolated react tree (what I guess you could consider to be a react application).
So while I could vaguely get the iframe to hydrate, I think it creates a separate react tree in the iframe that is disconnected from the rest of the editor.
I guess this is the main advantage of createPortal it ensures the iframe's children are still part of the editor's react tree and so things like using context work correctly.
There's a branch here if you'd like to experiment - https://github.com/WordPress/gutenberg/compare/try/hydrating-the-iframe?expand=1.
…interactive readyState to avoid flash of unstyled content
7d3a6cb
to
b235bd2
Compare
|
Hey @talldan, I'll try to review this later today, sorry for the delay. |
|
@ellatrix Do you think you'll have a chance to review this one? |
|
Fixed by #51136 |
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?
Fixes #47724
How?
Previously the
Iframecomponent was removing its existing<head>element from the iframe and attaching a new one.From my testing, removing the existing
<head>causes thestyleAssetsloaded in thesrcDoc(see #46706) to also be removed. Not sure why, but possibly those styles end up attached to the<head>even though you can't see them when inspecting.This PR tries keeping the existing
<head>which seems to allow thestyleAssetsin thesrcDocto be retained and removes the need to add them again to the<head>.Testing Instructions
In trunk: The group block placeholder and appender are briefly unstyled
In this PR: There's no flash of unstyled content.
Screenshots or screencast
Before
Kapture.2023-02-02.at.11.56.57.mp4
After
Kapture.2023-02-07.at.11.47.32.mp4