new_audit: preload-lcp-image #11486
new_audit: preload-lcp-image #11486
Conversation
I think this could go in default to start. |
My worry is that the reported savings won't be real and I don't want anyone being misled by the number. |
|
Could we leave out that column then? |
I asked @Beytoven to put it in experimental for now due to the PR split. The simple estimated savings calculation in this version won't handle any of the situations where preloading the image would actually make things worse and radically overstates the estimated savings in most cases. If there's a big rush to ship this (if so I'd love to hear why 😃), then at the least we should not put it into opportunities yet and move it to diagnostics instead. |
There was a problem hiding this comment.
thanks for splitting this up @Beytoven it'll be much easier to do in pieces :)
a basic smoke test would still be appropriate for this first iteration I think. (and a unit test based on some of our trace fixtures to make sure no major errors thrown)
|
|
||
| const UIStrings = { | ||
| /** Title of a lighthouse audit that tells a user to preload an image in order to improve their LCP time. */ | ||
| title: 'Preload Largest Contentful Paint image', |
There was a problem hiding this comment.
This title jumps out a bit to me as out of place among the other opportunities with the title cased metric in there. I don't really know what to do about it until we fix the broader opportunity redesign (which would eliminate the need to make the LCP connection in the title itself), but just flagging in case you had other alternative ideas you were thinking about as you wrote this :)
Preload largest content maaybe?
| * @param {LH.Artifacts.NetworkRequest} request | ||
| * @param {LH.Artifacts.NetworkRequest} mainResource | ||
| * @param {Array<LH.Gatherer.Simulation.GraphNode>} initiatorPath | ||
| * @param {string|undefined} lcpImageSource |
There was a problem hiding this comment.
feels like this should be defined or we should've exited before we ever got here :)
| * @param {LH.Gatherer.Simulation.Simulator} simulator | ||
| * @return {{wastedMs: number, results: Array<{url: string, wastedMs: number}>}} | ||
| */ | ||
| static computeWasteWithGraph(lcpUrl, graph, simulator) { |
There was a problem hiding this comment.
seems like this function isn't really necessary for this phase of the PR
or just replace all this with return Array.from(simulation.nodeTimings.entries()).find(([node, timing]) => node.record.url === lcpUrl)[1].duration
| .find(element => element.traceEventType === 'largest-contentful-paint'); | ||
|
|
||
| /** @type {LH.Config.Settings} */ | ||
| // @ts-expect-error |
There was a problem hiding this comment.
we should use the real settings from context.settings (just reuse simulatorOptions)
| * @param {HTMLElement} element | ||
| */ | ||
| /* instanbul ignore next */ | ||
| function getImageSource(element) { |
There was a problem hiding this comment.
it's a little more complicated unfortunately :/
lighthouse/lighthouse-core/gather/gatherers/image-elements.js
Lines 63 to 66 in 9cf4511
can we use the data from ImageElements artifact and find it using the devtools node path data or something instead of duplicating in TraceElements?
There was a problem hiding this comment.
For sure. Gonna look into this.
Co-authored-by: Patrick Hulce <[email protected]>
|
Just checking in on the status here. Is the next action still |
Yes. Still working on getting the tests working. |
connorjclark
changed the title
| }); | ||
|
|
||
| if (!lcpImageElement) return undefined; | ||
| let lcpUrl; |
There was a problem hiding this comment.
feels a little weird when we already know the URL going in to this :)
WDYT about...
| let lcpUrl; | |
| const lcpUrl = lcpImageElement.src; | |
| let shouldPreload = false; |
and then using shouldPreload as this?
| const path = traversalPath.slice(1).filter(initiator => initiator.type === 'network'); | ||
|
|
||
| // eslint-disable-next-line max-len | ||
| if (!PreloadLCPImageAudit.shouldPreloadRequest(record, mainResource, path, imageSource)) return; |
There was a problem hiding this comment.
want to move the record.url check into the traverse itself? we're essentially only trying to find the LCP node here so it's weird that we do all this other work for traversal path and whatnot for all nodes
(for example it would make sense to me if instead we had a separate findLcpNode function that just returned the node and then we called shouldPreloadRequest exactly once :D)
There was a problem hiding this comment.
I see what you mean. The current way I went it about it seems really unnecessary now that I look at it again haha.
| const simulation = simulator.simulate(graph, {flexibleOrdering: true}); | ||
| // For now, we are simply using the duration of the LCP image request as the wastedMS value | ||
| const timings = Array.from(simulation.nodeTimings.entries()); | ||
| // @ts-ignore |
There was a problem hiding this comment.
what is this ignoring? I suspect that this is actually an error (nodes can be CPU tasks which won't have a record)
There was a problem hiding this comment.
I actually meant to preemptively comment on this line. I wasn't sure if CPU tasks was something I needed to worry about so I set the ignore to suppress the error at the moment and have it sorted out in the PR. With your recommendations above, this shouldn't be an issue after I refactor anyways.
| static computeWasteWithGraph(lcpUrl, graph, simulator) { | ||
| const simulation = simulator.simulate(graph, {flexibleOrdering: true}); | ||
| // For now, we are simply using the duration of the LCP image request as the wastedMS value | ||
| const timings = Array.from(simulation.nodeTimings.entries()); |
There was a problem hiding this comment.
if we had the lcpNode object in my previous recommendations we could just do simulation.nodeTimings.get(lcpNode) :D
| @@ -459,6 +458,7 @@ const getNodeDetailsString = `function getNodeDetails(elem) { | |||
| boundingRect: getBoundingClientRect(elem), | |||
| snippet: getOuterHTMLSnippet(elem), | |||
| nodeLabel: getNodeLabel(elem), | |||
| elementType: elem.tagName.toLowerCase(), | |||
There was a problem hiding this comment.
seems like we don't need this anymore?
types/artifacts.d.ts
Outdated
| @@ -163,6 +163,7 @@ declare global { | |||
| boundingRect: Rect | null, | |||
| snippet: string, | |||
| nodeLabel: string, | |||
| elementType: string, | |||
| devtoolsNodePath: '1,HTML,1,BODY,3,DIV,2,P', | ||
| }, | ||
| ], | ||
| ImageElements: [], |
There was a problem hiding this comment.
is this different from the test before it? it'd be nice to get coverage of a clause or two in the shouldPreloadRequest conditions instead :)
There was a problem hiding this comment.
Oof. Didn't realize I tested the same case twice in two different ways. 🤦
Co-authored-by: Patrick Hulce <[email protected]>
| @@ -398,7 +398,6 @@ function getNodeLabel(node) { | |||
|
|
|||
| /** | |||
| * @param {HTMLElement} element | |||
| * @param {LH.Artifacts.Rect} | |||
There was a problem hiding this comment.
nice find! I think was supposed to be a @return :)
|
|
||
| const UIStrings = { | ||
| /** Title of a lighthouse audit that tells a user to preload an image in order to improve their LCP time. */ | ||
| title: 'Preload Largest Contentful Paint image', |
There was a problem hiding this comment.
Sitting on it for a while now, I don't love this audit title and ID, but I don't really have concrete ideas for improving it right away.
My thinking is that it feels a bit weird to use the metric name as an adjective for this image when the metric is based on other adjectives of the image that are more easily understood 😆
(conceptually I think we could do this audit for any very large hero-type image, not just the LCP one if text was LCP for example. In that case I might suggest Preload critical images even if we only target LCP to start)
It's all experimental for now though, so shouldn't be a blocker, maybe add a todo to finalize this decision or update the issue (is there one?) to include it as a final check?
This PR is a continuation of #11462.
This is audit will check if the image of an LCP element is preloaded and if not, will calculate the savings possible by doing so. It will be 'not applicable' if the LCP isn't an image.
For now, the potential savings will simply be the duration of the image request. The actual calculation for opportunity savings will be done in a follow-up PR. In the meantime, the audit will remain in experimental.
Design Doc