Skip to content
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

Open
wants to merge 14 commits into
base: trunk
Choose a base branch
from

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented May 21, 2024

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 the od_register_tag_visitors action to register callbacks that are invoked for each tag while iterating over the document. This callback is passed the OD_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 provided OD_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 provided OD_Preload_Link_Collection instance. If the callback returns true then this indicates that the tag is relevant for the visitor. Such visitor-related tags will automatically get the data-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:

<?php
add_action(
	'od_register_tag_visitors',
	function ( OD_Tag_Visitor_Registry $tag_visitor_registry ): void {
		$tag_visitor_registry->register(
			'img-dimensions',
			function ( OD_HTML_Tag_Walker $walker ): bool {
				if ( $walker->get_tag() !== 'IMG' ) {
					return false;
				}

				$src = trim( (string) $walker->get_attribute( 'src' ) );
				if ( ! str_starts_with( $src, 'http://' ) && ! str_starts_with( $src, 'https://' ) ) {
					return false;
				}

				$width  = (int) $walker->get_attribute( 'width' );
				$height = (int) $walker->get_attribute( 'height' );
				if ( 0 !== $width && 0 !== $height ) {
					return false;
				}

				// Example code to fetch dimensions (but without transient caching 🙈).
				$client = new \FasterImage\FasterImage();
				$images = $client->batch( array( $src ) );
				list( $width, $height ) = current( $images )['size'];

				$walker->set_attribute( 'width', (string) $width );
				$walker->set_attribute( 'height', (string) $height );

				return true;
			}
		);
	}
);

The image optimization logic is further refactored into two classes:

  • IP_Img_Tag_Visitor handles optimizing LCP img tags
  • IP_Background_Image_Styled_Tag_Visitor handles optimizing LCP elements with a background-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

@westonruter westonruter added [Type] Feature A new feature within an existing module [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels May 21, 2024
@westonruter westonruter added this to the image-prioritizer 0.1.0 milestone May 21, 2024
@westonruter westonruter force-pushed the add/image-prioritizer-plugin-for-reals branch from 9246cc7 to e4db9cf Compare May 21, 2024 23:37
Base automatically changed from add/image-prioritizer-plugin to trunk May 23, 2024 18:06
@westonruter westonruter force-pushed the add/image-prioritizer-plugin-for-reals branch 7 times, most recently from 28d8e08 to 130ba31 Compare May 29, 2024 00:16
Comment on lines +251 to 253
if ( array_key_exists( __FUNCTION__, $this->result_cache ) ) {
return $this->result_cache[ __FUNCTION__ ];
}
Copy link
Member Author

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

Copy link
Member Author

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.

@westonruter westonruter force-pushed the add/image-prioritizer-plugin-for-reals branch from 7fdad55 to 966275b Compare May 29, 2024 22:53
@westonruter westonruter marked this pull request as ready for review May 29, 2024 23:18
Copy link

github-actions bot commented May 29, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>
Co-authored-by: swissspidy <swissspidy@git.wordpress.org>
Co-authored-by: joemcgill <joemcgill@git.wordpress.org>
Co-authored-by: felixarntz <flixos90@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@westonruter

This comment was marked as resolved.

if ( count( $group ) === 0 ) {
return false;
}
if ( array_key_exists( __FUNCTION__, $this->result_cache ) ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter
Copy link
Member Author

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' );
Copy link
Member

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?

Copy link
Member Author

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
Copy link
Member

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?

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Feature A new feature within an existing module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Split out image-specific aspects of Optimization Detective to dependent plugin
3 participants