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

Preconnect links for embeds need not require URL Metrics to be collected from both mobile and desktop #1757

Open
westonruter opened this issue Dec 18, 2024 · 1 comment · May be fixed by #1764
Labels
[Plugin] Embed Optimizer Issues for the Embed Optimizer plugin (formerly Auto Sizes) [Type] Bug An existing feature is broken

Comments

@westonruter
Copy link
Member

westonruter commented Dec 18, 2024

Bug Description

On my blog I have a post with a YouTube video in the initial viewport. I'm seeing preconnect links added with media queries as expected:

<link data-od-added-tag rel="preconnect" href="https://i.ytimg.com/" media="(max-width: 480px)">
<link data-od-added-tag rel="preconnect" href="https://i.ytimg.com/" media="(min-width: 783px)">
<link data-od-added-tag rel="preconnect" href="https://www.youtube.com/" media="(max-width: 480px)">
<link data-od-added-tag rel="preconnect" href="https://www.youtube.com/" media="(min-width: 783px)">

This was implemented in #1654 to fix #1341.

However, I was confused when testing because I wasn't seeing any preconnect links added even after a URL Metric had been collected. The small problem in the implementation is that it requires for there to be URL Metrics for both mobile and desktop to be collected before it adds the preconnect links. This was a carryover from before when there were no media queries, so it was needing to make sure the embed is on both desktop and mobile. Now, however, since there are media queries that target the preconnects more granularly, this is no longer required. The requirement that there be URL Metrics collected for desktop and mobile is, however, still required for determining whether to lazy-load.

This is the code in question:

// Preconnect links and lazy-loading can only be done once there are URL Metrics collected for both mobile and desktop.
if (
$context->url_metric_group_collection->get_first_group()->count() > 0
&&
$context->url_metric_group_collection->get_last_group()->count() > 0
) {
$embed_wrapper_xpath = self::get_embed_wrapper_xpath( $processor->get_xpath() );
$max_intersection_ratio = $context->url_metric_group_collection->get_element_max_intersection_ratio( $embed_wrapper_xpath );
if ( $max_intersection_ratio > 0 ) {
/*
* The following embeds have been chosen for optimization due to their relative popularity among all embed types.
* See <https://colab.sandbox.google.com/drive/1nSpg3qoCLY-cBTV2zOUkgUCU7R7X2f_R?resourcekey=0-MgT7Ur0pT__vw-5_AHjgWQ#scrollTo=utZv59sXzXvS>.
* The list of hosts being preconnected to was obtained by inserting an embed into a post and then looking
* at the network log on the frontend as the embed renders. Each should include the host of the iframe src
* as well as URLs for assets used by the embed, _if_ the URL looks like it is not geotargeted (e.g. '-us')
* or load-balanced (e.g. 's0.example.com'). For the load balancing case, attempt to load the asset by
* incrementing the number appearing in the subdomain (e.g. s1.example.com). If the asset still loads, then
* it is a likely case of a load balancing domain name which cannot be safely preconnected since it could
* not end up being the load balanced domain used for the embed. Lastly, these domains are only for the URLs
* for GET requests, as POST requests are not likely to be part of the critical rendering path.
*/
$preconnect_hrefs = array();
$has_class = static function ( string $wanted_class ) use ( $processor ): bool {
return true === $processor->has_class( $wanted_class );
};
if ( $has_class( 'wp-block-embed-youtube' ) ) {
$preconnect_hrefs[] = 'https://www.youtube.com';
$preconnect_hrefs[] = 'https://i.ytimg.com';
} elseif ( $has_class( 'wp-block-embed-twitter' ) ) {
$preconnect_hrefs[] = 'https://syndication.twitter.com';
$preconnect_hrefs[] = 'https://pbs.twimg.com';
} elseif ( $has_class( 'wp-block-embed-vimeo' ) ) {
$preconnect_hrefs[] = 'https://player.vimeo.com';
$preconnect_hrefs[] = 'https://f.vimeocdn.com';
$preconnect_hrefs[] = 'https://i.vimeocdn.com';
} elseif ( $has_class( 'wp-block-embed-spotify' ) ) {
$preconnect_hrefs[] = 'https://apresolve.spotify.com';
$preconnect_hrefs[] = 'https://embed-cdn.spotifycdn.com';
$preconnect_hrefs[] = 'https://encore.scdn.co';
$preconnect_hrefs[] = 'https://i.scdn.co';
} elseif ( $has_class( 'wp-block-embed-videopress' ) || $has_class( 'wp-block-embed-wordpress-tv' ) ) {
$preconnect_hrefs[] = 'https://video.wordpress.com';
$preconnect_hrefs[] = 'https://public-api.wordpress.com';
$preconnect_hrefs[] = 'https://videos.files.wordpress.com';
$preconnect_hrefs[] = 'https://v0.wordpress.com'; // This does not appear to be a load-balanced domain since v1.wordpress.com is not valid.
} elseif ( $has_class( 'wp-block-embed-instagram' ) ) {
$preconnect_hrefs[] = 'https://www.instagram.com';
$preconnect_hrefs[] = 'https://static.cdninstagram.com';
$preconnect_hrefs[] = 'https://scontent.cdninstagram.com';
} elseif ( $has_class( 'wp-block-embed-tiktok' ) ) {
$preconnect_hrefs[] = 'https://www.tiktok.com';
// Note: The other domains used for TikTok embeds include https://lf16-tiktok-web.tiktokcdn-us.com,
// https://lf16-cdn-tos.tiktokcdn-us.com, and https://lf16-tiktok-common.tiktokcdn-us.com among others
// which either appear to be geo-targeted ('-us') _or_ load-balanced ('lf16'). So these are not added
// to the preconnected hosts.
} elseif ( $has_class( 'wp-block-embed-amazon' ) ) {
$preconnect_hrefs[] = 'https://read.amazon.com';
$preconnect_hrefs[] = 'https://m.media-amazon.com';
} elseif ( $has_class( 'wp-block-embed-soundcloud' ) ) {
$preconnect_hrefs[] = 'https://w.soundcloud.com';
$preconnect_hrefs[] = 'https://widget.sndcdn.com';
// Note: There is also https://i1.sndcdn.com which is for the album art, but the '1' indicates it may be geotargeted/load-balanced.
} elseif ( $has_class( 'wp-block-embed-pinterest' ) ) {
$preconnect_hrefs[] = 'https://assets.pinterest.com';
$preconnect_hrefs[] = 'https://widgets.pinterest.com';
$preconnect_hrefs[] = 'https://i.pinimg.com';
}
foreach ( $preconnect_hrefs as $preconnect_href ) {
foreach ( $context->url_metric_group_collection as $group ) {
if ( ! ( $group->get_element_max_intersection_ratio( $embed_wrapper_xpath ) > 0.0 ) ) {
continue;
}
$context->link_collection->add_link(
array(
'rel' => 'preconnect',
'href' => $preconnect_href,
),
$group->get_minimum_viewport_width(),
$group->get_maximum_viewport_width()
);
}
}
} elseif ( embed_optimizer_update_markup( $processor, false ) && ! $this->added_lazy_script ) {
$processor->append_body_html( wp_get_inline_script_tag( embed_optimizer_get_lazy_load_script(), array( 'type' => 'module' ) ) );
$this->added_lazy_script = true;
}
}

