-
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
core(full-page-screenshot): add full page screenshot to artifacts #9064
Changes from all commits
2ab034b
f53b83c
4d03200
c9f1593
012b81e
7125d42
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,39 @@ | ||
/** | ||
* @license Copyright 2019 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. | ||
*/ | ||
'use strict'; | ||
|
||
const Audit = require('./audit.js'); | ||
|
||
class FullPageScreenshot extends Audit { | ||
/** | ||
* @return {LH.Audit.Meta} | ||
*/ | ||
static get meta() { | ||
return { | ||
id: 'full-page-screenshot', | ||
scoreDisplayMode: Audit.SCORING_MODES.INFORMATIVE, | ||
title: 'Full-page screenshot', | ||
description: 'A full-height screenshot of the final rendered page', | ||
requiredArtifacts: ['FullPageScreenshot'], | ||
}; | ||
} | ||
|
||
/** | ||
* @param {LH.Artifacts} artifacts | ||
* @return {Promise<LH.Audit.Product>} | ||
*/ | ||
static async audit(artifacts) { | ||
return { | ||
score: 1, | ||
details: { | ||
type: 'screenshot', | ||
...artifacts.FullPageScreenshot, | ||
}, | ||
}; | ||
} | ||
} | ||
|
||
module.exports = FullPageScreenshot; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1423,13 +1423,15 @@ class Driver { | |
|
||
/** | ||
* @param {LH.Config.Settings} settings | ||
* @param {{height?: number, screenHeight: number?, deviceScaleFactor?: number}} [deviceMetricOverrides] | ||
* Override device size for taking screenshots. | ||
* @return {Promise<void>} | ||
*/ | ||
async beginEmulation(settings) { | ||
async beginEmulation(settings, deviceMetricOverrides) { | ||
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. my initial thought looking at this was that I was worried about creating a nexus 5x with arbitrary screen height seems counter-intuitive at first :) |
||
if (settings.emulatedFormFactor === 'mobile') { | ||
await emulation.enableNexus5X(this); | ||
await emulation.enableNexus5X(this, deviceMetricOverrides); | ||
} else if (settings.emulatedFormFactor === 'desktop') { | ||
await emulation.enableDesktop(this); | ||
await emulation.enableDesktop(this, deviceMetricOverrides); | ||
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. Would it be appropriate to hook the options on the driver itself? 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. What do you mean by 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. If the extra param could be removed in favor for having it as a prop on 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. I'm not sure we want to clutter driver with additional properties just for the method here. It's a pretty busy class already 😆 |
||
} | ||
|
||
await this.setThrottling(settings, {useThrottling: true}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
/** | ||
* @license Copyright 2019 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. | ||
*/ | ||
'use strict'; | ||
|
||
const Gatherer = require('./gatherer.js'); | ||
|
||
// JPEG quality setting | ||
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 we link to that awesome exploration doc you made for how we landed on 30? |
||
// Exploration and examples of reports using different quality settings: https://docs.google.com/document/d/1ZSffucIca9XDW2eEwfoevrk-OTl7WQFeMf0CgeJAA8M/edit# | ||
const FULL_PAGE_SCREENSHOT_QUALITY = 30; | ||
// Maximum screenshot height in Chrome https://bugs.chromium.org/p/chromium/issues/detail?id=770769 | ||
const MAX_SCREENSHOT_HEIGHT = 16384; | ||
// Maximum data URL size in Chrome https://bugs.chromium.org/p/chromium/issues/detail?id=69227 | ||
const MAX_DATA_URL_SIZE = 2 * 1024 * 1024; | ||
|
||
class FullPageScreenshot extends Gatherer { | ||
/** | ||
* @param {LH.Gatherer.PassContext} passContext | ||
* @param {number} maxScreenshotHeight | ||
* @return {Promise<LH.Artifacts.FullPageScreenshot>} | ||
*/ | ||
async _takeScreenshot(passContext, maxScreenshotHeight) { | ||
const driver = passContext.driver; | ||
const metrics = await driver.sendCommand('Page.getLayoutMetrics'); | ||
const width = await driver.evaluateAsync(`window.innerWidth`); | ||
const height = Math.min(metrics.contentSize.height, maxScreenshotHeight); | ||
|
||
await driver.beginEmulation(passContext.settings, { | ||
height, | ||
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. mostly talking to the air here did we ever figure out what it sets the screen rect but not sure what impact that has... seems like it notifies some observers and kicks off orientation changes, but doesn't actually affect viewport size? 🤷♂ 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. screen size is the physical device size (DIP): https://cs.chromium.org/chromium/src/third_party/blink/public/web/web_device_emulation_params.h?l=26&rcl=c6f1509e8c13f8b0739c99b343d628a38f8a9da7 |
||
screenHeight: height, | ||
deviceScaleFactor: 1, | ||
}); | ||
|
||
const result = await driver.sendCommand('Page.captureScreenshot', { | ||
format: 'jpeg', | ||
quality: FULL_PAGE_SCREENSHOT_QUALITY, | ||
}); | ||
const data = 'data:image/jpeg;base64,' + result.data; | ||
|
||
// Revert resized page | ||
await driver.beginEmulation(passContext.settings); | ||
|
||
return { | ||
data, | ||
width, | ||
height, | ||
}; | ||
} | ||
|
||
/** | ||
* @param {LH.Gatherer.PassContext} passContext | ||
* @return {Promise<LH.Artifacts.FullPageScreenshot>} | ||
*/ | ||
async afterPass(passContext) { | ||
let screenshot = await this._takeScreenshot(passContext, MAX_SCREENSHOT_HEIGHT); | ||
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 scroll to the top of the page first? I think devtools does that. |
||
|
||
if (screenshot.data.length > MAX_DATA_URL_SIZE) { | ||
// Hitting the data URL size limit is rare, it only happens for pages on tall | ||
// desktop sites with lots of images. | ||
// So just cutting down the height a bit fixes the issue. | ||
screenshot = await this._takeScreenshot(passContext, 5000); | ||
} | ||
|
||
return screenshot; | ||
} | ||
} | ||
|
||
module.exports = FullPageScreenshot; | ||
module.exports.MAX_SCREENSHOT_HEIGHT = MAX_SCREENSHOT_HEIGHT; |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -64,30 +64,51 @@ const NO_CPU_THROTTLE_METRICS = { | |||||
|
||||||
/** | ||||||
* @param {Driver} driver | ||||||
* @return {Promise<void>} | ||||||
* @param {EnableEmulationOptions} emulationOptions | ||||||
*/ | ||||||
async function enableNexus5X(driver) { | ||||||
await Promise.all([ | ||||||
driver.sendCommand('Emulation.setDeviceMetricsOverride', NEXUS5X_EMULATION_METRICS), | ||||||
async function enableEmulation( | ||||||
driver, | ||||||
{deviceMetrics, setTouchEmulationEnabled, userAgent, deviceMetricOverrides} | ||||||
) { | ||||||
if (deviceMetricOverrides) { | ||||||
deviceMetrics = Object.assign({}, deviceMetrics, deviceMetricOverrides); | ||||||
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.
Suggested change
|
||||||
} | ||||||
|
||||||
return Promise.all([ | ||||||
driver.sendCommand('Emulation.setDeviceMetricsOverride', deviceMetrics), | ||||||
// Network.enable must be called for UA overriding to work | ||||||
driver.sendCommand('Network.enable'), | ||||||
driver.sendCommand('Network.setUserAgentOverride', {userAgent: NEXUS5X_USERAGENT}), | ||||||
driver.sendCommand('Emulation.setTouchEmulationEnabled', {enabled: true}), | ||||||
driver.sendCommand('Network.setUserAgentOverride', {userAgent}), | ||||||
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 be balancing domain enable / disable? |
||||||
driver.sendCommand('Emulation.setTouchEmulationEnabled', {enabled: setTouchEmulationEnabled}), | ||||||
]); | ||||||
} | ||||||
|
||||||
/** | ||||||
* @param {Driver} driver | ||||||
* @param {DeviceMetricOverrides} [deviceMetricOverrides] | ||||||
* @return {Promise<void>} | ||||||
*/ | ||||||
async function enableDesktop(driver) { | ||||||
await Promise.all([ | ||||||
driver.sendCommand('Emulation.setDeviceMetricsOverride', DESKTOP_EMULATION_METRICS), | ||||||
// Network.enable must be called for UA overriding to work | ||||||
driver.sendCommand('Network.enable'), | ||||||
driver.sendCommand('Network.setUserAgentOverride', {userAgent: DESKTOP_USERAGENT}), | ||||||
driver.sendCommand('Emulation.setTouchEmulationEnabled', {enabled: false}), | ||||||
]); | ||||||
async function enableNexus5X(driver, deviceMetricOverrides) { | ||||||
await enableEmulation(driver, { | ||||||
deviceMetrics: NEXUS5X_EMULATION_METRICS, | ||||||
userAgent: NEXUS5X_USERAGENT, | ||||||
setTouchEmulationEnabled: true, | ||||||
deviceMetricOverrides, | ||||||
}); | ||||||
} | ||||||
|
||||||
/** | ||||||
* @param {Driver} driver | ||||||
* @param {DeviceMetricOverrides} [deviceMetricOverrides] | ||||||
* @return {Promise<void>} | ||||||
*/ | ||||||
async function enableDesktop(driver, deviceMetricOverrides) { | ||||||
await enableEmulation(driver, { | ||||||
deviceMetrics: DESKTOP_EMULATION_METRICS, | ||||||
userAgent: DESKTOP_USERAGENT, | ||||||
setTouchEmulationEnabled: false, | ||||||
deviceMetricOverrides, | ||||||
}); | ||||||
} | ||||||
|
||||||
/** | ||||||
|
@@ -155,3 +176,16 @@ module.exports = { | |||||
MOBILE_USERAGENT: NEXUS5X_USERAGENT, | ||||||
DESKTOP_USERAGENT, | ||||||
}; | ||||||
|
||||||
/** @typedef {{ | ||||||
* deviceMetrics: LH.Crdp.Emulation.SetDeviceMetricsOverrideRequest; | ||||||
* userAgent: string; | ||||||
* setTouchEmulationEnabled: boolean; | ||||||
* deviceMetricOverrides?: DeviceMetricOverrides; | ||||||
* }} EnableEmulationOptions */ | ||||||
|
||||||
/** @typedef {{ | ||||||
* height?: number; | ||||||
* screenHeight: number?; | ||||||
* deviceScaleFactor?: number; | ||||||
* }} DeviceMetricOverrides */ |
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.