Skip to content

Fix first image lazy loading with block themes #3538

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.

Sign up for GitHub

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

Closed
adamsilverstein wants to merge 3 commits into from
Closed

Fix first image lazy loading with block themes #3538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fcc58b3
Fix first image lazy loading with block themes
adamsilverstein Oct 28, 2022
4e54f93
Improve tests with context
adamsilverstein Oct 28, 2022
6d901d5
Refine tests
adamsilverstein Oct 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/wp-includes/media.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1736,7 +1736,7 @@ function wp_lazy_loading_enabled( $tag_name, $context ) {
// By default add to all 'img' and 'iframe' tags.
// See https://html.spec.whatwg.org/multipage/embedded-content.html#attr-img-loading
// See https://html.spec.whatwg.org/multipage/iframe-embed-object.html#attr-iframe-loading
$default = ( 'img' === $tag_name || 'iframe' === $tag_name );
$default = false !== $context && ( 'img' === $tag_name || 'iframe' === $tag_name );

@adamsilverstein adamsilverstein Oct 28, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several places we could adjust for block theme behavior, what do you think @felixarntz?

@felixarntz felixarntz Oct 28, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamsilverstein When is the $context false here? Does the post content in a block theme not go through the_content?

@adamsilverstein adamsilverstein Oct 28, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when a block theme is enabled, the_content filter is applied to the content with $context false which appears to happen before the main application of the filter. I'm going to dig a bit further to see why that is happening, maybe we can avoid it in the first place.

@adamsilverstein adamsilverstein Oct 28, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably this:

wordpress-develop/src/wp-includes/blocks/post-content.php

Line 55 in 874aa24

$content = apply_filters( 'the_content', str_replace( ']]>', ']]>', $content ) );

@felixarntz felixarntz Oct 28, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamsilverstein Hmm that code doesn't look odd to me. The $context is never explicitly specified in that filter, it uses current_filter() by default, which means here it should be set to the_content. This would only be a problem if some core code called wp_filter_content_tags() directly without the parameter (i.e. outside of a filter).

@adamsilverstein adamsilverstein Oct 31, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed I verified that isn't the problem, I'm going to dig in more to trace back and find where the false context value is coming from.

@adamsilverstein adamsilverstein Oct 31, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixarntz I dug in a bit further here...
wp_filter_content_tags is called several times by block themes in two places
: in src/wp-includes/blocks/template-part.php::render_block_core_template_part with no context and current_filter returning false.

Removing this call fixed the issue (but I don't think we can do that):

wordpress-develop/src/wp-includes/blocks/template-part.php

Line 137 in 20bccea

$content = wp_filter_content_tags( $content );

note: also called in src/wp-includes/block-template.php in get_the_block_template_html:

wordpress-develop/src/wp-includes/block-template.php

Line 244 in 5eddc8f

$content = wp_filter_content_tags( $content );

I considered some other fixes but the check in the current PR seemed the most reliable, what do you think?

@adamsilverstein adamsilverstein Oct 31, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sample trace:

wp_filter_content_tags (/wp-includes/media.php:1782)
render_block_core_template_part (/wp-includes/blocks/template-part.php:137)
WP_Block->render (/wp-includes/class-wp-block.php:256)
render_block (/wp-includes/blocks.php:1051)
do_blocks (/wp-includes/blocks.php:1089)
get_the_block_template_html (/wp-includes/block-template.php:240)
include (/wp-includes/template-canvas.php:12)
require_once (/wp-includes/template-loader.php:106)
require (/wp-blog-header.php:19)
{main} (/index.php:17)


/**
* Filters whether to add the `loading` attribute to the specified tag in the specified context.
Expand Down
4 changes: 2 additions & 2 deletions tests/phpunit/tests/media.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2332,7 +2332,7 @@ public function test_wp_filter_content_tags_handles_duplicate_img_and_iframe_tag

// Ensure the img and iframe filters only ran once because the content is a single duplicated img tag and a
// single duplicate iframe tag.
wp_filter_content_tags( $content );
wp_filter_content_tags( $content, 'the_content' );
$this->assertSame( 1, $img_filter->get_call_count() );
$this->assertSame( 1, $iframe_filter->get_call_count() );
}
Expand Down Expand Up @@ -3050,7 +3050,7 @@ public function test_wp_filter_content_tags_loading_lazy() {
add_filter( 'wp_img_tag_add_width_and_height_attr', '__return_false' );
add_filter( 'wp_img_tag_add_srcset_and_sizes_attr', '__return_false' );

$this->assertSame( $content_filtered, wp_filter_content_tags( $content_unfiltered ) );
$this->assertSame( $content_filtered, wp_filter_content_tags( $content_unfiltered, 'the_content' ) );

remove_filter( 'wp_img_tag_add_width_and_height_attr', '__return_false' );
remove_filter( 'wp_img_tag_add_srcset_and_sizes_attr', '__return_false' );
Expand Down