-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fix YouTube Embed block from flickering and crashing in Safari #21225
Conversation
Size Change: -6 B (0%) Total Size: 861 kB
ℹ️ View Unchanged
|
Shout out to @mcsf for debugging and collab'ing on this one! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
While I'm fine with this approach and have approved the PR, I'm wondering (nitpicking?) about the type juggling that happens due to trySandbox
being called either with a boolean or with an event as a handler of onLoad
.
(It's the kind of thing we would have caught with type checking, which is always a little frustrating.)
In my mind, the event should not be passed to the function in the first place, so the alternative fix would be to have onLoad={ () => this.trySandbox() }
— or the equivalent using components methods, e.g. onLoad={ this.onLoad }
and this.onLoad = this.trySandbox.bind( this, false )
.
@mcsf I agree! I'll make that update :) |
5e45245
to
3618795
Compare
@@ -232,7 +233,7 @@ class Sandbox extends Component { | |||
title={ title } | |||
className="components-sandbox" | |||
sandbox="allow-scripts allow-same-origin allow-presentation" | |||
onLoad={ this.trySandbox } | |||
onLoad={ () => this.trySandbox() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you test this to make sure it's not causing unnecessary re-renders down the tree? Since this component is already a class, then to avoid re-renders we might as well add:
constructor() {
…
this.onLoad = this.trySandbox.bind( this, false );
}
…
<FocusableIframe
onLoad={ this.onLoad }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcsf I just tested! Both solutions (the one I pushed) and the one you proposed, have the same amount of renders for both Sandbox
and FocusableIframe
(4 for me).
As far as implementation, I'm happy to go with either 👍 .
Just lemme know :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking — merge away :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, actually, make it an explicit => this.trySandbox( false )
!
Thank you so much @mcsf !! I'll merge as soon as Travis is green and happy |
Description
This update fixes the issue where a YouTube video embed in Safari would flicker (and crash).
This issue is due to the inner
Sandbox
component forcefully re-rendering itself in an loop. This is why there's flickering. This is also why your computer fans may be spinning up (due to requests and stack overflow + memory leaks).Digging deeper, it appears it has to do with the
onLoad
callback from theiFrame
component.The
onLoad
callback provided anevent
argument, which triggered a force rerender... resulting in a render loop.How has this been tested?
Tested in local Gutenberg with a YouTube video embed. Both in Chrome and in Safari.
Types of changes
forceRerender
checking to ensure it does not get accidentally triggered by theonLoad
callback.Checklist:
Resolves: #20614