Rely on server-side breadcrumbs for detection and optimization #892

westonruter · 2023-12-04T19:57:17Z

Summary

Eliminate client-side computation of element breadcrumbs in favor of generating them server-side while processing the output buffer. Breadcrumbs are serialized as XPath and added to image elements as a data-ilo-xpath attribute. This data attribute is used by the detection script instead of generating breadcrumbs client-side. Lastly, the detection script is now injected as part of the output buffer processing instead of a separate wp_footer code path.

Relevant technical choices

This improves the reliability of connecting elements from client-side detection with server-side optimization by exclusively computing breadcrumbs XPath on the server. This eliminates the issue encountered in #884 in which JavaScript injecting a new element. Namely it address the following todo from #884 (comment):

Improve handling of breadcrumb generation when there the document structure varies either due to admin bar being present or due to client-side logic. Currently breadcrumbs have to explicitly skip over the admin bar element to ignore it. Similarly, the client-side breadcrumb logic has to skip over .skip-link.screen-reader-text which is injected by wp_enqueue_block_template_skip_link(). This issue of client-side mutated elements is particularly problematic, which may require server-side tagging of elements with their breadcrumbs which would eliminate needing to also do that calculation on the client. But this would mean extraneous data- attributes on all images. When detection isn't needed, the tag processor could strip those attributes out before sending back the buffer. We may need to monitor this to see how much of a problem it is, such as by checking various themes to see what degree they inject elements to mess up breadcrumbs. As another alternative, we may consider relying on IDs and class names for breadcrumbs rather than element indices, although JS here too can cause problems by modifying the class names.

As part of this, XPath strings are used throughout the module rather than comparing breadcrumbs arrays.

Additionally, instead of injecting the detection script at wp_footer and then having some of the same conditions during output buffer processing for optimization, the detection script is now injected during optimization.

Checklist

PR has either [Focus] or Infrastructure label.
PR has a [Type] label.
PR has a milestone or the no milestone label.

felixarntz · 2023-12-05T23:27:12Z

@westonruter Would you like this PR to be reviewed and merged into the other PR's branch, or did you simply start work on this before and prefer #884 to be merged first?

westonruter · 2023-12-05T23:38:08Z

@felixarntz I'd like #884 to be merged first so that the PRs don't get excessively large.

…ver-side-breadcrumbs * add/image-loading-optimization-optimizing: Add comment explaining why for loop is used Clarify LCP element language in comment

westonruter · 2023-12-08T23:03:13Z

Note: I'm working on the tests for this in a sub-PR: #898.

In the meantime, this PR can be reviewed (and merged).

…e-breadcrumbs

westonruter · 2023-12-11T23:50:46Z

modules/images/image-loading-optimization/class-ilo-html-tag-processor.php

+	 * It would be nicer if this were like `/html[1]/body[2]` but in XPath the position() here refers to the
+	 * index of the preceding node set. So it has to rather be written `/*[1][self::html]/*[2][self::body]`.


