Skip to content
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

Enhance the embed block to support all WP_oEmbed embeds #816

Merged
merged 16 commits into from
Jun 2, 2017

Conversation

notnownikki
Copy link
Member

@notnownikki notnownikki commented May 17, 2017

Implements server side rendering for the Embed block so that it supports all urls processed by WP_oEmbed

Adds a REST API endpoint so we can post a serialized block and get back the rendered HTML.

Adds a server side block, 'core/oembed', that exists to render URLs with WP_oEmbed.

Extracts individual block serialization in the serializer, so that we can serialize individual blocks for server side rendering.

Adds awareness of domains we cannot preview content for in the editor, due to javascript issues.

Adds a Sandbox HtmlEmbed component that will put rendered HTML into an iframe a div, and make sure all script blocks get run. It uses the ResizableIframe from Calypso to make sure all rendered content is shown.

URLs for testing:

https://twitter.com/mrbiffo/status/864555311918714880 : short tweet, iframe should resize.

http://doctorwho.tumblr.com/post/160705586507/onaperduamedee-to-really-feel-it-you-need-the : tall tumblr post, iframe should resize to show it all.

https://www.facebook.com/DoctorWho/videos/1854859861194699/ : facebook video, should show the url as a hyperlink with a message saying we're sorry that we cannot preview that content in the editor.

http://example.com/ : not supported by oEmbed, should display an error saying that we cannot embed that content.

@notnownikki notnownikki added the [Status] In Progress Tracking issues with work in progress label May 17, 2017
@notnownikki
Copy link
Member Author

Because this supports so many different embeds (currently 53 supported by WP_oEmbed) we should think about how to present this to the user. Perhaps we have help on this block, or other blocks that extend it somehow... not sure!

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job on this PR. I know it's still WIP but I left some notes that I hope can help.

* @param {String} saveContent Content to save into the post
* @return {String} The post content
*/
export function serializeBlock( blockType, blockAttributes, blockSettings, saveContent ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of taking all these arguments, should we replace those with a block object instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had this on a list on clean-ups to do, glad you think it should be done too 👍

</figure>
);
render() {
const { oembed_html, error, fetching } = this.state;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the oembed_html be camelCased?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, my Python is showing! Will fix :)

{ wp.i18n.__( 'Embed' ) }
</Button>
</Placeholder>
doServerSideRender( event ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we extract this ServerSideRender to a HoC? Maybe something like:

<ServerSideBlock blockType={ blockType } arguments={ arguments }>
  { ( html, error ) => (
    <Sandbox html={ html } /> 
  ) }
</ServerSideBlock>

Any blocker to achieve this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only blocker is I need to read more React docs to figure out how to do it. There were async issues with having Sandbox as the containing block, this seems like a much better way, I've just never done it before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index.php Outdated
return $oembed->get_html( $content['url'] );
}

