Correctly handle image types when preloading LCP images

[piotrbak]

Before submitting an issue please check that you’ve completed the following steps:

• Made sure you’re on the latest version
• Used the search feature to ensure that the bug hasn’t been reported before

Describe the bug
In the 3.16 we introduced the new feature that allows to preload LCP images. When the image has AVIF/WEBP/Regular versions put together into the Picture markup, we're preloading different types of images causing browser to load more than without the feature applied.
Unfortunately, there's no way to preload images conditionally. Because of that we'll add to preload the very first <source> element from the <picture> tag.

Markups
The following markup:
https://pastebin.com/PSzP7Abq
Should become:
https://pastebin.com/jS2UHg83

The following markup:
https://pastebin.com/uvmYgm9E
Should become:
https://pastebin.com/6d4wTdv7

Expected behavior
We should only preload the first source element in the picture tag. If it's AVIF, then we should preload AVIF image(s), if it's WEBP, then we should preload the WEBP image(s).

Additional context
The markups live can be seen here and here.

Acceptance Criteria (for WP Media team use only)

1. Only the highest (most optimized) source element should be preloaded
2. Both shared markups preloaded in the described way
3. No errors in the JS console after adding the preload tags
4. No regressions in the current templates (Closes #6530 #6531 #6532 #6534 #6535 #6552: Preload images markup #6559)
5. This enhancement should be applied only if different image types are defined within the picture tag
6. All elements within the picture tag (source img) should be excluded from the lazyload

[jeawhanlee]

@piotrbak I have some points to clarify:

• The expected behaviour outlined above is contradictory to the first AC:

  We should only preload the first source element in the picture tag. If it's AVIF, then we should preload AVIF image(s), if it's WEBP, then we should preload the WEBP image(s).

  Only the highest (most optimized) source element should be preloaded

  Do we preload the first source element or the most optimized source element? so in a case where we have WEBP/AVIF/REGULAR, would we preload AVIF if were picking the most optimized?

• For source elements with media attributes, do we still pick only the first or most optimized or we leave it as it is?

[piotrbak]

@jeawhanlee We're assuming that the first source is the most optimised one.

For the media attributes we're leaving it like it is. @DahmaniAdame Do you think it's possible to have that kind of complicated picture structure that would contain both, different types declarations and different media queries?

[DahmaniAdame]

It's a possibility. The media attribute in source tags is used for responsive image switching, more of a manual approach to display a totally different or customized version of an image.
But it will be very uncommon on WordPress where automated responsive generation rely mainly on srcset.
Not a problem to solve for now.

[jeawhanlee]

@piotrbak @DahmaniAdame just to be clear here, Are we only preloading the first source element when there is a combination similar to AVIF/WEBP/REGULAR or we are preloading the first source element regardless of the image type but with exception to source element with media attributes?

[DahmaniAdame]

@jeawhanlee @piotrbak it needs to be the first source element under the picture tag because the browser will respect that order and will stop in the first occurrence of supported format.

Let's assume WEBP source was declared before AVIF. The browser will show WEBP and will stop there.

[piotrbak]

@DahmaniAdame @jeawhanlee But here we're talking about the image/types only. If it's not defined, it should not be part of this enhancement, as per 5. point in AC.

[jeawhanlee]

Reproduce the problem ✅

I did not reproduce but I can see it in the sample templates in the issue here

Identify the root cause ✅

We are preloading every source in the picture element here

Scope a solution ✅

assets/js/lcp-beacon.js
We need to parse the type property to the object.

Engine\Media\AboveTheFold\Frontend::Controller

• We can do a condition here
  Check if prev_max_width is null and if the types returned are different then get the first element in $lcp->sources[0] and construct the preload tag before returning it.

  We can check if the types are different by getting the first type and doing a condition in the loop here for the rest and if not assign a type array to false.

Update tests.

Estimate the effort ✅

[S]

[jeawhanlee]

What I got from @piotrbak, This enhancement will not be applied if the type attribute is not defined in the source tag.
CC: @DahmaniAdame.

[piotrbak]

I think this is correct, we don't want to mess with other possible conditions.

[Tabrisrp]

LGTM

[bwafels]

@piotrbak I have just seen Google report that the image being served with the new LCP function is a jpeg file. It makes my pagespeed results way lower on mobile. Will this fix it? I use imagify to create Webp versions of all.

[DahmaniAdame]

@bwafels yes, it should :)

