Enable AMP stories to be embedded in posts #1944

kienstra · 2019-03-11T05:29:16Z

When adding a WordPress (embed) block and entering an AMP story URL, this will display a styled card
Uses single card from the Latest Stories block carousel (AMP Latest Stories dynamic block #1916)

Closes #1931

This will display when entering the URL of the AMP story in the WordPress (embed) block. This still needs to enqueue, and alter, the styling from the Latest Stories block. It should ideally make this styling usable for both the embed and the block.

…y embed These style rules maybe be reusable, so enqueue this stylesheet also. There's more work needed, like possibly removing the .latest-stories-carousel from the selector

Ensure that the new embed template is used. Also, add some styling specific to that template.

kienstra · 2019-03-11T05:38:09Z

Current Display

kienstra · 2019-03-11T05:39:55Z

Styling And Layout

This could probably use styling and layout improvements. Also, it should probably have the sharing link that a post embed has:

westonruter · 2019-03-11T05:44:42Z

I don't think the sharing link is really that necessary. A user can just copy the URL of the link for the same effect, no? Normally a user would actually open the story and then share it from that point, I imagine.

The conflict was trivial, it was the same edit made in is_amp_endpoint() in both branches.

kienstra · 2019-03-11T06:24:55Z

I don't think the sharing link is really that necessary.

Sure, sounds good. Like you mentioned, someone could just copy the link.

Change the way this simulates being on an amp_story embed endpoint.

tests/test-class-amp-story-post-type.php

As Pascal mentioned, this can replace the need to set 3 properties of $wp_query.

includes/class-amp-story-post-type.php

westonruter · 2019-03-11T21:00:50Z

includes/class-amp-story-post-type.php

+	 */
+	public static function enqueue_embed_styling() {
+		if ( is_embed() && is_singular( self::POST_TYPE_SLUG ) ) {
+			wp_enqueue_style(


This style is being registered in two places. Can it be registered in one and then enqueued in two?

a9b5fdb registers the CSS file on the wp_loaded hook. Embed endpoints don't seem to fire wp_enqueue_scripts.

It's not ideal, but wp_loaded was the best hook I could find that works for the embed and the Latest Stories block.

@kienstra you can use the wp_default_scripts action instead, perhaps including the CSS registration in this function:

amp-wp/includes/amp-helper-functions.php

Lines 377 to 450 in 44a94ce

/**

* Register default scripts for AMP components.

*

* @param WP_Scripts $wp_scripts Scripts.

*/

function amp_register_default_scripts( $wp_scripts ) {

/*

* Polyfill dependencies that are registered in Gutenberg and WordPress 5.0.

* Note that Gutenberg will override these at wp_enqueue_scripts if it is active.

*/

$handles = array( 'wp-i18n', 'wp-dom-ready' );

foreach ( $handles as $handle ) {

if ( ! isset( $wp_scripts->registered[ $handle ] ) ) {

$wp_scripts->add( $handle, amp_get_asset_url( sprintf( 'js/%s-compiled.js', $handle ) ) );

}

}

// AMP Runtime.

$handle = 'amp-runtime';

$wp_scripts->add(

$handle,

'https://cdn.ampproject.org/v0.js',

array(),

null

);

$wp_scripts->add_data(

$handle,

'amp_script_attributes',

array(

'async' => true,

)

);

// Shadow AMP API.

$handle = 'amp-shadow';

$wp_scripts->add(

$handle,

'https://cdn.ampproject.org/shadow-v0.js',

array(),

null

);

$wp_scripts->add_data(

$handle,

'amp_script_attributes',

array(

'async' => true,

)

);

// Get all AMP components as defined in the spec.

$extensions = array();

foreach ( AMP_Allowed_Tags_Generated::get_allowed_tag( 'script' ) as $script_spec ) {

if ( isset( $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec']['name'], $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec']['version'] ) ) {

$versions = $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec']['version'];

array_pop( $versions );

$extensions[ $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec']['name'] ] = array_pop( $versions );

}

}

foreach ( $extensions as $extension => $version ) {

$src = sprintf(

'https://cdn.ampproject.org/v0/%s-%s.js',

$extension,

$version

);

$wp_scripts->add(

$extension,

$src,

array( 'amp-runtime' ),

null

);

}

}

Thanks, @westonruter! 2101161 moves the wp_register_style() call into amp_register_default_scripts().

A unit test failed in PHP 5.4 and 5.6 with WP 4.9,, but passed in the other versions.

westonruter · 2019-03-11T21:09:47Z

There are AMP validation errors related to the embed which I believe are out of scope of the changes here, but the link is showing up unexpectedly in the post editor:

The post embed looks good in the non-AMP page:

But it doesn't look right in AMP:

Again, this is largely due to poor post embed handling in WordPress: #809.

One idea to help deal with this is to add a render_block filter (or override/supply the render_callback) to then check if the URL being embedded is (1) on this site and (2) an AMP story post. If that is the case, then you could override the output to be the_single_story_card for that story post and avoid the iframe altogether. This would fix rendering of the post embed in AMP as well as eliminate the (known) validation errors.

Manually set $GLOBALS['wp_query']->is_singular to true. Still, this isn't an ideal solution.

As Weston mentioned, this will be tree-shaken on Latest Stories blocks, so it's not a problem including it in the stylesheet.

Thanks to Weston for the snippet. As he mentioned, the file will exist. So there's no need for a file_exists() check.

Before, it was fully enqueued in 2 places. But as Weston mentioned, it's better to register it, then enqueue it by only passing its slug. Use the 'wp_loaded' hook to register This isn't ideal, but on the embed endpoint, wp_enqueue_scripts doesn't run.

It looks like AMP_Story_Post_Type::register() exits if register_block_type() does not exist and that doesn't look to exist in 4.9. So mark the test skipped.

As Weston mentioned, the CSS registration seems to go there.

It's registered outside of the class that handles AMP stories, so describe what the stylesheet is.

Apply Weston's suggestion to override the render_callbck() for the embed block. It returns the card with at <div> wrapper, not an entire <iframe>.

kienstra · 2019-03-12T05:52:38Z

Embed Display In AMP

Hi @westonruter,
Thanks for your suggestion for overriding the embed block's render_callback for AMP stories.

6a81d3c does that, and an AMP story embed now looks better in AMP:

And there aren't any more validation errors in the block editor.

If it wasn't registered, it's not possible to create test posts of that post type.

kienstra · 2019-03-12T06:01:15Z

Ah, I still need to ensure that the embed URL is from the same site.

As Weston mentioned, this is a necessary part of overriding the embed callback. This is mainly taken from url_to_postid().

kienstra · 2019-03-12T06:53:42Z

Embedding Doesn't Look To Work With Plain Permalinks

Embedding an AMP Story URL with the Wordpress (embed) block doesn't look to work when permalinks are set to 'Plain.'

But this is the case even with another custom post type.

Steps To Reproduce

Add this to a plugin (mainly taken from this document):

/**
 * Registers a 'testimonials' post type.
 */
add_action( 'init', function() {
	register_post_type(
		'testimonials',
		array(
			'public' => true,
			'label'  => 'Testimonials',
		)
	 );
} );

Go to Settings > Permalinks, and select 'Plain':

Create a new 'Testimonial' post, with any title and content. Publish it, and copy its permalink:

Create a new post (with the post type of 'post')
Add a WordPress (embed) block
Enter the URL from the 'Testimonial' post in step 3.
Expected: ideally, it should embed
Actual: it displays 'Sorry, we could not embed that content'

Before, this function simply ran on its own. But it really need to be in an assertion.

override_story_embed_callback() probably needed more explanation, so add to the DocBlock.

kienstra · 2019-03-12T08:19:53Z

Steps To Test

Create a new post (of the post type 'post')
Add a WordPress (embed) block:

Enter the permalink of an AMP Story post that has a featured image, and click 'Embed'
Expected: the AMP Story is embedded:

Expected: the front-end display:

swissspidy · 2019-03-12T09:41:18Z

Should/Does this also support embedding from another site in the same network?

westonruter · 2019-03-12T16:00:23Z

Should/Does this also support embedding from another site in the same network?

That would be nice but it wouldn't be supported presently as currently implemented via:

$post = get_post( get_page_by_path( $path, OBJECT, self::POST_TYPE_SLUG ) );

Thinking about this some more, I wonder if overriding the render_block filter is actually the best approach. Perhaps actually what is needed is to override the actual HTML that is returned for oEmbed response (the embed_html filter and/or oembed_response_data filter)? If we did this, perhaps we could eliminate the need for the oEmbed iframe altogether? If the oEmbed response html just contained the contents of the_single_story_card() then that would be very cool. I am dubious, however, due to the styling required.

westonruter · 2019-03-12T16:05:53Z

I am dubious, however, due to the styling required.

Yeah, getting rid of the iframe entirely won't work due to wp_filter_oembed_result() and how it does:

	$allowed_html = array(
		'a'          => array(
			'href' => true,
		),
		'blockquote' => array(),
		'iframe'     => array(
			'src'          => true,
			'width'        => true,
			'height'       => true,
			'frameborder'  => true,
			'marginwidth'  => true,
			'marginheight' => true,
			'scrolling'    => true,
			'title'        => true,
		),
	);

	$html = wp_kses( $result, $allowed_html );

…al core/embed block

westonruter · 2019-03-12T16:24:19Z

I added a few @todo comments.

kienstra · 2019-03-12T17:08:52Z

Thanks, @westonruter! I'll carry out the @todo comments if you'd like.

westonruter · 2019-03-12T17:44:32Z

Yep, go for it.

Something I'm surprised by actually with the WordPress embed is that there is no auto-complete for the story URL. If I create a link, I can just start typing the title of the post to get it suggested. But there is no similar facility for the WordPress Embed. This is out of scope for this PR, however. It's really something that should be supported by WordPress itself. So I've opened WordPress/gutenberg#14393

As Weston mentioned in code review, these make more sense there than in AMP_Editor_Blocks.

There's already a check with: if ( $url_host !== $home_url_host ) { So remove the extra check that didn't take account of HTTP and HTTPS.

It looks like the style wasn't registered in WP 4.9, because the register() method exited. So call register_styles().

As Weston mentioned, this breaks the method into separate methods. Also, it keeps the STORY_CARD_CSS_SLUG registeration on a 'wp_default_styles' callback. It would have been good to run register_scripts() on the 'wp_default_scripts' hook. But get_current_screen() is null, and that's needed to verify that it's an AMP Story post type.

kienstra · 2019-03-12T20:01:56Z

@todos applied

Hi @westonruter,
Thanks for reviewing this again, and good idea to open WordPress/gutenberg#14393.

The @todos that you added here are now applied.

The main register() file will exit in WP 4.9, so there's little need to test the render_callback.

westonruter · 2019-03-12T20:31:23Z

includes/class-amp-story-post-type.php

@@ -222,6 +240,7 @@ public static function enqueue_block_editor_assets() {
 			return;
 		}

+		// @todo Move the following styles to register_styles method, and scripts to a new register_scripts method which runs at wp_default_scripts?


What I meant here was that potentially there could be methods that run at wp_default_scripts and wp_default_styles which then do $scripts->add() and $styles->add() respectively. That would then leave the existing enqueue_block_editor_assets method to just wp_enqueue_script($handle) and wp_enqueue_style($handle).

But this is not really necessary for any scripts and styles that are enqueued in multiple places.

Ah, that makes sense. Sorry for missing that.

It looks like you renamed some methods in 80b3a7e. Let me know if you'd like me to apply your suggestion of calling $scripts->add() and $styles->add at wp_default_scripts and wp_default_styles

westonruter · 2019-03-12T20:39:33Z

includes/class-amp-story-post-type.php

+			return $pre_render;
+		}
+
+		$path = str_replace( home_url( self::REWRITE_SLUG . '/' ), '', $url );


This still doesn't seem very robust. What if the $url is an HTTP one, but the site is in HTTPS? Since you've already verified above that the host is the same, it seems this should rather do something like:

Suggested change

$path = str_replace( home_url( self::REWRITE_SLUG . '/' ), '', $url );

$embed_url_path = wp_parse_url( $url, PHP_URL_PATH );

$base_url_path = wp_parse_url( trailingslashit( home_url( self::REWRITE_SLUG ) ), PHP_URL_PATH );

if ( 0 !== strpos( $embed_url_path, $base_url_path ) ) {

return $pre_render;

}

$path = substr( $embed_url_path, strlen( $base_url_path ) );

Thanks, that's a good point. I committed your code verbatim.

Co-Authored-By: kienstra <[email protected]>

As Weston mentioned, this is possible, and this would need to override the embed output.

It looks like there's another test that failed, and it may have been from $_SERVER['HTTPS'] being 'on'.

kienstra added 3 commits March 10, 2019 17:29

Enqueue the stylesheet from the Latest Stories block for the AMP Stor…

a9b829d

…y embed These style rules maybe be reusable, so enqueue this stylesheet also. There's more work needed, like possibly removing the .latest-stories-carousel from the selector

Add filter to get the embed template, improve styling

607cb87

Ensure that the new embed template is used. Also, add some styling specific to that template.

googlebot added the cla: yes Signed the Google CLA label Mar 11, 2019

kienstra mentioned this pull request Mar 11, 2019

[WIP] When an AMP story is embedded, display a story card #1940

Closed

Merge in amp-stories-redux, resolve conflicts

e801634

The conflict was trivial, it was the same edit made in is_amp_endpoint() in both branches.

Address failed unit test

ada585b

Change the way this simulates being on an amp_story embed endpoint.

swissspidy reviewed Mar 11, 2019

View reviewed changes

tests/test-class-amp-story-post-type.php Outdated Show resolved Hide resolved

Use $this->go_to() instead of setting the global $wp_query

767fee7

As Pascal mentioned, this can replace the need to set 3 properties of $wp_query.

westonruter requested changes Mar 11, 2019

View reviewed changes

Attempt to address failed PHPUnit tests with get_post_permalink()

cd281e8

A unit test failed in PHP 5.4 and 5.6 with WP 4.9,, but passed in the other versions.

kienstra added 8 commits March 11, 2019 15:21

Another attempt to fix a failed unit test in WP 4.9

7fc6b14

Manually set $GLOBALS['wp_query']->is_singular to true. Still, this isn't an ideal solution.

Move inline styling into a CSS file

2710702

As Weston mentioned, this will be tree-shaken on Latest Stories blocks, so it's not a problem including it in the stylesheet.

Simplify get_embed_template(), using Weston's code

8b0395f

Thanks to Weston for the snippet. As he mentioned, the file will exist. So there's no need for a file_exists() check.

Mark the test skipped in WP 4.9, as the post type won't be registered

1bbfc01

It looks like AMP_Story_Post_Type::register() exits if register_block_type() does not exist and that doesn't look to exist in 4.9. So mark the test skipped.

Move the style registration to amp_register_default_scripts()

2101161

As Weston mentioned, the CSS registration seems to go there.

Add a comment describing the styling being registered

347a8cc

It's registered outside of the class that handles AMP stories, so describe what the stylesheet is.

Improve AMP story post embed display in AMP

6a81d3c

Apply Weston's suggestion to override the render_callbck() for the embed block. It returns the card with at <div> wrapper, not an entire <iframe>.

Exit from the test if the AMP story post wasn't registered

cd24cb7

If it wasn't registered, it's not possible to create test posts of that post type.

Ensure that the URL is from the same site

0cd2a84

As Weston mentioned, this is a necessary part of overriding the embed callback. This is mainly taken from url_to_postid().

Assert that wp_style_is(), don't simply call it

34f8781

Before, this function simply ran on its own. But it really need to be in an assertion.

Improve positioning of comment, add to DocBlock for override filter

4265035

override_story_embed_callback() probably needed more explanation, so add to the DocBlock.

westonruter added 2 commits March 12, 2019 09:18

Move AMP story style registration to component class

1ab781a

Ensure override_story_embed_callback returns pre value; support gener…

c81770c

…al core/embed block

kienstra added 4 commits March 12, 2019 12:46

Move methods to AMP_Story_Post_Type

3f52d57

As Weston mentioned in code review, these make more sense there than in AMP_Editor_Blocks.

Remove the extra check that the URL is from the home_url()

bffe4a5

There's already a check with: if ( $url_host !== $home_url_host ) { So remove the extra check that didn't take account of HTTP and HTTPS.

Address a failed unit test

1809717

It looks like the style wasn't registered in WP 4.9, because the register() method exited. So call register_styles().

kienstra and others added 2 commits March 12, 2019 14:05

Mark test skipped in the post isn't registered

2bca1f0

The main register() file will exit in WP 4.9, so there's little need to test the render_callback.

Fix naming of block asset enqueueing methods

9b85281

westonruter reviewed Mar 12, 2019

View reviewed changes

westonruter and others added 3 commits March 12, 2019 15:16

Apply Weston's fix for if the $url uses HTTP and the site uses HTTPS

80b3a7e

Co-Authored-By: kienstra <[email protected]>

Add an assertion for when the site is HTTPS but the permalink is HTTP

c39e1e9

As Weston mentioned, this is possible, and this would need to override the embed output.

Unset $_SERVER['HTTPS'] on the tearDown() of the tests

b44b258

It looks like there's another test that failed, and it may have been from $_SERVER['HTTPS'] being 'on'.

westonruter approved these changes Mar 13, 2019

View reviewed changes

westonruter added the AMP-Stories label Mar 13, 2019

westonruter merged commit 427c3d7 into amp-stories-redux Mar 13, 2019

westonruter deleted the add/amp-stories-embed branch March 13, 2019 01:05

westonruter added this to the v1.2 milestone May 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable AMP stories to be embedded in posts #1944

Enable AMP stories to be embedded in posts #1944

kienstra commented Mar 11, 2019 •

edited

Loading

kienstra commented Mar 11, 2019

kienstra commented Mar 11, 2019

westonruter commented Mar 11, 2019

kienstra commented Mar 11, 2019 •

edited

Loading

westonruter Mar 11, 2019

kienstra Mar 11, 2019

kienstra Mar 11, 2019

westonruter Mar 12, 2019

kienstra Mar 12, 2019

westonruter commented Mar 11, 2019 •

edited

Loading

kienstra commented Mar 12, 2019

kienstra commented Mar 12, 2019

kienstra commented Mar 12, 2019

kienstra commented Mar 12, 2019 •

edited

Loading

swissspidy commented Mar 12, 2019 •

edited

Loading

westonruter commented Mar 12, 2019

westonruter commented Mar 12, 2019

westonruter commented Mar 12, 2019

kienstra commented Mar 12, 2019

westonruter commented Mar 12, 2019

kienstra commented Mar 12, 2019

westonruter Mar 12, 2019

kienstra Mar 12, 2019

westonruter Mar 12, 2019

kienstra Mar 12, 2019

	/**
	* Register default scripts for AMP components.
	*
	* @param WP_Scripts $wp_scripts Scripts.
	*/
	function amp_register_default_scripts( $wp_scripts ) {
	/*
	* Polyfill dependencies that are registered in Gutenberg and WordPress 5.0.
	* Note that Gutenberg will override these at wp_enqueue_scripts if it is active.
	*/
	$handles = array( 'wp-i18n', 'wp-dom-ready' );
	foreach ( $handles as $handle ) {
	if ( ! isset( $wp_scripts->registered[ $handle ] ) ) {
	$wp_scripts->add( $handle, amp_get_asset_url( sprintf( 'js/%s-compiled.js', $handle ) ) );
	}
	}

	// AMP Runtime.
	$handle = 'amp-runtime';
	$wp_scripts->add(
	$handle,
	'https://cdn.ampproject.org/v0.js',
	array(),
	null
	);
	$wp_scripts->add_data(
	$handle,
	'amp_script_attributes',
	array(
	'async' => true,
	)
	);

	// Shadow AMP API.
	$handle = 'amp-shadow';
	$wp_scripts->add(
	$handle,
	'https://cdn.ampproject.org/shadow-v0.js',
	array(),
	null
	);
	$wp_scripts->add_data(
	$handle,
	'amp_script_attributes',
	array(
	'async' => true,
	)
	);

	// Get all AMP components as defined in the spec.
	$extensions = array();
	foreach ( AMP_Allowed_Tags_Generated::get_allowed_tag( 'script' ) as $script_spec ) {
	if ( isset( $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec']['name'], $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec']['version'] ) ) {
	$versions = $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec']['version'];
	array_pop( $versions );
	$extensions[ $script_spec[ AMP_Rule_Spec::TAG_SPEC ]['extension_spec']['name'] ] = array_pop( $versions );
	}
	}

	foreach ( $extensions as $extension => $version ) {
	$src = sprintf(
	'https://cdn.ampproject.org/v0/%s-%s.js',
	$extension,
	$version
	);

	$wp_scripts->add(
	$extension,
	$src,
	array( 'amp-runtime' ),
	null
	);
	}
	}

-		$path = str_replace( home_url( self::REWRITE_SLUG . '/' ), '', $url );
+		$embed_url_path = wp_parse_url( $url, PHP_URL_PATH );
+		$base_url_path  = wp_parse_url( trailingslashit( home_url( self::REWRITE_SLUG ) ), PHP_URL_PATH );
+		if ( 0 !== strpos( $embed_url_path, $base_url_path ) ) {
+			return $pre_render;
+		}
+		$path = substr( $embed_url_path, strlen( $base_url_path ) );

Enable AMP stories to be embedded in posts #1944

Enable AMP stories to be embedded in posts #1944

Conversation

kienstra commented Mar 11, 2019 • edited Loading

kienstra commented Mar 11, 2019

kienstra commented Mar 11, 2019

westonruter commented Mar 11, 2019

kienstra commented Mar 11, 2019 • edited Loading

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 Mar 11, 2019 • edited Loading

kienstra commented Mar 12, 2019

kienstra commented Mar 12, 2019

kienstra commented Mar 12, 2019

kienstra commented Mar 12, 2019 • edited Loading

swissspidy commented Mar 12, 2019 • edited Loading

westonruter commented Mar 12, 2019

westonruter commented Mar 12, 2019

westonruter commented Mar 12, 2019

kienstra commented Mar 12, 2019

westonruter commented Mar 12, 2019

kienstra commented Mar 12, 2019

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 Mar 11, 2019 •

edited

Loading

kienstra commented Mar 11, 2019 •

edited

Loading

westonruter commented Mar 11, 2019 •

edited

Loading

kienstra commented Mar 12, 2019 •

edited

Loading

swissspidy commented Mar 12, 2019 •

edited

Loading