-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
core(preload-lcp-image): get LCP image url from trace #14695
Conversation
snippet: '<h2 id="toppy" style="background-image:url(\'\');">', | ||
nodeLabel: 'Do better web tester page', | ||
}, | ||
url: 'http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?lcp', |
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.
on main, this same updated dbw would have an LCP image URL of the main page, http://localhost:10200/dobetterweb/dbw_tester.html
instead of the actual image
@@ -221,5 +221,13 @@ | |||
{"method":"Network.dataReceived","params":{"requestId":"31161.49","timestamp":8710.379897,"dataLength":34,"encodedDataLength":0}}, | |||
{"method":"Network.dataReceived","params":{"requestId":"31161.49","timestamp":8710.380969,"dataLength":0,"encodedDataLength":45}}, | |||
{"method":"Network.loadingFinished","params":{"requestId":"31161.49","timestamp":8710.377607,"encodedDataLength":255,"shouldReportCorbBlocking":false}}, | |||
{"method":"Page.lifecycleEvent","params":{"frameId":"537BC44B4EE044D67F9FFE7A76173AB1","loaderId":"6913DB840A4B952A23AAEB21C42F130A","name":"networkIdle","timestamp":8710.380991}} | |||
{"method":"Page.lifecycleEvent","params":{"frameId":"537BC44B4EE044D67F9FFE7A76173AB1","loaderId":"6913DB840A4B952A23AAEB21C42F130A","name":"networkIdle","timestamp":8710.380991}}, |
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.
manually copy/pasted these from a fresh trace (adjusting timestamps) rather than refreshing all artifacts
@@ -1,202 +0,0 @@ | |||
[ |
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.
these other passes don't exist anymore
@@ -231,7 +231,7 @@ class TraceElements extends FRGatherer { | |||
} | |||
|
|||
// These should exist, but trace types are loose. | |||
const lcpData = processedNavigation.largestContentfulPaintEvt?.args?.data; |
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.
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.
wait, so is this a bug fix? we'll now be identifying the LCP element as being inside an iframe, whereas before we never did?
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.
wait, so is this a bug fix? we'll now be identifying the LCP element as being inside an iframe, whereas before we never did?
Yeah, unfortunately a definite bug since #14344. Fortunately only triggered if the page's LCP was an image in an iframe but the main frame's LCP was text. Not a common occurrence but not great :/
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.
good stuff
e.args.data?.DOMNodeId === lcpEvent.args.data?.nodeId && | ||
e.args.data?.size === lcpEvent.args.data?.size; | ||
// Get last candidate, in case there was more than one. | ||
}).sort((a, b) => b.ts - a.ts)[0]; |
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.
wouldn't this just be the last item in this array? assuming trace events are already sorted by ts. if so, could also do findLast
as a micro opt (as well as being more self-documenting)
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.
this is going back to the raw trace, so they will be mostly sorted, but not completely (unlike e.g. processedTrace.processEvents
which we sort ourselves)
* @param {LH.Artifacts.ProcessedNavigation} processedNavigation | ||
* @return {string | undefined} | ||
*/ | ||
static getLcpUrl(trace, processedNavigation) { |
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.
Will we be needing this logic elsewhere? maybe trace processor could do it?
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.
that could make sense, or in trace-elements
maybe? But maybe we should wait for the need (e.g. if we want to do something with #11629, the URL is included on the CDP event, so we won't need it there)
Part of #13738
Use the
LargestImagePaint::Candidate
trace event to determine the LCP image URL (if any) rather than going LCP event -> nodeId ->ImageElements
artifact ->img.src
, since a DOM node can actually have multiple images associated with it (through time, as well as an actual<img>
, cssbackground-image
, and associated pseduo-elements containing images). The trace event instead provides exactly the URL that was painted. After this PR, #14350 will no longer be an issue forpreload-lcp-audit
.Also adds a gross image LCP to dbw smoke in the style of #14350: in a pseudo-element while also adding a
background-image: url('')
to the same associated element to attempt to load an invalid image as LCP. Also updates the sample artifacts enough so this is reflected insample_v2.json
.