[bwafels]

@DahmaniAdame Ah good news. That means I do not need to make a new bug report :D. I did see that for my mobile version of the site the LCP does not seem to work but I send a message to support to check it out.

[DahmaniAdame]

@bwafels just had a look at the site you shared on the ticket.
This enhancement targets the <picture> tag.
Your cases is a responsive image, with what seems to be WebP cache applied. It should reference the .webp image out of the box.
@piotrbak looks like we are applying lcp (buffer 17) on buffer after webp (buffer 16), which would result on missing the preloaded image referencing the right format.
@bwafels I added a note for support to investigate to formally investigate that. A different bug will be opened on Github once confirmed.

[MathieuLamiot]

Moving back to Needs Grooming and adding product direction tag following this discussion: https://wp-media.slack.com/archives/CUT7FLHF1/p1716458584865109?thread_ts=1716418401.922879&cid=CUT7FLHF1

[Miraeld]

Reproduce the problem ✅

I did reproduce the problem,
Just create a page with each mentioned markup, and you can see it by yourself.

Identify the root cause ✅

There is a lot to say actually.

• When we detect potential candidates for LCP, here we base our elements list, by default, from here. Which means we are getting picture and img. While we do our document.querySelectorAll( this.config.elements ); we will get the <picture> AND the sub-tag <img> of this picture. So it's competing itself.
• When we check if an element is intersecting, with the viewport, for a picture element, we should check the <img> sub-tag as the actual image that gets displayed is provided by the <img> element nested inside the <picture> element. For this reason, the <picture> element itself does not have any visual content. It merely provides a context for the <img> element. This means that the dimensions and position of the <picture> element might not accurately represent the dimensions and position of the actual image being displayed.

Then about preloading the first <source> element of the <picture> tag, at the moment we are processing every <source> so we need to limit it.

And finally for the second case scenario in the initial message of the issue, we aren't getting the sizes attributes from the <picture> element, so we won't add it.

Scope a solution ✅

For the first 2 issues mentioned in the previous part, we need to modify the potential candidates part of the lcp-beacon.js like this:

const topCandidates = potentialCandidates.map(element => {
			// Skip if the element is an img and its parent is a picture.
			if ('img' === element.nodeName.toLowerCase() && 'picture' === element.parentElement.nodeName.toLowerCase() ) {
				return null;
			}
			let rect;
			if ('picture' === element.nodeName.toLowerCase()) {
				const imgElement = element.querySelector('img');
				if (imgElement) {
					rect = imgElement.getBoundingClientRect();
				} else {
					return null;
				}
			} else {
				rect = element.getBoundingClientRect();
			}

			return {
				element: element,
				rect: rect,
			};
		})
			.filter(item => item !== null) // Filter out null values here.
			.filter(item => {
				return (
					item.rect.width > 0 &&
					item.rect.height > 0 &&
					this._isIntersecting(item.rect)
				);
			})
			.map(item => ({
				item,
				area: this._getElementArea(item.rect),
				elementInfo: this._getElementInfo(item.element),
			}))
			.sort((a, b) => b.area - a.area)
			.slice(0, count);

In order to preload the first <source> element, we need to get the type (if the attribute exists) in order to make a difference between media / type.

element_info.sources = Array.from(element.querySelectorAll('source')).map(source => ({  
  srcset: source.srcset || '',  
    media: source.media || '',  
    type: source.type || ''
}));

Then in the Controller, we need to adapt the function generate_source_tags() here to add:

  
// Check if the media attribute is empty and the type attribute is not.  
if ( empty($media) && !empty($source->type) ) {  
  // Generate the link tag.  
  $tag .= $start_tag . 'href="' . $source->srcset . '"' . $end_tag;  
    break;  
} else {  
  // Append the source and media query to the tag string.  
  $tag .= $start_tag . 'href="' . $source->srcset . '" media="' . $media . '"' . $end_tag;  
}

This will make sure that we only preload the first <source>.

Finally, about the second case scenario, we need to add sizes to the attribute we save in the DB in order to add it later to the preload tag.
For this, we need to modify this

element_info.sources = Array.from(element.querySelectorAll('source')).map(source => ({  
  srcset: source.srcset || '',  
    media: source.media || '',  
    type: source.type || ''  
}));

to

