Use data-hero-candidate attribute in DetermineHeroImages

[schlessera]

Summary

This PR changes the DetermineHeroImages transformer to use data-hero-candidate attributes instead of data-hero attributes. It also adds detection of WordPress custom header images, fixes detection for images in Cover blocks, and adds detection for Image blocks. For Cover blocks and Image blocks, only contained images which are in the first .entry-content of the document which are additionally inside the first element child will be considered for hero image candidates.

NOTE: As discussed, this PR now removes the DetermineHeroImages transformer from the list of default transformers, as the logic is too error-prone at this point. The transformer is now enabled for all core themes by default. The transformer can be enabled for testing purposes (or for other themes) via the 'amp_optimizer_config' filter:

add_filter( 'amp_optimizer_config', static function ( $config ) {
	array_unshift( $config['transformers'], 'AmpProject\\AmpWP\\Transformer\\DetermineHeroImages' );
	return $config;
} );

It will likely be enabled by default in a subsequent release, even v2.1.x.

Fixes #5824
Fixes #5923

Checklist

• My code is tested and passes existing tests.
• My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

[codecov]

Codecov Report

Merging #5934 (3038aa4) into develop (2adfd39) will increase coverage by 0.09%.
The diff coverage is 80.64%.

❗ Current head 3038aa4 differs from pull request most recent head bcb50e5. Consider uploading reports for the commit bcb50e5 to get more accurate results

@@              Coverage Diff              @@
##             develop    #5934      +/-   ##
=============================================
+ Coverage      75.28%   75.38%   +0.09%     
- Complexity      5722     5735      +13     
=============================================
  Files            214      214              
  Lines          17283    17312      +29     
=============================================
+ Hits           13012    13050      +38     
+ Misses          4271     4262       -9     

Flag	Coverage Δ	Complexity Δ
javascript	79.44% <ø> (ø)	0.00 <ø> (ø)
php	75.22% <80.64%> (+0.09%)	0.00 <13.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...udes/sanitizers/class-amp-core-theme-sanitizer.php	35.87% <0.00%> (-0.26%)	229.00 <1.00> (+1.00)	⬇️
includes/sanitizers/class-amp-meta-sanitizer.php	79.79% <0.00%> (ø)	35.00 <0.00> (ø)
src/Transformer/DetermineHeroImages.php	91.04% <90.19%> (+38.66%)	25.00 <12.00> (+12.00)
includes/class-amp-theme-support.php	86.54% <100.00%> (-0.02%)	242.00 <0.00> (ø)
includes/utils/class-amp-dom-utils.php	91.76% <100.00%> (+0.19%)	35.00 <0.00> (ø)
includes/sanitizers/class-amp-img-sanitizer.php	97.59% <0.00%> (-0.02%)	83.00% <0.00%> (ø%)
src/Component/Carousel.php	100.00% <0.00%> (ø)	27.00% <0.00%> (ø%)

[westonruter]

Checking with Twenty Twelve, and the header image is not getting data-hero:

Same for Twenty Fourteen and Twenty Sixteen.

However, Twenty Seventeen does add data-hero to the header image and the featured image in the post. As discussed before, this is because only Twenty Seventeen uses the_custom_header_markup() and thus the wp-custom-header ID/className. So it's not accounting for themes that aren't using this newish template tag.

Also, if I have an Image block as the first block in a post, then I only get the header image in Twenty Seventeen gets prerendered. The Image block does not. Same for the Cover block. So in Twenty Twelve, I could have a header image set followed by a post with a Cover block or Image block as the first image, and nothing would be prerendered. This was an issue reported previously as well: #5824 (comment).

[westonruter]

@jono-alderson On your site, you are manually outputting your header image as well, as opposed to using any block or custom header code, correct? I don't believe it would be detected currently as a hero image candidate with this PR.

[westonruter]

I feel that there are still too many scenarios where valid hero images are going to be missed.

What if we rendered out an HTML comment at the loop_start action and then by default treated every non-tiny image before that in the DOM as being a hero image candidate? That should account for all images that are located in the header and would avoid accidentally prerendering any image that is not in the initial viewport. (Granted, sometimes singular templates will skip doing The Loop and will just put a the_post() at the top even before get_header(), however this is not done in any core theme and it would just mean none would be detected.) Consider this code:

