Ensure that first content image is not lazy-loaded in block themes either #3560

felixarntz · 2022-11-03T21:53:36Z

This PR addresses all specific problems that causes the lazy-loading logic to not work correctly in block themes, covering both content images and featured images (i.e. post-content block and post-featured-image block).
In general, for either of them, one problem is that wp_filter_content_tags() runs over the entire template content after both the featured images and the post content images have already been processed once though. While the underlying problem for that is fixed in Avoid double-processing post content when parsing block template HTML #3549, this PR fixes the loading="lazy" problem specifically in a decoupled manner, ensuring that by default the function always returns false when parsing the entire block template (via a new the_block_template context).
With that, there is still a remaining problem though that specifically the featured image in the post-featured-image block would still receive loading="lazy" by default. The reason for that is that, in a block theme, the relevant logic runs outside of the loop - which strictly speaking is a backward-compatibility break in the first place. A fix for the exact same problem is already present in the post-content block, so this PR simply adds the same fix to the post-featured-image block.
With these changes, the problems are addressed. Tests are included to verify all of the fixes work correctly.

Trac ticket: https://core.trac.wordpress.org/ticket/56930

Test instructions

Any images can be used for the below tests.

Use a block theme, e.g. Twenty Twenty-Three, with its factory templates.
Test 1: Create a post with 2 core/image blocks. When viewing them in the frontend, ensure that the first image does not have a loading="lazy" attribute, while the second does have it.
Test 2: On the same post, set a featured image. When viewing the home page (which includes that post and its featured image), ensure that the featured image does not have a loading="lazy" attribute.
Test 3: Go to the Site Editor and edit the "Header" template part. Insert a core/image block anywhere in it. Then do the same for the "Footer" template part, insert a core/image block anywhere in it. When viewing the frontend, ensure that the image you placed in the "Header" does not have a loading="lazy" attribute, while the image you placed in the "Footer" does have it.

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

…s either.

felixarntz · 2022-11-03T22:32:54Z

The e2e test failing here is solely because I added one of the editor block files directly. In order to address that bit properly, I've opened WordPress/gutenberg#45534 to bring the same change to Gutenberg (and eventually port it to core).

src/wp-includes/blocks/post-featured-image.php

adamsilverstein

left one question about calling the_post

felixarntz · 2022-11-16T21:05:39Z

