From f6f8069de865432935c41b509f0e4050d621755b Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Thu, 4 Feb 2021 20:16:26 -0800 Subject: [PATCH 01/22] core(image-elements): speed up by only fetching CSS rules for largest 50 images --- lighthouse-core/gather/gatherers/image-elements.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index cb2e42364bb5..bd596b4daeaf 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -249,6 +249,10 @@ class ImageElements extends Gatherer { } /** + * Images might be sized via CSS. In order to compute unsized-images failures, we need to collect + * matched CSS rules to see if this is the case. + * Perf warning: Running this for 700 elements takes 1s to 5s. + * @url http://go/dwoqq (googlers only) * @param {Driver} driver * @param {string} devtoolsNodePath * @param {LH.Artifacts.ImageElement} element @@ -322,7 +326,8 @@ class ImageElements extends Gatherer { const networkRecord = indexedNetworkRecords[element.src] || {}; element.mimeType = networkRecord.mimeType; - if (!element.isInShadowDOM && !element.isCss) { + // Need source rules to determine if sized via CSS (for unsized-images). + if (!element.isInShadowDOM && !element.isCss && top50Images.includes(networkRecord)) { await this.fetchSourceRules(driver, element.node.devtoolsNodePath, element); } // Images within `picture` behave strangely and natural size information isn't accurate, From 7c6f6eae894f71ac77675352aa3f6f6da1abe18c Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Mon, 8 Feb 2021 13:29:42 -0800 Subject: [PATCH 02/22] create a time budget --- .../gather/gatherers/image-elements.js | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index bd596b4daeaf..d31cdedb6845 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -312,31 +312,39 @@ class ImageElements extends Gatherer { /** @type {Array} */ const imageUsage = []; - const top50Images = Object.values(indexedNetworkRecords) - .sort((a, b) => b.resourceSize - a.resourceSize) - .slice(0, 50); + await Promise.all([ driver.sendCommand('DOM.enable'), driver.sendCommand('CSS.enable'), driver.sendCommand('DOM.getDocument', {depth: -1, pierce: true}), ]); + // Sort (in-place) as largest images descending + elements.sort((a, b) => { + const aRecord = indexedNetworkRecords[a.src] || {}; + const bRecord = indexedNetworkRecords[b.src] || {}; + return bRecord.resourceSize - aRecord.resourceSize; + }); + + // Don't allow more than 3s of this expensive protocol work + let hasBudget = true; + setTimeout(_ => (hasBudget = false), 3000); + for (let element of elements) { // Pull some of our information directly off the network record. const networkRecord = indexedNetworkRecords[element.src] || {}; element.mimeType = networkRecord.mimeType; // Need source rules to determine if sized via CSS (for unsized-images). - if (!element.isInShadowDOM && !element.isCss && top50Images.includes(networkRecord)) { + if (!element.isInShadowDOM && !element.isCss && hasBudget) { await this.fetchSourceRules(driver, element.node.devtoolsNodePath, element); } // 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. - // Additional fetch is expensive; don't bother if we don't have a networkRecord for the image, - // or it's not in the top 50 largest images. + // Additional fetch is expensive; don't bother if we don't have a networkRecord for the image if ( (element.isPicture || element.isCss || element.srcset) && - top50Images.includes(networkRecord) + networkRecord && hasBudget ) { element = await this.fetchElementWithSizeInformation(driver, element); } From f94f85134bd7c9435ff2282eacf63a82157c405d Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Tue, 9 Feb 2021 16:48:21 -0800 Subject: [PATCH 03/22] Update lighthouse-core/gather/gatherers/image-elements.js Co-authored-by: Connor Clark --- lighthouse-core/gather/gatherers/image-elements.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index d31cdedb6845..3af4db68cd57 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -341,7 +341,7 @@ class ImageElements extends Gatherer { } // 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. - // Additional fetch is expensive; don't bother if we don't have a networkRecord for the image + // Additional fetch is expensive; don't bother if we don't have a networkRecord for the image. if ( (element.isPicture || element.isCss || element.srcset) && networkRecord && hasBudget From a6b5ec2abf1128be2fb23a205c6cecd71e984e15 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Wed, 10 Feb 2021 11:11:43 -0800 Subject: [PATCH 04/22] comment and rename from feedback --- lighthouse-core/gather/gatherers/image-elements.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index 3af4db68cd57..49562755d9d9 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -251,7 +251,6 @@ class ImageElements extends Gatherer { /** * Images might be sized via CSS. In order to compute unsized-images failures, we need to collect * matched CSS rules to see if this is the case. - * Perf warning: Running this for 700 elements takes 1s to 5s. * @url http://go/dwoqq (googlers only) * @param {Driver} driver * @param {string} devtoolsNodePath @@ -326,9 +325,9 @@ class ImageElements extends Gatherer { return bRecord.resourceSize - aRecord.resourceSize; }); - // Don't allow more than 3s of this expensive protocol work - let hasBudget = true; - setTimeout(_ => (hasBudget = false), 3000); + // Don't do more than 3s of this expensive devtools protocol work. See #11289 + let hasRemainingCDPBudget = true; + setTimeout(_ => (hasRemainingCDPBudget = false), 3000); for (let element of elements) { // Pull some of our information directly off the network record. @@ -336,7 +335,7 @@ class ImageElements extends Gatherer { element.mimeType = networkRecord.mimeType; // Need source rules to determine if sized via CSS (for unsized-images). - if (!element.isInShadowDOM && !element.isCss && hasBudget) { + if (!element.isInShadowDOM && !element.isCss && hasRemainingCDPBudget) { await this.fetchSourceRules(driver, element.node.devtoolsNodePath, element); } // Images within `picture` behave strangely and natural size information isn't accurate, @@ -344,7 +343,7 @@ class ImageElements extends Gatherer { // Additional fetch is expensive; don't bother if we don't have a networkRecord for the image. if ( (element.isPicture || element.isCss || element.srcset) && - networkRecord && hasBudget + networkRecord && hasRemainingCDPBudget ) { element = await this.fetchElementWithSizeInformation(driver, element); } From b07eb63af41aed187262d9e377eb79adaf450ccc Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Wed, 10 Feb 2021 11:41:39 -0800 Subject: [PATCH 05/22] invert name and check once --- lighthouse-core/gather/gatherers/image-elements.js | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index 49562755d9d9..d0820bf0233d 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -326,25 +326,24 @@ class ImageElements extends Gatherer { }); // Don't do more than 3s of this expensive devtools protocol work. See #11289 - let hasRemainingCDPBudget = true; - setTimeout(_ => (hasRemainingCDPBudget = false), 3000); + let reachedGatheringBudget = false; + setTimeout(_ => (reachedGatheringBudget = true), 3000); for (let element of elements) { // Pull some of our information directly off the network record. const networkRecord = indexedNetworkRecords[element.src] || {}; element.mimeType = networkRecord.mimeType; + if (reachedGatheringBudget) continue; + // Need source rules to determine if sized via CSS (for unsized-images). - if (!element.isInShadowDOM && !element.isCss && hasRemainingCDPBudget) { + if (!element.isInShadowDOM && !element.isCss) { await this.fetchSourceRules(driver, element.node.devtoolsNodePath, element); } // 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. // Additional fetch is expensive; don't bother if we don't have a networkRecord for the image. - if ( - (element.isPicture || element.isCss || element.srcset) && - networkRecord && hasRemainingCDPBudget - ) { + if ((element.isPicture || element.isCss || element.srcset) && networkRecord) { element = await this.fetchElementWithSizeInformation(driver, element); } From 6e00733478948981d6ee501f6e6891108b0666f4 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Wed, 10 Feb 2021 11:50:53 -0800 Subject: [PATCH 06/22] add log message on reachedGatheringBudget --- .../gather/gatherers/image-elements.js | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index d0820bf0233d..4374d9baff08 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -9,6 +9,7 @@ */ 'use strict'; +const log = require('lighthouse-logger'); const Gatherer = require('./gatherer.js'); const pageFunctions = require('../../lib/page-functions.js'); const Driver = require('../driver.js'); // eslint-disable-line no-unused-vars @@ -309,9 +310,6 @@ class ImageElements extends Gatherer { ], }); - /** @type {Array} */ - const imageUsage = []; - await Promise.all([ driver.sendCommand('DOM.enable'), driver.sendCommand('CSS.enable'), @@ -328,13 +326,17 @@ class ImageElements extends Gatherer { // Don't do more than 3s of this expensive devtools protocol work. See #11289 let reachedGatheringBudget = false; setTimeout(_ => (reachedGatheringBudget = true), 3000); + let skippedCount = 0; for (let element of elements) { // Pull some of our information directly off the network record. const networkRecord = indexedNetworkRecords[element.src] || {}; element.mimeType = networkRecord.mimeType; - if (reachedGatheringBudget) continue; + if (reachedGatheringBudget) { + skippedCount++; + continue; + } // Need source rules to determine if sized via CSS (for unsized-images). if (!element.isInShadowDOM && !element.isCss) { @@ -347,7 +349,10 @@ class ImageElements extends Gatherer { element = await this.fetchElementWithSizeInformation(driver, element); } - imageUsage.push(element); + } + + if (reachedGatheringBudget) { + log.warn('ImageElements', `Reached gathering budget of 3s. Skipped extra details for ${skippedCount}/${elements.length}`); } await Promise.all([ @@ -355,7 +360,7 @@ class ImageElements extends Gatherer { driver.sendCommand('CSS.disable'), ]); - return imageUsage; + return elements; } } From 6908ee2a40ab29312bdaa64c73ee6f8f907d5996 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Wed, 10 Feb 2021 12:00:12 -0800 Subject: [PATCH 07/22] return null --- lighthouse-core/audits/unsized-images.js | 2 +- lighthouse-core/gather/gatherers/image-elements.js | 4 +++- types/artifacts.d.ts | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lighthouse-core/audits/unsized-images.js b/lighthouse-core/audits/unsized-images.js index a953765cbfaa..b71bc4605fd5 100644 --- a/lighthouse-core/audits/unsized-images.js +++ b/lighthouse-core/audits/unsized-images.js @@ -54,7 +54,7 @@ class UnsizedImages extends Audit { /** * An img css size property is valid for preventing CLS * if it is defined, not empty, and not equal to 'auto'. - * @param {string | undefined} property + * @param {LH.Artifacts.ImageElement['cssHeight']} property * @return {boolean} */ static isValidCss(property) { diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index 4374d9baff08..9b12b49607be 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -200,7 +200,7 @@ function findMostSpecificCSSRule(matchedCSSRules, property) { /** * @param {LH.Crdp.CSS.GetMatchedStylesForNodeResponse} matched CSS rules} * @param {string} property - * @returns {string | undefined} + * @returns {string | null} */ function getEffectiveSizingRule({attributesStyle, inlineStyle, matchedCSSRules}, property) { // CSS sizing can't be inherited. @@ -215,6 +215,8 @@ function getEffectiveSizingRule({attributesStyle, inlineStyle, matchedCSSRules}, // Rules directly referencing the node come next. const matchedRule = findMostSpecificCSSRule(matchedCSSRules, property); if (matchedRule) return matchedRule; + + return null; } class ImageElements extends Gatherer { diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 4bc7df0437b0..66a5b8cfa8b6 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -410,9 +410,9 @@ declare global { /** The raw height attribute of the image element. CSS images will be set to the empty string. */ attributeHeight: string; /** The CSS width property of the image element. */ - cssWidth?: string; + cssWidth?: string | null; /** The CSS height property of the image element. */ - cssHeight?: string; + cssHeight?: string | null; /** The BoundingClientRect of the element. */ clientRect: { top: number; From 8e411dd14f6a3e590783ab3250b206275fa39384 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Wed, 10 Feb 2021 12:42:21 -0800 Subject: [PATCH 08/22] rename these audit methods. its not about VALIDITY but SIZEDEDNESS --- lighthouse-core/audits/unsized-images.js | 23 +++--- .../test/audits/unsized-images-test.js | 74 +++++++++---------- 2 files changed, 48 insertions(+), 49 deletions(-) diff --git a/lighthouse-core/audits/unsized-images.js b/lighthouse-core/audits/unsized-images.js index fc8c627afc1b..990347af820a 100644 --- a/lighthouse-core/audits/unsized-images.js +++ b/lighthouse-core/audits/unsized-images.js @@ -40,12 +40,12 @@ class UnsizedImages extends Audit { } /** - * An img size attribute is valid if it is a non-negative integer (incl zero!). + * An img size attribute prevents CLS 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(attrValue) { + static doesHtmlAttrProvideExplicitSize(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. @@ -60,12 +60,11 @@ class UnsizedImages extends Audit { } /** - * An img css size property is valid for preventing CLS - * if it is defined, not empty, and not equal to 'auto'. + * An img css size property prevents CLS if it is defined, not empty, and not equal to 'auto'. * @param {LH.Artifacts.ImageElement['cssHeight']} property * @return {boolean} */ - static isValidCss(property) { + static doesCssPropProvideExplicitSize(property) { if (!property) return false; return property !== 'auto'; } @@ -80,13 +79,13 @@ class UnsizedImages extends Audit { const attrHeight = image.attributeHeight; const cssWidth = image.cssWidth; const cssHeight = image.cssHeight; - const widthIsValidAttribute = UnsizedImages.isValidAttr(attrWidth); - const widthIsValidCss = UnsizedImages.isValidCss(cssWidth); - const heightIsValidAttribute = UnsizedImages.isValidAttr(attrHeight); - const heightIsValidCss = UnsizedImages.isValidCss(cssHeight); - const validWidth = widthIsValidAttribute || widthIsValidCss; - const validHeight = heightIsValidAttribute || heightIsValidCss; - return validWidth && validHeight; + const htmlWidthIsExplicit = UnsizedImages.doesHtmlAttrProvideExplicitSize(attrWidth); + const cssWidthIsExplicit = UnsizedImages.doesCssPropProvideExplicitSize(cssWidth); + const htmlHeightIsExplicit = UnsizedImages.doesHtmlAttrProvideExplicitSize(attrHeight); + const cssHeightIsExplicit = UnsizedImages.doesCssPropProvideExplicitSize(cssHeight); + const explicitWidth = htmlWidthIsExplicit || cssWidthIsExplicit; + const explicitHeight = htmlHeightIsExplicit || cssHeightIsExplicit; + return explicitWidth && explicitHeight; } /** diff --git a/lighthouse-core/test/audits/unsized-images-test.js b/lighthouse-core/test/audits/unsized-images-test.js index 88afbe5415be..c6a6df5fdffe 100644 --- a/lighthouse-core/test/audits/unsized-images-test.js +++ b/lighthouse-core/test/audits/unsized-images-test.js @@ -144,7 +144,7 @@ describe('Sized images audit', () => { expect(result.score).toEqual(0); }); - describe('has valid width and height', () => { + describe('has explicit width and height', () => { it('passes when an image has attribute width and css height', async () => { const result = await runAudit({ attributeWidth: '100', @@ -268,7 +268,7 @@ describe('Sized images audit', () => { }); }); - describe('has invalid width', () => { + describe('has invalid or non-explicit width', () => { it('fails when an image has invalid width attribute', async () => { const result = await runAudit({ attributeWidth: '-200', @@ -289,7 +289,7 @@ describe('Sized images audit', () => { expect(result.score).toEqual(0); }); - it('fails when an image has invalid css width', async () => { + it('fails when an image has non-explicit css width', async () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', @@ -299,7 +299,7 @@ describe('Sized images audit', () => { expect(result.score).toEqual(0); }); - it('fails when an image has invalid css height', async () => { + it('fails when an image has non-explicit css height', async () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', @@ -309,7 +309,7 @@ describe('Sized images audit', () => { expect(result.score).toEqual(0); }); - it('passes when an image has invalid width attribute, and valid css width', async () => { + it('passes when an image has invalid width attribute, and explicit css width', async () => { const result = await runAudit({ attributeWidth: '-200', attributeHeight: '100', @@ -329,7 +329,7 @@ describe('Sized images audit', () => { expect(result.score).toEqual(1); }); - it('passes when an image has invalid css width, and valid attribute width', async () => { + it('passes when an image has nonexplicit css width, and valid attribute width', async () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '', @@ -339,7 +339,7 @@ describe('Sized images audit', () => { expect(result.score).toEqual(1); }); - it('passes when an image has invalid css height, and valid attribute height', async () => { + it('passes when an image has nonexplicit css height, and valid attribute height', async () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '100', @@ -349,7 +349,7 @@ describe('Sized images audit', () => { expect(result.score).toEqual(1); }); - it('passes when an image has invalid css width & height, and valid attribute width & height', + it('passes when an image has nonexplicit css width & height, and valid attribute width & height', async () => { const result = await runAudit({ attributeWidth: '100', @@ -371,7 +371,7 @@ describe('Sized images audit', () => { expect(result.score).toEqual(1); }); - it('fails when an image has invalid attribute width & height, and invalid css width & height', + it('fails when an image has invalid attribute width & height, and nonexplicit css width & height', async () => { const result = await runAudit({ attributeWidth: '-200', @@ -430,55 +430,55 @@ describe('Sized images audit', () => { describe('Size attribute validity check', () => { it('fails if it is empty', () => { - expect(UnsizedImagesAudit.isValidAttr('')).toEqual(false); + expect(UnsizedImagesAudit.doesHtmlAttrProvideExplicitSize('')).toEqual(false); }); it('handles non-numeric edgecases', () => { - expect(UnsizedImagesAudit.isValidAttr('zero')).toEqual(false); - expect(UnsizedImagesAudit.isValidAttr('1002$')).toEqual(true); // interpretted as 1002 - expect(UnsizedImagesAudit.isValidAttr('s-5')).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); // see isValidAttr comments about positive-sign + expect(UnsizedImagesAudit.doesHtmlAttrProvideExplicitSize('zero')).toEqual(false); + expect(UnsizedImagesAudit.doesHtmlAttrProvideExplicitSize('1002$')).toEqual(true); // interpretted as 1002 + expect(UnsizedImagesAudit.doesHtmlAttrProvideExplicitSize('s-5')).toEqual(false); + expect(UnsizedImagesAudit.doesHtmlAttrProvideExplicitSize('3,000')).toEqual(true); // interpretted as 3 + expect(UnsizedImagesAudit.doesHtmlAttrProvideExplicitSize('100.0')).toEqual(true); // interpretted as 100 + expect(UnsizedImagesAudit.doesHtmlAttrProvideExplicitSize('2/3')).toEqual(true); // interpretted as 2 + expect(UnsizedImagesAudit.doesHtmlAttrProvideExplicitSize('-2020')).toEqual(false); + expect(UnsizedImagesAudit.doesHtmlAttrProvideExplicitSize('+2020')).toEqual(false); // see doesHtmlAttrProvideExplicitSize comments about positive-sign }); it('passes on zero input', () => { - expect(UnsizedImagesAudit.isValidAttr('0')).toEqual(true); + expect(UnsizedImagesAudit.doesHtmlAttrProvideExplicitSize('0')).toEqual(true); }); it('passes on non-zero non-negative integer input', () => { - expect(UnsizedImagesAudit.isValidAttr('1')).toEqual(true); - expect(UnsizedImagesAudit.isValidAttr('250')).toEqual(true); - expect(UnsizedImagesAudit.isValidAttr('4000000')).toEqual(true); + expect(UnsizedImagesAudit.doesHtmlAttrProvideExplicitSize('1')).toEqual(true); + expect(UnsizedImagesAudit.doesHtmlAttrProvideExplicitSize('250')).toEqual(true); + expect(UnsizedImagesAudit.doesHtmlAttrProvideExplicitSize('4000000')).toEqual(true); }); }); -describe('CSS size property validity check', () => { +describe('CSS size property sized check', () => { it('fails if it was never defined', () => { - expect(UnsizedImagesAudit.isValidCss(undefined)).toEqual(false); + expect(UnsizedImagesAudit.doesCssPropProvideExplicitSize(undefined)).toEqual(false); }); it('fails if it is empty', () => { - expect(UnsizedImagesAudit.isValidCss('')).toEqual(false); + expect(UnsizedImagesAudit.doesCssPropProvideExplicitSize('')).toEqual(false); }); it('fails if it is auto', () => { - expect(UnsizedImagesAudit.isValidCss('auto')).toEqual(false); + expect(UnsizedImagesAudit.doesCssPropProvideExplicitSize('auto')).toEqual(false); }); it('passes if it is defined and not auto', () => { - expect(UnsizedImagesAudit.isValidCss('200')).toEqual(true); - expect(UnsizedImagesAudit.isValidCss('300.5')).toEqual(true); - expect(UnsizedImagesAudit.isValidCss('150px')).toEqual(true); - expect(UnsizedImagesAudit.isValidCss('80%')).toEqual(true); - expect(UnsizedImagesAudit.isValidCss('5cm')).toEqual(true); - expect(UnsizedImagesAudit.isValidCss('20rem')).toEqual(true); - expect(UnsizedImagesAudit.isValidCss('7vw')).toEqual(true); - expect(UnsizedImagesAudit.isValidCss('-20')).toEqual(true); - expect(UnsizedImagesAudit.isValidCss('0')).toEqual(true); - expect(UnsizedImagesAudit.isValidCss('three')).toEqual(true); - expect(UnsizedImagesAudit.isValidCss('-20')).toEqual(true); + expect(UnsizedImagesAudit.doesCssPropProvideExplicitSize('200')).toEqual(true); + expect(UnsizedImagesAudit.doesCssPropProvideExplicitSize('300.5')).toEqual(true); + expect(UnsizedImagesAudit.doesCssPropProvideExplicitSize('150px')).toEqual(true); + expect(UnsizedImagesAudit.doesCssPropProvideExplicitSize('80%')).toEqual(true); + expect(UnsizedImagesAudit.doesCssPropProvideExplicitSize('5cm')).toEqual(true); + expect(UnsizedImagesAudit.doesCssPropProvideExplicitSize('20rem')).toEqual(true); + expect(UnsizedImagesAudit.doesCssPropProvideExplicitSize('7vw')).toEqual(true); + expect(UnsizedImagesAudit.doesCssPropProvideExplicitSize('-20')).toEqual(true); + expect(UnsizedImagesAudit.doesCssPropProvideExplicitSize('0')).toEqual(true); + expect(UnsizedImagesAudit.doesCssPropProvideExplicitSize('three')).toEqual(true); + expect(UnsizedImagesAudit.doesCssPropProvideExplicitSize('-20')).toEqual(true); }); }); From 0fbc7d4ca7500b5198ffcbf0ffa62a2477b438a5 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Wed, 10 Feb 2021 13:11:11 -0800 Subject: [PATCH 09/22] handle the data-not-gathered case --- lighthouse-core/audits/unsized-images.js | 7 +++++++ .../test/audits/unsized-images-test.js | 18 ++++++++++++++++-- types/artifacts.d.ts | 4 ++-- 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/lighthouse-core/audits/unsized-images.js b/lighthouse-core/audits/unsized-images.js index 990347af820a..d1a8e598a80d 100644 --- a/lighthouse-core/audits/unsized-images.js +++ b/lighthouse-core/audits/unsized-images.js @@ -103,6 +103,13 @@ class UnsizedImages extends Audit { image.cssComputedPosition === 'fixed' || image.cssComputedPosition === 'absolute'; if (isFixedImage) continue; + // Perhaps we hit reachedGatheringBudget before collecting this image's cssWidth/Height + // in fetchSourceRules. In this case, we don't have enough information to determine if it's sized. + // We don't want to show the user a false positive, so we'll skip it. + // While this situation should only befall small-impact images, it means our analysis is incomplete. :( + // Handwavey TODO: explore ways to avoid this. + if (image.cssWidth === undefined || image.cssHeight === undefined) continue; + // The image was sized with HTML or CSS. Good job. if (UnsizedImages.isSizedImage(image)) continue; diff --git a/lighthouse-core/test/audits/unsized-images-test.js b/lighthouse-core/test/audits/unsized-images-test.js index c6a6df5fdffe..807ec3d08a08 100644 --- a/lighthouse-core/test/audits/unsized-images-test.js +++ b/lighthouse-core/test/audits/unsized-images-test.js @@ -426,9 +426,23 @@ describe('Sized images audit', () => { const srcs = result.details.items.map(item => item.url); expect(srcs).toEqual(['image1.png', 'image3.png']); }); + + describe('doesn\'t have enough data', () => { + // https://github.com/GoogleChrome/lighthouse/pull/12065#discussion_r573090652 + it('passes because we didnt gather the data we need to be conclusive', async () => { + const result = await runAudit({ + attributeWidth: '', + attributeHeight: '', + cssWidth: undefined, + cssHeight: undefined, + }); + expect(result.details.items.length).toEqual(0); + expect(result.score).toEqual(1); + }); + }); }); -describe('Size attribute validity check', () => { +describe('html attribute sized check', () => { it('fails if it is empty', () => { expect(UnsizedImagesAudit.doesHtmlAttrProvideExplicitSize('')).toEqual(false); }); @@ -455,7 +469,7 @@ describe('Size attribute validity check', () => { }); }); -describe('CSS size property sized check', () => { +describe('CSS property sized check', () => { it('fails if it was never defined', () => { expect(UnsizedImagesAudit.doesCssPropProvideExplicitSize(undefined)).toEqual(false); }); diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index ce1b5da37cea..add80879d249 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -409,9 +409,9 @@ declare global { attributeWidth: string; /** The raw height attribute of the image element. CSS images will be set to the empty string. */ attributeHeight: string; - /** The CSS width property of the image element. */ + /** The CSS width property of the image element. Set to `undefined` if Lighthouse didn't gather the data. Set to `null` if it was gathered, but there was no width set in CSS. */ cssWidth?: string | null; - /** The CSS height property of the image element. */ + /** The CSS height property of the image element. Set to `undefined` if Lighthouse didn't gather the data. Set to `null` if it was gathered, but there was no height set in CSS. */ cssHeight?: string | null; /** The BoundingClientRect of the element. */ clientRect: { From 6bfb5eae612a0afaa0d7dee5122ec5a65261d12d Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Wed, 10 Feb 2021 13:27:16 -0800 Subject: [PATCH 10/22] cssWidth/height as empty string never made sense. previously it was undefined. but anyway now its explicitly null. yay --- .../test/audits/unsized-images-test.js | 89 +++++++++++-------- 1 file changed, 50 insertions(+), 39 deletions(-) diff --git a/lighthouse-core/test/audits/unsized-images-test.js b/lighthouse-core/test/audits/unsized-images-test.js index 807ec3d08a08..116844b9d691 100644 --- a/lighthouse-core/test/audits/unsized-images-test.js +++ b/lighthouse-core/test/audits/unsized-images-test.js @@ -31,8 +31,8 @@ describe('Sized images audit', () => { isCss: true, attributeWidth: '', attributeHeight: '', - cssWidth: '', - cssHeight: '', + cssWidth: null, + cssHeight: null, }); expect(result.score).toEqual(1); }); @@ -42,8 +42,8 @@ describe('Sized images audit', () => { isInShadowDOM: true, attributeWidth: '', attributeHeight: '', - cssWidth: '', - cssHeight: '', + cssWidth: null, + cssHeight: null, }); expect(result.score).toEqual(1); }); @@ -53,8 +53,8 @@ describe('Sized images audit', () => { cssComputedPosition: 'absolute', attributeWidth: '', attributeHeight: '', - cssWidth: '', - cssHeight: '', + cssWidth: null, + cssHeight: null, }); expect(result.score).toEqual(1); }); @@ -64,8 +64,8 @@ describe('Sized images audit', () => { cssComputedPosition: 'fixed', attributeWidth: '', attributeHeight: '', - cssWidth: '', - cssHeight: '', + cssWidth: null, + cssHeight: null, }); expect(result.score).toEqual(1); }); @@ -75,8 +75,8 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '100', - cssWidth: '', - cssHeight: '', + cssWidth: null, + cssHeight: null, }); expect(result.score).toEqual(0); }); @@ -85,7 +85,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssWidth: '', + cssWidth: null, cssHeight: '100', }); expect(result.score).toEqual(0); @@ -95,7 +95,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '100', - cssWidth: '', + cssWidth: null, cssHeight: '100', }); expect(result.score).toEqual(0); @@ -107,8 +107,8 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '', - cssWidth: '', - cssHeight: '', + cssWidth: null, + cssHeight: null, }); expect(result.score).toEqual(0); }); @@ -118,7 +118,7 @@ describe('Sized images audit', () => { attributeWidth: '', attributeHeight: '', cssWidth: '100', - cssHeight: '', + cssHeight: null, }); expect(result.score).toEqual(0); }); @@ -128,7 +128,7 @@ describe('Sized images audit', () => { attributeWidth: '100', attributeHeight: '', cssWidth: '100', - cssHeight: '', + cssHeight: null, }); expect(result.score).toEqual(0); }); @@ -138,8 +138,8 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssWidth: '', - cssHeight: '', + cssWidth: null, + cssHeight: null, }); expect(result.score).toEqual(0); }); @@ -149,7 +149,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '', - cssWidth: '', + cssWidth: null, cssHeight: '100', }); expect(result.score).toEqual(1); @@ -159,8 +159,8 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '100', - cssWidth: '', - cssHeight: '', + cssWidth: null, + cssHeight: null, }); expect(result.score).toEqual(1); }); @@ -170,7 +170,7 @@ describe('Sized images audit', () => { attributeWidth: '', attributeHeight: '100', cssWidth: '100', - cssHeight: '', + cssHeight: null, }); expect(result.score).toEqual(1); }); @@ -200,7 +200,7 @@ describe('Sized images audit', () => { attributeWidth: '100', attributeHeight: '100', cssWidth: '100', - cssHeight: '', + cssHeight: null, }); expect(result.score).toEqual(1); }); @@ -219,7 +219,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '100', - cssWidth: '', + cssWidth: null, cssHeight: '100', }); expect(result.score).toEqual(1); @@ -239,8 +239,8 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '0', attributeHeight: '0', - cssWidth: '', - cssHeight: '', + cssWidth: null, + cssHeight: null, node: { boundingRect: { width: 0, @@ -255,8 +255,8 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssWidth: '', - cssHeight: '', + cssWidth: null, + cssHeight: null, node: { boundingRect: { width: 0, @@ -273,8 +273,8 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '-200', attributeHeight: '100', - cssWidth: '', - cssHeight: '', + cssWidth: null, + cssHeight: null, }); expect(result.score).toEqual(0); }); @@ -283,8 +283,8 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '-200', - cssWidth: '', - cssHeight: '', + cssWidth: null, + cssHeight: null, }); expect(result.score).toEqual(0); }); @@ -314,7 +314,7 @@ describe('Sized images audit', () => { attributeWidth: '-200', attributeHeight: '100', cssWidth: '100', - cssHeight: '', + cssHeight: null, }); expect(result.score).toEqual(1); }); @@ -323,7 +323,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '-200', - cssWidth: '', + cssWidth: null, cssHeight: '100', }); expect(result.score).toEqual(1); @@ -398,8 +398,8 @@ describe('Sized images audit', () => { { attributeWidth: '', attributeHeight: '', - cssWidth: '', - cssHeight: '', + cssWidth: null, + cssHeight: null, }, 'image1.png' ), @@ -414,8 +414,8 @@ describe('Sized images audit', () => { { attributeWidth: '', attributeHeight: '', - cssWidth: '', - cssHeight: '', + cssWidth: null, + cssHeight: null, }, 'image3.png' ), @@ -429,7 +429,7 @@ describe('Sized images audit', () => { describe('doesn\'t have enough data', () => { // https://github.com/GoogleChrome/lighthouse/pull/12065#discussion_r573090652 - it('passes because we didnt gather the data we need to be conclusive', async () => { + it('passes because we didnt gather the data we need to be conclusive', async () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', @@ -439,6 +439,17 @@ describe('Sized images audit', () => { expect(result.details.items.length).toEqual(0); expect(result.score).toEqual(1); }); + + it(`passes because it's html-sized, even we cannot be conclusive about css-sized`, async () => { + const result = await runAudit({ + attributeWidth: '10', + attributeHeight: '10', + cssWidth: undefined, + cssHeight: undefined, + }); + expect(result.details.items.length).toEqual(0); + expect(result.score).toEqual(1); + }); }); }); From 37663b40f74896d1a9938d8b0695164bf6488632 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Wed, 10 Feb 2021 13:29:54 -0800 Subject: [PATCH 11/22] 3s => 5s. lint. --- lighthouse-core/gather/gatherers/image-elements.js | 5 ++--- lighthouse-core/test/audits/unsized-images-test.js | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index b941f026d984..846eeab7ca60 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -327,7 +327,7 @@ class ImageElements extends Gatherer { // Don't do more than 3s of this expensive devtools protocol work. See #11289 let reachedGatheringBudget = false; - setTimeout(_ => (reachedGatheringBudget = true), 3000); + setTimeout(_ => (reachedGatheringBudget = true), 5000); let skippedCount = 0; for (let element of elements) { @@ -350,11 +350,10 @@ class ImageElements extends Gatherer { if ((element.isPicture || element.isCss || element.srcset) && networkRecord) { element = await this.fetchElementWithSizeInformation(driver, element); } - } if (reachedGatheringBudget) { - log.warn('ImageElements', `Reached gathering budget of 3s. Skipped extra details for ${skippedCount}/${elements.length}`); + log.warn('ImageElements', `Reached gathering budget of 5s. Skipped extra details for ${skippedCount}/${elements.length}`); // eslint-disable-line max-len } await Promise.all([ diff --git a/lighthouse-core/test/audits/unsized-images-test.js b/lighthouse-core/test/audits/unsized-images-test.js index 116844b9d691..e87792ea7b25 100644 --- a/lighthouse-core/test/audits/unsized-images-test.js +++ b/lighthouse-core/test/audits/unsized-images-test.js @@ -349,7 +349,7 @@ describe('Sized images audit', () => { expect(result.score).toEqual(1); }); - it('passes when an image has nonexplicit css width & height, and valid attribute width & height', + it('passes when an image has nonexplicit css width & height, and valid attribute width & height', // eslint-disable-line max-len async () => { const result = await runAudit({ attributeWidth: '100', @@ -371,7 +371,7 @@ describe('Sized images audit', () => { expect(result.score).toEqual(1); }); - it('fails when an image has invalid attribute width & height, and nonexplicit css width & height', + it('fails when an image has invalid attribute width & height, and nonexplicit css width & height', // eslint-disable-line max-len async () => { const result = await runAudit({ attributeWidth: '-200', From 9bc2c893140753d82f41ccab4a5d0a69ae7e97ce Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Wed, 10 Feb 2021 13:54:14 -0800 Subject: [PATCH 12/22] a cssSizing object. keep cssWidth/height the same for now --- lighthouse-core/audits/unsized-images.js | 20 +- .../gather/gatherers/image-elements.js | 12 +- .../test/audits/unsized-images-test.js | 216 ++++++++++++------ .../test/results/artifacts/artifacts.json | 44 ++-- types/artifacts.d.ts | 21 +- 5 files changed, 206 insertions(+), 107 deletions(-) diff --git a/lighthouse-core/audits/unsized-images.js b/lighthouse-core/audits/unsized-images.js index d1a8e598a80d..096975963379 100644 --- a/lighthouse-core/audits/unsized-images.js +++ b/lighthouse-core/audits/unsized-images.js @@ -61,7 +61,7 @@ class UnsizedImages extends Audit { /** * An img css size property prevents CLS if it is defined, not empty, and not equal to 'auto'. - * @param {LH.Artifacts.ImageElement['cssHeight']} property + * @param {string | null} property * @return {boolean} */ static doesCssPropProvideExplicitSize(property) { @@ -75,10 +75,17 @@ class UnsizedImages extends Audit { * @return {boolean} */ static isSizedImage(image) { + // Perhaps we hit reachedGatheringBudget before collecting this image's cssWidth/Height + // in fetchSourceRules. In this case, we don't have enough information to determine if it's sized. + // We don't want to show the user a false positive, so we'll call it sized to give it as pass. + // While this situation should only befall small-impact images, it means our analysis is incomplete. :( + // Handwavey TODO: explore ways to avoid this. + if (image.cssSizing === undefined) return true; + const attrWidth = image.attributeWidth; const attrHeight = image.attributeHeight; - const cssWidth = image.cssWidth; - const cssHeight = image.cssHeight; + const cssWidth = image.cssSizing.width; + const cssHeight = image.cssSizing.height; const htmlWidthIsExplicit = UnsizedImages.doesHtmlAttrProvideExplicitSize(attrWidth); const cssWidthIsExplicit = UnsizedImages.doesCssPropProvideExplicitSize(cssWidth); const htmlHeightIsExplicit = UnsizedImages.doesHtmlAttrProvideExplicitSize(attrHeight); @@ -103,13 +110,6 @@ class UnsizedImages extends Audit { image.cssComputedPosition === 'fixed' || image.cssComputedPosition === 'absolute'; if (isFixedImage) continue; - // Perhaps we hit reachedGatheringBudget before collecting this image's cssWidth/Height - // in fetchSourceRules. In this case, we don't have enough information to determine if it's sized. - // We don't want to show the user a false positive, so we'll skip it. - // While this situation should only befall small-impact images, it means our analysis is incomplete. :( - // Handwavey TODO: explore ways to avoid this. - if (image.cssWidth === undefined || image.cssHeight === undefined) continue; - // The image was sized with HTML or CSS. Good job. if (UnsizedImages.isSizedImage(image)) continue; diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index 846eeab7ca60..5c80358c905c 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -75,6 +75,7 @@ function getHTMLImages(allElements) { attributeHeight: element.getAttribute('height') || '', cssWidth: undefined, // this will get overwritten below cssHeight: undefined, // this will get overwritten below + cssSizing: undefined, // this will get overwritten below cssComputedPosition: getPosition(element, computedStyle), isCss: false, isPicture, @@ -121,6 +122,7 @@ function getCSSImages(allElements) { attributeHeight: '', cssWidth: undefined, cssHeight: undefined, + cssSizing: undefined, cssComputedPosition: getPosition(element, style), isCss: true, isPicture: false, @@ -269,10 +271,12 @@ class ImageElements extends Gatherer { const matchedRules = await driver.sendCommand('CSS.getMatchedStylesForNode', { nodeId: nodeId, }); - const sourceWidth = getEffectiveSizingRule(matchedRules, 'width'); - const sourceHeight = getEffectiveSizingRule(matchedRules, 'height'); - const sourceRules = {cssWidth: sourceWidth, cssHeight: sourceHeight}; - Object.assign(element, sourceRules); + const width = getEffectiveSizingRule(matchedRules, 'width'); + const height = getEffectiveSizingRule(matchedRules, 'height'); + // Maintain backcompat for <= 7.0.1 + element.cssWidth = width === null ? undefined : width; + element.cssHeight = height === null ? undefined : height; + Object.assign(element, {cssSizing: {width, height}}); } catch (err) { if (/No node.*found/.test(err.message)) return; throw err; diff --git a/lighthouse-core/test/audits/unsized-images-test.js b/lighthouse-core/test/audits/unsized-images-test.js index e87792ea7b25..dca7fae47f61 100644 --- a/lighthouse-core/test/audits/unsized-images-test.js +++ b/lighthouse-core/test/audits/unsized-images-test.js @@ -31,8 +31,10 @@ describe('Sized images audit', () => { isCss: true, attributeWidth: '', attributeHeight: '', - cssWidth: null, - cssHeight: null, + cssSizing: { + width: null, + height: null, + } }); expect(result.score).toEqual(1); }); @@ -42,8 +44,10 @@ describe('Sized images audit', () => { isInShadowDOM: true, attributeWidth: '', attributeHeight: '', - cssWidth: null, - cssHeight: null, + cssSizing: { + width: null, + height: null, + } }); expect(result.score).toEqual(1); }); @@ -53,8 +57,10 @@ describe('Sized images audit', () => { cssComputedPosition: 'absolute', attributeWidth: '', attributeHeight: '', - cssWidth: null, - cssHeight: null, + cssSizing: { + width: null, + height: null, + } }); expect(result.score).toEqual(1); }); @@ -64,8 +70,10 @@ describe('Sized images audit', () => { cssComputedPosition: 'fixed', attributeWidth: '', attributeHeight: '', - cssWidth: null, - cssHeight: null, + cssSizing: { + width: null, + height: null, + } }); expect(result.score).toEqual(1); }); @@ -75,8 +83,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '100', - cssWidth: null, - cssHeight: null, + cssSizing: { + width: null, + height: null, + } }); expect(result.score).toEqual(0); }); @@ -85,8 +95,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssWidth: null, - cssHeight: '100', + cssSizing: { + width: null, + height: '100', + } }); expect(result.score).toEqual(0); }); @@ -95,8 +107,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '100', - cssWidth: null, - cssHeight: '100', + cssSizing: { + width: null, + height: '100', + } }); expect(result.score).toEqual(0); }); @@ -107,8 +121,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '', - cssWidth: null, - cssHeight: null, + cssSizing: { + width: null, + height: null, + } }); expect(result.score).toEqual(0); }); @@ -117,8 +133,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssWidth: '100', - cssHeight: null, + cssSizing: { + width: '100', + height: null, + } }); expect(result.score).toEqual(0); }); @@ -127,8 +145,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '', - cssWidth: '100', - cssHeight: null, + cssSizing: { + width: '100', + height: null, + } }); expect(result.score).toEqual(0); }); @@ -138,8 +158,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssWidth: null, - cssHeight: null, + cssSizing: { + width: null, + height: null, + } }); expect(result.score).toEqual(0); }); @@ -149,8 +171,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '', - cssWidth: null, - cssHeight: '100', + cssSizing: { + width: null, + height: '100', + } }); expect(result.score).toEqual(1); }); @@ -159,8 +183,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '100', - cssWidth: null, - cssHeight: null, + cssSizing: { + width: null, + height: null, + } }); expect(result.score).toEqual(1); }); @@ -169,8 +195,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '100', - cssWidth: '100', - cssHeight: null, + cssSizing: { + width: '100', + height: null, + } }); expect(result.score).toEqual(1); }); @@ -179,8 +207,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssWidth: '100', - cssHeight: '100', + cssSizing: { + width: '100', + height: '100', + } }); expect(result.score).toEqual(1); }); @@ -189,8 +219,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '', - cssWidth: '100', - cssHeight: '100', + cssSizing: { + width: '100', + height: '100', + } }); expect(result.score).toEqual(1); }); @@ -199,8 +231,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '100', - cssWidth: '100', - cssHeight: null, + cssSizing: { + width: '100', + height: null, + } }); expect(result.score).toEqual(1); }); @@ -209,8 +243,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '100', - cssWidth: '100', - cssHeight: '100', + cssSizing: { + width: '100', + height: '100', + } }); expect(result.score).toEqual(1); }); @@ -219,8 +255,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '100', - cssWidth: null, - cssHeight: '100', + cssSizing: { + width: null, + height: '100', + } }); expect(result.score).toEqual(1); }); @@ -229,8 +267,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '100', - cssWidth: '100', - cssHeight: '100', + cssSizing: { + width: '100', + height: '100', + } }); expect(result.score).toEqual(1); }); @@ -239,8 +279,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '0', attributeHeight: '0', - cssWidth: null, - cssHeight: null, + cssSizing: { + width: null, + height: null, + }, node: { boundingRect: { width: 0, @@ -255,8 +297,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssWidth: null, - cssHeight: null, + cssSizing: { + width: null, + height: null, + }, node: { boundingRect: { width: 0, @@ -273,8 +317,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '-200', attributeHeight: '100', - cssWidth: null, - cssHeight: null, + cssSizing: { + width: null, + height: null, + } }); expect(result.score).toEqual(0); }); @@ -283,8 +329,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '-200', - cssWidth: null, - cssHeight: null, + cssSizing: { + width: null, + height: null, + } }); expect(result.score).toEqual(0); }); @@ -293,8 +341,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssWidth: 'auto', - cssHeight: '100', + cssSizing: { + width: 'auto', + height: '100', + } }); expect(result.score).toEqual(0); }); @@ -303,8 +353,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssWidth: '100', - cssHeight: 'auto', + cssSizing: { + width: '100', + height: 'auto', + } }); expect(result.score).toEqual(0); }); @@ -313,8 +365,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '-200', attributeHeight: '100', - cssWidth: '100', - cssHeight: null, + cssSizing: { + width: '100', + height: null, + } }); expect(result.score).toEqual(1); }); @@ -323,8 +377,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '-200', - cssWidth: null, - cssHeight: '100', + cssSizing: { + width: null, + height: '100', + } }); expect(result.score).toEqual(1); }); @@ -333,8 +389,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '', - cssWidth: 'auto', - cssHeight: '100', + cssSizing: { + width: 'auto', + height: '100', + } }); expect(result.score).toEqual(1); }); @@ -343,8 +401,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '100', - cssWidth: '100', - cssHeight: 'auto', + cssSizing: { + width: '100', + height: 'auto', + } }); expect(result.score).toEqual(1); }); @@ -354,8 +414,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '100', - cssWidth: 'auto', - cssHeight: 'auto', + cssSizing: { + width: 'auto', + height: 'auto', + } }); expect(result.score).toEqual(1); }); @@ -365,8 +427,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '-200', attributeHeight: '-200', - cssWidth: '100', - cssHeight: '100', + cssSizing: { + width: '100', + height: '100', + } }); expect(result.score).toEqual(1); }); @@ -376,8 +440,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '-200', attributeHeight: '-200', - cssWidth: 'auto', - cssHeight: 'auto', + cssSizing: { + width: 'auto', + height: 'auto', + } }); expect(result.score).toEqual(0); }); @@ -398,8 +464,10 @@ describe('Sized images audit', () => { { attributeWidth: '', attributeHeight: '', - cssWidth: null, - cssHeight: null, + cssSizing: { + width: null, + height: null, + } }, 'image1.png' ), @@ -414,8 +482,10 @@ describe('Sized images audit', () => { { attributeWidth: '', attributeHeight: '', - cssWidth: null, - cssHeight: null, + cssSizing: { + width: null, + height: null, + } }, 'image3.png' ), @@ -433,8 +503,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssWidth: undefined, - cssHeight: undefined, + cssSizing: undefined, }); expect(result.details.items.length).toEqual(0); expect(result.score).toEqual(1); @@ -444,8 +513,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '10', attributeHeight: '10', - cssWidth: undefined, - cssHeight: undefined, + cssSizing: undefined, }); expect(result.details.items.length).toEqual(0); expect(result.score).toEqual(1); diff --git a/lighthouse-core/test/results/artifacts/artifacts.json b/lighthouse-core/test/results/artifacts/artifacts.json index 512ec58859d5..b866ec47e373 100644 --- a/lighthouse-core/test/results/artifacts/artifacts.json +++ b/lighthouse-core/test/results/artifacts/artifacts.json @@ -1214,8 +1214,10 @@ "nodeLabel": "img" }, "mimeType": "image/jpeg", - "cssWidth": "120px", - "cssHeight": "15px" + "cssSizing": { + "width": "120px", + "height": "15px" + } }, { "src": "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?iar2", @@ -1255,8 +1257,10 @@ "nodeLabel": "img" }, "mimeType": "image/jpeg", - "cssWidth": "120px", - "cssHeight": "80px" + "cssSizing": { + "width": "120px", + "height": "80px" + } }, { "src": "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr1", @@ -1296,8 +1300,10 @@ "nodeLabel": "img" }, "mimeType": "image/jpeg", - "cssWidth": "4800px", - "cssHeight": "3180px" + "cssSizing": { + "width": "4800px", + "height": "3180px" + } }, { "src": "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr2", @@ -1337,8 +1343,10 @@ "nodeLabel": "img" }, "mimeType": "image/jpeg", - "cssWidth": "120px", - "cssHeight": "80px" + "cssSizing": { + "width": "120px", + "height": "80px" + } }, { "src": "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr3", @@ -1378,8 +1386,10 @@ "nodeLabel": "img" }, "mimeType": "image/jpeg", - "cssWidth": "960px", - "cssHeight": "636px" + "cssSizing": { + "width": "960px", + "height": "636px" + } }, { "src": "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg", @@ -1419,8 +1429,10 @@ "nodeLabel": "img" }, "mimeType": "image/jpeg", - "cssWidth": "960px", - "cssHeight": "636px" + "cssSizing": { + "width": "960px", + "height": "636px" + } }, { "src": "http://localhost:10200/dobetterweb/lighthouse-rotating.gif", @@ -1460,8 +1472,10 @@ "nodeLabel": "img" }, "mimeType": "image/gif", - "cssWidth": "811px", - "cssHeight": "462px" + "cssSizing": { + "width": "811px", + "height": "462px" + } }, { "src": "blob:http://localhost:10200/822c70a0-b912-41c7-9a21-56c3d309e75b", @@ -3518,4 +3532,4 @@ "columnNumber": 36 } ] -} \ No newline at end of file +} diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index add80879d249..7ef0efda022c 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -409,10 +409,23 @@ declare global { attributeWidth: string; /** The raw height attribute of the image element. CSS images will be set to the empty string. */ attributeHeight: string; - /** The CSS width property of the image element. Set to `undefined` if Lighthouse didn't gather the data. Set to `null` if it was gathered, but there was no width set in CSS. */ - cssWidth?: string | null; - /** The CSS height property of the image element. Set to `undefined` if Lighthouse didn't gather the data. Set to `null` if it was gathered, but there was no height set in CSS. */ - cssHeight?: string | null; + /** + * The CSS width property of the image element. + * @deprecated Use `element?.cssSizing.width` instead. + */ + cssWidth?: string; + /** + * The CSS height property of the image element. + * @deprecated Use `element?.cssSizing.width` instead + */ + cssHeight?: string; + /** The width/height of the element as defined by matching CSS rules. Set to `undefined` if the data was not collected. */ + cssSizing: undefined | { + /** The CSS width property of the image element. Set to `null` if there was no width set in CSS. */ + width: string | null; + /** The CSS height property of the image element. Set to `null` if there was no height set in CSS. */ + height: string | null; + } /** The BoundingClientRect of the element. */ clientRect: { top: number; From fea888563d674d1b8a6dbeb577265e4e9b6fd7c9 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Wed, 10 Feb 2021 14:08:25 -0800 Subject: [PATCH 13/22] 5s --- lighthouse-core/gather/gatherers/image-elements.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index 5c80358c905c..6f7e3a943c08 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -329,7 +329,7 @@ class ImageElements extends Gatherer { return bRecord.resourceSize - aRecord.resourceSize; }); - // Don't do more than 3s of this expensive devtools protocol work. See #11289 + // Don't do more than 5s of this expensive devtools protocol work. See #11289 let reachedGatheringBudget = false; setTimeout(_ => (reachedGatheringBudget = true), 5000); let skippedCount = 0; From dc7f4a024d360e5c56d23f65e2531e72a1ab8d25 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Wed, 10 Feb 2021 14:23:31 -0800 Subject: [PATCH 14/22] yarn update:sample-artifacts ImageElements plus v minor handtweaks --- .../test/results/artifacts/artifacts.json | 30 ++++++++++++++++--- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/lighthouse-core/test/results/artifacts/artifacts.json b/lighthouse-core/test/results/artifacts/artifacts.json index b866ec47e373..57e837cedd16 100644 --- a/lighthouse-core/test/results/artifacts/artifacts.json +++ b/lighthouse-core/test/results/artifacts/artifacts.json @@ -1214,6 +1214,8 @@ "nodeLabel": "img" }, "mimeType": "image/jpeg", + "cssWidth": "120px", + "cssHeight": "15px", "cssSizing": { "width": "120px", "height": "15px" @@ -1257,6 +1259,8 @@ "nodeLabel": "img" }, "mimeType": "image/jpeg", + "cssWidth": "120px", + "cssHeight": "80px", "cssSizing": { "width": "120px", "height": "80px" @@ -1300,6 +1304,8 @@ "nodeLabel": "img" }, "mimeType": "image/jpeg", + "cssWidth": "4800px", + "cssHeight": "3180px", "cssSizing": { "width": "4800px", "height": "3180px" @@ -1343,6 +1349,8 @@ "nodeLabel": "img" }, "mimeType": "image/jpeg", + "cssWidth": "120px", + "cssHeight": "80px", "cssSizing": { "width": "120px", "height": "80px" @@ -1386,6 +1394,8 @@ "nodeLabel": "img" }, "mimeType": "image/jpeg", + "cssWidth": "960px", + "cssHeight": "636px", "cssSizing": { "width": "960px", "height": "636px" @@ -1402,8 +1412,6 @@ "left": 252, "right": 1212 }, - "naturalWidth": 480, - "naturalHeight": 318, "attributeWidth": "960", "attributeHeight": "636", "cssComputedPosition": "absolute", @@ -1425,14 +1433,18 @@ "width": 960, "height": 636 }, - "snippet": "", + "snippet": "", "nodeLabel": "img" }, "mimeType": "image/jpeg", + "cssWidth": "960px", + "cssHeight": "636px", "cssSizing": { "width": "960px", "height": "636px" - } + }, + "naturalWidth": 480, + "naturalHeight": 318 }, { "src": "http://localhost:10200/dobetterweb/lighthouse-rotating.gif", @@ -1472,6 +1484,8 @@ "nodeLabel": "img" }, "mimeType": "image/gif", + "cssWidth": "811px", + "cssHeight": "462px", "cssSizing": { "width": "811px", "height": "462px" @@ -1513,6 +1527,10 @@ }, "snippet": "", "nodeLabel": "img" + }, + "cssSizing": { + "width": null, + "height": null } }, { @@ -1551,6 +1569,10 @@ }, "snippet": "", "nodeLabel": "img" + }, + "cssSizing": { + "width": null, + "height": null } } ], From b80096c9e84b425c2b25a2f25c88d357ec0831dc Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Wed, 10 Feb 2021 15:48:00 -0800 Subject: [PATCH 15/22] Apply suggestions from code review Co-authored-by: Connor Clark --- lighthouse-core/audits/unsized-images.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/audits/unsized-images.js b/lighthouse-core/audits/unsized-images.js index 096975963379..dad8c50c1156 100644 --- a/lighthouse-core/audits/unsized-images.js +++ b/lighthouse-core/audits/unsized-images.js @@ -40,7 +40,7 @@ class UnsizedImages extends Audit { } /** - * An img size attribute prevents CLS if it is a non-negative integer (incl zero!). + * An img size attribute prevents layout shifts 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} @@ -60,7 +60,7 @@ class UnsizedImages extends Audit { } /** - * An img css size property prevents CLS if it is defined, not empty, and not equal to 'auto'. + * An img css size property prevents layout shifts if it is defined, not empty, and not equal to 'auto'. * @param {string | null} property * @return {boolean} */ From 7d80b317ae63ef483eaa0e8255f98edc8a982859 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Wed, 10 Feb 2021 15:48:38 -0800 Subject: [PATCH 16/22] lint --- .../gather/gatherers/image-elements.js | 2 +- .../test/audits/unsized-images-test.js | 66 +++++++++---------- 2 files changed, 34 insertions(+), 34 deletions(-) diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index 6f7e3a943c08..2480d4d60a9e 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -273,7 +273,7 @@ class ImageElements extends Gatherer { }); const width = getEffectiveSizingRule(matchedRules, 'width'); const height = getEffectiveSizingRule(matchedRules, 'height'); - // Maintain backcompat for <= 7.0.1 + // COMPAT: Maintain backcompat for <= 7.0.1 element.cssWidth = width === null ? undefined : width; element.cssHeight = height === null ? undefined : height; Object.assign(element, {cssSizing: {width, height}}); diff --git a/lighthouse-core/test/audits/unsized-images-test.js b/lighthouse-core/test/audits/unsized-images-test.js index dca7fae47f61..3475bd1920d4 100644 --- a/lighthouse-core/test/audits/unsized-images-test.js +++ b/lighthouse-core/test/audits/unsized-images-test.js @@ -34,7 +34,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: null, - } + }, }); expect(result.score).toEqual(1); }); @@ -47,7 +47,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: null, - } + }, }); expect(result.score).toEqual(1); }); @@ -60,7 +60,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: null, - } + }, }); expect(result.score).toEqual(1); }); @@ -73,7 +73,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: null, - } + }, }); expect(result.score).toEqual(1); }); @@ -86,7 +86,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: null, - } + }, }); expect(result.score).toEqual(0); }); @@ -98,7 +98,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: '100', - } + }, }); expect(result.score).toEqual(0); }); @@ -110,7 +110,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: '100', - } + }, }); expect(result.score).toEqual(0); }); @@ -124,7 +124,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: null, - } + }, }); expect(result.score).toEqual(0); }); @@ -136,7 +136,7 @@ describe('Sized images audit', () => { cssSizing: { width: '100', height: null, - } + }, }); expect(result.score).toEqual(0); }); @@ -148,7 +148,7 @@ describe('Sized images audit', () => { cssSizing: { width: '100', height: null, - } + }, }); expect(result.score).toEqual(0); }); @@ -161,7 +161,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: null, - } + }, }); expect(result.score).toEqual(0); }); @@ -174,7 +174,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: '100', - } + }, }); expect(result.score).toEqual(1); }); @@ -186,7 +186,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: null, - } + }, }); expect(result.score).toEqual(1); }); @@ -198,7 +198,7 @@ describe('Sized images audit', () => { cssSizing: { width: '100', height: null, - } + }, }); expect(result.score).toEqual(1); }); @@ -210,7 +210,7 @@ describe('Sized images audit', () => { cssSizing: { width: '100', height: '100', - } + }, }); expect(result.score).toEqual(1); }); @@ -222,7 +222,7 @@ describe('Sized images audit', () => { cssSizing: { width: '100', height: '100', - } + }, }); expect(result.score).toEqual(1); }); @@ -234,7 +234,7 @@ describe('Sized images audit', () => { cssSizing: { width: '100', height: null, - } + }, }); expect(result.score).toEqual(1); }); @@ -246,7 +246,7 @@ describe('Sized images audit', () => { cssSizing: { width: '100', height: '100', - } + }, }); expect(result.score).toEqual(1); }); @@ -258,7 +258,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: '100', - } + }, }); expect(result.score).toEqual(1); }); @@ -270,7 +270,7 @@ describe('Sized images audit', () => { cssSizing: { width: '100', height: '100', - } + }, }); expect(result.score).toEqual(1); }); @@ -320,7 +320,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: null, - } + }, }); expect(result.score).toEqual(0); }); @@ -332,7 +332,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: null, - } + }, }); expect(result.score).toEqual(0); }); @@ -344,7 +344,7 @@ describe('Sized images audit', () => { cssSizing: { width: 'auto', height: '100', - } + }, }); expect(result.score).toEqual(0); }); @@ -356,7 +356,7 @@ describe('Sized images audit', () => { cssSizing: { width: '100', height: 'auto', - } + }, }); expect(result.score).toEqual(0); }); @@ -368,7 +368,7 @@ describe('Sized images audit', () => { cssSizing: { width: '100', height: null, - } + }, }); expect(result.score).toEqual(1); }); @@ -380,7 +380,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: '100', - } + }, }); expect(result.score).toEqual(1); }); @@ -392,7 +392,7 @@ describe('Sized images audit', () => { cssSizing: { width: 'auto', height: '100', - } + }, }); expect(result.score).toEqual(1); }); @@ -404,7 +404,7 @@ describe('Sized images audit', () => { cssSizing: { width: '100', height: 'auto', - } + }, }); expect(result.score).toEqual(1); }); @@ -417,7 +417,7 @@ describe('Sized images audit', () => { cssSizing: { width: 'auto', height: 'auto', - } + }, }); expect(result.score).toEqual(1); }); @@ -430,7 +430,7 @@ describe('Sized images audit', () => { cssSizing: { width: '100', height: '100', - } + }, }); expect(result.score).toEqual(1); }); @@ -443,7 +443,7 @@ describe('Sized images audit', () => { cssSizing: { width: 'auto', height: 'auto', - } + }, }); expect(result.score).toEqual(0); }); @@ -467,7 +467,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: null, - } + }, }, 'image1.png' ), @@ -485,7 +485,7 @@ describe('Sized images audit', () => { cssSizing: { width: null, height: null, - } + }, }, 'image3.png' ), From 99b3d4a5fcee783fbf53ae54c2e4b4fbf5106c21 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Wed, 10 Feb 2021 15:57:21 -0800 Subject: [PATCH 17/22] rename to _privateCssSizing --- lighthouse-core/audits/unsized-images.js | 6 +- .../gather/gatherers/image-elements.js | 6 +- .../test/audits/unsized-images-test.js | 74 +++++++++---------- .../test/results/artifacts/artifacts.json | 18 ++--- types/artifacts.d.ts | 11 ++- 5 files changed, 59 insertions(+), 56 deletions(-) diff --git a/lighthouse-core/audits/unsized-images.js b/lighthouse-core/audits/unsized-images.js index dad8c50c1156..4fead2b4a571 100644 --- a/lighthouse-core/audits/unsized-images.js +++ b/lighthouse-core/audits/unsized-images.js @@ -80,12 +80,12 @@ class UnsizedImages extends Audit { // We don't want to show the user a false positive, so we'll call it sized to give it as pass. // While this situation should only befall small-impact images, it means our analysis is incomplete. :( // Handwavey TODO: explore ways to avoid this. - if (image.cssSizing === undefined) return true; + if (image._privateCssSizing === undefined) return true; const attrWidth = image.attributeWidth; const attrHeight = image.attributeHeight; - const cssWidth = image.cssSizing.width; - const cssHeight = image.cssSizing.height; + const cssWidth = image._privateCssSizing.width; + const cssHeight = image._privateCssSizing.height; const htmlWidthIsExplicit = UnsizedImages.doesHtmlAttrProvideExplicitSize(attrWidth); const cssWidthIsExplicit = UnsizedImages.doesCssPropProvideExplicitSize(cssWidth); const htmlHeightIsExplicit = UnsizedImages.doesHtmlAttrProvideExplicitSize(attrHeight); diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index 2480d4d60a9e..5c83cd27b2c9 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -75,7 +75,7 @@ function getHTMLImages(allElements) { attributeHeight: element.getAttribute('height') || '', cssWidth: undefined, // this will get overwritten below cssHeight: undefined, // this will get overwritten below - cssSizing: undefined, // this will get overwritten below + _privateCssSizing: undefined, // this will get overwritten below cssComputedPosition: getPosition(element, computedStyle), isCss: false, isPicture, @@ -122,7 +122,7 @@ function getCSSImages(allElements) { attributeHeight: '', cssWidth: undefined, cssHeight: undefined, - cssSizing: undefined, + _privateCssSizing: undefined, cssComputedPosition: getPosition(element, style), isCss: true, isPicture: false, @@ -276,7 +276,7 @@ class ImageElements extends Gatherer { // COMPAT: Maintain backcompat for <= 7.0.1 element.cssWidth = width === null ? undefined : width; element.cssHeight = height === null ? undefined : height; - Object.assign(element, {cssSizing: {width, height}}); + Object.assign(element, {_privateCssSizing: {width, height}}); } catch (err) { if (/No node.*found/.test(err.message)) return; throw err; diff --git a/lighthouse-core/test/audits/unsized-images-test.js b/lighthouse-core/test/audits/unsized-images-test.js index 3475bd1920d4..4d3c9602ac8b 100644 --- a/lighthouse-core/test/audits/unsized-images-test.js +++ b/lighthouse-core/test/audits/unsized-images-test.js @@ -31,7 +31,7 @@ describe('Sized images audit', () => { isCss: true, attributeWidth: '', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: null, height: null, }, @@ -44,7 +44,7 @@ describe('Sized images audit', () => { isInShadowDOM: true, attributeWidth: '', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: null, height: null, }, @@ -57,7 +57,7 @@ describe('Sized images audit', () => { cssComputedPosition: 'absolute', attributeWidth: '', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: null, height: null, }, @@ -70,7 +70,7 @@ describe('Sized images audit', () => { cssComputedPosition: 'fixed', attributeWidth: '', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: null, height: null, }, @@ -83,7 +83,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '100', - cssSizing: { + _privateCssSizing: { width: null, height: null, }, @@ -95,7 +95,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: null, height: '100', }, @@ -107,7 +107,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '100', - cssSizing: { + _privateCssSizing: { width: null, height: '100', }, @@ -121,7 +121,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: null, height: null, }, @@ -133,7 +133,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: '100', height: null, }, @@ -145,7 +145,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: '100', height: null, }, @@ -158,7 +158,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: null, height: null, }, @@ -171,7 +171,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: null, height: '100', }, @@ -183,7 +183,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '100', - cssSizing: { + _privateCssSizing: { width: null, height: null, }, @@ -195,7 +195,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '100', - cssSizing: { + _privateCssSizing: { width: '100', height: null, }, @@ -207,7 +207,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: '100', height: '100', }, @@ -219,7 +219,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: '100', height: '100', }, @@ -231,7 +231,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '100', - cssSizing: { + _privateCssSizing: { width: '100', height: null, }, @@ -243,7 +243,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '100', - cssSizing: { + _privateCssSizing: { width: '100', height: '100', }, @@ -255,7 +255,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '100', - cssSizing: { + _privateCssSizing: { width: null, height: '100', }, @@ -267,7 +267,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '100', - cssSizing: { + _privateCssSizing: { width: '100', height: '100', }, @@ -279,7 +279,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '0', attributeHeight: '0', - cssSizing: { + _privateCssSizing: { width: null, height: null, }, @@ -297,7 +297,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: null, height: null, }, @@ -317,7 +317,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '-200', attributeHeight: '100', - cssSizing: { + _privateCssSizing: { width: null, height: null, }, @@ -329,7 +329,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '-200', - cssSizing: { + _privateCssSizing: { width: null, height: null, }, @@ -341,7 +341,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: 'auto', height: '100', }, @@ -353,7 +353,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: '100', height: 'auto', }, @@ -365,7 +365,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '-200', attributeHeight: '100', - cssSizing: { + _privateCssSizing: { width: '100', height: null, }, @@ -377,7 +377,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '-200', - cssSizing: { + _privateCssSizing: { width: null, height: '100', }, @@ -389,7 +389,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: 'auto', height: '100', }, @@ -401,7 +401,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '100', - cssSizing: { + _privateCssSizing: { width: '100', height: 'auto', }, @@ -414,7 +414,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '100', - cssSizing: { + _privateCssSizing: { width: 'auto', height: 'auto', }, @@ -427,7 +427,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '-200', attributeHeight: '-200', - cssSizing: { + _privateCssSizing: { width: '100', height: '100', }, @@ -440,7 +440,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '-200', attributeHeight: '-200', - cssSizing: { + _privateCssSizing: { width: 'auto', height: 'auto', }, @@ -464,7 +464,7 @@ describe('Sized images audit', () => { { attributeWidth: '', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: null, height: null, }, @@ -482,7 +482,7 @@ describe('Sized images audit', () => { { attributeWidth: '', attributeHeight: '', - cssSizing: { + _privateCssSizing: { width: null, height: null, }, @@ -503,7 +503,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssSizing: undefined, + _privateCssSizing: undefined, }); expect(result.details.items.length).toEqual(0); expect(result.score).toEqual(1); @@ -513,7 +513,7 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '10', attributeHeight: '10', - cssSizing: undefined, + _privateCssSizing: undefined, }); expect(result.details.items.length).toEqual(0); expect(result.score).toEqual(1); diff --git a/lighthouse-core/test/results/artifacts/artifacts.json b/lighthouse-core/test/results/artifacts/artifacts.json index 57e837cedd16..dee9adf49728 100644 --- a/lighthouse-core/test/results/artifacts/artifacts.json +++ b/lighthouse-core/test/results/artifacts/artifacts.json @@ -1216,7 +1216,7 @@ "mimeType": "image/jpeg", "cssWidth": "120px", "cssHeight": "15px", - "cssSizing": { + "_privateCssSizing": { "width": "120px", "height": "15px" } @@ -1261,7 +1261,7 @@ "mimeType": "image/jpeg", "cssWidth": "120px", "cssHeight": "80px", - "cssSizing": { + "_privateCssSizing": { "width": "120px", "height": "80px" } @@ -1306,7 +1306,7 @@ "mimeType": "image/jpeg", "cssWidth": "4800px", "cssHeight": "3180px", - "cssSizing": { + "_privateCssSizing": { "width": "4800px", "height": "3180px" } @@ -1351,7 +1351,7 @@ "mimeType": "image/jpeg", "cssWidth": "120px", "cssHeight": "80px", - "cssSizing": { + "_privateCssSizing": { "width": "120px", "height": "80px" } @@ -1396,7 +1396,7 @@ "mimeType": "image/jpeg", "cssWidth": "960px", "cssHeight": "636px", - "cssSizing": { + "_privateCssSizing": { "width": "960px", "height": "636px" } @@ -1439,7 +1439,7 @@ "mimeType": "image/jpeg", "cssWidth": "960px", "cssHeight": "636px", - "cssSizing": { + "_privateCssSizing": { "width": "960px", "height": "636px" }, @@ -1486,7 +1486,7 @@ "mimeType": "image/gif", "cssWidth": "811px", "cssHeight": "462px", - "cssSizing": { + "_privateCssSizing": { "width": "811px", "height": "462px" } @@ -1528,7 +1528,7 @@ "snippet": "", "nodeLabel": "img" }, - "cssSizing": { + "_privateCssSizing": { "width": null, "height": null } @@ -1570,7 +1570,7 @@ "snippet": "", "nodeLabel": "img" }, - "cssSizing": { + "_privateCssSizing": { "width": null, "height": null } diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 7ef0efda022c..c26c0c174c99 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -411,16 +411,19 @@ declare global { attributeHeight: string; /** * The CSS width property of the image element. - * @deprecated Use `element?.cssSizing.width` instead. + * @deprecated Use `element?._privateCssSizing.width` instead. */ cssWidth?: string; /** * The CSS height property of the image element. - * @deprecated Use `element?.cssSizing.width` instead + * @deprecated Use `element?._privateCssSizing.width` instead */ cssHeight?: string; - /** The width/height of the element as defined by matching CSS rules. Set to `undefined` if the data was not collected. */ - cssSizing: undefined | { + /** + * The width/height of the element as defined by matching CSS rules. Set to `undefined` if the data was not collected. + * @private + */ + _privateCssSizing: undefined | { /** The CSS width property of the image element. Set to `null` if there was no width set in CSS. */ width: string | null; /** The CSS height property of the image element. Set to `null` if there was no height set in CSS. */ From 850300105e4f9767d999efe3e66261b0482a0527 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Wed, 10 Feb 2021 15:57:46 -0800 Subject: [PATCH 18/22] set as optional. thx brendan --- types/artifacts.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index c26c0c174c99..ca14aa93701e 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -423,7 +423,7 @@ declare global { * The width/height of the element as defined by matching CSS rules. Set to `undefined` if the data was not collected. * @private */ - _privateCssSizing: undefined | { + _privateCssSizing?: { /** The CSS width property of the image element. Set to `null` if there was no width set in CSS. */ width: string | null; /** The CSS height property of the image element. Set to `null` if there was no height set in CSS. */ From 7e26ae2948300010d26346e38dd61c460e1391e4 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Thu, 11 Feb 2021 10:50:30 -0800 Subject: [PATCH 19/22] Update lighthouse-core/gather/gatherers/image-elements.js Co-authored-by: Brendan Kenny --- lighthouse-core/gather/gatherers/image-elements.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index 5c83cd27b2c9..da5f3d62424a 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -276,7 +276,7 @@ class ImageElements extends Gatherer { // COMPAT: Maintain backcompat for <= 7.0.1 element.cssWidth = width === null ? undefined : width; element.cssHeight = height === null ? undefined : height; - Object.assign(element, {_privateCssSizing: {width, height}}); + element._privateCssSizing = {width, height}; } catch (err) { if (/No node.*found/.test(err.message)) return; throw err; From b19a8d3aa87b52c1d70bb983e9b98115d48d530a Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Thu, 11 Feb 2021 10:52:15 -0800 Subject: [PATCH 20/22] remove unncessary return value from fetchElementWithSizeInformation() --- lighthouse-core/gather/gatherers/image-elements.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index da5f3d62424a..04b05aaa476f 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -231,12 +231,11 @@ class ImageElements extends Gatherer { /** * @param {Driver} driver * @param {LH.Artifacts.ImageElement} element - * @return {Promise} */ async fetchElementWithSizeInformation(driver, element) { const url = element.src; if (this._naturalSizeCache.has(url)) { - return Object.assign(element, this._naturalSizeCache.get(url)); + Object.assign(element, this._naturalSizeCache.get(url)); } try { @@ -246,10 +245,10 @@ class ImageElements extends Gatherer { args: [url], }); this._naturalSizeCache.set(url, size); - return Object.assign(element, size); + Object.assign(element, size); } catch (_) { // determineNaturalSize fails on invalid images, which we treat as non-visible - return element; + return; } } @@ -352,7 +351,7 @@ class ImageElements extends Gatherer { // CSS images have no natural size information at all. Try to get the actual size if we can. // Additional fetch is expensive; don't bother if we don't have a networkRecord for the image. if ((element.isPicture || element.isCss || element.srcset) && networkRecord) { - element = await this.fetchElementWithSizeInformation(driver, element); + await this.fetchElementWithSizeInformation(driver, element); } } From 3a5eea3e616fff24825ea349d2bf883fed79f045 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Thu, 11 Feb 2021 11:01:37 -0800 Subject: [PATCH 21/22] adjust type definitions, based on brendan's feedback --- .../gather/gatherers/image-elements.js | 2 +- types/artifacts.d.ts | 15 +++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/lighthouse-core/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index 04b05aaa476f..e8970c122bed 100644 --- a/lighthouse-core/gather/gatherers/image-elements.js +++ b/lighthouse-core/gather/gatherers/image-elements.js @@ -333,7 +333,7 @@ class ImageElements extends Gatherer { setTimeout(_ => (reachedGatheringBudget = true), 5000); let skippedCount = 0; - for (let element of elements) { + for (const element of elements) { // Pull some of our information directly off the network record. const networkRecord = indexedNetworkRecords[element.src] || {}; element.mimeType = networkRecord.mimeType; diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index ca14aa93701e..9df729370244 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -403,7 +403,10 @@ declare global { displayedHeight: number; /** The natural width of the underlying image, uses img.naturalWidth. See https://codepen.io/patrickhulce/pen/PXvQbM for examples. */ naturalWidth?: number; - /** The natural height of the underlying image, uses img.naturalHeight. See https://codepen.io/patrickhulce/pen/PXvQbM for examples. */ + /** + * The natural height of the underlying image, uses img.naturalHeight. See https://codepen.io/patrickhulce/pen/PXvQbM for examples. + * TODO: explore revising the shape of this data. https://github.com/GoogleChrome/lighthouse/issues/12077 + */ naturalHeight?: number; /** The raw width attribute of the image element. CSS images will be set to the empty string. */ attributeWidth: string; @@ -411,22 +414,22 @@ declare global { attributeHeight: string; /** * The CSS width property of the image element. - * @deprecated Use `element?._privateCssSizing.width` instead. + * TODO: explore deprecating this in favor of _privateCssSizing. https://github.com/GoogleChrome/lighthouse/issues/12077 */ cssWidth?: string; /** * The CSS height property of the image element. - * @deprecated Use `element?._privateCssSizing.width` instead + * TODO: explore deprecating this in favor of _privateCssSizing */ cssHeight?: string; /** * The width/height of the element as defined by matching CSS rules. Set to `undefined` if the data was not collected. - * @private + * @deprecated Not really "deprecated", but marking so we finalize naming/shape of this data prior to Lighthouse 8. */ _privateCssSizing?: { - /** The CSS width property of the image element. Set to `null` if there was no width set in CSS. */ + /** The width of the image as expressed by CSS rules. Set to `null` if there was no width set in CSS. */ width: string | null; - /** The CSS height property of the image element. Set to `null` if there was no height set in CSS. */ + /** The height of the image as expressed by CSS rules. Set to `null` if there was no height set in CSS. */ height: string | null; } /** The BoundingClientRect of the element. */ From 36b31f322d039b38e99315b3735c582580b8e454 Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Thu, 11 Feb 2021 11:05:15 -0800 Subject: [PATCH 22/22] drop @deprecated per connor --- types/artifacts.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 9df729370244..6af8a444b009 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -424,7 +424,7 @@ declare global { cssHeight?: string; /** * The width/height of the element as defined by matching CSS rules. Set to `undefined` if the data was not collected. - * @deprecated Not really "deprecated", but marking so we finalize naming/shape of this data prior to Lighthouse 8. + * TODO: Finalize naming/shape of this data prior to Lighthouse 8. https://github.com/GoogleChrome/lighthouse/issues/12077 */ _privateCssSizing?: { /** The width of the image as expressed by CSS rules. Set to `null` if there was no width set in CSS. */