core(unsized-images): skip images that are zero-sized#12054
Conversation
patrickhulce
left a comment
There was a problem hiding this comment.
this policy is great, but there's something worse going wrong if these are being flagged because they have an explicit CSS size of 0 already and should be ignored for that reason.
I'm concerned about the deeper issue here :/
If we don't have time to look into it now, maybe we file an issue but save this repro with what to uncomment?
you mean explicit HTML size of 0, right?
yeah... here's the function which handles zero's oddly.. lighthouse/lighthouse-core/audits/unsized-images.js Lines 42 to 52 in b292ae0 the non-negative bit comes from the spec. the spec also says "If the two attributes are both zero, it indicates that the element is not intended for the user (e.g. it might be a part of a service to count page views)." which is nice. but, I don't really follow why 0 as a width/height attribute size would not be a valid sized attr value. soo.... i'll update this fn to not reject cases where attr value is "0" |
No, I was actually talking about the inline CSS
SG 👍 |
ah true. yeah. well i've fixed the HTML one just now. 😛 small detail about CSS width/height 0.. so with this PR (and whatever change we're about to make to the gatherer (re #11289)) .. it's possible for images where 0/0 is set in inline styles... will make it through to the results.. because we'd only see the 0/0 from fetchSourceRules and soon we're not guaranteed to be doing that on all our images. but... hey i think that's okay. |
| expect(UnsizedImagesAudit.isValidAttr('3,000')).toEqual(false); | ||
| expect(UnsizedImagesAudit.isValidAttr('100.0')).toEqual(false); | ||
| expect(UnsizedImagesAudit.isValidAttr('2/3')).toEqual(false); | ||
| expect(UnsizedImagesAudit.isValidAttr('3,000')).toEqual(true); // interpretted as 3 |
There was a problem hiding this comment.
perhaps the subject of a different future audit :)
Co-authored-by: Patrick Hulce <phulce@google.com>
running kohls i saw these in the unsized-images results..
of course anything that's zero width & height isn't part of the layout (equivalent to display:none) but we hadn't really been checking that.
the boundingRect we have ends up being ideal for this calculation and includes if a parent is hidden or w/e.
sweet.