wp-media / wp-rocket

Performance optimization plugin for WordPress
https://wp-rocket.me
GNU General Public License v2.0
688 stars 215 forks source link

Exclude the regular image when a nextgen is detected on OCI #6786

Closed DahmaniAdame closed 1 month ago

DahmaniAdame commented 1 month ago

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

Describe the bug After the following enhancement - https://github.com/wp-media/wp-rocket/issues/6638

Images are preloaded correctly but not excluded from lazyload.

This is caused by the exclusion targeting the img element on lazyload that won't find the next-gen image detected as LCP.

The exclusion system needs to include the source element as a verifiable exclusion targe.

To Reproduce Steps to reproduce the behavior:

  1. Add an image with a plug that will add a picture tag with nextgen
  2. Run beacon
  3. See image preloaded but not excluded from lazyload

Expected behavior When a next gen is detected as LCP, the right source image should be excluded from lazyload.

Screenshots N/A

Additional context Slack - https://wp-media.slack.com/archives/C072P5EU5DF/p1721065409522049

Acceptance Criteria (for WP Media team use only) Clear instructions for developers, to be added before the grooming

Tabrisrp commented 1 month ago

Looked into this, for picture elements:

So if the LCP images is a next-gen one, it should still match, as the check is not done specifically against the img element, but the whole picture element.

Are we in a case where the picture element is generated after WP Rocket is running?

joejoe04 commented 1 month ago

This seems to also happen for CSS BG images too.

Ticket

The LCP image is Preloaded: https://imageshack.com/a/img922/5278/m4hdfg.png

But it's also still lazy loade as CSS background image: https://imageshack.com/a/img922/8401/aMU6pC.png

DahmaniAdame commented 1 month ago

I couldn't replicate the issue anymore when testing using a couple of plugins injecting the picture tag. Mai has a working template. @joejoe04 for background, what you shared is the expected behavior. And exclusion will still use the CSS variable. The difference is that the variable will be active without requiring JS to assess when it needed to be replaced.

Mai-Saad commented 1 month ago

Reproducible on the test site with the following steps: 1- imagify is enabled and display picture image is on (WebP option is selected) 2- WPR 3.16.3 installed and activated (LL image is enabled) 3- Visit page with image optimized by imagify (have webP version) i.e https://new.rocketlabsqa.ovh/next-gen-image/ 4- Clear cache and revisit the page ( make sure that LCP image is preloaded) 5- Check LCP picture image markup

notice the image markup on the cached page: https://goonlinetools.com/snapshot/code/#3m2dm0dlpj9okkl6peink

on nowprocket markup is: https://goonlinetools.com/snapshot/code/#8krzx0nmrqps17479u5

Note: in NW tab, image is preloaded then loaded from LL Screenshot from 2024-08-02 07-51-20

DahmaniAdame commented 1 month ago

@Khadreal 👆

MathieuLamiot commented 1 month ago

@Khadreal Moving back to Grooming In Progress as the exact root cause is not properly identified. Let's describe precisely the root cause and if possible, share reproducible steps or even an environment where the issue is happening, with logs to showcase it. Thanks

Khadreal commented 1 month ago

From what I've seen, this issue happens when the flow of order break between Imagify and WPR. The flow should be imagify converts img to picture > cache process and lazy load applied. But in this case we have imagify process starting a bit late after lazy load process have started which means lcp and lazyload will process for img instead of picture(which imagify will converts to)

Khadreal commented 1 month ago

For this edge scenario, this is what is happening: The LCP is adding the picture's srcset value to the exclusion list, which is normal. However, since the Imagify rewrite is not happening before the lazy load method, the lazyloadImages method will add the lazy load attribute to the image. This happens because the srcset in the exclusion array list is not the same as the src value, hence the lazy load. At this point, before the Imagify process, we would have something like this: <img fetchpriority="high" decoding="async" width="1200" height="800" src="data:image/svg+xml,%3Csvg%20xmlns=&#039;http://www.w3.org/2000/svg&#039;%20viewBox=&#039;0%200%201200%20800&#039;%3E%3C/svg%3E" alt="" data-lazy-srcset="https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-1200x800.jpeg 1200w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-600x400.jpeg 600w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-768x512.jpeg 768w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-1536x1024.jpeg 1536w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-2048x1365.jpeg 2048w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-1980x1320.jpeg 1980w" data-lazy-sizes="(max-width: 1200px) 100vw, 1200px" data-lazy-src="https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-1200x800.jpeg"/> And after the rewrite we would have the below

<picture fetchpriority="high" decoding="async" class="wp-image-88009">
<source type="image/webp" data-lazy-srcset="https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-1200x800.jpeg.webp 1200w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-600x400.jpeg.webp 600w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-768x512.jpeg.webp 768w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-1536x1024.jpeg.webp 1536w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-2048x1365.jpeg.webp 2048w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-1980x1320.jpeg.webp 1980w" srcset="data:image/svg+xml,%3Csvg%20xmlns=&#039;http://www.w3.org/2000/svg&#039;%20viewBox=&#039;0%200%201200%20800&#039;%3E%3C/svg%3E" data-lazy-sizes="(max-width: 1200px) 100vw, 1200px"/>
<img fetchpriority="high" decoding="async" width="1200" height="800" src="data:image/svg+xml,%3Csvg%20xmlns=&#039;http://www.w3.org/2000/svg&#039;%20viewBox=&#039;0%200%201200%20800&#039;%3E%3C/svg%3E" alt="" data-lazy-srcset="https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-1200x800.jpeg 1200w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-600x400.jpeg 600w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-768x512.jpeg 768w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-1536x1024.jpeg 1536w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-2048x1365.jpeg 2048w, https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-1980x1320.jpeg 1980w" data-lazy-sizes="(max-width: 1200px) 100vw, 1200px" data-lazy-src="https://new.rocketlabsqa.ovh/wp-content/uploads/2024/08/premium_photo-1674406481284-43eba097a291-1200x800.jpeg"/>
</picture>
MathieuLamiot commented 1 month ago

As discussed during the daily: based on the last comment, the issue could actually be on Imagify's end rather than WP Rocket (if a rewrite is not happening from time to time). Hence, fixing it on WP Rocket side might not be the best approach. I'll put this back to Needs Grooming to investigate the root cause of the missing rewrite further. We can keep the PR aside, and use it maybe depending on the outcome of the investigation.

Tabrisrp commented 1 month ago

So I believe I found what was happening with the next-gen issue. we have 2 callbacks on template_redirect:

So we should expect imagify to be finished first, and WP Rocket after. BUT, both callbacks have an ob_start() call, and when multiple are called, they become nested until the buffering stops:

When output buffering ends (on shutdown action of WP), lower level content is passed to upper, so the order becomes WP Rocket optimizations first, then Imagify second, and HTML is output. While callback priority is as expected, the output buffering works in the opposite order.

To verify, I tested by changing the priority of Imagify callback to 10, reverting both orders, and it works as expected, the picture element is applied first in the HTML, and then lazyload exclusion works as needed.

The solution to the issue would be to change the priority on filter callback for Imagify. I’m not completely sure if that could cause issues elsewhere, at first glance I don’t think so, but it would probably need some testing

MathieuLamiot commented 1 month ago

Moved to Imagify