element_info.sources = Array.from(element.querySelectorAll('source')).map(source => ({  
  srcset: source.srcset || '',  
    media: source.media || '',  
    type: source.type || '', 
    sizes: source.sizes || '' 
}));

And finally in the Controller::generate_source_tags, we need to add in the loop

if ( ! empty( $source->sizes ) ) {  
  $sizes = 'imagesizes="'.$source->sizes.'"';  
}

// Check if the media attribute is empty and the type attribute is not.
if ( empty($media) && !empty($source->type) ) {  
  // Generate the link tag.  
  $tag .= $start_tag . 'href="' . $source->srcset . '" ' . ($sizes ?? '') . $end_tag ;  
    break;  
} else {  
  // Append the source and media query to the tag string.  
  $tag .= $start_tag . 'href="' . $source->srcset . '" media="' . $media . '" '. ($sizes ?? '') . $end_tag;  
  
    // If a max-width is found in the source's media attribute, update the previous max-width.  
  if ( preg_match( '/\(max-width: (\d+(\.\d+)?)px\)/', $source->media, $matches ) ) {  
  $prev_max_width = floatval( $matches[1] );  
    }  
}

This will ensure that the tag gets the attribute imagesizes="(max-width: 1024px) 100vw, 1024px".

Finally we need to Update tests.

Estimate the effort ✅

[M]

[Mai-Saad]

Test on hold
https://wp-media.slack.com/archives/CUT7FLHF1/p1717052590108899?thread_ts=1716965466.492079&cid=CUT7FLHF1

[DahmaniAdame]

Dismiss. I got tests confused 😅 My bad 🙏
This approach is still valid.
What is missing from @Mai-Saad QA tests is that source element picked was not faithfully turned into its link rel=preload counter part.
Refer to - https://wp-media.slack.com/archives/CUT7FLHF1/p1717050042763859?thread_ts=1716965466.492079&cid=CUT7FLHF1 and the comment that followed it.
If the element doesn't have a src element, there is no need to create one. The remaining attributes should be mapped to their preload counter part.
Example:

<picture>
    <source
        srcset="https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-1024x683.avif 1024w,https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-300x200.avif 300w,https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-768x512.avif 768w,https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-1536x1024.avif 1536w,https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries.avif 1600w"
        sizes="(max-width: 1024px) 100vw, 1024px" type="image/avif">
    <source
        srcset="https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-1024x683.jpg.webp 1024w,https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-300x200.jpg.webp 300w,https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-768x512.jpg.webp 768w,https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-1536x1024.jpg.webp 1536w,https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries.jpg.webp 1600w"
        sizes="(max-width: 1024px) 100vw, 1024px" type="image/webp">
    <img
        src="https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-1024x683.jpg"
        height="683" width="1024"
        srcset="https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-1024x683.jpg 1024w, https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-300x200.jpg 300w, https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-768x512.jpg 768w, https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-1536x1024.jpg 1536w, https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries.jpg 1600w"
        sizes="(max-width: 1024px) 100vw, 1024px" class="wp-image-905 sp-no-webp" alt="" fetchpriority="high"
        decoding="async">
</picture>

Should translate to:

<link rel="preload" as="image" imagesrcset="https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-1024x683.avif 1024w,https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-300x200.avif 300w,https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-768x512.avif 768w,https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries-1536x1024.avif 1536w,https://domain.ext/wp-content/uploads/2024/05/coffee-sweets-and-waffles-with-berries.avif 1600w"  imagesizes="(max-width: 1024px) 100vw, 1024px" type="image/avif" fetchpriority="high" />

[piotrbak]

To sum up everything. The only change that's needed is to make sure that:
https://pastebin.com/uvmYgm9E

becomes:
https://pastebin.com/g07YvH0k

Acceptance criteria:

1. When there are more than one images in the srcset (within the source in the picture tag) we should not be adding href attribute, only the imagesrcset
2. No changes to the https://pastebin.com/PSzP7Abq becoming https://pastebin.com/jS2UHg83 (as it only has one image in srcset, so it needs to become href)
3. No changes to Preload the responsive image markup #6531
4. No changes to Preload the picture markup #6530

The last two points are here to remind that we're still working only on the preload with different image type defined.

CC @DahmaniAdame

[piotrbak]

@MathieuLamiot @Miraeld Should it go back to the grooming or the change can be implemented without it?

[Miraeld]

I think can be without it,

I will check it tomorrow with clear head.