I believe the code should be refactored like this:

// Add preconnect links.
/*
 * The following embeds have been chosen for optimization due to their relative popularity among all embed types.
 * See <https://colab.sandbox.google.com/drive/1nSpg3qoCLY-cBTV2zOUkgUCU7R7X2f_R?resourcekey=0-MgT7Ur0pT__vw-5_AHjgWQ#scrollTo=utZv59sXzXvS>.
 * The list of hosts being preconnected to was obtained by inserting an embed into a post and then looking
 * at the network log on the frontend as the embed renders. Each should include the host of the iframe src
 * as well as URLs for assets used by the embed, _if_ the URL looks like it is not geotargeted (e.g. '-us')
 * or load-balanced (e.g. 's0.example.com'). For the load balancing case, attempt to load the asset by
 * incrementing the number appearing in the subdomain (e.g. s1.example.com). If the asset still loads, then
 * it is a likely case of a load balancing domain name which cannot be safely preconnected since it could
 * not end up being the load balanced domain used for the embed. Lastly, these domains are only for the URLs
 * for GET requests, as POST requests are not likely to be part of the critical rendering path.
 */
$preconnect_hrefs = array();
$has_class        = static function ( string $wanted_class ) use ( $processor ): bool {
	return true === $processor->has_class( $wanted_class );
};
if ( $has_class( 'wp-block-embed-youtube' ) ) {
	$preconnect_hrefs[] = 'https://www.youtube.com';
	$preconnect_hrefs[] = 'https://i.ytimg.com';
} elseif ( $has_class( 'wp-block-embed-twitter' ) ) {
	$preconnect_hrefs[] = 'https://syndication.twitter.com';
	$preconnect_hrefs[] = 'https://pbs.twimg.com';
} elseif ( $has_class( 'wp-block-embed-vimeo' ) ) {
	$preconnect_hrefs[] = 'https://player.vimeo.com';
	$preconnect_hrefs[] = 'https://f.vimeocdn.com';
	$preconnect_hrefs[] = 'https://i.vimeocdn.com';
} elseif ( $has_class( 'wp-block-embed-spotify' ) ) {
	$preconnect_hrefs[] = 'https://apresolve.spotify.com';
	$preconnect_hrefs[] = 'https://embed-cdn.spotifycdn.com';
	$preconnect_hrefs[] = 'https://encore.scdn.co';
	$preconnect_hrefs[] = 'https://i.scdn.co';
} elseif ( $has_class( 'wp-block-embed-videopress' ) || $has_class( 'wp-block-embed-wordpress-tv' ) ) {
	$preconnect_hrefs[] = 'https://video.wordpress.com';
	$preconnect_hrefs[] = 'https://public-api.wordpress.com';
	$preconnect_hrefs[] = 'https://videos.files.wordpress.com';
	$preconnect_hrefs[] = 'https://v0.wordpress.com'; // This does not appear to be a load-balanced domain since v1.wordpress.com is not valid.
} elseif ( $has_class( 'wp-block-embed-instagram' ) ) {
	$preconnect_hrefs[] = 'https://www.instagram.com';
	$preconnect_hrefs[] = 'https://static.cdninstagram.com';
	$preconnect_hrefs[] = 'https://scontent.cdninstagram.com';
} elseif ( $has_class( 'wp-block-embed-tiktok' ) ) {
	$preconnect_hrefs[] = 'https://www.tiktok.com';
	// Note: The other domains used for TikTok embeds include https://lf16-tiktok-web.tiktokcdn-us.com,
	// https://lf16-cdn-tos.tiktokcdn-us.com, and https://lf16-tiktok-common.tiktokcdn-us.com among others
	// which either appear to be geo-targeted ('-us') _or_ load-balanced ('lf16'). So these are not added
	// to the preconnected hosts.
} elseif ( $has_class( 'wp-block-embed-amazon' ) ) {
	$preconnect_hrefs[] = 'https://read.amazon.com';
	$preconnect_hrefs[] = 'https://m.media-amazon.com';
} elseif ( $has_class( 'wp-block-embed-soundcloud' ) ) {
	$preconnect_hrefs[] = 'https://w.soundcloud.com';
	$preconnect_hrefs[] = 'https://widget.sndcdn.com';
	// Note: There is also https://i1.sndcdn.com which is for the album art, but the '1' indicates it may be geotargeted/load-balanced.
} elseif ( $has_class( 'wp-block-embed-pinterest' ) ) {
	$preconnect_hrefs[] = 'https://assets.pinterest.com';
	$preconnect_hrefs[] = 'https://widgets.pinterest.com';
	$preconnect_hrefs[] = 'https://i.pinimg.com';
}

