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

XPaths for elements in URL Metrics can omit index for children of BODY to avoid needing to vary URL Metrics by whether a user is logged-in #1787

Open
westonruter opened this issue Jan 10, 2025 · 3 comments
Labels
[Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Enhancement A suggestion for improvement of an existing feature

Comments

@westonruter
Copy link
Member

Discussed initially at #1759 (comment)

URL Metrics currently are varied by whether or not a user is logged-in. This is all of the XPaths used on the page differ based on whether or not the admin bar is present, since the element index is included with each element reference. For example, the following is the XPath for the first IMG in post content when logged-out vs logged-in:

- /*[1][self::HTML]/*[2][self::BODY]/*[1][self::DIV]/*[2][self::DIV]/*[1][self::DIV]/*[1][self::DIV]/*[1][self::ARTICLE]/*[2][self::DIV]/*[1][self::DIV]/*[1][self::FIGURE]/*[1][self::PICTURE]/*[2][self::IMG]
+ /*[1][self::HTML]/*[2][self::BODY]/*[3][self::DIV]/*[2][self::DIV]/*[1][self::DIV]/*[1][self::DIV]/*[1][self::ARTICLE]/*[2][self::DIV]/*[1][self::DIV]/*[1][self::FIGURE]/*[1][self::PICTURE]/*[2][self::IMG]

The difference is that when logged-in, two new elements appear at the beginning of the BODY:

  1. A SCRIPT coming from wp_customize_support_script() (for admin users).
  2. A DIV#wpadminbar for the admin bar.

These result in the page wrapper DIV's XPath going *[1][self::DIV] to *[3][self::DIV].

The purpose of the element index in the XPath is to differentiate one element from another. If there are 3 IMG tags appearing next to each other in a post, and only the first is LCP, then it's important to be able to uniquely reference that IMG. Nevertheless, at the root level of a page (when directly under the BODY) it is highly unlikely for there to be any such ambiguity. This is because themes almost always wrap the content between wp_body_open() and wp_footer(). Classic themes typically wrap the content in a DIV#page element, whereas Block Themes always wrap the content in a DIV.wp-site-blocks element. The only exception in core is that the Twenty Twenty theme which does not have one root wrapper but instead has a HEADER, MAIN, and FOOTER at the root. In this case too there is no possibility for ambiguity since these three are unique.

So I suggest we do the following:

  1. Eliminate this logic from od_get_normalized_query_vars():

// Vary URL Metrics by whether the user is logged in since additional elements may be present.
if ( is_user_logged_in() ) {
$normalized_query_vars['user_logged_in'] = true;
}

  1. Omit the element index from the HTML, BODY, and children directly under the BODY. So the XPaths above would become the following:
/HTML/BODY/DIV/*[2][self::DIV]/*[1][self::DIV]/*[1][self::DIV]/*[1][self::ARTICLE]/*[2][self::DIV]/*[1][self::DIV]/*[1][self::FIGURE]/*[1][self::PICTURE]/*[2][self::IMG]
  1. Eliminate the restriction that Optimization Detective be disabled for admins. This was put in place because additional scripts often get added at wp_body_open() (e.g. the wp_customize_support_script()). With the presence/absence of such elements printed at wp_body_open (and wp_footer) eliminated, we can eliminate this code from od_can_optimize_response() which would resolve Optimization Detective should be enabled by default for admins #1425:

// The aim is to optimize pages for the majority of site visitors, not for those who administer the site, unless
// in 'plugin' development mode. For admin users, additional elements will be present, like the script from
// wp_customize_support_script(), which will interfere with the XPath indices. Note that
// od_get_normalized_query_vars() is varied by is_user_logged_in(), so membership sites and e-commerce sites
// will still be able to be optimized for their normal visitors.
( current_user_can( 'customize' ) && ! wp_is_development_mode( 'plugin' ) ) ||

  1. During this transitionary period in which old URL Metrics still contain the old XPaths, we can patch OD_Element to convert to this new format so existing optimizations will continue to apply based on old URL Metrics:
--- a/plugins/optimization-detective/class-od-element.php
+++ b/plugins/optimization-detective/class-od-element.php
@@ -52,6 +52,17 @@ class OD_Element implements ArrayAccess, JsonSerializable {
 	 */
 	public function __construct( array $data, OD_URL_Metric $url_metric ) {
 		$this->data       = $data;
+
+		// Convert old XPath scheme.
+		$xpath = preg_replace(
+			'#^/\*\[1\]\[self::HTML\]/\*\[2\]\[self::BODY\]/\*\[\d+\]\[self::(DIV)\]#',
+			'/HTML/BODY/$1',
+			$this->data['xpath']
+		);
+		if ( is_string( $xpath ) && '' !== $xpath ) {
+			$this->data['xpath'] = $xpath;
+		}
+
 		$this->url_metric = $url_metric;
 	}

Then in the next release or two we can eliminate this code since the old URL Metrics should have all been purged by then.

@westonruter westonruter added [Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Enhancement A suggestion for improvement of an existing feature labels Jan 10, 2025
@github-project-automation github-project-automation bot moved this to Not Started/Backlog 📆 in WP Performance 2024 Jan 10, 2025
@westonruter
Copy link
Member Author

@felixarntz @adamsilverstein @swissspidy Any concerns with this?

@swissspidy
Copy link
Member

Makes sense to me 👍

@felixarntz
Copy link
Member

SGTM as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Enhancement A suggestion for improvement of an existing feature
Projects
Status: Not Started/Backlog 📆
Development

No branches or pull requests

3 participants