-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix handling NOSCRIPT and IMG tags with JS-based lazy-loading #1745
base: trunk
Are you sure you want to change the base?
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
return false; | ||
} | ||
|
||
// Abort if the srcset is a data: URL since there is nothing to optimize. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, there could be something to optimize. We can't preload the URL, but we could still add fetchpriority=high
to the IMG
.
@@ -53,13 +97,21 @@ public function __invoke( OD_Tag_Visitor_Context $context ): bool { | |||
* @return bool Whether the tag should be tracked in URL Metrics. | |||
*/ | |||
private function process_img( OD_HTML_Tag_Processor $processor, OD_Tag_Visitor_Context $context ): bool { | |||
$src = $this->get_valid_src( $processor ); | |||
if ( null === $src ) { | |||
if ( ! $this->is_img_with_valid_src_and_srcset( $processor ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted below, this may be too conservative. We could still process this IMG
. It's just we would skip any parts related to preloading the src
or srcset
.
I'm bumping this to the next release. |
When analyzing sites using Image Prioritizer in HTTP Archive, I came across some cases that hadn't been accounted for.
Tags inside
NOSCRIPT
First of all, any tag which appears in a
NOSCRIPT
should always be excluded from visiting by any tag visitor. For example:This was always showing up as:
Image Prioritizer adds the
data-od-unknown-tag
attribute to any visited tagIMG
which is not located in URL Metrics. However, such anIMG
can never be added to URL Metrics since it is not located in the DOM.This PR skips over visiting any tag which is inside of a
NOSCRIPT
element.Such
NOSCRIPT
tags are often found with JS-based lazy-loading implementations.JS-based Lazy-Loading
On quite a few sites I found JS-based lazy-loading causing problems with Image Prioritizer. In particular, the Avada theme's Fusion Images lazy-loading functionality replaces
srcset
withdata-srcset
but it doesn't change thesrc
, for example:This was resulting in an erroneous preload link being added, in particular the
imagesrcset
is pointing to adata:
URL:So this PR adds checks to make sure that if an
IMG
has such asrcset
it is also excluded from processing.In a future PR we could consider undoing such JS-based lazy-loading, rewriting
data-src
tosrc
,data-srcset
back tosrcset
and so on, and then to process theIMG
as normal. This would have a dramatic improvement to LCP especially when the such JS-based lazy-loading is being done for images in the initial viewport, especially for anIMG
which is the LCP element. See #1746.