core(tsc): fix ImageUsage artifact type and gather bug #5113
Conversation
| // Images within `picture` behave strangely and natural size information isn't accurate, | ||
| // CSS images have no natural size information at all. | ||
| // Try to get the actual size if we can. | ||
| const elementPromise = (element.isPicture || element.isCss) && element.networkRecord ? |
There was a problem hiding this comment.
this was the bug. element.networkRecord was never defined at this point
There was a problem hiding this comment.
nice catch! good news is looks like this was introduced just 11 days ago so no one's bumped into it yet :) https://github.com/GoogleChrome/lighthouse/pull/5019/files#diff-a76f9d68fa433ad16daad08121c7411fR182
| // Images within `picture` behave strangely and natural size information isn't accurate, | ||
| // CSS images have no natural size information at all. | ||
| // Try to get the actual size if we can. | ||
| if (element.isPicture || element.isCss) { |
There was a problem hiding this comment.
should we maintain the original concern and only fetch when we can find the network record? fetchElementWithSize is the expensive path
There was a problem hiding this comment.
should we maintain the original concern and only fetch when we can find the network record?
Only uses-responsive-images seems to use naturalWidth/naturalHeight from css/<picture> images, and it requires a networkRecord to work, so makes sense
There was a problem hiding this comment.
when we can find the network record?
...actually this conversation clearly shows that it should have been an optional property...
lol, hadn't looked at the blame yet :) Shows the danger of trying to do a |
|
sorry for the expanding scope, but this should properly reflect the |
| @@ -55,11 +57,15 @@ class OffscreenImages extends ByteEfficiencyAudit { | |||
| } | |||
|
|
|||
| /** | |||
| * @param {!Object} image | |||
There was a problem hiding this comment.
not sure how this slipped by :)
The type for the
ImageUsageartifact accidentally left off the optional imagewidthandheight, which made using it in theimage-aspect-ratioaudit impossible before. Fixed the artifact and removed the@ts-nocheckfrom the audit.In the process, uncovered a bug in the gatherer. At some point shifting types made the extra fetch of size information never run for css or
<picture>images. As a result, thenaturalWidthandnaturalHeightwas being left atNumber.MAX_VALUEfor those and so thewastedPercentfor those images was 100% in theuses-responsive-imagesaudit :)There was a clue in the smoke code coverage, but we hadn't noticed yet :)
Fixed the bug and added more specific expectations for that audit.