$on_loop_start = function () use (&$on_loop_start) {
	remove_action( 'loop_start', $on_loop_start );
	if ( ! did_action( 'get_header' ) ) {
		return;
	}
	echo '<!--AMP:LOOP_START-->';
};
add_action( 'loop_start', $on_loop_start );

The transformer could walk over all elements, marking non-tiny images as candidates, until it gets to this node and then exits.

Beyond the header, when is_singular(), if there is a Cover block or Image block at the beginning of post content, then I think it should be made a hero image candidate as well. (Not even blocks-specifically, but rather whether the queried post content starts with an image at all, such as in the case of sites using the Classic Editor.)

[westonruter]

An alternative way to identify the header image which may be more robust as opposed to relying on element IDs is to obtain the header image URL and then query for image elements that have that src:

Suggested change

	$elements = $document->xpath->query(
	self::CUSTOM_HEADER_XPATH_QUERY,
	$document->body
	);
	$header_image = get_header_image();
	if ( ! $header_image ) {
	return null;
	}
	$elements = $document->xpath->query(
	sprintf(
	'//*[ ( self::img or self::amp-img ) and @src = "%s" ]',
	addcslashes( $custom_header->url, '"\\' )
	),
	$document->body
	);

But this is going to have other problems, namely because get_header_image_tag() allows for the markup to be filtered, meaning the image URL could be replaced with one that is pointing to a CDN.

[westonruter]

An alternative idea to injecting a <!--AMP:START_LOOP--> comment could be to inject a data attribute on all images before the loop starts:

add_filter( 'wp_get_attachment_image_attributes', function ( $attrs ) {
	if ( ! did_action( 'loop_start' ) ) {
		$attrs['data-hero-candidate'] = '';
	}
	return $attrs;
} );

Nevertheless, this won't help with Twenty Sixteen which outputs its header image as follows:

<img src="<?php header_image(); ?>" srcset="<?php echo esc_attr( wp_get_attachment_image_srcset( get_custom_header()->attachment_id ) ); ?>" sizes="<?php echo esc_attr( $custom_header_sizes ); ?>" width="<?php echo esc_attr( get_custom_header()->width ); ?>" height="<?php echo esc_attr( get_custom_header()->height ); ?>" alt="<?php echo esc_attr( get_bloginfo( 'name', 'display' ) ); ?>">

[github-actions]

Plugin builds for bcb50e5 are ready 🛎️!

• Download development build
• Download production build

[westonruter]

@schlessera I went about fixing #5923 in this PR as well, and while I was at fixing Cover Block detection I also added detection for Image blocks. As I've indicated in the updated description, only Cover/Image Blocks which are in the first child element of the first .entry-content will be considered.

Since we know that the transformer works for core themes, I've decided to enable it via the core theme sanitizer. Other themes can opt-in at the 2.1 release, but we can flip it on for all themes at 2.1.x once we get testing with more themes.

Please review the changes.

[westonruter]

Rather than having to do this for static analysis, the getElementId method could indicate the $element is a DOMElement or an Element.

[schlessera]

@westonruter The cover block image will now only work on the very latest WP release, and not be detecting on any prior release. Was that intended? IIRC we claim to support not only the latest stable version of Core, but all versions from 4.9 onwards or so?

[westonruter]

Prior versions of the Cover Block didn't use an img at all, so wouldn't they be ineligible for prerendering anyway? Or rather, they would have been "pre-rendered" already since they used a background-image.

[westonruter]

(Not sure why code coverage is reporting untested lines.)

[schlessera]

Prior versions of the Cover Block didn't use an img at all, so wouldn't they be ineligible for prerendering anyway? Or rather, they would have been "pre-rendered" already since they used a background-image.

Oh, right. So they would not need prerendering, but they would benefit from preloading. However, we were discussing removing the preloading from hero images anyway and just keep the prerendering, so this means it would be useless then. The amp-toolbox-php is still missing this change, but it will be added with the next release, so I guess we're good at the WP front for now.

[schlessera]

I replaced a constant and I added covers annotations to fix the code coverage.

LGTM now.