Skip to content
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.

Sign up for GitHub

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

Jump to bottom

core(full-page-screenshot): leave emulated width unchanged #13643

Merged
merged 10 commits into from
Mar 8, 2022
Merged

core(full-page-screenshot): leave emulated width unchanged #13643

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dd6db6b
core(full-page-screenshot): leave emulated width unchanged
adamraine Feb 8, 2022
d6b15cb
review
adamraine Feb 9, 2022
8e7f036
Merge branch 'master' into fps-scale
adamraine Feb 16, 2022
6e30d53
hold dpr
adamraine Feb 18, 2022
c258a74
comments
adamraine Feb 18, 2022
15d7e1d
test
adamraine Mar 1, 2022
de92e5d
Merge branch 'master' into fps-scale
adamraine Mar 1, 2022
66147e1
Merge remote-tracking branch 'origin/master' into fps-scale
connorjclark Mar 8, 2022
dc9f4c8
fix
adamraine Mar 8, 2022
f49b1c7
Merge branch 'fps-scale' of github.com:GoogleChrome/lighthouse into f…
adamraine Mar 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 33 additions & 25 deletions lighthouse-core/gather/gatherers/full-page-screenshot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,21 +70,22 @@ class FullPageScreenshot extends FRGatherer {

/**
* @param {LH.Gatherer.FRTransitionalContext} context
* @param {{height: number, width: number, mobile: boolean}} deviceMetrics
* @return {Promise<LH.Artifacts.FullPageScreenshot['screenshot']>}
*/
async _takeScreenshot(context) {
async _takeScreenshot(context, deviceMetrics) {
const session = context.driver.defaultSession;
const maxTextureSize = await this.getMaxTextureSize(context);
const metrics = await session.sendCommand('Page.getLayoutMetrics');

// Width should match emulated width, without considering content overhang.
// Both layoutViewport and visualViewport capture this. visualViewport accounts
// for page zoom/scale, which we currently don't account for (or expect). So we use layoutViewport.width.
// Note: If the page is zoomed, many assumptions fail.
//
// Height should be as tall as the content. So we use contentSize.height
const width = Math.min(metrics.layoutViewport.clientWidth, maxTextureSize);
const height = Math.min(metrics.contentSize.height, maxTextureSize);
// Height should be as tall as the content.
// Scale the emulated height to reach the content height.
const fullHeight = Math.round(
deviceMetrics.height *
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i definitely don't follow how this results in a meaningful number.... but.... i can confirm that it does indeed result in something good. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

metrics.contentSize.height represents the desired metrics.layoutViewport.clientHeight for the FPS, and we control metrics.layoutViewport.clientHeight by setting the emulated height over CDP.

However, the emulated height (deviceMetrics.height) is not necessarily equivalent to metrics.layoutViewport.clientHeight (because of DPR maybe).

So we use the ratio of metrics.contentSize.height/metrics.layoutViewport.clientHeight to determine how much we need to scale the emulated height to arrive at the desired metrics.layoutViewport.clientHeight.

metrics.contentSize.height /
metrics.layoutViewport.clientHeight
);
const height = Math.min(fullHeight, maxTextureSize);

// Setup network monitor before we change the viewport.
const networkMonitor = new NetworkMonitor(session);
Expand All @@ -98,13 +99,10 @@ class FullPageScreenshot extends FRGatherer {
await networkMonitor.enable();

await session.sendCommand('Emulation.setDeviceMetricsOverride', {
// If we're gathering with mobile screenEmulation on (overlay scrollbars, etc), continue to use that for this screenshot.
mobile: context.settings.screenEmulation.mobile,
height,
width,
mobile: deviceMetrics.mobile,
deviceScaleFactor: 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the https://output.jsbin.com/koqamuh/4/quiet test case does find a related problem in this branch..

on master the colorcontrast results work all the way up to 16300 px
image
but in this branch it wraps around at 16300/2:
image
this wrapping around at maxtexturesize/dpr is behavior we worked around in #11688 (i think)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah setting a DPR of 1 will also reduce the image "filesize". Going off #11121 (comment) I'm fine keeping the DPR at 1 for the FPS. The content change in #13642 appears to be from us changing the viewport width anyway.

scale: 1,
screenOrientation: {angle: 0, type: 'portraitPrimary'},
height,
width: 0, // Leave width unchanged
});

// Now that the viewport is taller, give the page some time to fetch new resources that
Expand All @@ -127,7 +125,7 @@ class FullPageScreenshot extends FRGatherer {

return {
data,
width,
width: deviceMetrics.width,
height,
};
}
Expand Down Expand Up @@ -185,29 +183,37 @@ class FullPageScreenshot extends FRGatherer {
const session = context.driver.defaultSession;
const executionContext = context.driver.executionContext;
const settings = context.settings;

// In case some other program is controlling emulation, remember what the device looks
// like now and reset after gatherer is done.
let observedDeviceMetrics;
const lighthouseControlsEmulation = !settings.screenEmulation.disabled;

/**
* In case some other program is controlling emulation, remember what the device looks like now and reset after gatherer is done.
* If we're gathering with mobile screenEmulation on (overlay scrollbars, etc), continue to use that for this screenshot.
* @type {{width: number, height: number, deviceScaleFactor: number, mobile: boolean}}
*/
adamraine marked this conversation as resolved.
Show resolved Hide resolved
const deviceMetrics = settings.screenEmulation;
if (!lighthouseControlsEmulation) {
observedDeviceMetrics = await executionContext.evaluate(getObservedDeviceMetrics, {
const observedDeviceMetrics = await executionContext.evaluate(getObservedDeviceMetrics, {
args: [],
useIsolation: true,
deps: [kebabCaseToCamelCase],
});
deviceMetrics.height = observedDeviceMetrics.height;
deviceMetrics.width = observedDeviceMetrics.width;
deviceMetrics.deviceScaleFactor = observedDeviceMetrics.deviceScaleFactor;
// If screen emulation is disabled, use formFactor to determine if we are on mobile.
deviceMetrics.mobile = settings.formFactor === 'mobile';
}

try {
return {
screenshot: await this._takeScreenshot(context),
screenshot: await this._takeScreenshot(context, deviceMetrics),
nodes: await this._resolveNodes(context),
};
} finally {
// Revert resized page.
if (lighthouseControlsEmulation) {
await emulation.emulate(session, settings);
} else if (observedDeviceMetrics) {
} else {
// Best effort to reset emulation to what it was.
// https://github.com/GoogleChrome/lighthouse/pull/10716#discussion_r428970681
// TODO: seems like this would be brittle. Should at least work for devtools, but what
Expand All @@ -216,8 +222,10 @@ class FullPageScreenshot extends FRGatherer {
// and then just call that to reset?
// https://github.com/GoogleChrome/lighthouse/issues/11122
await session.sendCommand('Emulation.setDeviceMetricsOverride', {
mobile: settings.formFactor === 'mobile',
...observedDeviceMetrics,
mobile: deviceMetrics.mobile,
deviceScaleFactor: deviceMetrics.deviceScaleFactor,
height: deviceMetrics.height,
width: 0, // Leave width unchanged
});
}
}
Expand Down
43 changes: 30 additions & 13 deletions lighthouse-core/test/gather/gatherers/full-page-screenshot-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,15 @@ jest.setTimeout(10_000);

beforeEach(() => {
contentSize = {width: 100, height: 100};
screenSize = {dpr: 1};
screenSize = {width: 100, height: 100, dpr: 1};
screenshotData = [];
mockContext = createMockContext();
mockContext.driver.defaultSession.sendCommand.mockImplementation(method => {
mockContext.driver.defaultSession.sendCommand.mockImplementation((method) => {
if (method === 'Page.getLayoutMetrics') {
return {
contentSize,
// See comment within _takeScreenshot() implementation
layoutViewport: {clientWidth: contentSize.width, clientHeight: contentSize.height},
layoutViewport: {clientWidth: screenSize.width, clientHeight: screenSize.height},
};
}
if (method === 'Page.captureScreenshot') {
Expand Down Expand Up @@ -76,10 +76,13 @@ describe('FullPageScreenshot gatherer', () => {
it('captures a full-page screenshot', async () => {
const fpsGatherer = new FullPageScreenshotGatherer();
contentSize = {width: 412, height: 2000};
screenSize = {width: 412, height: 412};

mockContext.settings = {
formFactor: 'mobile',
screenEmulation: {
height: screenSize.height,
width: screenSize.width,
mobile: true,
disabled: false,
},
Expand All @@ -99,17 +102,29 @@ describe('FullPageScreenshot gatherer', () => {
it('resets the emulation correctly when Lighthouse controls it', async () => {
const fpsGatherer = new FullPageScreenshotGatherer();
contentSize = {width: 412, height: 2000};
screenSize = {width: 412, height: 412};

mockContext.settings = {
formFactor: 'mobile',
screenEmulation: {
height: screenSize.height,
width: screenSize.width,
mobile: true,
disabled: false,
},
};

await fpsGatherer.getArtifact(mockContext.asContext());

const expectedArgs = {formFactor: 'mobile', screenEmulation: {disabled: false, mobile: true}};
const expectedArgs = {
formFactor: 'mobile',
screenEmulation: {
height: 412,
width: 412,
disabled: false,
mobile: true,
},
};
expect(mocks.emulationMock.emulate).toHaveBeenCalledTimes(1);
expect(mocks.emulationMock.emulate).toHaveBeenCalledWith(
mockContext.driver.defaultSession,
Expand All @@ -123,6 +138,8 @@ describe('FullPageScreenshot gatherer', () => {
screenSize = {width: 500, height: 500, dpr: 2};
mockContext.settings = {
screenEmulation: {
height: screenSize.height,
width: screenSize.width,
mobile: true,
disabled: true,
},
Expand All @@ -138,7 +155,7 @@ describe('FullPageScreenshot gatherer', () => {
mobile: true,
deviceScaleFactor: 1,
height: 1500,
width: 500,
width: 0,
})
);

Expand All @@ -149,11 +166,7 @@ describe('FullPageScreenshot gatherer', () => {
mobile: true,
deviceScaleFactor: 2,
height: 500,
width: 500,
screenOrientation: {
type: 'landscapePrimary',
angle: 30,
},
width: 0,
})
);
});
Expand All @@ -162,10 +175,12 @@ describe('FullPageScreenshot gatherer', () => {
const fpsGatherer = new FullPageScreenshotGatherer();

contentSize = {width: 412, height: 100000};
screenSize = {dpr: 1};
screenSize = {width: 412, height: 412, dpr: 1};
mockContext.settings = {
formFactor: 'mobile',
screenEmulation: {
height: screenSize.height,
width: screenSize.width,
mobile: true,
disabled: false,
},
Expand All @@ -175,10 +190,12 @@ describe('FullPageScreenshot gatherer', () => {

expect(mockContext.driver.defaultSession.sendCommand).toHaveBeenCalledWith(
'Emulation.setDeviceMetricsOverride',
expect.objectContaining({
{
mobile: true,
deviceScaleFactor: 1,
width: 0,
height: maxTextureSizeMock,
})
}
);
});
});