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

Incorporate media queries into preconnect links to account for whether embeds are in viewport #1654

Merged

Conversation

ShyamGadde
Copy link
Contributor

@ShyamGadde ShyamGadde commented Nov 15, 2024

Summary

Fixes #1341

For example, if an embed is not visible on mobile:
image

Relevant technical choices

  • Sorting and Merging of Links: The href attributes of preconnect links are sorted to group identical URLs together. If the URLs are identical, they are further sorted by minimum_viewport_width. This strategy enables merging of consecutive links, reducing redundancy. Refer to the discussion in Incorporate breakpoints into preconnect links added by Embed Optimizer #1341 (comment).
  • Introduced OD_URL_Metric_Group::get_xpath_elements_map(), OD_URL_Metric_Group::get_all_element_max_intersection_ratios() and OD_URL_Metric_Group::get_element_max_intersection_ratio() methods to help determine if a link should get preconnected on that particular viewport group.
  • But the lazy load script is injected only when an embed element is not visible in any viewport group.

Previously, links were sorted only by `minimum_viewport_width`, which
caused issues when merging consecutive preconnect links, especially when
dealing with breakpoints and different URLs.

This update changes the sorting logic to first sort links by the `href`
attribute, grouping identical URLs together. Once grouped, links are
then sorted by `minimum_viewport_width`. This ensures that preconnect
links are correctly merged when attributes align.
@@ -37,7 +37,7 @@ function od_initialize_extensions(): void {
*/
function od_generate_media_query( ?int $minimum_viewport_width, ?int $maximum_viewport_width ): ?string {
if ( is_int( $minimum_viewport_width ) && is_int( $maximum_viewport_width ) && $minimum_viewport_width > $maximum_viewport_width ) {
_doing_it_wrong( __FUNCTION__, esc_html__( 'The minimum width must be greater than the maximum width.', 'optimization-detective' ), 'Optimization Detective 0.7.0' );
_doing_it_wrong( __FUNCTION__, esc_html__( 'The minimum width cannot be greater than the maximum width.', 'optimization-detective' ), 'Optimization Detective 0.7.0' );
Copy link
Member

Choose a reason for hiding this comment

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

Right 😅

$href_a = $a['attributes']['href'] ?? '';
$href_b = $b['attributes']['href'] ?? '';

$href_comparison = strcmp( $href_a, $href_b );
Copy link
Member

Choose a reason for hiding this comment

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

This would be the same as:

$href_comparison = ( $href_a <=> $href_b );

Right?

Copy link
Contributor Author

@ShyamGadde ShyamGadde Nov 15, 2024

Choose a reason for hiding this comment

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

I believe in this case it would be the same.

@ShyamGadde ShyamGadde marked this pull request as ready for review November 15, 2024 17:42
Copy link

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: ShyamGadde <[email protected]>
Co-authored-by: westonruter <[email protected]>

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

@westonruter
Copy link
Member

FYI: The screenshot is broken.

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Looking good!

Added some feedback.

Let's also add a test case specifically for when there is an embed visible on a desktop viewport but hidden on a mobile viewport.

Comment on lines 176 to 189
foreach ( $context->url_metric_group_collection as $group ) {
if ( $group->get_element_max_intersection_ratio( $embed_wrapper_xpath ) <= 0 ) {
continue;
}

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

Choose a reason for hiding this comment

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

The analogous logic that adds preload links for the LCP image:

foreach ( $context->url_metric_group_collection->get_groups_by_lcp_element( $xpath ) as $group ) {
$link_attributes = array_merge(
array(
'rel' => 'preload',
'fetchpriority' => 'high',
'as' => 'image',
),
array_filter(
array(
'href' => (string) $processor->get_attribute( 'src' ),
'imagesrcset' => (string) $processor->get_attribute( 'srcset' ),
'imagesizes' => (string) $processor->get_attribute( 'sizes' ),
),
static function ( string $value ): bool {
return '' !== $value;
}
)
);
$crossorigin = $this->get_attribute_value( $processor, 'crossorigin' );
if ( null !== $crossorigin ) {
$link_attributes['crossorigin'] = 'use-credentials' === $crossorigin ? 'use-credentials' : 'anonymous';
}
$link_attributes['media'] = 'screen';
$context->link_collection->add_link(
$link_attributes,
$group->get_minimum_viewport_width(),
$group->get_maximum_viewport_width()
);
}
return true;
}

For that we use $context->url_metric_group_collection->get_groups_by_lcp_element() to get the groups, whereas here it is looping over each group in the collection and then checking for each one whether the current embed is visible in it. So this makes sense to me.

)
);
foreach ( $context->url_metric_group_collection as $group ) {
if ( $group->get_element_max_intersection_ratio( $embed_wrapper_xpath ) <= 0 ) {
Copy link
Member

Choose a reason for hiding this comment

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

Can the intersection ratio ever be less than 0? I don't think so, and Gemini confirms. Since we're comparing floats, it's probably not safe to check if the return value is exactly equal to 0. How about:

Suggested change
if ( $group->get_element_max_intersection_ratio( $embed_wrapper_xpath ) <= 0 ) {
if ( ! ( $group->get_element_max_intersection_ratio( $embed_wrapper_xpath ) > 0.0 ) ) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. Changed it in 5ecfcf2.

@westonruter westonruter added this to the embed-optimizer n.e.x.t milestone Nov 15, 2024
@westonruter westonruter added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Optimization Detective Issues for the Optimization Detective plugin [Plugin] Embed Optimizer Issues for the Embed Optimizer plugin (formerly Auto Sizes) labels Nov 20, 2024
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Great work!

@westonruter westonruter merged commit e82a04b into WordPress:trunk Nov 20, 2024
13 checks passed
@westonruter westonruter changed the title Incorporate breakpoints into preconnect links added by Embed Optimizer Incorporate media queries into preconnect links to account for whether embeds are in viewport Dec 17, 2024
@westonruter
Copy link
Member

In testing this I found a small issue preventing preconnect links from being added unless URL Metrics are collected for both desktop and mobile: #1757

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) [Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorporate breakpoints into preconnect links added by Embed Optimizer
2 participants