AMP: Render amp-gist from gist shortcode/oEmbed during AMP requests

[westonruter]

Changes proposed in this Pull Request:

• Return amp-gist from a gist shortcode or oEmbed when in an AMP response.

Testing instructions:

1. Install the AMP plugin.
2. Add a gist shortcode or gist oEmbed URL to a post. Then view that post in AMP.
3. View the AMP version of the post.

Example content:

oEmbed: Full gist:

https://gist.github.com/sebastianbenz/1d449dee039202d8b7464f1131eae449

oEmbed: Linking to a file via in a Gist:

https://gist.github.com/sebastianbenz/1d449dee039202d8b7464f1131eae449#file-sw-html

oEmbed: Linking to file without username in URL.

https://gist.github.com/1d449dee039202d8b7464f1131eae449#file-sw-html

Example from WordPress.com docs:

[gist https://gist.github.com/2314628 /]

Second example from WordPress.com docs:

[gist]2314628[/gist]

Originally proposed in ampproject/amp-wp#375

See also ampproject/amphtml#17738 for details on the amp-gist's height attribute.

Proposed changelog entry for your changes:

• Add Gist support for AMP responses.

[jetpackbot]

That's a great PR description, thank you so much for your effort!

Generated by 🚫 dangerJS

[jeherve]

Do you think you could move most of this to the existing AMP compatibility file, to keep things in one place?
https://github.com/Automattic/jetpack/blob/master/3rd-party/class.jetpack-amp-support.php

It may also be useful to have unit tests for this, as introducing new regex in shortcodes has already bitten us in the past :)

[westonruter]

I think it would be harder to maintain if the AMP code is disconnected from the modules' non-AMP code.

You're right about this needing unit tests. I'll try to add some.

[westonruter]

@jeherve It seems there aren't any tests for the Gist shortcode/oEmbed yet. Is that right? I'd rather not start from scratch.

[westonruter]

Temporary Jetpack extension which can be used until this is part of a release: https://gist.github.com/westonruter/8c91e3a218eeb1403ce2eacb832c0783

[jeherve]

It seems there aren't any tests for the Gist shortcode/oEmbed yet. Is that right? I'd rather not start from scratch.

You're right, I thought we had some. I will add a first set of tests in #10127.

[jeherve]

I think it would be harder to maintain if the AMP code is disconnected from the modules' non-AMP code.

This is true. Let's keep that code here. That said, I think it would be useful to create a new method such as Jetpack::is_amp_request() instead of using Jetpack_AMP_Support::is_amp_request() everywhere. This makes the code easier to maintain compatible with the WordPress.com codebase for us, where Jetpack_AMP_Support does not exist. This will allow us to easily bypass sections on WordPress.com for now.

Does that make sense? Do you think you could add that in until the unit tests get merged?

Thank you!

[westonruter]

Does that make sense?

Yes, that makes sense to me! It doesn't matter to me where the is_amp_request() method lives.

However, there are some changes need to be made to the code in Jetpack_AMP_Support. In particular, the method \Jetpack_AMP_Support::is_amp_canonical() doesn't make sense as of 1.0-beta, nor does checking for isset( $_GET[ amp_get_slug() ] ). The is_amp_request() method should really just be simply:

return function_exists( 'is_amp_endpoint' ) && is_amp_endpoint();

The reason why Jetpack_AMP_Support is as it is today is due to how the AMP plugin was in 1.0-alpha as outlined in this reverted issue ampproject/amp-wp#1148. This necessarily had to be undone in ampproject/amp-wp#1235 because the template mode (native vs paired) and the presence of the ?amp query var is not sufficient to determine whether it is an AMP response. Since AMP now can be disabled for certain template types or for individual posts/pages—even in native mode when there is no AMP query var—a plugin must wait until the parse_query action is triggered so that the query vars are populated and the queried object is available.

This will mean that Jetpack will need updated in places such as this:

jetpack/3rd-party/class.jetpack-amp-support.php

Lines 49 to 50 in 05ebb6c

	// disable Likes metabox for post editor if AMP canonical disabled
	add_filter( 'post_flair_disable', array( 'Jetpack_AMP_Support', 'is_amp_canonical' ), 99 );

So all of this to say: when Jetpack::is_amp_endpoint() is added, it should abandon how the checks are being done in Jetpack_AMP_Support. I think this should be done in a separate PR as it will require more changes than are in scope for adding AMP support for Gist embeds.

In the mean time, I can add unit tests for amp-gist once you've merged the PR with the unit tests.

[jeherve]

Thanks for all the details; that makes perfect sense! I'll let you know once the Unit Tests are in.

[kraftbj]

Unit tests have been merged.

[westonruter]

@jeherve tests have been added.

[kraftbj]

I'm having trouble testing this. Using my own gist or directly using the snippet above with this branch, tested on the stable AMP plugin (q: do I need a beta version of AMP 1.0?)

No console errors. Validates as valid AMP. It produces <div class="gist-oembed" data-gist="kraftbj/f39f6e93a4e7e63934bb097bf7ccc525.json"></div>. At the end of my day so can attempt to dig in to determine what might be amiss tonight or tomorrow.

[westonruter]

@kraftbj I just checked on the 0.7.2 release and 1.0-beta4 alike and both are working for me. Is your test environment on HTTPS? That is required.

[kraftbj]

False alarm @westonruter — something was weird with pulling down our beta server's built version of the branch.

On desktop Chrome, everything looks as expected. For kicks, in Firefox Dev on desktop, as more items are embedded, it begins to cut off the ends of the embed.

Notice how the gists start complete, then slowly the height is reduced until it fully loses the footer, plus some:

I'm open to accepting this with that defect present given the normal usage of AMP and it is somewhat a corner case, but want to get your thoughts on where the issue may be with it.

[westonruter]

@kraftbj Interesting. I think you've identified a bug with the amp-gist component itself, and this isn't something that could be fixed with the integration here.

[kraftbj]

Thanks @westonruter! That makes sense.

[jeherve]

This works for me. Thank you! Merging.