-
Notifications
You must be signed in to change notification settings - Fork 84
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
Add Image Prioritizer plugin #1235
base: trunk
Are you sure you want to change the base?
Conversation
9246cc7
to
e4db9cf
Compare
28d8e08
to
130ba31
Compare
if ( array_key_exists( __FUNCTION__, $this->result_cache ) ) { | ||
return $this->result_cache[ __FUNCTION__ ]; | ||
} |
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.
I'm not entirely happy with this. There's too much boilerplate. I wanted to instead have a get_cached_result()
helper method that relied on generics for PHPStan strict type checks. However, I wasn't able to get it to work. See https://phpstan.org/r/b203ec8c-2d70-469c-b24f-9e7143cff72d
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.
Granted, it would also have to be updated to support caching by the method argument as in the get_group_for_viewport_width
example above.
7fdad55
to
966275b
Compare
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. |
This comment was marked as resolved.
This comment was marked as resolved.
if ( count( $group ) === 0 ) { | ||
return false; | ||
} | ||
if ( array_key_exists( __FUNCTION__, $this->result_cache ) ) { |
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.
See #1235 (comment)
An area for future optimization would be to consider whether we can avoid invoking all tag visitor callbacks for every tag. |
// Note: The $all_breakpoints_have_url_metrics condition here allows for server-side heuristics to | ||
// continue to apply while waiting for all breakpoints to have metrics collected for them. | ||
$walker->set_attribute( 'data-od-removed-fetchpriority', $walker->get_attribute( 'fetchpriority' ) ); | ||
$walker->remove_attribute( 'fetchpriority' ); |
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.
could we set fetchpriority to low here instead of removing?
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.
I see the comment here explaining the removal is not fleshed out enough. I've elaborated in 489d1f6. It may be that the element here is actually the LCP element for some of the viewport groups. In this case, fetchpriority=high
is removed from the img
but it is added to the preload link below which also includes the media query to ensure it is only prioritized for the right viewports.
Adding fetchpriority=low
is something I want to explore in the context of images that are above the fold but obscured, such as in a mega menu or as subsequent slides in a carousel.
Requires at least: 6.4 | ||
Tested up to: 6.5 | ||
Requires PHP: 7.2 | ||
Stable tag: 0.1.0 |
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.
I'd go with 1.0.0 - what have we done n the past for new plugins?
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.
Optimization Detective started with 0.1.0, as did Embed Optimizer. I'd keep it below 1.x since it's still experimental. Once something graduates from being experimental, then I think it makes sense to go 1.x.
This splits out the image-specific optimizations from Optimization Detective into a separate dependent plugin: Image Prioritizer.
Please review the commits one-by-one as they have been tailored to be as atomic as possible. Also, suppressing whitespace changes will help with reviewing methods for which caching was added.
The image optimization logic previously located in the
od_optimize_template_output_buffer()
function has been heavily refactored in the following ways:A new
OD_Tag_Visitor_Registry
class has been introduced which allows plugins at theod_register_tag_visitors
action to register callbacks that are invoked for each tag while iterating over the document. This callback is passed theOD_HTML_Tag_Walker
instance, allowing the callback to inspect the current tag and attributes, as well as to obtain the XPath. With this information in hand, it is able to look up the element in the providedOD_URL_Metrics_Group_Collection
instance to see if it is an LCP element (or in future, whether it was in the viewport, etc). It can also add preload links via the providedOD_Preload_Link_Collection
instance. If the callback returnstrue
then this indicates that the tag is relevant for the visitor. Such visitor-related tags will automatically get thedata-od-xpath
attributes added to them when the collection of URL metric groups is not complete; this ensures that the element is captured in the URL metrics.Here's a basic example of how a visitor could be implemented with a closure:
The image optimization logic is further refactored into two classes:
IP_Img_Tag_Visitor
handles optimizing LCPimg
tagsIP_Background_Image_Styled_Tag_Visitor
handles optimizing LCP elements with abackground-image
style.Both of these classes inherit from an
IP_Tag_Visitor
abstract class that sets up the common interface. Note that these classes implement an__invoke()
method, allowing them to be passed to the tag visitor registry to be called the same way as a closure.Fixes #1088