register_block( 'core/oembed', 'gutenberg_block_core_oembed' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: This index.php is getting bigger and bigger, we may want to split it soon.

@jasmussen
Copy link
Contributor

Starting to look at this a bit, and I'm having some dumb questions, please forgive me.

Can the "Generic Embed" block convert itself to being a different block once a URL is entered? Take for example this tweet: https://twitter.com/sydneysuks/status/757616209814654976 — if you embed that it gets cropped, presumably because we do that responsive video trick to resize youtube videos. It'd be nice if it could convert from core/oembed to either core/oembed/twitter or core/twitter or something like that, so we could provide separate styles for each embed.

We might even separate out into two groups. One group which is videos, these all get the "prevent click overlay" and the responsive size stuff, and the other group are simply freeform "unknown size" embeds, for which the block resizes itself into.

@notnownikki
Copy link
Member Author

notnownikki commented May 17, 2017 via email

@jasmussen
Copy link
Contributor

Ah I understand now! Yes, will take a look. I suspect that we are doing the responsive video stuff (which does this cropping) right now to all embeds, and we should be doing that only to video embeds.

@notnownikki
Copy link
Member Author

notnownikki commented May 17, 2017 via email

@jasmussen
Copy link
Contributor

Okay I've had a look, nice work!

It seems like what we are doing with the video overlay (to prevent accidental play clicks) as well as the responsive video trick may be a bit too clever for what is essentially embeds. It's still what we should do, but it's probably a little more work.

The thing is — right now the markup we output is the same whether we're embedding a youtube video or a tweet:

<div class="editor-visual-editor__block" data-type="core/embed" tabindex="0">
    <div>
        <figure class="blocks-embed">
            <div class="iframe-overlay">
                <iframe title="" seamless="" scrolling="no" sandbox="allow-same-origin allow-scripts" width="525" height="508"></iframe>
            </div>
        </figure>
    </div>
</div>

The above is kind of perfect for videos. It means we can decide on a 16:9 aspect ration using CSS, and then allow the video inside to scale to 100% width and height of the container, so it's a responsive video. It also allows us the "click preventer" suggested in #483. All good.

The problem is, there's no way to scope the necessary CSS to apply only to video embeds, because the embeds inside are more or less unknown to us.

If we were able to have the following markup, I could write CSS that would work for both videos and anything else.

For video embeds:

<div class="editor-visual-editor__block" data-type="core/embed/video" tabindex="0">
    <div>
        <figure class="blocks-embed">
            <iframe title="" seamless="" scrolling="no" sandbox="allow-same-origin allow-scripts" width="525" height="508"></iframe>
        </figure>
    </div>
</div>

Note the core/embed/video.

For anything else:

<div class="editor-visual-editor__block" data-type="core/embed" tabindex="0">
    <div>
        <figure class="blocks-embed">
            <iframe title="" seamless="" scrolling="no" sandbox="allow-same-origin allow-scripts" width="525" height="508"></iframe>
        </figure>
    </div>
</div>

I don't know if this is possible or not. Perhaps @iseulde might know more?

@notnownikki
Copy link
Member Author

notnownikki commented May 17, 2017 via email

@jasmussen
Copy link
Contributor

So what we could do is have a list of video domains where we apply the
click preventer? We already have to check the domain the url is on to
display the "Sorry, can't preview this" for facebook stuff.

That seems like it could work. A whitelist of domains that get the responsive treatment and click preventer, anything else gets the fallback style.

Can it result in a CSS class being applied to the block, or a data attribute? I just need to be able to distinguish in CSS whether it's one or the other.

Also, if you can get me this list of 54 supported, I volunteer to narrow down which ones are video, and get them back to you in an array format of your choosing.

@notnownikki
Copy link
Member Author

notnownikki commented May 17, 2017 via email

@jasmussen
Copy link
Contributor

Here's a tentative list of embeds I think we should treat as "Video" embeds:

youtube.com
youtu.be
vimeo.com
dailymotion.com
dai.ly
hulu.com
wordpress.tv
funnyordie.com
collegehumor.com

Let me know if you need it in another format.

@notnownikki
Copy link
Member Author

Just tested the tumblr post in the other branch (the one that doesn't use

and just has an enclosing diff) and it does weird things, I think because there are height limitations on the block? Tumblr tries to resize for the height of the div, and progressively gets thinner and thinner until you can't see anything... in this branch, it's the correct width, but clipped heightwise.

Anyway, I guess this is why it's WIP :)

this.noPreview = [
'facebook.com',
];
this.videoEmbeds = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The oEmbed response will indicate the type that the media is, e.g. video. So maybe this isn't necessary?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it'd be good to not have to handle this here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fantastic, thank you for letting me know about this! I'll work on changing this over.

} );
form.append( 'content', oembed_block );
this.setState( { error: false, fetching: true } );
fetch( api_url, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an oEmbed Proxy endpoint now in core which should be used, I believe. See https://core.trac.wordpress.org/ticket/40450

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it fails to embed because this is not the proper path. Needs path to the WP site. https://developer.wordpress.org/reference/functions/rest_url/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I hardcoded the api url here because I didn't know how to discover it. I see that function is how to do it in PHP, is there a JS version too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need the URL and the nonce alike. They can be exported from PHP to JS as was done for Media: https://github.com/WordPress/wordpress-develop/blob/255bd917f2bc3c0d2b324ebbc979ab4147631c06/src/wp-includes/media.php#L3421-L3431

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to access that from JS... do you have an example of the JS side you could link me to?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't have to get a new one. In WordPress, a nonce is not strictly a number-used-once. It is more like a CSRF token that will expire after a day. You can presume that the WP-API should be responsible for keeping wpApiSettings.nonce updated, though I don't think it is currently.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@notnownikki Alternatively, if you need other PHP settings, you can use https://developer.wordpress.org/reference/functions/wp_localize_script/ in WordPress.

Copy link
Contributor

@youknowriad youknowriad May 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking we could use the proxy endpoint as well but I like the idea of having a more generic server side rendering endpoint (like the one implemented in the PR) 

Edit: I guess we're interested in the video flag returned by the proxy endpoint.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a few problems implementing this...

If I fetch the URL resulting from

wpApiSettings.root + 'oembed/1.0/proxy?url=' + encodeUriComponent( url ) + '&_wpnonce=' + wpApiSettings.nonce

...then I get back "Cookie nonce is invalid". (I've tried to set the none in a header too, but same error.)

However, if I visit the URL in a browser, I don't get a nonce error, but I do get a 404, the route isn't found.

Can you point me in the right direction? I've been through the REST API Handbook, and it all looks like it should work...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you running WordPress 4.8-beta2? This is a new endpoint in 4.8.

<iframe
ref={ ( node ) => this.iframe = node }
title={ this.props.title }
seamless="seamless"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there currently any support for it? If not, is it worth adding here?

@nylen
Copy link
Member

nylen commented May 26, 2017

After #849 which added a bunch of tests for block parsing and serialization, this PR should probably be rebased against master, either because the build will fail upon merge, or because this PR can add some new tests using this structure (details here).

@notnownikki
Copy link
Member Author

I rebased this against master, and simplified things.

We had a lot of rendering issues with the resizing iframe, so after some discussions we decided to just use a div to hold the embedded html, and tackle the iframe rendering issues in the future.

The rendering has been simplified, to use the oembed proxy API included in WP4.8.

@notnownikki
Copy link
Member Author

Also, previewing facebook embeds is disabled, because fb scripts want to change global things about the document they're running in (which the sandboxing iframe highlighted and wouldn't allow).

@notnownikki
Copy link
Member Author

I've just seen this doesn't render properly loading from the saved post. Off to fix that now.

@notnownikki
Copy link
Member Author

....aaaaand fixed.

@jasmussen
Copy link
Contributor

Tweets work really well! Wow! Pushed some tweaks to the placeholder, and I'll be looking at videos in a minute.

@jasmussen
Copy link
Contributor

jasmussen commented Jun 1, 2017

I pushed a little change so the videos are now responsive again.

I know there are some sandboxing challenges worth exploring in a separate PR, but right now, in this branch, embeds are working 🌟🌟🌟🌟🌟!

In other words, 👍 on user experience.

@notnownikki notnownikki removed the [Status] In Progress Tracking issues with work in progress label Jun 1, 2017
<form onSubmit={ this.doServerSideRender }>
<input
type="url"
className="components-placeholder__input"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we using components-placeholder instead of placeholder?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an @aduth request, if I recall correctly.

Copy link
Member

@mtias mtias Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we are using components as prefix for the whole folder. This is so specific to blocks, though. Mmm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are to adhere to the proposed guideline, this should be blocks- prefix, not components-

https://github.com/WordPress/gutenberg/blob/master/docs/coding-guidelines.md#naming

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's truly generic to be considered a component, it should be moved to the components directory to reflect as such.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I agree this is not generic enough and should be blocks-

@mtias mtias added the [Feature] Blocks Overall functionality of blocks label Jun 1, 2017
notnownikki and others added 10 commits June 1, 2017 12:34
Added "html-embed" component which embeds html from the
ombed proxy inside a div and makes sure any script elements
get run. This initial version uses a div instead of a
sandbox iframe due to some tricky rendering issues with
reactive embdedded content from sites like tumblr.
This makes videos responsive again.
- Use spinner component
- Adjust some parenthesis syntax
</Button>
</Placeholder>
componentWillUnmount() {
// can't about the fetch promise, so let it know we will unmount
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo abort

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome Job. Let's get this merged 🚢

@notnownikki notnownikki merged commit a5c1487 into master Jun 2, 2017
@mtias mtias deleted the update/embed-block branch June 2, 2017 11:18
const { url } = this.props.attributes;
const api_url = wpApiSettings.root + 'oembed/1.0/proxy?url=' + encodeURIComponent( url ) + '&_wpnonce=' + wpApiSettings.nonce;

this.setState( { error: false, fetching: true } );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When loading the page on master now, I see the following warning from this line:

Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so this needs to be moved into componentWillMount?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe. I'd have to look again at where this function is called originally, and if by componentDidMount, why the component would not be mounted at this point.

To a more general point, I think this is a handy reference for when side effects and state changes should occur during component lifecycle:

https://gist.github.com/bvaughn/923dffb2cd9504ee440791fade8db5f9

);
}

const domain = url.split( '/' )[ 2 ].replace( /^www\./, '' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a bit fragile, since it could easily throw errors if an unexpected url reaches it (one without at least two slashes). We've already started using Node's url module elsewhere, which can perform much more reliable parsing, specifically via url.parse. Do we need the host here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, the host without www. because we want to blacklist certain embeds from previewing in the editor (facebook specifically). I'll look at the url module :)

}

const domain = url.split( '/' )[ 2 ].replace( /^www\./, '' );
const cannotPreview = this.noPreview.includes( domain );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array#includes is not polyfilled (yet) and this will error in IE11.

@niranjan-uma-shankar
Copy link

It should help to refactor the code to make use of withAPIData. Though embed endpoint uses transients to cache, withAPIData already provides this out of the box, and network calls can be avoided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants