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

#841 Improve support for default embeds #889

Merged
merged 18 commits into from Jan 29, 2018

Conversation

Projects
None yet
3 participants
@kaitnyl
Copy link
Contributor

kaitnyl commented Jan 23, 2018

This addresses issue #841.
See full list of previous embed compatibility here.

I'm just going to highlight the updated/troubled ones below:

Update: this list is now updated, and located in the project wiki

AMP Support Status* Note
Media Video playlist The amp-video element doesn't currently support playlists. I think we can add to this, but since it's already implemented at their level, we likely need some discussion around how to approach this.
Media Audio playlist The amp-audio element doesn't currently support playlists (see above).
Post Embed 🔸 Blockquote containing link is required because links in iframe cannot be clicked. See follow-up in #914.
CollegeHumor Video Embed 🔸 The embed attempts to load http content through an https amp-iframe src (amp-iframe requires https). This causes the browser to reject it.
Issuu Embed There's no AMP element ready for this type of element, but I've added a solution that wraps it within an amp-iframe so it will display.
Meetup Embed This embed uses an inline style tag. I've captured it from the default embed and output it in the head, while the embed will output similar to desktop.
Reddit Embed Now appears the same as desktop version via the amp-reddit element.
Screencast Embed 🔸 I'm seeing the same error appear on the desktop version. This looks like it might be the downside of Screencast using .swfs.
Tumblr Embed 🔸 There's no AMP element ready for this type of element, but I've added a solution that wraps it within an amp-iframe so it will display. Remaining issue is that the iframe does not auto-resize as needed.
WordPress Plugin Directory Embed Updated to no longer display the extra blockquote above it.

*Status as of this diff.

Should we try and create our own version of amp-video and amp-audio element playlists? If we do now, they also likely wouldn't be valid AMP elements anymore.

@kaitnyl kaitnyl self-assigned this Jan 23, 2018

@kaitnyl kaitnyl requested review from westonruter and ThierryA Jan 23, 2018

@kaitnyl kaitnyl changed the title [WIP] #841 Support for default embeds #841 Support for default embeds Jan 23, 2018

@kaitnyl

This comment has been minimized.

Copy link
Contributor Author

kaitnyl commented Jan 23, 2018

Hi @ThierryA and @westonruter!

I'd love to get your input on some of the embed notes listed above. Ones like CollegeHumor and Screencast seem like they may be a bit incompatible here, but we do have some room for improvement with the addition of amp-video and amp-audio playlists.

/**
* Include the required AMP Reddit scripts.
*/
public function get_scripts() {

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 26, 2018

Member

It should be unnecessary to implement get_scripts() because now the whitelist sanitizer will automatically discover the amp-reddit script as being required through sanitization when it runs across the amp-reddit element.

*/
/**
* Class AMP_Reddit_Embed_Handler

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 26, 2018

Member

Please add @since tags to the phpdoc.

*
* @var string
*/
private static $script_src = 'https://cdn.ampproject.org/v0/amp-reddit-0.1.js';

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 26, 2018

Member

Per below, the $script_src needn't be defined at the embed level. See #885.

'layout' => 'responsive',
'data-embedtype' => 'post',
'width' => '596',
'height' => '141',

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 26, 2018

Member

Why are the width and height being hard-coded like this?

*/
/**
* Class AMP_Tumblr_Embed_Handler

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 26, 2018

Member

Please add @since 0.7 tags to the phpdoc.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jan 26, 2018

@kaitnyl Sorry for the delay:

MeetUp: This one is a bit tricky. We want the embed that AMP is currently using, but there's inline styling that we don't have added. We can add it inline but risk not being valid with the CSS sanitizer. I'll continue investigating how to approach this one.

Leaving the CSS on the embed output should successfully get sanitized by the CSS sanitizer. It will move any inline style attributes to the HEAD.

The embed attempts to load http content through an https amp-iframe src (amp-iframe requires https). This causes the browser to reject it.

Yeah, this embed doesn't work on desktop either. So it is broken on their side and we shouldn't try to fix it on our side.

I'm seeing the same error appear on the desktop version. This looks like it might be the downside of Screencast using .swfs.

If it is trying to embed a SWF then I think it should be intentionally not supported.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jan 26, 2018

The amp-video element doesn't currently support playlists. I think we can add to this, but since it's already implemented at their level, we likely need some discussion around how to approach this. ¶ Should we try and create our own version of amp-video and amp-audio element playlists? If we do now, they also likely wouldn't be valid AMP elements anymore.

Good question. One possibility would be to just create endpoints from WP that render out the playlist in standalone form and then render it in an amp-iframe. This would be like the current Post Embeds. That would work but it wouldn't be ideal. In particular it would not be ideal because each embed would get its own separate copy of MediaElement.js and any other scripts that the playlists depend on.

What would be great to see is a custom AMP implementation of the audio and video playlist which made use of amp-bind and amp-state to manage the playback of amp-video and amp-audio components. A tutorial for how to make use of these can be seen at https://www.ampproject.org/docs/tutorials/interactivity This should be done in a separate PR as it would be more work, but it will be extremely interesting and exciting!

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jan 26, 2018

Oh, and I should also mention amp-selector as the other key component that could be used to create playlists.

*
* @var int
*/
protected $DEFAULT_WIDTH = 596; // phpcs:ignore WordPress.NamingConventions.ValidVariableName.MemberNotSnakeCase

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 27, 2018

Member

Maybe the oEmbed handler can make a request off to a URL like https://www.reddit.com/comments/7t6msa.json?limit=1&app=embed to obtain the width/height specific for the given Reddit embed.

This comment has been minimized.

Copy link
@kaitnyl

kaitnyl Jan 27, 2018

Author Contributor

I've gone ahead and made it follow 100%/100% for the responsive layout type. This solves longer embeds using media that were getting cut off by the fixed dimensions, but now suffers from the same issue as reported here.

The dimensions given in https://www.reddit.com/comments/7t6msa.json?limit=1&app=embed are just for the media attachments and wouldn't encompass the entire embed content.

// Outlying style tag from oembed.
if ( isset( $result[0] ) ) {
$css = str_replace( '<style type="text/css">', '<style amp-meetup>', $result[0] );
echo $css; // WPCS: XSS ok.

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 28, 2018

Member

In the builtin paired mode theme, the CSS is getting printed before the <html> starts. It should get removed entirely I believe.

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 28, 2018

Member

One additional reason why the CSS should be discarded is that it includes !important properties, which are illegal in AMP. If it didn't, then maybe it could make sense to move the CSS to the amp-custom style in the head. But alas, I think we just have to strip it entirely.

Remove echo of Meetup style before returning content
* Add phpdoc for returns
* Use -1 priority for embed handlers.
return '';
}
$content = wp_oembed_get( $args['url'] );

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 28, 2018

Member

I'm seeing that wp_oembed_get() here is resulting in an HTTP request with every call. I was expecting it to be cached. Apparently to cache we need to use $wp_embed->shortcode( array(), $args['url'] ). This seems to be a problem in core as well with the Video widget: https://github.com/WordPress/wordpress-develop/blob/afb4f45e62520397b147e560e051d3940797121e/src/wp-includes/widgets/class-wp-widget-media-video.php#L144

I think core is erroneously doing an HTTP request for every load of a page with a video widget that is a non-YouTube and non-Vimeo oEmbed.

}
// Must enforce https for amp-iframe, but editors can supply either on desktop.
$args['url'] = str_replace( 'http://', 'https://', $args['url'] );

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 29, 2018

Member

Unfortunately it seems this is not working fully. When attempting to embed https://ifpaintingscouldtext.tumblr.com/post/92003045635/grant-wood-american-gothic-1930 then Tumblr will redirect from HTTPS to HTTP for some reason. When serving a page over HTTPS, as they should be, then the iframe is blocked from rendering.

@westonruter westonruter added this to the v0.7 milestone Jan 29, 2018

@westonruter westonruter changed the title #841 Support for default embeds #841 Improve support for default embeds Jan 29, 2018

@westonruter westonruter merged commit fab7520 into develop Jan 29, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the fix/841-default-embeds branch Jan 29, 2018

@kienstra

This comment has been minimized.

Copy link
Collaborator

kienstra commented Feb 14, 2018

Playlist Embeds

PR #954 (WIP) is handling playlist embeds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.