Header image fails to be prerendered

[westonruter]

Bug Description

I've found that header images (via custom-header theme support) fail to get pre-rendered when there is a custom logo or featured image also on the page. For example, in themes that render the header image with an img as opposed to a background-image, the presence of a featured image in the page will prevent the header image from getting SSR'ed, as any presence of data-hero will prevent any hero-image candidate detection (via PreloadHeroImage::detectHeroImageCandidate) from running.

In all four examples below, the header image lacks any prerendering. Only the “Horizontal” featured image gets prerendering:

Twenty Twelve	Twenty Fourteen	Twenty Sixteen	Twenty Seventeen

This would have a negative impact on LCP which should be entirely avoidable because both the header image and the featured image can both be prerendered as data-hero images.

Expected Behaviour

The header image should be prerendered whenever possible.

Steps to reproduce

1. Activate Twenty Twelve, Twenty Fourteen, Twenty Sixteen, or Twenty Seventeen.
2. Add a header image in the Customizer.
3. Navigate to a post that has a featured image assigned.
4. On the AMP page, see that the featured image has data-hero whereas the header image does not.

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• When no images on a page initially have data-hero, first two non-tiny images on a page should have get prerendered, including the custom logo, header image, featured image, cover blocks, and other detected image candidates.

Implementation brief

One way to implement this is to extend \AmpProject\AmpWP\Transformer\DetermineHeroImages::transform() to also include detection for header images in the same way it is detecting the custom logo, featured image, and cover blocks. In Twenty Seventeen, this could be accomplished by injecting data-hero into the header markup like so:

add_filter(
	'get_header_image_tag',
	function ( $html ) {
		return preg_replace( '/(?<=<img\s)/', 'data-hero ', $html );
	}
);

This works in Twenty Seventeen since that theme uses the_custom_header_markup(). Unfortunately, none of the other themes use this function to output the header. They instead construct the img manually so there is no opportunity for filtering. One way to address that would be for us to add theme-specific XPath selectors to match the header image (e.g. via the core theme sanitizer), but this is not ideal as it would not automatically work for other themes.

I think the problem boils down to DetermineHeroImages injecting the data-hero attribute into images. This is causing all other candidates, such as the header image, to be omitted during PreloadHeroImage::findHeroImages(). I think there needs to be a differentiation between a user-specified data-hero which should indeed override any others, but then for our DetermineHeroImages to add data-hero-candidate attributes instead of plain data-hero attributes, so that the PreloadHeroImage can include all images that have data-hero and then also include the first one or two images that have data-hero-candidate to also have server-side-rendering.

1. If there are two images that have (author-supplied)data-hero on the page already, then none of the data-hero-candidate images would be included.
2. If there was one image that had data-hero, then it would get prerendered in addition to the first image that has data-hero-candidate.
3. If there were no images that have data-hero, then the first two images that have data-hero-candidate would get prerendered.

Prerendering any image with data-hero-candidate would naturally include adding the data-hero attribute as well.

This will require changes to both the AMP plugin here and also the amp-toolbox-php project.

QA testing instructions

Demo

Changelog entry

[westonruter]

cc @schlessera

[westonruter]

The issue also is present for a user who has a regular Image block in hero position: https://wordpress.org/support/topic/data-hero-for-amp-optimizer/#post-14016250

[delawski]

QA passed (tentatively - see the comment below).

---

I've tested a few scenarios on Twenty Seventeen. I each case, I've ran the following code in the dev tools console so that any img that has the data-hero attribute gets a thick red border:

document.querySelectorAll( 'amp-img[data-hero]' ).forEach( el => el.style.border = '10px solid red' );

In each test case, 2 images at most that are within a viewport were assigned the data-hero attribute.

Questions:

• If there is no custom header, no logo, and no post thumbnail (4.b), only 1 image (within the Cover block) has been assigned the data-hero attribute. Is it expected? The same applies if relatively small images are added at the very top of the post - only one of them gets the data-hero attribute (5).
• Whenever a logo is present (2, 3), it feels that it's relatively small compared to other images. Should it still take precedence over the first Cover block image or regular image within a post content?

1. With custom header, without logo

a) With post thumbnail	b) Without post thumbnail

2. With custom header, with logo

a) With post thumbnail	b) Without post thumbnail

3. Without custom header, with logo

a) With post thumbnail	b) Without post thumbnail

4. Without custom header, without logo

a) With post thumbnail	b) Without post thumbnail

5. Without custom header, without logo, without featured image