#841 Improve support for default embeds #889

kaitnyl · 2018-01-23T06:50:33Z

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.

…/child classes.

kaitnyl · 2018-01-23T06:56:35Z

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.

westonruter · 2018-01-26T03:39:05Z

includes/embeds/class-amp-reddit-embed-handler.php

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


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.

westonruter · 2018-01-26T03:39:19Z

includes/embeds/class-amp-reddit-embed-handler.php

+ */
+
+/**
+ * Class AMP_Reddit_Embed_Handler


Please add @since tags to the phpdoc.

westonruter · 2018-01-26T03:40:13Z

includes/embeds/class-amp-reddit-embed-handler.php

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


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

westonruter · 2018-01-26T03:41:19Z

includes/embeds/class-amp-reddit-embed-handler.php

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


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

westonruter · 2018-01-26T03:42:16Z

includes/embeds/class-amp-tumblr-embed-handler.php

+ */
+
+/**
+ * Class AMP_Tumblr_Embed_Handler


Please add @since 0.7 tags to the phpdoc.

westonruter · 2018-01-26T03:53:51Z

@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 · 2018-01-26T03:58:37Z

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 · 2018-01-26T06:22:00Z

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

…mbed, and use class vars for height/width.

westonruter · 2018-01-27T00:26:04Z

includes/embeds/class-amp-reddit-embed-handler.php

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


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.

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.

westonruter · 2018-01-28T00:01:14Z

includes/embeds/class-amp-meetup-embed-handler.php

+		// 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.


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

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.

* Add phpdoc for returns * Use -1 priority for embed handlers.

westonruter · 2018-01-28T00:11:43Z

includes/embeds/class-amp-meetup-embed-handler.php

+			return '';
+		}
+
+		$content = wp_oembed_get( $args['url'] );


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.

…ix/841-default-embeds

…not be clicked

westonruter · 2018-01-29T01:27:12Z

includes/embeds/class-amp-tumblr-embed-handler.php

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


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.

kienstra · 2018-02-14T18:58:12Z

Playlist Embeds

PR #954 (WIP) is handling playlist embeds.

kaitnyl added 5 commits January 23, 2018 00:06

Issuu, WP post, and WP plugin embed output updates.

2fda05e

Reddit and Tumblr embeds added.

754fe97

New files to use correct comment formatting.

657b227

Update file names to follow class names.

807a84e

Remove variable overrides and use inline to avoid updating all parent…

941a858

…/child classes.

kaitnyl self-assigned this Jan 23, 2018

kaitnyl requested review from westonruter and ThierryA January 23, 2018 06:53

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

kienstra mentioned this pull request Jan 24, 2018

Address an issue where <script> tags aren't stripped. #892

Merged

westonruter reviewed Jan 26, 2018

View reviewed changes

kaitnyl added 2 commits January 26, 2018 17:44

Merge branch 'develop' into fix/841-default-embeds

5c33545

Add @SInCE to each new class doc, remove script include from Reddit e…

eefdb70

…mbed, and use class vars for height/width.

westonruter reviewed Jan 27, 2018

View reviewed changes

kaitnyl added 2 commits January 26, 2018 21:09

MeetUp embed class to help output the CSS returned from ombed.

6c9079f

Use responsive width/height for Reddit embeds.

1d7cacd

westonruter reviewed Jan 28, 2018

View reviewed changes

Remove echo of Meetup style before returning content

35a62f4

* Add phpdoc for returns * Use -1 priority for embed handlers.

westonruter reviewed Jan 28, 2018

View reviewed changes

westonruter added 7 commits January 27, 2018 20:10

Filter embed_oembed_html to capture CSS; move to amp-custom styles

e2cc3e1

Mock soundcloud oEmbed response to speed up phpunit tests

e1b74cd

Merge branch 'develop' of https://github.com/Automattic/amp-wp into f…

634392b

…ix/841-default-embeds

Add support for sanitizing style elements

509b74e

Remove illegal wp_post_preview_js() from wp_head when AMP

c6d6258

Obtain issuu width/height via embed_oembed_html filter

c2efa47

Restore visibility of blockquote.wp-embedded-content since iframe can…

708e577

…not be clicked

westonruter mentioned this pull request Jan 29, 2018

[WIP] Add post embed support #914

Closed

westonruter reviewed Jan 29, 2018

View reviewed changes

Fix displaying Tumblr embeds on HTTPS

be133bf

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

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

westonruter approved these changes Jan 29, 2018

View reviewed changes

westonruter merged commit fab7520 into develop Jan 29, 2018

westonruter deleted the fix/841-default-embeds branch January 29, 2018 02:43

kienstra mentioned this pull request Feb 14, 2018

Support all default embeds where relevant #841

Closed

kienstra mentioned this pull request Mar 1, 2018

List AMP support for all native WordPress widgets #864

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#841 Improve support for default embeds #889

#841 Improve support for default embeds #889

kaitnyl commented Jan 23, 2018 •

edited by kienstra

Loading

kaitnyl commented Jan 23, 2018

westonruter Jan 26, 2018

westonruter Jan 26, 2018

westonruter Jan 26, 2018

westonruter Jan 26, 2018

westonruter Jan 26, 2018

westonruter commented Jan 26, 2018 •

edited

Loading

westonruter commented Jan 26, 2018 •

edited

Loading

westonruter commented Jan 26, 2018

westonruter Jan 27, 2018

kaitnyl Jan 27, 2018

westonruter Jan 28, 2018

westonruter Jan 28, 2018

westonruter Jan 28, 2018

westonruter Jan 29, 2018

kienstra commented Feb 14, 2018

#841 Improve support for default embeds #889

#841 Improve support for default embeds #889

Conversation

kaitnyl commented Jan 23, 2018 • edited by kienstra Loading

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

kaitnyl commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Jan 26, 2018 • edited Loading

westonruter commented Jan 26, 2018 • edited Loading

westonruter commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kienstra commented Feb 14, 2018

kaitnyl commented Jan 23, 2018 •

edited by kienstra

Loading

westonruter commented Jan 26, 2018 •

edited

Loading

westonruter commented Jan 26, 2018 •

edited

Loading