Improve AMP support for Photon

[westonruter]

In testing a site using the AMP plugin in Standard/Transitional mode and in AMP Stories, I noticed a couple issues where Photon could better support AMP.

Proposed Changes

Support photonification of amp-img like img

I noticed that an <amp-img> put directly into post_content does not get processed by Photon. This isn't particularly surprising since it would be looking for <img> not <amp-img> (or <amp-anim>). To fix this, the regex in \Jetpack_Photon::parse_images_from_html() just needs to be updated like so:

- (?:<a[^>]+?href=["|\'](?P<link_url>[^\s]+?)["|\'][^>]*?>\s*)?(?P<img_tag><img[^>]*?\s+?src=["|\'](?P<img_url>[^\s]+?)["|\'].*?>){1}(?:\s*</a>)?
+ (?:<a[^>]+?href=["|\'](?P<link_url>[^\s]+?)["|\'][^>]*?>\s*)?(?P<img_tag><(?:img|amp-img|amp-anim)[^>]*?\s+?src=["|\'](?P<img_url>[^\s]+?)["|\'].*?>){1}(?:\s*</a>)?

Given post_content (e.g. a Custom HTML block) containing:

<amp-img src="https://example.com/wp-content/uploads/2019/06/downasaur-150x150.jpg" alt="" class="wp-image-9451" width="100" height=100></amp-img>

Before, with no change because Photon didn't see it:

<amp-img src="https://example.com/wp-content/uploads/2019/06/downasaur-150x150.jpg" alt="" class="wp-image-9451" width="100" height="100"></amp-img>

After, with image being replaced with Photon CDN as expected:

<amp-img src="https://i2.wp.com/example.com/wp-content/uploads/2019/06/downasaur.jpg?resize=100%2C100&amp;ssl=1" alt="" class="wp-image-9451" width="100" height="100"></amp-img>

Remove unnecessary data-recalc-dims=1

Additionally I noticed that Photon was adding the data-recalc-dims=1 attribute to images, but this is irrelevant in AMP since the devicepx JS is not loaded. So I'm not omitting this in AMP.

Prevent constraining to content_width in Stories

Originally reported in ampproject/amp-wp#2595.

The AMP Stories experience (amp_story plugin) intends to constrain the height of images to be 1440px, given the vertical resolution of mobile devices. This turned out to fail to work as expected when Photon supplied, since it constrains based on the theme's content_width.

For example, when setting the background image for a story page when a theme has a 640 content width, the generated markup with Photon is:

<amp-img layout="fill" src="https://i2.wp.com/example.com/wp-content/uploads/2019/06/downasaur.jpg?w=640&amp;ssl=1" class="amp-wp-6ed12b8"></amp-img>

The w=640 is the problem here. What is actually desired is h=1440.

In order to accomplish this, this PR adds a jetpack_photon_post_image_args filter which runs when on a singular amp_story post to override the args to be appropriate for the format.

In order to achieve this, additional context params needed to be passed to jetpack_photon_post_image_args in order to basically re-factor the following logic using a fixed height of 1440px instead of the content_width:

jetpack/class.photon.php

Lines 422 to 458 in 596bdd6

	// If width is available, constrain to $content_width
	if ( false !== $width && false === strpos( $width, '%' ) && is_numeric( $content_width ) ) {
	if ( $width > $content_width && false !== $height && false === strpos( $height, '%' ) ) {
	$height = round( ( $content_width * $height ) / $width );
	$width = $content_width;
	} elseif ( $width > $content_width ) {
	$width = $content_width;
	}
	}
	// Set a width if none is found and $content_width is available
	// If width is set in this manner and height is available, use `fit` instead of `resize` to prevent skewing
	if ( false === $width && is_numeric( $content_width ) ) {
	$width = (int) $content_width;
	if ( false !== $height )
	$transform = 'fit';
	}
	// Detect if image source is for a custom-cropped thumbnail and prevent further URL manipulation.
	if ( ! $fullsize_url && preg_match_all( '#-e[a-z0-9]+(-\d+x\d+)?\.(' . implode('|', self::$extensions ) . '){1}$#i', basename( $src ), $filename ) )
	$fullsize_url = true;
	// Build URL, first maybe removing WP's resized string so we pass the original image to Photon
	if ( ! $fullsize_url ) {
	$src = self::strip_image_dimensions_maybe( $src );
	}
	// Build array of Photon args and expose to filter before passing to Photon URL function
	$args = array();
	if ( false !== $width && false !== $height && false === strpos( $width, '%' ) && false === strpos( $height, '%' ) )
	$args[ $transform ] = $width . ',' . $height;
	elseif ( false !== $width )
	$args['w'] = $width;
	elseif ( false !== $height )
	$args['h'] = $height;

When this change is paired with support for photonizing <amp-img> in post_content (which the amp_story post type does), the result is a user can successfully upload new large images and have them appear with the expected 1440px height as opposed to the full size or a content_width-constrained size.

Testing instructions:

0. Upload a large image to the media library (do this first before the following).
1. Activate photon module
2. Install and activate AMP v1.2 and enable the Stories experience.
3. Upload another large image.
4. Create a new story and select the first uploaded image as a page background, then add another page and use the second image as its background.
5. Confirm that both story pages have images that include h=1440.

Proposed changelog entry for your changes:

• Improve AMP support in Photon, including handling of <amp-img> and <amp-anim> in post_content and ensuring proper height-constrained images are used in AMP Stories. Adds additional context to jetpack_photon_post_image_args filter, and remove AMP-unnecessary data-recalc-dims attribute.

[jetpackbot]

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: August 6, 2019.
Scheduled code freeze: July 30, 2019

Generated by 🚫 dangerJS against b7875e1

[jeherve]

That would be a nice improvement!

Do you think you could add some tests in tests/php/test_functions.photon.php, to make sure we don't break this in the future with other changes?

[westonruter]

@jeherve Thanks for the review. I believe I've addressed your feedback in what I just pushed.

Changes have been addressed

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello westonruter! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D30922-code before merging this PR. Thank you!

[jeherve]

This works well for me. This should be good to merge once we fix the Unit Test breakage caused by https://core.trac.wordpress.org/changeset/45687/