FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Implement new experimental `fetchpriority` module by pbearne · Pull Request #528 · WordPress/performance · GitHub

Implement new experimental fetchpriority module - #528

Merged
felixarntz merged 36 commits into
trunkfrom
experiment/fetchpriority
Nov 23, 2022
Merged

Implement new experimental fetchpriority module#528
felixarntz merged 36 commits into
trunkfrom
experiment/fetchpriority

Conversation

pbearne commented Sep 13, 2022
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Fixes #477

Relevant technical choices

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

pbearne added this to the no milestone milestone Sep 13, 2022
Comment thread modules/images/fetchpriority/load.php Outdated
Comment thread modules/images/fetchpriority/load.php Outdated
Comment thread modules/images/fetchpriority/load.php Outdated
Comment thread modules/images/fetchpriority/load.php Outdated
Comment thread modules/images/fetchpriority/load.php Outdated
pbearne and others added 3 commits September 13, 2022 11:58
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>

felixarntz left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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:

  1. We need to add support for the featured image output, as it is covered by the conditional lazy-loading logic as well.
  2. We should add at least one more test to cover the actual wp_filter_content_tags() integration of the code here.

Comment thread modules/images/fetchpriority/load.php Outdated
Comment thread modules/images/fetchpriority/load.php Outdated
Comment thread modules/images/fetchpriority/load.php Outdated
Comment thread modules/images/fetchpriority/load.php Outdated
Comment thread modules/images/fetchpriority/load.php Outdated
Comment thread modules/images/fetchpriority/load.php Outdated
Comment thread modules/images/fetchpriority/load.php Outdated
Comment thread modules/images/fetchpriority/load.php Outdated
felixarntz changed the title Experiment/fetchpriority Implement new experimental fetchpriority module Sep 13, 2022
pbearne and others added 4 commits September 13, 2022 15:29
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>

Copy link
Copy Markdown
Member

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

adamsilverstein left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Copy link
Copy Markdown
Member

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.

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 $context is false, so arrived at this simple solution which fixes the issue in my testing: WordPress/wordpress-develop#3538

I will open up a Trac ticket to discuss further.

Copy link
Copy Markdown
Member

@pbearne this is ready for review.

Copy link
Copy Markdown
Member

@felixarntz this is ready for review

felixarntz left a comment
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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.

Comment thread modules/images/fetchpriority/load.php Outdated

Copy link
Copy Markdown
Member

@felixarntz Addressed feedback and ready for another review

Copy link
Copy Markdown
Member

we should make sure to test prior to merge that this works as expected in a block theme.

I will double check this.

felixarntz added this to the 1.8.0 milestone Nov 16, 2022
felixarntz removed the no milestone PRs that do not have a defined milestone for release label Nov 16, 2022

Copy link
Copy Markdown
Member

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.

felixarntz left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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".
  • 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.

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.

Comment thread modules/images/fetchpriority/load.php Outdated

Copy link
Copy Markdown
Member

FYI the PR whatwg/html#8470 is where adding fetchpriority to the HTML spec is being worked on. This will be critical before we can eventually merge it into WordPress core. But for the plugin shipping this already should be fine.

mukeshpanchal27 left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

felixarntz merged commit 3cc0edc into trunk Nov 23, 2022
felixarntz deleted the experiment/fetchpriority branch November 23, 2022 17:53
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Feature A new feature within an existing module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fetchpriority - add a fetchpriority=high attribute to the non-lazy image(s)

6 participants


Back | FazBrowse Home | New Git URL