Replace injected html with sandboxing iframe #1392
Conversation
Test with the following content: Short tweet: https://twitter.com/notnownikki/status/876229494465581056 Tall tweet: https://twitter.com/PattyJenks/status/874034832430424065 Youtube: https://www.youtube.com/watch?v=PfKUdmTq2MI Photo: https://cloudup.com/cl3Oq5MU8Rm Long tumblr post: http://doctorwho.tumblr.com/post/162052108791
|
Meta note. That was a damn good "tall tweet". This works pretty amazingly well. Tweets, tumblr worked superbly. Not necessary but "would be nice" — can we inject the following style into the iframe? The first time I embedded the cloudup image, it didn't work. The second time it worked fine. The video wasn't responsive like they are in master currently, but if we can inject a little CSS into this I can fix it in a separate PR. Unlike the embed previously, this one responds to my width/height changes in the inspector! 🎉 To summarize:
Overall stellar work 💓 |
|
Sounds like there's still a race condition on cloudup there... (well, on all photo embeds, but I now know how to fix this properly, seems like we have to go to a three stage render like Calypso after all...) Yeah, we can inject CSS, JavaScript, anything we like into the iframe! Should we work on the race condition and CSS in separate PRs? I'm quite keen to get this merged, as I really dislike the current "inject it all into the main editor context" solution we have now, and I'm not sure how long the work to properly fix the race condition will take. |
That's fine with me, especially if we can open tickets so we don't forget. aduth is on vacation, so if you need a code review I'd recommend @youknowriad or @iseulde |
|
Reviews requested from @youknowriad and @iseulde :) |
| // Make sure that we don't miss very quick loading documents here that the observer | ||
| // doesn't see load, but haven't completely loaded when we call sendResize for the | ||
| // first time. | ||
| setTimeout( sendResize, 1000 ); |
There was a problem hiding this comment.
This works... most of the time. There's still something going on though that stops some embeds sending a resize when they load.
I'm going to tackle that specific problem in another PR. I have a good idea of how to solve it from looking at the embed code in Calypso, but I don't want to delay this and have this branch fall out of sync too much, as I'm not sure how long it will take to fully solve this.
| @@ -0,0 +1,173 @@ | |||
| /** | |||
| * Imported from Calypso, with some lint fixes and gutenburg specific changes. | |||
| */ | |||
There was a problem hiding this comment.
I wonder if we should keep this comment.
There was a problem hiding this comment.
🤷♀️ I mean, it's a direct copy, but with some react changes for how it handles the references, and lint fixes.
| @@ -158,6 +161,7 @@ function getEmbedBlockSettings( { title, icon, category = 'embed' } ) { | |||
|
|
|||
| const parsedUrl = parse( url ); | |||
| const cannotPreview = includes( HOSTS_NO_PREVIEWS, parsedUrl.host.replace( /^www\./, '' ) ); | |||
| const iframeTitle = 'Embedded content from ' + parsedUrl.host; | |||
| export { default as ResponsiveWrapper } from './responsive-wrapper'; | ||
| export { default as SandBox } from './sandbox'; |
There was a problem hiding this comment.
"Sandbox" is a single word, shouldn't need the PascalCase-ing here.
Test with the following content:
Short tweet:
https://twitter.com/notnownikki/status/876229494465581056
Tall tweet:
https://twitter.com/PattyJenks/status/874034832430424065
Youtube:
https://www.youtube.com/watch?v=PfKUdmTq2MI
Photo:
https://cloudup.com/cl3Oq5MU8Rm
Long tumblr post:
http://doctorwho.tumblr.com/post/162052108791