foreach ( $preconnect_hrefs as $preconnect_href ) {
	foreach ( $context->url_metric_group_collection as $group ) {
		if ( ! ( $group->get_element_max_intersection_ratio( $embed_wrapper_xpath ) > 0.0 ) ) {
			continue;
		}

		$context->link_collection->add_link(
			array(
				'rel'  => 'preconnect',
				'href' => $preconnect_href,
			),
			$group->get_minimum_viewport_width(),
			$group->get_maximum_viewport_width()
		);
	}
}

// Lazy-loading can only be done once there are URL Metrics collected for both mobile and desktop.
if (
	$context->url_metric_group_collection->get_first_group()->count() > 0
	&&
	$context->url_metric_group_collection->get_last_group()->count() > 0
) {
	$max_intersection_ratio = $context->url_metric_group_collection->get_element_max_intersection_ratio( $embed_wrapper_xpath );
	if ( ! ( $max_intersection_ratio > 0 ) && embed_optimizer_update_markup( $processor, false ) && ! $this->added_lazy_script ) {
		$processor->append_body_html( wp_get_inline_script_tag( embed_optimizer_get_lazy_load_script(), array( 'type' => 'module' ) ) );
		$this->added_lazy_script = true;
	}
}

We should also take this opportunity to extract the preconnect and lazy-loading logic into separate methods similar to as we have done for reduce_layout_shifts.

Steps to reproduce

  1. Add an embed block at the beginning of a blog post
  2. Activate both Embed Optimizer and Optimization Detective
  3. View the post on the frontend so that a URL Metric is collected.
  4. Reload the page, and notice that no preconnect link is present.
  5. Emulate mobile and reload the page after a minute so that another URL Metric will be collected.
  6. Reload the page again, and now see the preconnect links are present.
@westonruter westonruter added [Plugin] Embed Optimizer Issues for the Embed Optimizer plugin (formerly Auto Sizes) [Type] Bug An existing feature is broken labels Dec 18, 2024
@westonruter westonruter added this to the embed-optimizer n.e.x.t milestone Dec 18, 2024
@github-project-automation github-project-automation bot moved this to Not Started/Backlog 📆 in WP Performance 2024 Dec 18, 2024
@westonruter westonruter moved this from Not Started/Backlog 📆 to To Do 🔧 in WP Performance 2024 Dec 18, 2024
@westonruter
Copy link
Member Author

westonruter commented Dec 18, 2024

Related to this bit of code:

! ( $max_intersection_ratio > 0 )

We may be tempted to instead do:

$max_intersection_ratio === 0

But it turns out that in PHP, 0.0 === 0 is false: https://3v4l.org/MOLA8

We could rather do:

$max_intersection_ratio === 0.0

And this appears to always be true.

To be extra safe we could instead do:

$max_intersection_ratio < 0.000001

And actually since we require PHP 7.2 now we can do this:

$max_intersection_ratio < PHP_FLOAT_EPSILON

@westonruter westonruter moved this from To Do 🔧 to In Progress 🚧 in WP Performance 2024 Dec 23, 2024
@westonruter westonruter moved this from In Progress 🚧 to Code Review 👀 in WP Performance 2024 Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Embed Optimizer Issues for the Embed Optimizer plugin (formerly Auto Sizes) [Type] Bug An existing feature is broken
Projects
Status: Code Review 👀
1 participant