Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use loading=lazy as a signal for images to skip as candidates in DetermineHeroImages transformer #6061

Closed
westonruter opened this issue Apr 9, 2021 · 1 comment · Fixed by #6062
Assignees
Labels
Enhancement New feature or improvement of an existing one Optimizer Performance
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Apr 9, 2021

Feature description

Update DetermineHeroImages transformer to use loading=lazy as a signal for images which should not be prerendered. As can be seen in wpcore#52139, the featured image omits the loading attribute for the featured image because it should not be loaded lazily. This is in contrast with images which are added to the content, which should be (normally). Another example of this is the Custom Logo which likewise does not get loading=lazy as per wpcore#50425.

The DetermineHeroImages transformer currently does SSR for Image blocks and Cover blocks which are at the beginning of the content:

/**
* XPath query to find the first entry-content.
*
* @var string
*/
const FIRST_ENTRY_CONTENT_XPATH_QUERY = ".//*[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' entry-content ' ) ][1]";
/**
* XPath query to find background image of a Cover Block at the beginning of post content (including nested inside of another block).
*
* @var string
*/
const INITIAL_COVER_BLOCK_XPATH_QUERY = "./*[1]/descendant-or-self::div[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-block-cover ' ) ]/amp-img[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-block-cover__image-background ' ) ][ not( @data-hero ) ]";
/**
* XPath query to find Image Block at the beginning of post content (including nested inside of another block).
*
* @var string
*/
const INITIAL_IMAGE_BLOCK_XPATH_QUERY = "./*[1]/descendant-or-self::figure[ contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-block-image ' ) ]/amp-img[ not( @data-hero ) ]";

That can remain in place.

What could change however is the logic for determine the custom header and custom logo which currently rely on specific class names being present (and where there is a lack of standardization for custom headers):

/**
* XPath query to find the custom logo.
*
* @var string
*/
const CUSTOM_HEADER_XPATH_QUERY = ".//*[ @id = 'wp-custom-header' or @id = 'masthead' or @id = 'site-header' or contains( concat( ' ', normalize-space( @class ), ' ' ), ' wp-custom-header ' ) ]//amp-img[ not( @data-hero ) and not( contains( concat( ' ', normalize-space( @class ), ' ' ), ' custom-logo ' ) ) ]";

The CUSTOM_HEADER_XPATH_QUERY in particular could be replaced as follows:

const CUSTOM_HEADER_XPATH_QUERY = ".//amp-img[ not( @data-hero ) and not( contains( concat( ' ', normalize-space( @class ), ' ' ), ' custom-logo ' ) ) ][ not( noscript/img/@loading ) or noscript/img/@loading != 'lazy' ][1]";

This will select the first image that was not originally marked with loading=lazy (as the AMP_Img_Sanitizer will preserve that in the noscript fallback).

For fear of CUSTOM_HEADER_XPATH_QUERY accidentally selecting an image out of the first viewport, some protection for this is proposed in ampproject/amp-toolbox#1196. However, we'll have to decide whether data-hero-candidate should also get loading=lazy added to the SSR'ed image. Also, while using loop_start as suggested in #5934 (review) wasn't ideal, something else we could consider for the header image is only image elements that occur before main or else FIRST_ENTRY_CONTENT_XPATH_QUERY.

Aside: The CUSTOM_LOGO_XPATH_QUERY should also be modified to only select the first result by adding [1] to the end of the expression. (That doesn't work as it just selects the first child, not the first instance overall.)

With these changes, we can then the DetermineHeroImages transformer by default, whereas currently we only enable it for the core themes.


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

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

@westonruter westonruter added Enhancement New feature or improvement of an existing one Performance Optimizer labels Apr 9, 2021
@westonruter westonruter added this to the v2.1 milestone Apr 9, 2021
@westonruter westonruter self-assigned this Apr 9, 2021
@westonruter westonruter self-assigned this Apr 27, 2021
@westonruter
Copy link
Member Author

westonruter commented Apr 27, 2021

QA Passed

I tried adding an image with loading=lazy to the header via:

add_action( 'wp_body_open', function () {
	echo "<img src='https://wordpress-stable.lndo.site/wp-content/uploads/2021/04/American_bison_k5680-1-1200x783-1.jpg' loading=lazy>";
});

The resulting amp-img did not have data-hero. However, when loading=lazy was omitted, then the image did get data-hero.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement of an existing one Optimizer Performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant