-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DBW: Responsive Images Audit #1497
Changes from 5 commits
f0765a2
b4fe416
0116b4c
4f77f2d
a63e3c7
0116da0
8a4bd4a
c499a4e
b66df60
28a8a0a
f3ad900
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,139 @@ | ||
/** | ||
* @license | ||
* Copyright 2017 Google Inc. All rights reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
/** | ||
* @fileoverview Checks to see if the images used on the page are larger than | ||
* their display sizes. The audit will list all images that are larger than | ||
* their display size regardless of DPR (a 1000px wide image displayed as a | ||
* 500px high-res image on a Retina display will show up as 75% unused); | ||
* however, the audit will only fail pages that use images that have waste | ||
* when computed with DPR taken into account. | ||
*/ | ||
'use strict'; | ||
|
||
const Audit = require('../audit'); | ||
const URL = require('../../lib/url-shim'); | ||
const Formatter = require('../../formatters/formatter'); | ||
|
||
const KB_IN_BYTES = 1024; | ||
const WASTEFUL_THRESHOLD_AS_RATIO = 0.1; | ||
|
||
class UsesResponsiveImages extends Audit { | ||
/** | ||
* @return {!AuditMeta} | ||
*/ | ||
static get meta() { | ||
return { | ||
category: 'Images', | ||
name: 'uses-responsive-images', | ||
description: 'Site uses appropriate image sizes', | ||
helpText: | ||
'Image sizes served should be based on the device display size to save network bytes. ' + | ||
'Learn more about [responsive images](https://developers.google.com/web/fundamentals/design-and-ui/media/images) ' + | ||
'and [client hints](https://developers.google.com/web/updates/2015/09/automating-resource-selection-with-client-hints).', | ||
requiredArtifacts: ['ImageUsage', 'ContentWidth'] | ||
}; | ||
} | ||
|
||
/** | ||
* @param {!Object} image | ||
* @param {number} DPR devicePixelRatio | ||
* @return {?Object} | ||
*/ | ||
static computeWaste(image, DPR) { | ||
const url = URL.getDisplayName(image.src); | ||
const actualPixels = image.naturalWidth * image.naturalHeight; | ||
const usedPixels = image.clientWidth * image.clientHeight; | ||
const usedPixelsFullDPR = usedPixels * Math.pow(DPR, 2); | ||
const wastedRatio = 1 - (usedPixels / actualPixels); | ||
const wastedRatioFullDPR = 1 - (usedPixelsFullDPR / actualPixels); | ||
|
||
if (!Number.isFinite(wastedRatio)) { | ||
return new Error(`Invalid image sizing information ${url}`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. worth including size info for help debugging? could only be OTOH this may never really happen There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this theoretically should never happen and was just for catching a bug I had lower down. |
||
} else if (wastedRatio <= 0) { | ||
// Image did not have sufficient resolution to fill display at DPR=1 | ||
return null; | ||
} | ||
|
||
// TODO(phulce): use an average transfer time for data URI images | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add the issue number to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
const size = image.networkRecord.resourceSize; | ||
const transferTimeInMs = 1000 * (image.networkRecord.endTime - | ||
image.networkRecord.responseReceivedTime); | ||
const wastedBytes = Math.round(size * wastedRatio); | ||
const wastedTime = Math.round(transferTimeInMs * wastedRatio); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how confident are we that this relationship of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More confident than the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing in particular. Just thinking out loud. Maybe the final label should be clear that this is an estimate? https://github.com/GoogleChrome/lighthouse/pull/1497/files/b4fe416f62b776844a9a743864921256f01dc86a#diff-31529c3403e44b3ff053b53ab95cca7dR118 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done! |
||
const percentSavings = Math.round(100 * wastedRatio); | ||
const label = `${Math.round(size / KB_IN_BYTES)}KB total, ${percentSavings}% potential savings`; | ||
|
||
return { | ||
wastedBytes, | ||
wastedTime, | ||
isWasteful: wastedRatioFullDPR > WASTEFUL_THRESHOLD_AS_RATIO, | ||
result: {url, label}, | ||
}; | ||
} | ||
|
||
/** | ||
* @param {!Artifacts} artifacts | ||
* @return {!AuditResult} | ||
*/ | ||
static audit(artifacts) { | ||
const images = artifacts.ImageUsage; | ||
const contentWidth = artifacts.ContentWidth; | ||
|
||
let debugString; | ||
let totalWastedBytes = 0; | ||
let totalWastedTime = 0; | ||
let hasWastefulImage = false; | ||
const DPR = contentWidth.devicePixelRatio; | ||
const results = images.reduce((results, image) => { | ||
if (!image.networkRecord) { | ||
return results; | ||
} | ||
|
||
const processed = UsesResponsiveImages.computeWaste(image, DPR); | ||
if (!processed) { | ||
return results; | ||
} else if (processed instanceof Error) { | ||
debugString = processed.message; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. purposefully only using the last error (overwriting each time)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that seemed to be the pattern followed a few other places |
||
return results; | ||
} | ||
|
||
hasWastefulImage = hasWastefulImage || processed.isWasteful; | ||
totalWastedTime += processed.wastedTime; | ||
totalWastedBytes += processed.wastedBytes; | ||
results.push(processed.result); | ||
return results; | ||
}, []); | ||
|
||
let displayValue; | ||
if (results.length) { | ||
const totalWastedKB = Math.round(totalWastedBytes / KB_IN_BYTES); | ||
displayValue = `${totalWastedKB}KB potential waste took ~${totalWastedTime}ms`; | ||
} | ||
|
||
return UsesResponsiveImages.generateAuditResult({ | ||
debugString, | ||
displayValue, | ||
rawValue: !hasWastefulImage, | ||
extendedInfo: { | ||
formatter: Formatter.SUPPORTED_FORMATS.URLLIST, | ||
value: results | ||
} | ||
}); | ||
} | ||
} | ||
|
||
module.exports = UsesResponsiveImages; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,9 +24,11 @@ const Gatherer = require('./gatherer'); | |
function getContentWidth() { | ||
// window.innerWidth to get the scrollable size of the window (irrespective of zoom) | ||
// window.outerWidth to get the size of the visible area | ||
// window.devicePixelRatio to get ratio of logical pixels to physical pixels | ||
return Promise.resolve({ | ||
scrollWidth: window.innerWidth, | ||
viewportWidth: window.outerWidth | ||
viewportWidth: window.outerWidth, | ||
devicePixelRatio: window.devicePixelRatio, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we rename this gatherer? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. eh, I actually opened it expecting to find it there since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, sounds good for now. If we add another measure here we can change it :) |
||
}); | ||
} | ||
|
||
|
@@ -39,15 +41,17 @@ class ContentWidth extends Gatherer { | |
|
||
.then(returnedValue => { | ||
if (!Number.isFinite(returnedValue.scrollWidth) || | ||
!Number.isFinite(returnedValue.viewportWidth)) { | ||
!Number.isFinite(returnedValue.viewportWidth) || | ||
!Number.isFinite(returnedValue.devicePixelRatio)) { | ||
throw new Error(`ContentWidth results were not numeric: ${JSON.stringify(returnedValue)}`); | ||
} | ||
|
||
return returnedValue; | ||
}, _ => { | ||
return { | ||
scrollWidth: -1, | ||
viewportWidth: -1 | ||
viewportWidth: -1, | ||
devicePixelRatio: -1, | ||
}; | ||
}); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,157 @@ | ||
/** | ||
* @license | ||
* Copyright 2017 Google Inc. All rights reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
/** | ||
* @fileoverview Gathers all images used on the page with their src, size, | ||
* and attribute information. Executes script in the context of the page. | ||
*/ | ||
'use strict'; | ||
|
||
const Gatherer = require('./gatherer'); | ||
|
||
/* global window, document, Image, URL */ | ||
|
||
/* istanbul ignore next */ | ||
function collectImageElementInfo() { | ||
function parseSrcsetUrls(srcset) { | ||
if (!srcset) { | ||
return []; | ||
} | ||
|
||
const entries = srcset.split(','); | ||
const relativeUrls = entries.map(entry => entry.trim().split(' ')[0]); | ||
return relativeUrls.map(url => { | ||
try { | ||
return new URL(url, document.baseURI).href; | ||
} catch (e) { | ||
return url; | ||
} | ||
}); | ||
} | ||
|
||
/** | ||
* @param {!HTMLImageElement|HTMLSourceElement} element | ||
* @return {!Object} | ||
*/ | ||
function getElementInfo(element) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doc element as a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will doc |
||
return { | ||
tagName: element.tagName, | ||
// currentSrc used over src to get the url as determined by the browser | ||
// after taking into account srcset/media/sizes/etc. | ||
src: element.currentSrc, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add comment for later that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
srcset: element.srcset, | ||
srcsetUrls: parseSrcsetUrls(element.srcset), | ||
sizes: element.sizes, | ||
media: element.media, | ||
clientWidth: element.clientWidth, | ||
clientHeight: element.clientHeight, | ||
naturalWidth: element.naturalWidth, | ||
naturalHeight: element.naturalHeight, | ||
}; | ||
} | ||
|
||
return [...document.querySelectorAll('img')].map(element => { | ||
const imgElementInfo = getElementInfo(element); | ||
if (element.parentElement.tagName !== 'PICTURE') { | ||
return Object.assign(imgElementInfo, {isPicture: false}); | ||
} | ||
|
||
const sources = [...element.parentElement.children] | ||
.filter(element => element.tagName === 'SOURCE') | ||
.filter(element => !element.media || window.matchMedia(element.media).matches) | ||
.map(getElementInfo) | ||
.concat(imgElementInfo); | ||
return Object.assign(imgElementInfo, { | ||
isPicture: true, | ||
// nested chain is too deep for DevTools to handle so stringify | ||
sources: JSON.stringify(sources), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you don't appear to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah not anymore, originally I had thought we would want to see if they were already using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, maybe remove if we don't use it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
}); | ||
}); | ||
} | ||
|
||
/* istanbul ignore next */ | ||
function determineNaturalSize(url) { | ||
return new Promise((resolve, reject) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we should start reporting analytics on what gatherers are taking up the most time :) This could be another expensive one, depending on cache behavior (could be especially painful on a real device). Worth combining this with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haha, there should be very few network requests made here. It only applies to images in picture and FWIW even on server that does not use any cache headers whatsoever and images that took 28s to load the gatherer completes in <100ms There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. whoops, missed that |
||
const img = new Image(); | ||
img.addEventListener('error', reject); | ||
img.addEventListener('load', () => { | ||
resolve({ | ||
naturalWidth: img.naturalWidth, | ||
naturalHeight: img.naturalHeight | ||
}); | ||
}); | ||
|
||
img.src = url; | ||
}); | ||
} | ||
|
||
class ImageUsage extends Gatherer { | ||
|
||
/** | ||
* @param {{src: string}} element | ||
* @return {!Promise<!Object>} | ||
*/ | ||
fetchElementWithSizeInformation(element) { | ||
const url = JSON.stringify(element.src); | ||
return this.driver.evaluateAsync(`(${determineNaturalSize.toString()})(${url})`) | ||
.then(size => { | ||
return Object.assign(element, size); | ||
}); | ||
} | ||
|
||
afterPass(options, traceData) { | ||
const driver = this.driver = options.driver; | ||
const indexedNetworkRecords = traceData.networkRecords.reduce((map, record) => { | ||
if (/^image/.test(record._mimeType)) { | ||
map[record._url] = { | ||
url: record.url, | ||
resourceSize: record.resourceSize, | ||
startTime: record.startTime, | ||
endTime: record.endTime, | ||
responseReceivedTime: record.responseReceivedTime | ||
}; | ||
} | ||
|
||
return map; | ||
}, {}); | ||
|
||
return driver.evaluateAsync(`(${collectImageElementInfo.toString()})()`) | ||
.then(elements => { | ||
return elements.reduce((promise, element) => { | ||
return promise.then(collector => { | ||
// rehydrate the sources property | ||
element.sources = element.sources && JSON.parse(element.sources); | ||
// link up the image with its network record | ||
element.networkRecord = indexedNetworkRecords[element.src]; | ||
|
||
// Images within `picture` behave strangely and natural size information | ||
// isn't accurate. Try to get the actual size if we can. | ||
const elementPromise = element.isPicture && element.networkRecord ? | ||
this.fetchElementWithSizeInformation(element) : | ||
Promise.resolve(element); | ||
|
||
return elementPromise.then(element => { | ||
collector.push(element); | ||
return collector; | ||
}); | ||
}); | ||
}, Promise.resolve([])); | ||
}); | ||
} | ||
} | ||
|
||
module.exports = ImageUsage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL about
naturalWidth
andnaturalHeight
:)