Implement new experimental fetchpriority module
#528
Conversation
Co-authored-by: Adam Silverstein <adamjs@google.com>
Co-authored-by: Adam Silverstein <adamjs@google.com>
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
There was a problem hiding this comment.
@pbearne This is a solid start. I left a few mostly small comments below, but there are two bigger things among them which are critical but missing here:
- We need to add support for the featured image output, as it is covered by the conditional lazy-loading logic as well.
- We should add at least one more test to cover the actual
wp_filter_content_tags()integration of the code here.
felixarntz
changed the title
fetchpriority module
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
|
I verified this works as expected in a classic theme like twentytwelve. when testing with twentytwentythree however, I noticed that the first image is lazy loaded, so something isn't quite working with the logic in core for block themes. I'm going to dig into this a bit further to see why. cc: @felixarntz |
As we had discussed earlier, this is caused by the block theme "double" pass at the content (which can help performance). To fix this we need to avoid "counting" the first pass by block themes. I noticed in this case the I will open up a Trac ticket to discuss further. |
|
@pbearne this is ready for review. |
|
@felixarntz this is ready for review |
There was a problem hiding this comment.
@pbearne @adamsilverstein Overall this looks solid, though there are a few things we need to fix as they can introduce bugs.
In light of the currently ongoing work to fix image lazy-loading in block themes (see https://core.trac.wordpress.org/ticket/56930), we should make sure to test prior to merge that this works as expected in a block theme.
Looking at the code here, things look like this may not be affected by the double execution bug from core (due to the more explicit context check present here), but we need to verify that. Adding fetchpriority="high" to too many images may be even worse than lazy-loading too many images.
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
|
@felixarntz Addressed feedback and ready for another review |
I will double check this. |
felixarntz
removed
the
no milestone
|
Milestoning this for the 1.8.0 release coming in December, as it's shaping up for merge by then. I'll give it another review later this week. |
There was a problem hiding this comment.
@pbearne @adamsilverstein The implementation here LGTM, with the following caveats / known issues:
- For block themes, it currently doesn't work on featured images since the featured image block is broken in regards to
loading="lazy".- This was recently fixed in Gutenberg (see Ensure post-featured-image block is in_the_loop() for BC with core and plugins, and to fix lazy-loading gutenberg#45534).
- For block themes, it already works for regular content images, however the first content image will still have
loading="lazy"on it due to the double content parsing bug in core.- A WordPress core fix is being worked on (see Ensure that first content image is not lazy-loaded in block themes either wordpress-develop#3560).
Both of these problems are not critical, since at least this PR doesn't make it worse than it is; and both of these problems need to be fixed separately, so this is good to merge IMO.
Will update the module description though, as it's a bit too technical and doesn't describe the performance impact.
|
FYI the PR whatwg/html#8470 is where adding |
Summary
Fixes #477
Relevant technical choices
Checklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.