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

Author: adamraine

lighthouse-core/gather/gatherers/full-page-screenshot.js

@@ -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 *
+      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);
@@ -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,
-      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
@@ -127,7 +125,7 @@ class FullPageScreenshot extends FRGatherer {
 
     return {
       data,
-      width,
+      width: deviceMetrics.width,
       height,
     };
   }
@@ -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;
+
+    // Make a copy so we don't modify the config settings.
+    /** @type {{width: number, height: number, deviceScaleFactor: number, mobile: boolean}} */
+    const deviceMetrics = {...settings.screenEmulation};
+
+    // 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.
     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
@@ -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
         });
       }
     }

lighthouse-core/test/gather/gatherers/full-page-screenshot-test.js

@@ -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') {
@@ -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,
       },
@@ -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,
@@ -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,
       },
@@ -138,7 +155,7 @@ describe('FullPageScreenshot gatherer', () => {
         mobile: true,
         deviceScaleFactor: 1,
         height: 1500,
-        width: 500,
+        width: 0,
       })
     );
 
@@ -149,11 +166,7 @@ describe('FullPageScreenshot gatherer', () => {
         mobile: true,
         deviceScaleFactor: 2,
         height: 500,
-        width: 500,
-        screenOrientation: {
-          type: 'landscapePrimary',
-          angle: 30,
-        },
+        width: 0,
       })
     );
   });
@@ -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,
       },
@@ -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,
-      })
+      }
     );
   });
 });