-
Notifications
You must be signed in to change notification settings - Fork 104
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
[TRY] Optimization Detective debug helper #1759
base: trunk
Are you sure you want to change the base?
Conversation
$processor->append_body_html( | ||
<<<HTML | ||
<button | ||
class="od-debug-dot" | ||
type="button" | ||
popovertarget="od-debug-popover-$uuid" | ||
popovertargetaction="toggle" | ||
style="--anchor-name: --od-debug-dot-$uuid; position-anchor: --od-debug-element-$uuid;" | ||
aria-details="od-debug-popover-$uuid" | ||
> | ||
$anchor_text | ||
</button> | ||
<div | ||
id="od-debug-popover-$uuid" | ||
popover | ||
class="od-debug-popover" | ||
style="position-anchor: --od-debug-dot-$uuid;" | ||
> | ||
$popover_text | ||
</div> | ||
HTML |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So cool!
plugins/optimization-detective/class-optimization-detective-debug-tag-visitor.php
Outdated
Show resolved
Hide resolved
plugins/optimization-detective/class-optimization-detective-debug-tag-visitor.php
Outdated
Show resolved
Hide resolved
plugins/optimization-detective/class-optimization-detective-debug-tag-visitor.php
Outdated
Show resolved
Hide resolved
plugins/optimization-detective/class-optimization-detective-debug-tag-visitor.php
Outdated
Show resolved
Hide resolved
'items' => array( | ||
'type' => 'object', | ||
'required' => true, | ||
'properties' => array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want to include the LoAF data here at some point.
'group' => null, | ||
'title' => __( 'Optimization Detective', 'optimization-detective' ), | ||
'meta' => array( | ||
'onclick' => 'document.body.classList.toggle("od-debug");', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O invoker commands, where art thou?
if ( ! od_can_optimize_response() ) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since pages aren't optimized by default for users who can customize
, this presents a dilemma. We'd really want to show data collected for end users who aren't logged-in, but as it stands right now only the URL Metrics collected from an admin user would be collected here, which again would normally be none.
Otherwise, URL Metrics are collected for logged-in users but they are stored discretely from those stored for logged-out users. See the user_logged_in
"query var" added in the od_get_normalized_query_vars()
function:
performance/plugins/optimization-detective/storage/data.php
Lines 66 to 86 in c446d8c
function od_get_normalized_query_vars(): array { | |
global $wp; | |
// Note that the order of this array is naturally normalized since it is | |
// assembled by iterating over public_query_vars. | |
$normalized_query_vars = $wp->query_vars; | |
// Normalize unbounded query vars. | |
if ( is_404() ) { | |
$normalized_query_vars = array( | |
'error' => 404, | |
); | |
} | |
// 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; | |
} | |
return $normalized_query_vars; | |
} |
I'm not entirely happy with how I "abused" the query vars in this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, well, if we want to access the URL Metrics for unauthenticated requests then we just need to unset the user_logged_in
key from the array returned by od_get_normalized_query_vars()
below. In fact, we could obtain the URL Metrics from both and combine them into one OD_URL_Metric_Group_Collection
, perhaps with a sample size of 2 * od_get_url_metrics_breakpoint_sample_size()
.
The remaining issue is what to do about the XPaths from the unauthenticated URL Metrics not applying to the current authenticated response (e.g. due to the admin bar). I think we may need an alternative to how we currently check if the current XPath via $processor->get_xpath()
matches an XPath stored in a URL Metric. In particular, consider
Note the slight difference in the index of the DIV
caused by the admin bar:
- /*[1][self::HTML]/*[2][self::BODY]/*[1][self::DIV]
+ /*[1][self::HTML]/*[2][self::BODY]/*[2][self::DIV]
Maybe we should have a helper that strips out the index from elements which appear as direct children of BODY
given that arbitrary elements are added/removed at wp_body_open
and wp_footer
? If this instead became normalized to:
/*[1][self::HTML]/*[2][self::BODY]/*[self::DIV]
Or more simply:
/*[1][self::HTML]/*[2][self::BODY]/DIV
Then this would match the DIV.wp-site-blocks
both for when the user is logged-in and logged-out. This would be particularly effective for block themes which always have this root DIV.wp-site-blocks
element from get_the_block_template_html()
. It would be more prone to confusion in classic themes, for example, if there are two root DIV
elements for both the header and footer. However, in looking at the core themes, every single theme except for Twenty Twenty has a root DIV#page
wrapper for the page contents after wp_body_open()
. The one exception is Twenty Twenty but it uses HEADER
, MAIN
, and FOOTER
at the root. So my concerns are likely unfounded and we can just leave the index off of elements which are children of BODY
.
$slug = od_get_url_metrics_slug( od_get_normalized_query_vars() ); | ||
$post = OD_URL_Metrics_Post_Type::get_post( $slug ); | ||
|
||
global $wp_the_query; | ||
|
||
$tag_visitor_registry = new OD_Tag_Visitor_Registry(); | ||
|
||
$current_etag = od_get_current_url_metrics_etag( $tag_visitor_registry, $wp_the_query, od_get_current_theme_template() ); | ||
$group_collection = new OD_URL_Metric_Group_Collection( | ||
$post instanceof WP_Post ? OD_URL_Metrics_Post_Type::get_url_metrics_from_post( $post ) : array(), | ||
$current_etag, | ||
od_get_breakpoint_max_widths(), | ||
od_get_url_metrics_breakpoint_sample_size(), | ||
od_get_url_metric_freshness_ttl() | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this code is duplicated with what is located in od_optimize_template_output_buffer()
, maybe we should factor that out into a helper function to return the $group_collection
.
Co-authored-by: Weston Ruter <[email protected]>
Summary
Fixes #1736
Idea: add a "dot" on relevant elements (e.g. LCP elements). Click on the dot to learn more about the element.
Relevant technical choices