Leverage URL metrics to reserve space for embeds to reduce CLS #1373

westonruter · 2024-07-17T21:39:12Z

The changes in this pull request grew larger than the strict scope of reducing CLS for embeds since the work to implement this uncovered shortcomings with the current Optimization Detective API. This PR includes the following key pieces:

Introduce client-side extensions in the form of script modules which are loaded when the detection logic runs. The client-side script modules are able to register functionality to run as soon as the module is loaded by implementing an exported initialize function. They also are able to register functionality to run just before the URL metric data is being submitted by implementing an exported finalize function which amends the URL metric data as needed. This builds on Allow URL metric schema to be extended #1492 which enabled the URL metric schema to be extended. This PR includes Embed Optimizer extending the URL metric element schema to include a resizedBoundingClientRect key which is populated with the script module via a ResizeObserver which starts observing in the module's initialize function. New script modules are added simply by appending to the array passed to the od_extension_module_urls filter.
The gathered URL metric data is now sent when the page is hidden as opposed to when the page is "done" loading. This is essential in order to capture the resized heights of embeds once the user scrolls them into view and they are loaded. This will also open up new opportunities to track INP over the life of the page to include in the submitted URL metric data.
New APIs are added to Optimization Detective which Embed Optimizer depends on (e.g. OD_URL_Metric_Group_Collection::get_all_denormalized_elements()). In order to avoid the need to do method_exists() checks and the like, a new od_init action is introduced which passes the Optimization Detective version as its only argument. Embed Optimizer and Image Prioritizer then check this passed version to see if it meets the requirements. If not, then an admin notice is displayed. If yes, then it goes ahead and loads the Optimization Detective extension code.
Related to the above, I found that certain plugins initialize their module code after the after_setup_theme action. In proposing embedding Optimization Detective in Elementor (Leverage URL metrics to reserve space for embeds to reduce CLS #1373), I found that modules load at init with priority 0. This makes the current bootstrap code unusable when it is loaded in such a module, necessitating an ugly workaround. Since Optimization Detective doesn't need to bootstrap before init anyway, the solution seems to simply be to move the bootstrapping logic to init as well. This is implemented in this PR by moving to init at priority 9 since the plugin has code that runs at init with the default priority of 10.

Accounting for responsive embeds

Initially I was thinking the optimization logic should wait to set the min-height until there are URL metrics gathered for the smallest breakpoint group. For example, let's say the page is responsive and on desktop an embed is much taller than it is on mobile. If the desktop metrics are collected before mobile metrics are collected, then the min-height could be being set for desktop could be too high for mobile, resulting in a gap below the embed on mobile. For example, consider this tweet on mobile vs desktop:

Mobile (320w)	Desktop

The tweet on mobile here is 574px high whereas on desktop it is 825px, so if desktop metrics were only collected then the result would be a worst case gap of 251px at the bottom before the next content:

A simple way to solve this would not to set the min-height on the figure.wp-block-embed element at all, but rather to create a new style rule with responsive styles that sets multiple min-height styles for the element according to each breakpoint. This is implemented in this PR. For example, the following CSS is added to the head for a Twitter embed:

<style>
@media (max-width: 480px) { #embed-optimizer-a7659db28ecaa36ddee6ae66857dabd8 { min-height: 500px; } }
@media (min-width: 481px) and (max-width: 600px) { #embed-optimizer-a7659db28ecaa36ddee6ae66857dabd8 { min-height: 500px; } }
@media (min-width: 601px) and (max-width: 782px) { #embed-optimizer-a7659db28ecaa36ddee6ae66857dabd8 { min-height: 500px; } }
@media (min-width: 783px) { #embed-optimizer-a7659db28ecaa36ddee6ae66857dabd8 { min-height: 500px; } }
</style>

Demo Videos

Before

bad-cls-without-embed-optimizer.webm

After

Once URL metrics have been collected from visitors by Optimization Detective:

good-cls-with-embed-optimizer.webm

westonruter · 2024-07-26T22:07:38Z

plugins/embed-optimizer/class-embed-optimizer-tag-visitor.php

 			return false;
 		}

-		$max_intersection_ratio = $context->url_metrics_group_collection->get_element_max_intersection_ratio( $processor->get_xpath() );
+		$embed_wrapper_xpath = $processor->get_xpath() . '/*[1][self::DIV]';


It's not ideal to be constructing this XPath manually.

I've opened #1407 to explore this, but it's not necessary for this PR to move forward.

westonruter · 2024-07-26T22:38:25Z

I noticed Query Monitor flagging a PHP deprecation error when loading a page with Optimization Detective and Embed Optimizer active. A TikTok embed is present on the page. I'm getting:

PHP Deprecated: Implicit conversion from float 739.9976196289062 to int loses precision in /var/www/html/wp-content/plugins/optimization-detective/class-od-url-metrics-group-collection.php on line 486

Call stack:

Update: Addressed in #1411.

westonruter · 2024-07-26T22:40:30Z

I also saw this for some reason:

PHP Notice: OD_HTML_Tag_Processor::get_updated_html(): Unable to append markup to optimization_detective_end_of_body since the bookmark no longer exists. in /var/www/html/wp-includes/functions.php on line 6085

Update: See fix below in #1373 (comment)

plugins/optimization-detective/class-od-url-metric.php

plugins/embed-optimizer/class-embed-optimizer-tag-visitor.php

westonruter · 2024-08-02T00:07:37Z

plugins/embed-optimizer/class-embed-optimizer-tag-visitor.php

+			$style = $processor->get_attribute( 'style' );
+			if ( is_string( $style ) ) {
+				$style = rtrim( trim( $style ), ';' ) . '; ';
+			} else {
+				$style = '';
+			}
+			$style .= sprintf( 'min-height: %dpx;', $minimum_height );
+			$processor->set_attribute( 'style', $style );


This should be a helper method on OD_HTML_Tag_Processor.

See prior art in the AMP plugin: https://github.com/ampproject/amp-toolbox-php/blob/78531851c59fa5f306315372719f0064b5542cf4/src/Dom/Element.php#L49-L69

westonruter · 2024-08-14T00:47:33Z

Scratched code to explore using onCLS to detect when an embed has loaded (and causes a layout shift. Problem: In the case of the Twitter embed, the source node being reported is REMOVED from the DOM, so we cannot see the ancestor which has a data-od-xpath attribute.

Code scratch

	const { onLCP, onCLS } = await import( webVitalsLibrarySrc );

	onCLS(
		( metric ) => {
			for ( const entry of metric.entries ) {
				if (
					entry.entryType === 'layout-shift' &&
					! entry.hadRecentInput
				) {
					console.info( entry );
					for ( const source of entry.sources ) {
						console.info( source.node );
					}
				}
			}
		},
		{
			// This is necessary in order to collect all layout shifts, even those which don't cause a bad CLS score.
			reportAllChanges: true,
		}
	);

westonruter · 2024-10-09T00:08:40Z

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

+	 * (the default sample size). Therefore, given the number (n) of visited elements on the page this will only
+	 * end up running n*4*3 times.
+	 *
+	 * @todo Should there be an OD_Element class which has a $url_metric property which then in turn has a $group property. Then this would only need to return array<string, OD_Element[]>.


I want to address this in a follow-up PR since it will require more refactoring and I don't want to make this PR even larger.

Sounds good. For the future, ideally the different plugin modifications would happen in separate PRs too. It's more convenient all in one PR, but would be better for review, plus you could still use sub-PRs like you've done before, to e.g. build something in Embed Optimizer on top of another PR change to the Optimization Detective infrastructure.

Yes, I agree. Sorry for the large PR. It was a bit of a journey.

westonruter · 2024-10-09T00:09:53Z

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

+					$element_intersection_ratios[] = $element['intersectionRatio'];
+				}
+				$elements_max_intersection_ratios[ $xpath ] = count( $element_intersection_ratios ) > 1
+					? (float) max( ...$element_intersection_ratios )


Interestingly, if there is only one element in the array than this is a fatal error: https://3v4l.org/nQNZh

Too bad it doesn't just return the initial item.

westonruter · 2024-10-09T00:44:08Z

plugins/embed-optimizer/class-embed-optimizer-tag-visitor.php

+		$denormalized_elements = $context->url_metric_group_collection->get_all_denormalized_elements()[ $embed_wrapper_xpath ] ?? array();
+		foreach ( $denormalized_elements as list( $group, $url_metric, $element ) ) {


See note below on the get_all_denormalized_elements() method that I'd like to instead have $element be an OD_Element class instance which has a $url_metric property to access the OD_URL_Metric instance if is a part of. In the same way, the OD_URL_Metric class can have a $group property to indicate the OD_URL_Metric_Group it is a part of. This would eliminate the need to pass back an array of tuples and instead. So instead, to get the $group this code could do $element->url_metric->group.

This I want to do in a follow-up PR.

github-actions · 2024-10-09T01:04:27Z

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: 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 · 2024-10-09T01:05:29Z

@felixarntz Finally! Ready for review.

felixarntz

@westonruter At first glance this looks good, for now mostly a few questions for clarification.

plugins/embed-optimizer/class-embed-optimizer-tag-visitor.php

plugins/embed-optimizer/detect.js

Co-authored-by: felixarntz <flixos90@git.wordpress.org>

felixarntz · 2024-10-09T21:05:58Z

plugins/image-prioritizer/load.php

-			// Wait until after the plugins have loaded and the theme has loaded. The after_setup_theme action is used
-			// because it is the first action that fires once the theme is loaded.
-			add_action( 'after_setup_theme', $bootstrap, PHP_INT_MIN );
+			/*
+			 * Wait until after the plugins have loaded and the theme has loaded. The after_setup_theme action could be
+			 * used since it is the first action that fires once the theme is loaded. However, plugins may embed this
+			 * logic inside a module which initializes even later at the init action. The earliest action that this
+			 * plugin has hooks for is the init action at the default priority of 10 (which includes the rest_api_init
+			 * action), so this is why it gets initialized at priority 9.
+			 */
+			add_action( 'init', $bootstrap, 9 );


I'm not sure I understand this. Why is the timing of this (in Image Prioritizer and Embed Optimizer) relevant for other plugins? Are you thinking of extensions to those plugins (rather than extensions to Optimization Detective)?

Another thought: Why can't this use the od_init action?

Bootstrapping at init is all about the embeddability of these plugins inside of other plugins, which is why this whole bootstrap logic was first devised (#1081, #1159, #1185).

For example, in elementor/elementor#28844 I've created a new module for Elementor that embeds the Optimization Detective plugin and its extensions. Elementor initializes its modules at init priority 0. Since none of the functionality in Optimization Detective needs to be initialized before init, there's no need for the current use of after_setup_theme to initialize. So this postpones initialization to latest possible point which opens the door to be embedded in more places.

This still feels like a rather shaky foundation to me. For example, why does it use priority 9?

I think I'm still not following what this is meant to do: I think I understand now why certain code needs to run before this callback, but why does certain other code need to run after this callback? For instance, why couldn't we use init on 999999, or even wp_loaded or a later hook?

Priority 9 is one less than the default priority of 10, as noted in the comment. There are callbacks added at this default priority in the plugin. So priority 9 is the last.

If we used init with an even later priority, then we'd need to adjust the other init actions' priorities as well in this plugin, for example:

performance/plugins/optimization-detective/storage/class-od-url-metrics-post-type.php

Line 48 in 94fc5e7

add_action( 'init', array( __CLASS__, 'register_post_type' ) );

performance/plugins/embed-optimizer/hooks.php

Line 27 in 94fc5e7

add_action( 'init', 'embed_optimizer_add_hooks' );

If we went with wp_loaded, then we couldn't use the init action in the plugins at all, which as I understand is too late for when post types should be registered.

So basically this needs to be called as late as possible, but before any of Optimization Detective's own hooks would fire?

Yes, exactly. As late as possible, and hopefully after any other possible embedding plugins' modules initialize (e.g. Elementor initializes at init priority 0).

Makes sense. Since we have full control over the code in Optimization Detective, we may want to consider moving the plugin's init callbacks to a very late priority, so that we can also push this back even further, while still being on init. That way we can cater for other plugins even better, since I wouldn't find it unreasonable for other plugins to load modules on init with the default priority either.

Given the callbacks from Optimization Detective do not have to run after everything else, maybe we just use a very high number (rather than PHP_INT_MAX which I would only use if it technically has to be the last thing).

We can open a follow up PR for this though, if you agree?

Sounds good!

felixarntz · 2024-10-09T21:09:16Z

plugins/embed-optimizer/load.php

+			 * plugin has hooks for is the init action at the default priority of 10 (which includes the rest_api_init
+			 * action), so this is why it gets initialized at priority 9.
+			 */
+			add_action( 'init', $bootstrap, 9 );


See other comment.

See #1373 (comment)

felixarntz · 2024-10-09T21:11:35Z

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

+	 * (the default sample size). Therefore, given the number (n) of visited elements on the page this will only
+	 * end up running n*4*3 times.
+	 *
+	 * @todo Should there be an OD_Element class which has a $url_metric property which then in turn has a $group property. Then this would only need to return array<string, OD_Element[]>.


Sounds good. For the future, ideally the different plugin modifications would happen in separate PRs too. It's more convenient all in one PR, but would be better for review, plus you could still use sub-PRs like you've done before, to e.g. build something in Embed Optimizer on top of another PR change to the Optimization Detective infrastructure.

felixarntz · 2024-10-09T21:14:11Z

plugins/optimization-detective/detect.js

-			setStorageLock( getCurrentTime() );
+	for ( const extension of extensions ) {
+		if ( extension.finalize instanceof Function ) {
+			extension.finalize( {


Looking at this old comment thread, does it still require an update? Asking since urlMetric is still being passed below.

…ion of URL Metric object

westonruter added the [Type] Enhancement A suggestion for improvement of an existing feature label Jul 17, 2024

swissspidy added the [Plugin] Embed Optimizer Issues for the Embed Optimizer plugin (formerly Auto Sizes) label Jul 26, 2024

westonruter force-pushed the add/embed-optimizer-min-height-reservation branch from f38247b to 947ca41 Compare July 26, 2024 18:23

westonruter added this to the embed-optimizer n.e.x.t milestone Jul 26, 2024

westonruter added the [Plugin] Optimization Detective Issues for the Optimization Detective plugin label Jul 26, 2024

westonruter force-pushed the add/embed-optimizer-min-height-reservation branch from 3f38eb0 to b3ca4ad Compare July 26, 2024 22:06

westonruter commented Jul 26, 2024

View reviewed changes

westonruter mentioned this pull request Jul 26, 2024

Add helper function to construct XPath from indexed breadcrumbs #1407

Draft

westonruter commented Jul 27, 2024

View reviewed changes

plugins/optimization-detective/class-od-url-metric.php Outdated Show resolved Hide resolved

westonruter force-pushed the add/embed-optimizer-min-height-reservation branch from 02b8fb3 to b17d8ba Compare July 30, 2024 00:40

westonruter changed the base branch from trunk to fix/od-schema July 30, 2024 00:40

westonruter force-pushed the add/embed-optimizer-min-height-reservation branch from b17d8ba to 76369b4 Compare July 30, 2024 00:42

westonruter commented Jul 30, 2024

View reviewed changes

plugins/embed-optimizer/class-embed-optimizer-tag-visitor.php Outdated Show resolved Hide resolved

Base automatically changed from fix/od-schema to trunk July 30, 2024 14:58

westonruter force-pushed the add/embed-optimizer-min-height-reservation branch from 1f5e5f8 to 77bea30 Compare August 2, 2024 00:06

westonruter commented Aug 2, 2024

View reviewed changes

westonruter force-pushed the add/embed-optimizer-min-height-reservation branch 2 times, most recently from 019526c to 36e5620 Compare August 13, 2024 23:03

westonruter modified the milestones: embed-optimizer 0.3.0, embed-optimizer n.e.x.t Aug 15, 2024

westonruter added 7 commits August 16, 2024 17:15

Introduce methods to get minumum height of element

0a376c1

Set the min-height of an embed prior to it loading

8aa7e63

Set min-height on embed-wrapper instead of figure container

11f98f4

Use 500px as a better representation of an element that could be LCP

d7cc7cf

Add test for existing style manipulation

48e57e8

Add helper generator method to get all elements

0d285f2

Try using MutationObserve to watch for embed height changes

4778a3d

westonruter added 4 commits October 8, 2024 15:53

Account for error when passing single-item array to min() or max()

a70df28

Add test for Image Prioritizer

4e48d3d

Remove now-unused method to get element minimum hights

d17cace

Improve handling of get_updated_html

5574081

westonruter force-pushed the add/embed-optimizer-min-height-reservation branch from ec1b5c6 to 5574081 Compare October 8, 2024 23:53

Add test for get_all_denormalized_elements

19c0425

westonruter commented Oct 9, 2024

View reviewed changes

Add tests for new OD code

ea36bac

westonruter mentioned this pull request Oct 9, 2024

Fix plugin bootstrapping when embedded after the after_setup_theme action #1566

Closed

westonruter commented Oct 9, 2024

View reviewed changes

westonruter added 2 commits October 8, 2024 17:48

Clarify purpose of overridden get_updated_html method

01c083d

Add missing since tags

c5d6991

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

westonruter self-assigned this Oct 9, 2024

westonruter marked this pull request as ready for review October 9, 2024 01:04

westonruter requested a review from felixarntz as a code owner October 9, 2024 01:04

felixarntz reviewed Oct 9, 2024

View reviewed changes

plugins/embed-optimizer/class-embed-optimizer-tag-visitor.php Outdated Show resolved Hide resolved

plugins/embed-optimizer/class-embed-optimizer-tag-visitor.php Outdated Show resolved Hide resolved

plugins/embed-optimizer/detect.js Show resolved Hide resolved

westonruter and others added 3 commits October 9, 2024 12:55

Clarify handling of embed block tags and embed wrapper tags

455ef4f

Replace tuple with assoc array

a390e15

Co-authored-by: felixarntz <flixos90@git.wordpress.org>

Add doc block for detect.js

a760705

westonruter requested a review from felixarntz October 9, 2024 20:09

westonruter mentioned this pull request Oct 9, 2024

Introduce OD_Element class #1585

Draft

felixarntz reviewed Oct 9, 2024

View reviewed changes

westonruter requested a review from felixarntz October 10, 2024 19:47

westonruter added 3 commits October 10, 2024 17:59

Add API functions to pass to finalize callbacks to avoid direct mutat…

7ca1fbc

…ion of URL Metric object

Improve error handling

f66445f

Harden types and disallow setting core properties

6e0aa8e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leverage URL metrics to reserve space for embeds to reduce CLS #1373

Leverage URL metrics to reserve space for embeds to reduce CLS #1373

westonruter commented Jul 17, 2024 •

edited

Loading

westonruter Jul 26, 2024

westonruter Jul 26, 2024

westonruter commented Jul 26, 2024 •

edited

Loading

westonruter commented Jul 26, 2024 •

edited

Loading

westonruter Aug 2, 2024

westonruter Aug 2, 2024

westonruter commented Aug 14, 2024

westonruter Oct 9, 2024

westonruter Oct 9, 2024

felixarntz Oct 9, 2024

westonruter Oct 9, 2024

westonruter Oct 9, 2024

westonruter Oct 9, 2024 •

edited

Loading

westonruter Oct 9, 2024

github-actions bot commented Oct 9, 2024 •

edited

Loading

westonruter commented Oct 9, 2024

felixarntz left a comment

felixarntz Oct 9, 2024

westonruter Oct 9, 2024

felixarntz Oct 10, 2024

westonruter Oct 10, 2024

felixarntz Oct 10, 2024

westonruter Oct 10, 2024

felixarntz Oct 10, 2024

westonruter Oct 10, 2024

felixarntz Oct 9, 2024

westonruter Oct 9, 2024

felixarntz Oct 9, 2024

felixarntz Oct 9, 2024

		$denormalized_elements = $context->url_metric_group_collection->get_all_denormalized_elements()[ $embed_wrapper_xpath ] ?? array();
		foreach ( $denormalized_elements as list( $group, $url_metric, $element ) ) {

Leverage URL metrics to reserve space for embeds to reduce CLS #1373

Are you sure you want to change the base?

Leverage URL metrics to reserve space for embeds to reduce CLS #1373

Conversation

westonruter commented Jul 17, 2024 • edited Loading

Accounting for responsive embeds

Demo Videos

Before

After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Jul 26, 2024 • edited Loading

westonruter commented Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Aug 14, 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

westonruter Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 9, 2024 • edited Loading

westonruter commented Oct 9, 2024

felixarntz left a 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

Choose a reason for hiding this comment

westonruter commented Jul 17, 2024 •

edited

Loading

westonruter commented Jul 26, 2024 •

edited

Loading

westonruter commented Jul 26, 2024 •

edited

Loading

westonruter Oct 9, 2024 •

edited

Loading

github-actions bot commented Oct 9, 2024 •

edited

Loading