Make WordPress Core

Opened 8 months ago

Closed 8 months ago

Last modified 8 months ago

#58635 closed defect (bug) (fixed)

Images before main query loop are not counted towards lazy-loading threshold

Reported by: flixos90's profile flixos90 Owned by: flixos90's profile flixos90
Milestone: 6.3 Priority: normal
Severity: normal Version: 6.3
Component: Media Keywords: has-patch, has-unit-tests, has-dev-note
Focuses: performance Cc:

Description

#56930 (for block themes) and #58211 (for classic themes) added logic to also omit images before the main query loop from being lazy-loaded. While all of those images are now correctly omitting the loading="lazy" attribute, an issue introduced by that is that those images do not count towards the lazy-loading threshold (see wp_omit_loading_attr_threshold() function). This leads to a potentially excessive number of images to be lazy-loaded, for example even if the header already includes a few large enough images, WordPress core will only start counting images within the main query loop and thus omit loading="lazy" on the first x images (3 as of #58213) among those, which is pointless if the header already contained enough large images to cover all images within the viewport.

This ticket is about fixing that so that images with a certain minimum size in the header of the page are also counted towards the threshold. We can rely on the same squarepixel threshold introduced as part of #58235 used for fetchpriority="high" here.

The proposed fix is as follows: For the two conditions where images have lazy-loading omitted before the loop (i.e. the template_part_header context clause for block themes, and the $wp_query->before_loop clause for classic themes), the image should be counted towards the lazy-loading threshold if its size is greater than the minimum squarepixel threshold from the wp_min_priority_img_pixels filter.

Change History (13)

This ticket was mentioned in PR #4726 on WordPress/wordpress-develop by kt-12.


8 months ago
#1

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

kt-12 commented on PR #4726:


8 months ago
#2

@kt-12 This is a creative solution that relies on as little code changes as possible, which is great given we're already at the beta stage.

It makes the postprocessing closure a bit more clunky, but as that is an internal closure, we can always change it without any risk of breakage. The alternative would be separate closure just for that part, for example something like $maybe_increase_content_media_count = static function( $attr ) { .... That helps keeping the two functions simpler, which is nice, but on the flipside it means we have to add calls to the closure to the two places rather than a new parameter on the existing closure.

I don't feel strongly either way, happy to go with what you and others prefer. Probably not a decision to get hung up on since it's all temporary before a logic refactoring to make all of this function easier to follow (which we should do for 6.4 though, not now as it's too late in the release cycle for that).

I feel both are good and bad in there own way. We can wait for what other have to say here.

@spacedmonkey commented on PR #4726:


8 months ago
#3

@kt-12 @felixarntz I did some testing. With the 2017, I uploaded a custom header in the customizer. Images uploaded using get_header_image_tag function are not counted. This seems like it should be converted by this PR.

@flixos90 commented on PR #4726:


8 months ago
#4

@spacedmonkey

I did some testing. With the 2017, I uploaded a custom header in the customizer. Images uploaded using get_header_image_tag function are not counted. This seems like it should be converted by this PR.

This is an unrelated problem to this PR, yet certainly a bug we should address. Can you open a Trac ticket for it?

#5 @spacedmonkey
8 months ago

  • Keywords commit added

Making as ready to commit @flixos90

@flixos90 commented on PR #4726:


8 months ago
#6

@kt-12 Now that https://core.trac.wordpress.org/changeset/56142 has been committed, I have pushed two follow-up commits to make this PR support that change:

#7 @flixos90
8 months ago

I have tested this PR with Twenty Twenty-Three and Twenty Seventeen and have verified that it works as expected. With an image in the header, and 3 images in post content, the 3rd post content image (4th image overall) is lazy-loaded. Prior to the PR, none of the 4 images would have been lazy-loaded because the header image wasn't counted towards the threshold.

#8 @flixos90
8 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 56143:

Media: Ensure that large images before the main query loop are counted towards lazy-loading threshold.

Following [55318], [55847], and [56142], certain images in the header of the page have received support for potentially receiving fetchpriority="high" and having the loading="lazy" attribute omitted. However, these images being present did not affect the "content media count" which counts the images towards a certain threshold so that the first ones are not lazy-loaded.

This changeset also increases that count for such header images if they are larger than a certain threshold. The threshold is used to avoid e.g. a header with lots of small images such as icon graphics to skew the lazy-loading behavior.

Props thekt12, spacedmonkey, flixos90.
Fixes #58635.

#10 @flixos90
8 months ago

  • Keywords needs-dev-note added; commit removed

This ticket was mentioned in Slack in #hosting by javier. View the logs.


8 months ago

#13 @flixos90
8 months ago

  • Keywords needs-dev-note removed
Note: See TracTickets for help on using tickets.