Add Image Prioritizer plugin #1235

westonruter · 2024-05-21T22:10:40Z

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

.wp-env.json

westonruter · 2024-05-29T22:19:27Z

plugins/optimization-detective/class-od-url-metrics-group-collection.php

+		if ( array_key_exists( __FUNCTION__, $this->result_cache ) ) {
+			return $this->result_cache[ __FUNCTION__ ];
 		}


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

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.

I've reported this as a possible bug to PHPStan: phpstan/phpstan#11138

Hmm I thought I had a similar issue recently but now I can't find the code anymore 😅 Curious to hear the response on that issue 👍

…asses

github-actions · 2024-05-29T23:18:16Z

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 <[email protected]>
Co-authored-by: adamsilverstein <[email protected]>
Co-authored-by: swissspidy <[email protected]>
Co-authored-by: joemcgill <[email protected]>
Co-authored-by: felixarntz <[email protected]>

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

westonruter · 2024-05-29T23:29:23Z

plugins/optimization-detective/class-od-url-metrics-group-collection.php

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


See #1235 (comment)

westonruter · 2024-05-29T23:31:11Z

An area for future optimization would be to consider whether we can avoid invoking all tag visitor callbacks for every tag.

adamsilverstein · 2024-05-30T17:31:25Z

plugins/image-prioritizer/class-ip-img-tag-visitor.php

+			// 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' );


could we set fetchpriority to low here instead of removing?

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.

adamsilverstein · 2024-05-30T20:25:16Z

plugins/image-prioritizer/readme.txt

+Requires at least: 6.4
+Tested up to:      6.5
+Requires PHP:      7.2
+Stable tag:        0.1.0


I'd go with 1.0.0 - what have we done n the past for new plugins?

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.

… add/image-prioritizer-plugin-for-reals * 'trunk' of https://github.com/WordPress/performance: (33 commits) Fix typo Fix lint command Add micromatch Add lint-staged.config.js Remove lint-staged config from package.json Remove composer autoloading from tests Remove `class_exists` check Ignore tests from plugin builds Remove sniffs which are no longer applicable to be ignored in test files Update file name as per WPCS Update dominant-color-images tests helper class and namespace Update webp-uploads tests helper class and namespace Update lint scripts Fix image paths Fix PHPUnit autloading error Fix WP Filesystsem Mock file path Update tests path Move helper files to tests/data Move performance-lab tests FS mock helper to plugins/performance-lab/tests/data Move webp-uploads tests constraints to plugins/webp-uploads/tests/data ...

… add/image-prioritizer-plugin-for-reals * 'trunk' of https://github.com/WordPress/performance: Include -- before file names for good measure Fix lint-staged to only check staged plugin files

… add/image-prioritizer-plugin-for-reals * 'trunk' of https://github.com/WordPress/performance: Tests: Remove Test Case - Remove Test case that checks that the fallback script is added when a post with updated images is rendered. Refactor: Remove function and associated action. Refactor: Removed File fallback.js Apply suggestions from code review Improve translation string Improve translators comment Reuse Performance Lab string Add install notice after each PerfLab feature plugin

swissspidy · 2024-06-03T19:28:16Z

tests/bootstrap.php