This being said, the breadcrumbs could be updated to keep track of the index of the tag of a given type rather than the index of the tag in general. This would make breadcrumbs more in line with the natural XPath syntax, so instead /*[1][self::html]/*[2][self::body] we could do just /html[1]/body[1].

Nevertheless, what is implemented now is what the :nth-child() pseudo-selector implements in CSS, as opposed to :nth-of-type().

But doing this would take some extra bookkeeping, and I don't see a clear benefit.

I also just came across a Chromium doc that is looking at the same problem space: Identifying element consistently across reloads.

felixarntz

This looks great to me! It's more reliable now that the source of truth is on the server - and while I don't like XPath, at least its usage here is simple enough (mostly just to compare against it) that I feel it doesn't make the code unapproachable. Also +1 to combining the logic for when to print the detection script to avoid duplicate logic.

Just a few minor points below.

felixarntz · 2023-12-20T18:10:56Z

modules/images/image-loading-optimization/class-ilo-html-tag-processor.php

-			$breadcrumbs[] = array(
-				'tag'   => $breadcrumb_tag_name,
-				'index' => $this->open_stack_indices[ $i ],
-			);
+			yield array( $breadcrumb_tag_name, $this->open_stack_indices[ $i ] );


Any particular reason for the data type change here? I find an associative array with named keys easier to understand than an indexed array that isn't really for a list of something.

Mainly because it makes it more compact and slightly easier to iterate over per below:

foreach ( $this->get_breadcrumbs() as list( $tag_name, $index ) ) {

Originally too I had envisioned that a breadcrumbs could have other keys, like a class name or maybe some attributes to make it more like a CSS selector. But this didn't end up being the case, as we only ever need the tag name and index. So making it a tuple seems to make sense to me.

I realize it's a few fewer lines of code, but it's harder to understand. Keys allow clarifying what something is, while now that's less apparent, particularly when looking at where the function is used and only finding list.

I'd prefer to go for clarity over compactness, but not a blocker as long as this function remains purely internal.

felixarntz · 2023-12-20T18:17:39Z

modules/images/image-loading-optimization/optimization.php

-	if ( ! $post ) {
-		return $buffer;
-	}
+	$url_metrics = $post ? ilo_parse_stored_url_metrics( $post ) : array();


Why remove the early return here? Isn't that more efficient if no URL metrics post is found?

Good question. Because now if no URL metrics are found, we need to proceed and add breadcrumbs to the elements so that we can gather the URL metrics. You can see below too that the detection script is now injected in this function as well.

felixarntz · 2023-12-20T18:19:10Z

modules/images/image-loading-optimization/storage/rest-api.php

-										),
-									),
-								),
+								'pattern'  => '^(/\*\[\d+\]\[self::.+?\])+$', // See ILO_HTML_Tag_Processor::get_xpath() for format.


Just a suggestion: Make this a const on ILO_HTML_Tag_Processor since that's where the definition really comes from.

Good idea. Done in 2df915d

adamsilverstein

Splendid

westonruter added 3 commits December 4, 2023 11:40

Improve construction of breadcrumbs

a50a8a2

Eliminate use of client-side breadcrumbs

6498b6b

Replace breadcrumbs with xpath where relevant

0bbe216

westonruter added no milestone PRs that do not have a defined milestone for release [Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Enhancement A suggestion for improvement of an existing feature labels Dec 4, 2023

westonruter added 2 commits December 4, 2023 12:12

Inject detection script during optimization

1da6d5d

Move XPath generator to ILO_HTML_Tag_Processor class

c58609c

westonruter mentioned this pull request Dec 4, 2023

Optimize the loading of images using stored URL metrics #884

Merged

3 tasks

westonruter added 3 commits December 4, 2023 13:14

Remove unused function

4468d93

Improve phpdoc for get_xpath

3102f4c

Update data attribute in comment

255aa45

westonruter marked this pull request as ready for review December 4, 2023 21:27

westonruter requested review from adamsilverstein and felixarntz December 4, 2023 21:28

westonruter mentioned this pull request Dec 6, 2023

Add PHPUnit tests for Image Loading Optimization #898

Merged

3 tasks

Merge branch 'add/image-loading-optimization-optimizing' into add/ser…

d674c49

…ver-side-breadcrumbs * add/image-loading-optimization-optimizing: Add comment explaining why for loop is used Clarify LCP element language in comment

westonruter added the [Focus] Images label Dec 6, 2023

Base automatically changed from add/image-loading-optimization-optimizing to feature/image-loading-optimization December 7, 2023 19:04

Merge branch 'feature/image-loading-optimization' into add/server-sid…

6921e6f

…e-breadcrumbs

westonruter commented Dec 11, 2023

View reviewed changes

This was referenced Dec 14, 2023

Optimization Detective: Server-applied optimizations informed by client-side detection #869

Closed

Preload image for LCP element with background-image #914

Merged

felixarntz approved these changes Dec 20, 2023

View reviewed changes

Move XPath pattern definition to ILO_HTML_Tag_Processor

2df915d

adamsilverstein approved these changes Dec 21, 2023

View reviewed changes

westonruter merged commit 7c0eb74 into feature/image-loading-optimization Dec 21, 2023
8 checks passed

westonruter deleted the add/server-side-breadcrumbs branch December 21, 2023 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rely on server-side breadcrumbs for detection and optimization #892

Rely on server-side breadcrumbs for detection and optimization #892

westonruter commented Dec 4, 2023 •

edited

Loading

felixarntz commented Dec 5, 2023

westonruter commented Dec 5, 2023

westonruter commented Dec 8, 2023

westonruter Dec 11, 2023

felixarntz left a comment

felixarntz Dec 20, 2023

westonruter Dec 20, 2023

felixarntz Dec 20, 2023

felixarntz Dec 20, 2023

westonruter Dec 20, 2023

felixarntz Dec 20, 2023

westonruter Dec 21, 2023

adamsilverstein left a comment

		* It would be nicer if this were like `/html[1]/body[2]` but in XPath the position() here refers to the
		* index of the preceding node set. So it has to rather be written `/[1][self::html]/[2][self::body]`.

Rely on server-side breadcrumbs for detection and optimization #892

Rely on server-side breadcrumbs for detection and optimization #892

Conversation

westonruter commented Dec 4, 2023 • edited Loading

Summary

Relevant technical choices

Checklist

felixarntz commented Dec 5, 2023

westonruter commented Dec 5, 2023

westonruter commented Dec 8, 2023

Choose a reason for hiding this comment

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

adamsilverstein left a comment

Choose a reason for hiding this comment

westonruter commented Dec 4, 2023 •

edited

Loading