core(image-elements): make naturalWidth/Height property optional #11703
Conversation
adrianaixba
changed the title
adrianaixba
changed the title
| @@ -196,7 +196,8 @@ describe('Page uses responsive images', () => { | |||
| }); | |||
|
|
|||
| assert.equal(auditResult.items.length, 0); | |||
| assert.equal(auditResult.warnings.length, 1); | |||
| // Images with non valid naturalWidth or naturalHeight are ignored. | |||
| assert.equal(auditResult.warnings.length, 0); | |||
There was a problem hiding this comment.
Since we intend to skip the images with invalid width/height, would we still need the warnings property of the uses-responsive-images.js audit? Is it reasonable to remove it?
There was a problem hiding this comment.
or include the invalid images to the warnings object? 👀
There was a problem hiding this comment.
If the warning ever triggers it would be a bug, so I would be in favor of removing the warning but still capturing in sentry.
image-aspect-ratio has a similar warning.
There was a problem hiding this comment.
I'm not sure there's ever been complete alignment on what we should warn vs. skip going back several years, but I'm also +1 to removing the warnings here :)
I'm of the mindset that the warning information in cases like these is not actionable and of little value to the user, so given the current prominence we provide warnings I am against keeping them as warnings.
I see the value in something that lets a developer know that something weird happened during Lighthouse run (like the errors/warnings sidebar design) but until that day comes, I think it's mostly just noise and should be avoided unless there's actually an action for the developer to take (like the crossorigin warnings we have).
lighthouse-core/audits/byte-efficiency/uses-optimized-images.js
Outdated
Show resolved
Hide resolved
| @@ -196,7 +196,8 @@ describe('Page uses responsive images', () => { | |||
| }); | |||
|
|
|||
| assert.equal(auditResult.items.length, 0); | |||
| assert.equal(auditResult.warnings.length, 1); | |||
| // Images with non valid naturalWidth or naturalHeight are ignored. | |||
| assert.equal(auditResult.warnings.length, 0); | |||
There was a problem hiding this comment.
If the warning ever triggers it would be a bug, so I would be in favor of removing the warning but still capturing in sentry.
image-aspect-ratio has a similar warning.
lighthouse-core/audits/byte-efficiency/uses-optimized-images.js
Outdated
Show resolved
Hide resolved
lighthouse-core/audits/byte-efficiency/uses-responsive-images.js
Outdated
Show resolved
Hide resolved
lighthouse-core/audits/byte-efficiency/uses-responsive-images.js
Outdated
Show resolved
Hide resolved
lighthouse-core/audits/byte-efficiency/uses-optimized-images.js
Outdated
Show resolved
Hide resolved
Second checkbox of #11642
Makes naturalWidth/Height property optional. Allows for these properties to be set only when sure.