+/**
+ * Load plugin bootstrap and any dependencies.
+ *
+ * @param string $plugin_name Plugin slug to load.
+ */
+$load_plugin = static function ( string $plugin_name ) use ( &$load_plugin ): void {


plugins/optimization-detective/tests/test-class-od-tag-visitor-registry.php

plugins/optimization-detective/readme.txt

plugins/image-prioritizer/tests/test-helper.php

swissspidy · 2024-06-03T19:35:40Z

plugins/image-prioritizer/readme.txt

+
+This plugin requires the [Optimization Detective](https://wordpress.org/plugins/optimization-detective/) plugin as a dependency.
+
+TODO: Flesh out description.


Let's not forget this before release

Indeed! I've fleshed it out in the sub-PR: #1261

swissspidy · 2024-06-03T19:39:03Z

plugins/image-prioritizer/helper.php

+ *
+ * @since n.e.x.t
+ */
+function ip_render_generator_meta_tag(): void {


We should consider using image_prioritzer_ as the prefix.

It's very likely that many other plugins (public or private) use ip_ already.

The plugin review guidelines also recommend using longer prefixes (or namespaces) nowadays because conflicts are less likely.

Oh, namespaces? 😋

Used longer prefix in d438024.

Using namespaces would be very nice, but probably deserves more discussion first.

plugins/image-prioritizer/helper.php

swissspidy · 2024-06-03T19:43:49Z

plugins/image-prioritizer/load.php

+
+(
+	/**
+	 * Register this copy of the plugin among other potential copies embedded in plugins or themes.


I thought this whole logic was only relevant for allowing Optimization Detective to be embedded. Are we assuming plugins will embed this plugin as well?

Yes, if a plugin wants to embed image optimization logic then they'd have to embed both Optimization Detective and Image Prioritizer.

swissspidy · 2024-06-03T19:57:37Z

plugins/optimization-detective/class-od-url-metrics-group-collection.php

+		if ( array_key_exists( __FUNCTION__, $this->result_cache ) ) {
+			return $this->result_cache[ __FUNCTION__ ];
 		}


Hmm I thought I had a similar issue recently but now I can't find the code anymore 😅 Curious to hear the response on that issue 👍

swissspidy

Nice work! 🚀

Just a few minor thoughts

Co-authored-by: Pascal Birchler <[email protected]>

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 added this to the image-prioritizer 0.1.0 milestone May 21, 2024

westonruter mentioned this pull request May 21, 2024

Refactor optimization logic in Optimization Detective #1172

Merged

westonruter force-pushed the add/image-prioritizer-plugin-for-reals branch from 9246cc7 to e4db9cf Compare May 21, 2024 23:37

westonruter commented May 21, 2024

View reviewed changes

.wp-env.json Show resolved Hide resolved

Base automatically changed from add/image-prioritizer-plugin to trunk May 23, 2024 18:06

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

westonruter added 9 commits May 29, 2024 14:48

Add skeleton for Image Prioritizer plugin

669d43f

Update PHPUnit bootstrap to support plugin dependencies

718608b

Initially move all image optimization logic to Image Prioritizer

56e2041

Refactor Image Prioritizer visitor into class

b7096a3

Move ip_is_data_url function to method

8d58e52

Apply strict PHPStan rules

6c7f3e0

Replace od_html_tag_walker_visitors filter with registry

526dcec

Introduce get_groups_by_lcp_element helper method on groups collection

da2a660

Add get_common_lcp_element method

546e892

westonruter commented May 29, 2024

View reviewed changes

westonruter added 2 commits May 29, 2024 15:53

Split up IP_Image_Tag_Visitor class into abstract and subclasses

cb5206e

Add caching for method return values in Group and Group_Collection cl…

966275b

…asses

westonruter force-pushed the add/image-prioritizer-plugin-for-reals branch from 7fdad55 to 966275b Compare May 29, 2024 22:53

Add TODO to flesh out readme

1c73ac9

westonruter marked this pull request as ready for review May 29, 2024 23:18

westonruter requested a review from felixarntz as a code owner May 29, 2024 23:18

westonruter requested review from swissspidy and adamsilverstein May 29, 2024 23:18

westonruter requested a review from joemcgill May 29, 2024 23:18

This comment was marked as resolved.

Sign in to view

westonruter commented May 29, 2024

View reviewed changes

Merge branch 'trunk' into add/image-prioritizer-plugin-for-reals

9c3eb84

adamsilverstein reviewed May 30, 2024

View reviewed changes

Elaborate on why fetchpriority=high is removed and add test case

489d1f6

westonruter mentioned this pull request May 30, 2024

Optimize lazy-loading of IMG elements in Image Prioritizer #1261

Merged

adamsilverstein reviewed May 30, 2024

View reviewed changes

westonruter force-pushed the add/image-prioritizer-plugin-for-reals branch from 1df7f47 to 7c0b003 Compare May 31, 2024 21:05

westonruter added 5 commits May 31, 2024 16:23

Remove 'developer preview' designation from Optimization Detective

5105d86

Bump OD versions and begin updating readme

1809bb4

Add Upgrade Notice for Performance Lab

78613a1

westonruter requested a review from adamsilverstein June 3, 2024 19:04

swissspidy reviewed Jun 3, 2024

View reviewed changes

swissspidy approved these changes Jun 3, 2024

View reviewed changes

westonruter and others added 5 commits June 3, 2024 12:59

Fix grammar typo

450f8bd

Co-authored-by: Pascal Birchler <[email protected]>

Fix grammar in phpdoc

ca4f8e6

Co-authored-by: Pascal Birchler <[email protected]>

Fix reference to the actual plugin

302f88b

Co-authored-by: Pascal Birchler <[email protected]>

Use verbose prefix instead of IP

d438024

Co-authored-by: Pascal Birchler <[email protected]>

Remove extraneous assertion

48b930b

Co-authored-by: Pascal Birchler <[email protected]>

westonruter merged commit b547f79 into trunk Jun 3, 2024
15 checks passed

westonruter deleted the add/image-prioritizer-plugin-for-reals branch June 3, 2024 20:25

westonruter added the [Plugin] Image Prioritizer Issues for the Image Prioritizer plugin (dependent on Optimization Detective) label Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Image Prioritizer plugin #1235

Add Image Prioritizer plugin #1235

westonruter commented May 21, 2024 •

edited

Loading

westonruter May 29, 2024

westonruter May 29, 2024

westonruter Jun 3, 2024 •

edited

Loading

swissspidy Jun 3, 2024

github-actions bot commented May 29, 2024 •

edited

Loading

This comment was marked as resolved.

westonruter May 29, 2024

westonruter commented May 29, 2024

adamsilverstein May 30, 2024

westonruter May 30, 2024

adamsilverstein May 30, 2024

westonruter May 30, 2024

swissspidy Jun 3, 2024

swissspidy Jun 3, 2024

westonruter Jun 3, 2024

swissspidy Jun 3, 2024

westonruter Jun 3, 2024

westonruter Jun 3, 2024

swissspidy Jun 3, 2024

westonruter Jun 3, 2024

swissspidy Jun 3, 2024

swissspidy left a comment


		This plugin requires the [Optimization Detective](https://wordpress.org/plugins/optimization-detective/) plugin as a dependency.

		TODO: Flesh out description.

Add Image Prioritizer plugin #1235

Add Image Prioritizer plugin #1235

Conversation

westonruter commented May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 29, 2024 • edited Loading

This comment was marked as resolved.

Choose a reason for hiding this comment

westonruter commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swissspidy left a comment

Choose a reason for hiding this comment

westonruter commented May 21, 2024 •

edited

Loading

westonruter Jun 3, 2024 •

edited

Loading

github-actions bot commented May 29, 2024 •

edited

Loading