core(unsized-images): skip images that are zero-sized (#12054)

GoogleChrome · Feb 10, 2021 · 86a5f08 · 86a5f08
1 parent c6e14d7
commit 86a5f08
Show file tree

Hide file tree

Showing 2 changed files with 66 additions and 19 deletions.
diff --git a/lighthouse-core/audits/unsized-images.js b/lighthouse-core/audits/unsized-images.js
@@ -40,15 +40,23 @@ class UnsizedImages extends Audit {
   }
 
   /**
-   * An img size attribute is valid for preventing CLS
-   * if it is a non-negative, non-zero integer.
-   * @param {string} attr
+   * An img size attribute is valid if it is a non-negative integer (incl zero!).
+   * @url https://html.spec.whatwg.org/multipage/embedded-content-other.html#dimension-attributes
+   * @param {string} attrValue
    * @return {boolean}
    */
-  static isValidAttr(attr) {
-    const NON_NEGATIVE_INT_REGEX = /^\d+$/;
-    const ZERO_REGEX = /^0+$/;
-    return NON_NEGATIVE_INT_REGEX.test(attr) && !ZERO_REGEX.test(attr);
+  static isValidAttr(attrValue) {
+    // First, superweird edge case of using the positive-sign. The spec _sorta_ says it's valid...
+    // https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#rules-for-parsing-integers
+    //   > Otherwise, if the character is … (+): Advance position to the next character.
+    //   > (The "+" is ignored, but it is not conforming.)
+    // lol.  Irrelevant though b/c Chrome (at least) doesn't ignore. It rejects this as a non-conforming value.
+    if (attrValue.startsWith('+')) return false;
+
+    // parseInt isn't exactly the same as the html's spec for parsing integers, but it's close enough
+    // https://tc39.es/ecma262/#sec-parseint-string-radix
+    const int = parseInt(attrValue, 10);
+    return int >= 0;
   }
 
   /**
@@ -91,14 +99,21 @@ class UnsizedImages extends Audit {
     const unsizedImages = [];
 
     for (const image of images) {
+      // Fixed images are out of document flow and won't cause layout shifts
       const isFixedImage =
         image.cssComputedPosition === 'fixed' || image.cssComputedPosition === 'absolute';
+      if (isFixedImage) continue;
+
+      // The image was sized with HTML or CSS. Good job.
+      if (UnsizedImages.isSizedImage(image)) continue;
 
-      if (isFixedImage || UnsizedImages.isSizedImage(image)) continue;
-      const url = URL.elideDataURI(image.src);
+      // Images with a 0-size bounding rect (due to hidden parent) aren't part of layout. Cool.
+      const boundingRect = image.node.boundingRect;
+      const isNotDisplayed = boundingRect.width === 0 && boundingRect.height === 0;
+      if (isNotDisplayed) continue;
 
       unsizedImages.push({
-        url,
+        url: URL.elideDataURI(image.src),
         node: Audit.makeNodeItem(image.node),
       });
     }

diff --git a/lighthouse-core/test/audits/unsized-images-test.js b/lighthouse-core/test/audits/unsized-images-test.js
@@ -10,7 +10,7 @@ const UnsizedImagesAudit = require('../../audits/unsized-images.js');
 /* eslint-env jest */
 
 function generateImage(props, src = 'https://google.com/logo.png', isCss = false,
-  isInShadowDOM = false, cssComputedPosition = 'static', node = {}) {
+  isInShadowDOM = false, cssComputedPosition = 'static', node = {boundingRect: {}}) {
   const image = {src, isCss, isInShadowDOM, cssComputedPosition, node};
   Object.assign(image, props);
   return image;
@@ -234,6 +234,38 @@ describe('Sized images audit', () => {
       });
       expect(result.score).toEqual(1);
     });
+
+    it('passes when an image has attribute width/height of zero', async () => {
+      const result = await runAudit({
+        attributeWidth: '0',
+        attributeHeight: '0',
+        cssWidth: '',
+        cssHeight: '',
+        node: {
+          boundingRect: {
+            width: 0,
+            height: 0,
+          },
+        },
+      });
+      expect(result.score).toEqual(1);
+    });
+
+    it('passes when an image is unsized, but its parent is not displayed', async () => {
+      const result = await runAudit({
+        attributeWidth: '',
+        attributeHeight: '',
+        cssWidth: '',
+        cssHeight: '',
+        node: {
+          boundingRect: {
+            width: 0,
+            height: 0,
+          },
+        },
+      });
+      expect(result.score).toEqual(1);
+    });
   });
 
   describe('has invalid width', () => {
@@ -401,19 +433,19 @@ describe('Size attribute validity check', () => {
     expect(UnsizedImagesAudit.isValidAttr('')).toEqual(false);
   });
 
-  it('fails on non-numeric characters', () => {
+  it('handles non-numeric edgecases', () => {
     expect(UnsizedImagesAudit.isValidAttr('zero')).toEqual(false);
-    expect(UnsizedImagesAudit.isValidAttr('1002$')).toEqual(false);
+    expect(UnsizedImagesAudit.isValidAttr('1002$')).toEqual(true); // interpretted as 1002
     expect(UnsizedImagesAudit.isValidAttr('s-5')).toEqual(false);
-    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
+    expect(UnsizedImagesAudit.isValidAttr('100.0')).toEqual(true); // interpretted as 100
+    expect(UnsizedImagesAudit.isValidAttr('2/3')).toEqual(true); // interpretted as 2
     expect(UnsizedImagesAudit.isValidAttr('-2020')).toEqual(false);
-    expect(UnsizedImagesAudit.isValidAttr('+2020')).toEqual(false);
+    expect(UnsizedImagesAudit.isValidAttr('+2020')).toEqual(false); // see isValidAttr comments about positive-sign
   });
 
-  it('fails on zero input', () => {
-    expect(UnsizedImagesAudit.isValidAttr('0')).toEqual(false);
+  it('passes on zero input', () => {
+    expect(UnsizedImagesAudit.isValidAttr('0')).toEqual(true);
   });
 
   it('passes on non-zero non-negative integer input', () => {