core: reduce image hotlinking in the report #13185
Conversation
| const isCrossOrigin = !URL.originsMatch(pageURL, image.url); | ||
|
|
||
| items.push({ | ||
| node: imageElement ? Audit.makeNodeItem(imageElement.node) : undefined, |
There was a problem hiding this comment.
this and uses-optimized-images are the only image audits we have that can have an undefined node, because they iterate over OptimizedImages and use its url to look up a ImageElement–OptimizedImages is gatherered from networkRecords, and ImageElements are gathered from querying the DOM (it also includes css images).
AFAICT, there should only ever be no ImageElement for an associated OptimizedImage if JS is used to download an image without ever adding it to the DOM. In that case, there's nothing to show via our full page screenshot. Should we fallback to the thumbnail in that case?
There was a problem hiding this comment.
Should we fallback to the thumbnail in that case?
Is this possible without changing the display option for the entire column?
There was a problem hiding this comment.
Should we fallback to the thumbnail in that case?
Is this possible without changing the display option for the entire column?
Yeah, the individual item can set its own type, overriding the column type.
One problem with falling back to thumbnail, though, is it would still run into issues with hot linking/CSP/mixed-content warnings, so it's not clear we can do that.
really bad, terrible idea: should we attempt to take data url versions of important images that have 0-sized area in the full page screenshot and include them in the LHR if they're super important? like an LCP element that was removed by the time we took the screenshot.
| const resultsMap = images.reduce((results, image) => { | ||
| /** @type {Map<string, WasteResult>} */ | ||
| const resultsMap = new Map(); | ||
| for (const image of images) { |
There was a problem hiding this comment.
drive by unreduce
| const isCrossOrigin = !URL.originsMatch(pageURL, image.url); | ||
|
|
||
| items.push({ | ||
| node: imageElement ? Audit.makeNodeItem(imageElement.node) : undefined, |
There was a problem hiding this comment.
Should we fallback to the thumbnail in that case?
Is this possible without changing the display option for the entire column?
report/renderer/details-renderer.js
Outdated
| // We use `node` to show images in the report withhout hotlinking. In that case, | ||
| // we don't want to show the html alongside the node thumbnail, as that information | ||
| // is likely not useful and just adds noise. | ||
| const hasVisibleBoundingRect = item.boundingRect && | ||
| item.boundingRect.width > 1 && item.boundingRect.height > 1; | ||
| const shouldHideSnippetAndLabel = tagName === 'IMG' && hasVisibleBoundingRect; |
There was a problem hiding this comment.
this makes for a closer match for the existing thumbnail behavior, but seems like we wouldn't want to drop these for some existing audits that may have <img> nodes (e.g. accessibility audits)?
also needs some tests :)
There was a problem hiding this comment.
How would you suggest choosing when to elide this? It's pretty noisy when not needed. Gimme a minute and I'll put up a sample report.
There was a problem hiding this comment.
I'm not sure :/
A brute force approach would be the audit could just strip out the snippet and label in its own details.
There was a problem hiding this comment.
That could work. Audit.makeNodeItem could have an option to replace those values with empty strings.
I'll remove this eliding for now, and we can handle this in a followup PR.
|
Should be good to merge, any last thoughts? |
5 participants
fixes #12923
thumbnaildetail types in all audits withnodeimgand is visible (not a tracking pixel...)EDIT: images are a tad old: now they all show HTML snippets. will hide in some cases in a followup PR. see #13185 (comment)