Update: The related Gutenberg PR has been merged (see WordPress/gutenberg#45534). Once that update is in WordPress core, I can update this PR to have the e2e test pass.

felixarntz · 2023-01-16T21:14:25Z

src/wp-includes/media.php

+	// Skip lazy-loading for the overall block template, as it is handled more granularly.
+	if ( 'the_template' === $context ) {
+		return false;
 	}

-	// Only elements within the main query loop have special handling.
-	if ( is_admin() || ! in_the_loop() || ! is_main_query() ) {
-		return 'lazy';
+	// Do not lazy-load images in the header block template part, as they are likely above the fold.
+	$header_area = WP_TEMPLATE_PART_AREA_HEADER;
+	if ( "the_template_part_{$header_area}" === $context ) {
+		return false;
 	}


Note for reviewers: These green lines are the only two things that are new in this function.

All remaining changes to this function look like more than they are due to GitHub: I wrapped the existing code in the condition for the_content and the_post_thumbnail to make the function logic easier to follow, and because of that GitHub makes it hard to decipher what's actually new here and what isn't. The logic for handling the_content and the_post_thumbnail is exactly the same as before.

felixarntz · 2023-01-16T21:19:47Z

@adamsilverstein This PR is now ready for a full review, since https://core.trac.wordpress.org/changeset/55079 is now committed to unblock this.

I have updated this against latest trunk today and gave it another test, everything appears to be working well.

The latest version of the PR has a more comprehensive solution than before, also covering block template parts that run through this logic as well. An easy win here was to also not lazy-load images by default that are in the header template part, since those images are very likely above the fold anyway (they appear certainly before the content images that we have been handling so far). The most relevant changes for this are https://github.com/WordPress/wordpress-develop/pull/3560/files#diff-136b3429427993c3d47e44fe6c9a046635aa8d2c78542a90c8a172468addcb78R137 and https://github.com/WordPress/wordpress-develop/pull/3560/files#diff-4d28b6652f0dab208c19aa45e1cf09f3c967060d8cc77502bfcd473a3216b612R5441-R5444.

felixarntz · 2023-01-16T21:59:25Z

Too bad I didn't spot the issue with the template-part block before... Now that I edited it, I of course shouldn't have done that, instead we need to fix that bit in Gutenberg. I've opened WordPress/gutenberg#47203 for that purpose. This is the only thing that makes the e2e test above fail.

While this PR has to wait for that to be merged and backported, in terms of code this is already good to review.

…g#47203.

felixarntz · 2023-01-27T22:57:48Z

Update: This PR is simply waiting for WordPress/gutenberg#47203 to be backported to WordPress core, which will happen as part of the Gutenberg 15.1 RC backport.

The pull request can already be reviewed as is and should be fully functional and ready for commit as soon as the backport has happened. Please ignore the e2e test failure since that failure is purely due to this PR modifying the one file that should come from Gutenberg instead. This also means the changes in the template-part.php file can be pretty much ignored for code review, since they have already been merged in Gutenberg and will be made available in core soon.

anton-vlasenko

Nitpick: it would be nice to add @covers tags to the test methods as this helps to calculate code coverage correctly.
The rest of the code looks good to me.

tests/phpunit/tests/media.php

felixarntz · 2023-02-07T20:46:46Z

@adamsilverstein @mukeshpanchal27 @hellofromtonya This is now ready for a full review. Since the Gutenberg package updates have been backported, the remaining conflict is now resolved.

It would be great if we could land this in time for the 6.2 Beta 2, as it would benefit from further testing on real websites using the Beta.

tests/phpunit/tests/media.php

mukeshpanchal27 · 2023-02-10T09:28:31Z

@felixarntz The code changes appear to be correct to me, and I tested all of the test scenarios listed in the description, and they all worked properly. Great work!

costdev

Thanks @felixarntz! The functionality in the PR tests well. I followed the testing instructions for Twenty-Twenty Three, Twenty-Twenty Two and TT1 Blocks and all tests were successful.

Additionally, after test 3, I browsed to the single post page:

Header: Not lazy loaded.
Featured image: Not lazy loaded.
First post image: Lazy loaded.
Second post image: Lazy loaded.
Footer image: Lazy loaded.

I've left some suggestions in the review regarding some additional tests and some minor docs additions.

A coverage report shows that line/branch coverage for wp_get_loading_attr_default() is complete (note: not necessarily all applicable scenarios to protect BC going forward). As the function appears to be called twice during the execution of these integration tests, it took a little closer review to make sure each branch is hit correctly. 😅

src/wp-includes/media.php

tests/phpunit/tests/media.php

costdev · 2023-02-10T09:46:52Z

tests/phpunit/tests/media.php

+	 * @covers ::wp_filter_content_tags
+	 * @covers ::wp_get_loading_attr_default
+	 */
+	public function test_wp_filter_content_tags_does_not_lazy_load_first_featured_image_in_block_theme() {


Query: Should this have "first_featured_image", or just "featured_image"?

"first_featured_image" is correct, as other featured images further down in the page should still be lazy-loaded.

tests/phpunit/tests/media.php

costdev · 2023-02-10T09:54:56Z

tests/phpunit/tests/media.php

+	 * @covers ::wp_filter_content_tags
+	 * @covers ::wp_get_loading_attr_default
+	 */
+	public function test_wp_filter_content_tags_does_not_lazy_load_first_featured_image_in_block_theme() {


In addition to testing:

Doesn't have a featured image, and the first image is not lazy loaded.

Has a featured image, which is not lazy loaded.

Should we also cover this?

Has a featured image, and the first image afterwards is lazy loaded.

@costdev

Has a featured image, and the first image afterwards is lazy loaded.

Hmm, this is already being tested here I think. The final assertion tests that the featured image is not lazy-loaded, while the first image afterwards (an in-content image) is lazy-loaded.

costdev · 2023-02-10T09:56:10Z

tests/phpunit/tests/media.php

+	 * @covers ::wp_filter_content_tags
+	 * @covers ::wp_get_loading_attr_default
+	 */
+	public function test_wp_filter_content_tags_does_not_lazy_load_images_in_header() {


Should we also add another test to ensure that images in a footer template part are always lazy loaded? I'm thinking in case something is later changed that accidentally excludes footer images from lazy loading.

@costdev This is already being tested by this method, the final assertion checks the entire template which includes both a header and footer template part, each with an image. Only the one in the header template part is expected to not be lazy-loaded, while the one in the footer should be.

adamsilverstein

+1 looks good to me, like the suggestion of one more unit test for footer if you have a chance.

…same test.

felixarntz · 2023-02-11T01:27:05Z

@mukeshpanchal27 @costdev Thanks for the feedback and testing! I think I have addressed it all, please take a look.

robinwpdeveloper · 2023-02-11T03:18:12Z

Some checks are failing!
Do we need to re-run or is it legit issue?
All of errors saying:
Error: Command failed: docker-compose run php ./vendor/bin/phpunit --verbose -c phpunit.xml.dist

costdev

Thanks for the updates and responses @felixarntz! LGTM 👍

Looks like the test failures are related to the order of the is-layout-flow, entry-content, and wp-block-post-content classes in the markup.

mukeshpanchal27

Thanks @felixarntz, The changes look good to me.

felixarntz · 2023-02-13T18:11:38Z

@robinwpdeveloper @costdev Test failures have been fixed.

felixarntz · 2023-02-13T18:34:29Z

Committed in https://core.trac.wordpress.org/changeset/55318.

Fix bug so that first content image is not lazy-loaded in block theme…

2be5c02

…s either.

felixarntz requested a review from adamsilverstein November 3, 2022 21:54

adamsilverstein reviewed Nov 9, 2022

View reviewed changes

src/wp-includes/blocks/post-featured-image.php Outdated Show resolved Hide resolved

adamsilverstein reviewed Nov 9, 2022

View reviewed changes

Mamaduka mentioned this pull request Nov 29, 2022

Update packages for featured image fix #3702

Closed

felixarntz added 2 commits January 16, 2023 21:01

Merge branch 'trunk' into fix/56930-fse-lcp-image-lazyloaded

0f2b239

Enhance fix to also provide basic support for block templates.

6f61ab5

felixarntz commented Jan 16, 2023

View reviewed changes

Fix WPCS violation.

af3dc5e

felixarntz requested a review from adamsilverstein January 16, 2023 21:21

This was referenced Jan 16, 2023

Fix first image lazy loading with block themes #3538

Closed

Parse (area-specific) context in template-part block. WordPress/gutenberg#47203

Merged

felixarntz added 3 commits January 18, 2023 08:54

Merge branch 'trunk' into fix/56930-fse-lcp-image-lazyloaded

6fe3831

Merge branch 'trunk' into fix/56930-fse-lcp-image-lazyloaded

6230509

Remove unnecessary the_ prefix, in accordance with WordPress/gutenber…

9597731

…g#47203.

felixarntz requested review from mukeshpanchal27 and Mamaduka January 27, 2023 22:53

Make code changes for parity with WordPress/gutenberg#47203.

9ab8c44

felixarntz requested a review from hellofromtonya January 27, 2023 22:57

anton-vlasenko reviewed Feb 3, 2023

View reviewed changes

tests/phpunit/tests/media.php Show resolved Hide resolved

tests/phpunit/tests/media.php Show resolved Hide resolved

felixarntz added 2 commits February 3, 2023 11:15

Merge branch 'trunk' into fix/56930-fse-lcp-image-lazyloaded

ed2b3ef

Add covers test annotations.

061565a

Merge branch 'trunk' into fix/56930-fse-lcp-image-lazyloaded

bf56e30

felixarntz requested a review from joemcgill February 9, 2023 00:25

mukeshpanchal27 reviewed Feb 10, 2023

View reviewed changes

tests/phpunit/tests/media.php Outdated Show resolved Hide resolved

tests/phpunit/tests/media.php Outdated Show resolved Hide resolved

costdev reviewed Feb 10, 2023

View reviewed changes

adamsilverstein approved these changes Feb 10, 2023

View reviewed changes

felixarntz added 4 commits February 10, 2023 17:14

Merge branch 'trunk' into fix/56930-fse-lcp-image-lazyloaded

b5ba312

Update test docs spacing and use multiple messages for assertions in …

36fb703

…same test.

Update multiline comment format.

f4ccd96

Add descriptions to test methods.

eaa4d61

felixarntz requested review from mukeshpanchal27 and costdev February 11, 2023 01:27

costdev approved these changes Feb 11, 2023

View reviewed changes

mukeshpanchal27 approved these changes Feb 13, 2023

View reviewed changes

felixarntz added 2 commits February 13, 2023 09:50

Merge branch 'trunk' into fix/56930-fse-lcp-image-lazyloaded

30b9443

Fix broken tests after recent trunk update.

bdb0121

felixarntz closed this Feb 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that first content image is not lazy-loaded in block themes either #3560

Ensure that first content image is not lazy-loaded in block themes either #3560

felixarntz commented Nov 3, 2022 •

edited

Loading

felixarntz commented Nov 3, 2022 •

edited

Loading

adamsilverstein left a comment

felixarntz commented Nov 16, 2022

felixarntz Jan 16, 2023 •

edited

Loading

felixarntz commented Jan 16, 2023

felixarntz commented Jan 16, 2023

felixarntz commented Jan 27, 2023

anton-vlasenko left a comment •

edited

Loading

felixarntz commented Feb 7, 2023

mukeshpanchal27 commented Feb 10, 2023

costdev left a comment

costdev Feb 10, 2023

felixarntz Feb 11, 2023

costdev Feb 10, 2023

felixarntz Feb 11, 2023

costdev Feb 10, 2023

felixarntz Feb 11, 2023

adamsilverstein left a comment

felixarntz commented Feb 11, 2023

robinwpdeveloper commented Feb 11, 2023

costdev left a comment •

edited

Loading

mukeshpanchal27 left a comment •

edited

Loading

felixarntz commented Feb 13, 2023

felixarntz commented Feb 13, 2023

Ensure that first content image is not lazy-loaded in block themes either #3560

Ensure that first content image is not lazy-loaded in block themes either #3560

Conversation

felixarntz commented Nov 3, 2022 • edited Loading

Test instructions

felixarntz commented Nov 3, 2022 • edited Loading

adamsilverstein left a comment

Choose a reason for hiding this comment

felixarntz commented Nov 16, 2022

felixarntz Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

felixarntz commented Jan 16, 2023

felixarntz commented Jan 16, 2023

felixarntz commented Jan 27, 2023

anton-vlasenko left a comment • edited Loading

Choose a reason for hiding this comment

felixarntz commented Feb 7, 2023

mukeshpanchal27 commented Feb 10, 2023

costdev left a comment

Choose a reason for hiding this comment

costdev Feb 10, 2023

Choose a reason for hiding this comment

felixarntz Feb 11, 2023

Choose a reason for hiding this comment

costdev Feb 10, 2023

Choose a reason for hiding this comment

felixarntz Feb 11, 2023

Choose a reason for hiding this comment

costdev Feb 10, 2023

Choose a reason for hiding this comment

felixarntz Feb 11, 2023

Choose a reason for hiding this comment

adamsilverstein left a comment

Choose a reason for hiding this comment

felixarntz commented Feb 11, 2023

robinwpdeveloper commented Feb 11, 2023

costdev left a comment • edited Loading

Choose a reason for hiding this comment

mukeshpanchal27 left a comment • edited Loading

Choose a reason for hiding this comment

felixarntz commented Feb 13, 2023

felixarntz commented Feb 13, 2023

felixarntz commented Nov 3, 2022 •

edited

Loading

felixarntz commented Nov 3, 2022 •

edited

Loading

felixarntz Jan 16, 2023 •

edited

Loading

anton-vlasenko left a comment •

edited

Loading

costdev left a comment •

edited

Loading

mukeshpanchal27 left a comment •

edited

Loading