core(image-elements): ignore invalid images where possible #14295
Conversation
| function mockElement(partial = {}) { | ||
| return { | ||
| src: 'https://example.com/img.png', | ||
| src: partial.src ?? 'https://example.com/img.png', |
There was a problem hiding this comment.
Isn't this what's happening already? Where partial.src overrides this value if provided?
|
This might be going too aggressive, though :/ About 1.6% of sites in HTTP Archive (July 2022) currently have an image LCP which would no longer be detected after this. It is catching a bunch of accidental css files (at least going by filenames), but there also just seem to be a ton of people serving images as |
| imageRecords.push({ | ||
| ...element, | ||
| mimeType: mimeType ? mimeType : URL.guessMimeType(element.src), | ||
| mimeType: networkRecord.mimeType, |
There was a problem hiding this comment.
brendan said:
This might be going too aggressive, though :/ About 1.6% of sites in HTTP Archive (July 2022) currently have an image LCP which would no longer be detected after this. It is catching a bunch of accidental css files (at least going by filenames), but there also just seem to be a ton of people serving images as
application/images,application/png, or good oldapplication/xml.
and then I was like... "i thought we were gonna used resourceType but then see #13338 (comment) where you showed an HTML page getting described as resourceType: Image
i tried to reproduce this locally and failed. but then tried <img src="page.html">. Yeah very interesting.
These resourceTypes are defined in the blink loader and set (for non-scripts) here…
I guess this type is still in the "loader" before it's determined that it's an invalid asset for the intended use.
Ah well. So you're right about resourceType not being useful for this situation.
humph.
Can you produce a list of these faulty mime-types (or is what's listed the vast majority)? As a first pass, without blink instrumentation, we could just add all these bogus things to an allow-list. That said, I'll dig around in Chrome and see where this instrumentation could be added. |
|
caseq informed me that the CDT frontend used to display incongruences in image resources by comparing the resourceType (which is the intent/context) with the mimeType (which is post-sniffing). There's also the raw mime type header available, so we can do both to detect known bads. Need to test what the "post-sniff" CDP mimeType is for a valid image served as |
Yes, but I believe the top mimetype that fails this check is |
Result (for .jpg):
in both cases the image displays, and the |
|
We could move forward with this PR but also introduce a new audit for "hey, all these resources used as images need to better identify themselves" in best practices... If we're really talking about a small subset of misconfigured websites, that seems fine? esp. to avoid the super weird/common-ish |
|
@brendankenny thoughts on #14295 (comment) ? |
ref #13338
This is partial progress towards never surfacing image elements in audits if they are associated with a bogus image (such as:
src=''being surfaced in an image audit).This PR does two things:
1 - ImageRecords
This computed artifact now filters out image elements that are probably not a usable image resource. It uses mimeType to do this, but also has an affordance for avif/webp being sent with a borked mimeType. In the future, we should reach into blink/tracing to get more accurate information here.
A follow up PR would add a new audit that details these likely-problematic images, though that should wait for better instrumentation.
We currently don't have network records available to use in snapshot mode, so this PR does nothing for them.
2 - Using ImageRecords more
There are two audits that presently don't support snapshot mode and weren't already utilizing ImageRecords:
lcp-lazy-loadedpreload-lcp-imageNow they use ImageRecords instead of ImageElements directly.