From 7d3238054c7af030a3fa60772120be68c58bbe3d Mon Sep 17 00:00:00 2001 From: Paul Irish Date: Thu, 11 Feb 2021 11:36:16 -0800 Subject: [PATCH] core(image-elements): set 5s time budget, add _privateCssSizing (#12065) --- lighthouse-core/audits/unsized-images.js | 36 +- .../gather/gatherers/image-elements.js | 68 ++-- .../test/audits/unsized-images-test.js | 309 ++++++++++++------ .../test/results/artifacts/artifacts.json | 58 +++- types/artifacts.d.ts | 25 +- 5 files changed, 335 insertions(+), 161 deletions(-) diff --git a/lighthouse-core/audits/unsized-images.js b/lighthouse-core/audits/unsized-images.js index 18ed00bd6669..4fead2b4a571 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 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} */ - 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'. - * @param {string | undefined} property + * 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} */ - static isValidCss(property) { + static doesCssPropProvideExplicitSize(property) { if (!property) return false; return property !== 'auto'; } @@ -76,17 +75,24 @@ 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._privateCssSizing === undefined) return true; + const attrWidth = image.attributeWidth; 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 cssWidth = image._privateCssSizing.width; + const cssHeight = image._privateCssSizing.height; + 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/gather/gatherers/image-elements.js b/lighthouse-core/gather/gatherers/image-elements.js index e5b757fde135..e8970c122bed 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 @@ -74,6 +75,7 @@ function getHTMLImages(allElements) { attributeHeight: element.getAttribute('height') || '', cssWidth: undefined, // this will get overwritten below cssHeight: undefined, // this will get overwritten below + _privateCssSizing: undefined, // this will get overwritten below cssComputedPosition: getPosition(element, computedStyle), isCss: false, isPicture, @@ -120,6 +122,7 @@ function getCSSImages(allElements) { attributeHeight: '', cssWidth: undefined, cssHeight: undefined, + _privateCssSizing: undefined, cssComputedPosition: getPosition(element, style), isCss: true, isPicture: false, @@ -199,7 +202,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. @@ -214,6 +217,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 { @@ -226,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 { @@ -241,14 +245,17 @@ 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; } } /** + * 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. + * @url http://go/dwoqq (googlers only) * @param {Driver} driver * @param {string} devtoolsNodePath * @param {LH.Artifacts.ImageElement} element @@ -263,10 +270,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'); + // COMPAT: Maintain backcompat for <= 7.0.1 + element.cssWidth = width === null ? undefined : width; + element.cssHeight = height === null ? undefined : height; + element._privateCssSizing = {width, height}; } catch (err) { if (/No node.*found/.test(err.message)) return; throw err; @@ -306,37 +315,48 @@ 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}), ]); - for (let element of elements) { + // 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 do more than 5s of this expensive devtools protocol work. See #11289 + let reachedGatheringBudget = false; + setTimeout(_ => (reachedGatheringBudget = true), 5000); + let skippedCount = 0; + + for (const element of elements) { // Pull some of our information directly off the network record. const networkRecord = indexedNetworkRecords[element.src] || {}; element.mimeType = networkRecord.mimeType; + if (reachedGatheringBudget) { + skippedCount++; + continue; + } + + // Need source rules to determine if sized via CSS (for unsized-images). 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, - // or it's not in the top 50 largest images. - if ( - (element.isPicture || element.isCss || element.srcset) && - top50Images.includes(networkRecord) - ) { - element = await this.fetchElementWithSizeInformation(driver, element); + // 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) { + await this.fetchElementWithSizeInformation(driver, element); } + } - imageUsage.push(element); + if (reachedGatheringBudget) { + log.warn('ImageElements', `Reached gathering budget of 5s. Skipped extra details for ${skippedCount}/${elements.length}`); // eslint-disable-line max-len } await Promise.all([ @@ -344,7 +364,7 @@ class ImageElements extends Gatherer { driver.sendCommand('CSS.disable'), ]); - return imageUsage; + return elements; } } diff --git a/lighthouse-core/test/audits/unsized-images-test.js b/lighthouse-core/test/audits/unsized-images-test.js index 88afbe5415be..4d3c9602ac8b 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: '', - cssHeight: '', + _privateCssSizing: { + width: null, + height: null, + }, }); expect(result.score).toEqual(1); }); @@ -42,8 +44,10 @@ describe('Sized images audit', () => { isInShadowDOM: true, attributeWidth: '', attributeHeight: '', - cssWidth: '', - cssHeight: '', + _privateCssSizing: { + width: null, + height: null, + }, }); expect(result.score).toEqual(1); }); @@ -53,8 +57,10 @@ describe('Sized images audit', () => { cssComputedPosition: 'absolute', attributeWidth: '', attributeHeight: '', - cssWidth: '', - cssHeight: '', + _privateCssSizing: { + width: null, + height: null, + }, }); expect(result.score).toEqual(1); }); @@ -64,8 +70,10 @@ describe('Sized images audit', () => { cssComputedPosition: 'fixed', attributeWidth: '', attributeHeight: '', - cssWidth: '', - cssHeight: '', + _privateCssSizing: { + 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: '', - cssHeight: '', + _privateCssSizing: { + width: null, + height: null, + }, }); expect(result.score).toEqual(0); }); @@ -85,8 +95,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssWidth: '', - cssHeight: '100', + _privateCssSizing: { + 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: '', - cssHeight: '100', + _privateCssSizing: { + 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: '', - cssHeight: '', + _privateCssSizing: { + 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: '', + _privateCssSizing: { + 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: '', + _privateCssSizing: { + width: '100', + height: null, + }, }); expect(result.score).toEqual(0); }); @@ -138,19 +158,23 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssWidth: '', - cssHeight: '', + _privateCssSizing: { + width: null, + height: null, + }, }); 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', attributeHeight: '', - cssWidth: '', - cssHeight: '100', + _privateCssSizing: { + 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: '', - cssHeight: '', + _privateCssSizing: { + 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: '', + _privateCssSizing: { + 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', + _privateCssSizing: { + 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', + _privateCssSizing: { + 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: '', + _privateCssSizing: { + 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', + _privateCssSizing: { + 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: '', - cssHeight: '100', + _privateCssSizing: { + 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', + _privateCssSizing: { + 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: '', - cssHeight: '', + _privateCssSizing: { + width: null, + height: null, + }, node: { boundingRect: { width: 0, @@ -255,8 +297,10 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '', attributeHeight: '', - cssWidth: '', - cssHeight: '', + _privateCssSizing: { + width: null, + height: null, + }, node: { boundingRect: { width: 0, @@ -268,13 +312,15 @@ 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', attributeHeight: '100', - cssWidth: '', - cssHeight: '', + _privateCssSizing: { + width: null, + height: null, + }, }); expect(result.score).toEqual(0); }); @@ -283,38 +329,46 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '-200', - cssWidth: '', - cssHeight: '', + _privateCssSizing: { + width: null, + height: null, + }, }); 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: '', - cssWidth: 'auto', - cssHeight: '100', + _privateCssSizing: { + width: 'auto', + height: '100', + }, }); 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: '', - cssWidth: '100', - cssHeight: 'auto', + _privateCssSizing: { + width: '100', + height: 'auto', + }, }); 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', - cssWidth: '100', - cssHeight: '', + _privateCssSizing: { + width: '100', + height: null, + }, }); expect(result.score).toEqual(1); }); @@ -323,39 +377,47 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '-200', - cssWidth: '', - cssHeight: '100', + _privateCssSizing: { + width: null, + height: '100', + }, }); 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: '', - cssWidth: 'auto', - cssHeight: '100', + _privateCssSizing: { + width: 'auto', + height: '100', + }, }); 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', - cssWidth: '100', - cssHeight: 'auto', + _privateCssSizing: { + width: '100', + height: 'auto', + }, }); 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', // eslint-disable-line max-len async () => { const result = await runAudit({ attributeWidth: '100', attributeHeight: '100', - cssWidth: 'auto', - cssHeight: 'auto', + _privateCssSizing: { + width: 'auto', + height: 'auto', + }, }); expect(result.score).toEqual(1); }); @@ -365,19 +427,23 @@ describe('Sized images audit', () => { const result = await runAudit({ attributeWidth: '-200', attributeHeight: '-200', - cssWidth: '100', - cssHeight: '100', + _privateCssSizing: { + width: '100', + height: '100', + }, }); 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', // eslint-disable-line max-len async () => { const result = await runAudit({ attributeWidth: '-200', attributeHeight: '-200', - cssWidth: 'auto', - cssHeight: 'auto', + _privateCssSizing: { + width: 'auto', + height: 'auto', + }, }); expect(result.score).toEqual(0); }); @@ -398,8 +464,10 @@ describe('Sized images audit', () => { { attributeWidth: '', attributeHeight: '', - cssWidth: '', - cssHeight: '', + _privateCssSizing: { + width: null, + height: null, + }, }, 'image1.png' ), @@ -414,8 +482,10 @@ describe('Sized images audit', () => { { attributeWidth: '', attributeHeight: '', - cssWidth: '', - cssHeight: '', + _privateCssSizing: { + width: null, + height: null, + }, }, 'image3.png' ), @@ -426,59 +496,82 @@ 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: '', + _privateCssSizing: undefined, + }); + 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', + _privateCssSizing: 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.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 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); }); }); diff --git a/lighthouse-core/test/results/artifacts/artifacts.json b/lighthouse-core/test/results/artifacts/artifacts.json index bd2ef989efac..ec8088412774 100644 --- a/lighthouse-core/test/results/artifacts/artifacts.json +++ b/lighthouse-core/test/results/artifacts/artifacts.json @@ -1215,7 +1215,11 @@ }, "mimeType": "image/jpeg", "cssWidth": "120px", - "cssHeight": "15px" + "cssHeight": "15px", + "_privateCssSizing": { + "width": "120px", + "height": "15px" + } }, { "src": "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?iar2", @@ -1256,7 +1260,11 @@ }, "mimeType": "image/jpeg", "cssWidth": "120px", - "cssHeight": "80px" + "cssHeight": "80px", + "_privateCssSizing": { + "width": "120px", + "height": "80px" + } }, { "src": "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr1", @@ -1297,7 +1305,11 @@ }, "mimeType": "image/jpeg", "cssWidth": "4800px", - "cssHeight": "3180px" + "cssHeight": "3180px", + "_privateCssSizing": { + "width": "4800px", + "height": "3180px" + } }, { "src": "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr2", @@ -1338,7 +1350,11 @@ }, "mimeType": "image/jpeg", "cssWidth": "120px", - "cssHeight": "80px" + "cssHeight": "80px", + "_privateCssSizing": { + "width": "120px", + "height": "80px" + } }, { "src": "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr3", @@ -1379,7 +1395,11 @@ }, "mimeType": "image/jpeg", "cssWidth": "960px", - "cssHeight": "636px" + "cssHeight": "636px", + "_privateCssSizing": { + "width": "960px", + "height": "636px" + } }, { "src": "http://localhost:10200/dobetterweb/lighthouse-480x318.jpg", @@ -1392,8 +1412,6 @@ "left": 252, "right": 1212 }, - "naturalWidth": 480, - "naturalHeight": 318, "attributeWidth": "960", "attributeHeight": "636", "cssComputedPosition": "absolute", @@ -1415,12 +1433,18 @@ "width": 960, "height": 636 }, - "snippet": "", + "snippet": "", "nodeLabel": "img" }, "mimeType": "image/jpeg", "cssWidth": "960px", - "cssHeight": "636px" + "cssHeight": "636px", + "_privateCssSizing": { + "width": "960px", + "height": "636px" + }, + "naturalWidth": 480, + "naturalHeight": 318 }, { "src": "http://localhost:10200/dobetterweb/lighthouse-rotating.gif", @@ -1461,7 +1485,11 @@ }, "mimeType": "image/gif", "cssWidth": "811px", - "cssHeight": "462px" + "cssHeight": "462px", + "_privateCssSizing": { + "width": "811px", + "height": "462px" + } }, { "src": "blob:http://localhost:10200/822c70a0-b912-41c7-9a21-56c3d309e75b", @@ -1499,6 +1527,10 @@ }, "snippet": "", "nodeLabel": "img" + }, + "_privateCssSizing": { + "width": null, + "height": null } }, { @@ -1537,6 +1569,10 @@ }, "snippet": "", "nodeLabel": "img" + }, + "_privateCssSizing": { + "width": null, + "height": null } } ], @@ -3523,4 +3559,4 @@ "columnNumber": 36 } ] -} \ No newline at end of file +} diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index d0d5eb12ee47..8ed68ae781e4 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -402,16 +402,35 @@ 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; /** 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. + * 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. */ + /** + * The CSS height property of the image element. + * 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. + * 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. */ + width: string | null; + /** 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. */ clientRect: { top: number;