[screenshotting]fix resize during capture [8.2] (#132295)

* [screenshotting]fix resize during capture [8.2]

* Update x-pack/plugins/screenshotting/server/screenshots/observable.ts

* fix test

x-pack/plugins/screenshotting/server/browsers/chromium/driver.ts

@@ -34,12 +34,6 @@ export interface ElementPosition {
   };
 }
 
-export interface Viewport {
-  zoom: number;
-  width: number;
-  height: number;
-}
-
 interface OpenOptions {
   context?: Context;
   headers: Headers;
@@ -156,7 +150,7 @@ export class HeadlessChromiumDriver {
   }
 
   /*
-   * Call Page.screenshot and return a base64-encoded string of the image
+   * Receive a PNG buffer of the page screenshot from Chromium
    */
   async screenshot(elementPosition: ElementPosition): Promise<Buffer | undefined> {
     const { boundingClientRect, scroll } = elementPosition;
@@ -167,6 +161,7 @@ export class HeadlessChromiumDriver {
         height: boundingClientRect.height,
         width: boundingClientRect.width,
       },
+      captureBeyondViewport: false, // workaround for an internal resize. See: https://github.com/puppeteer/puppeteer/issues/7043
     });
 
     if (Buffer.isBuffer(screenshot)) {
@@ -216,14 +211,18 @@ export class HeadlessChromiumDriver {
     await this.page.waitForFunction(fn, { timeout, polling: WAIT_FOR_DELAY_MS }, ...args);
   }
 
+  /**
+   * Setting the viewport is required to ensure that all capture elements are visible: anything not in the
+   * viewport can not be captured.
+   */
   async setViewport(
-    { width: _width, height: _height, zoom }: Viewport,
+    { width: _width, height: _height, zoom }: { zoom: number; width: number; height: number },
     logger: Logger
   ): Promise<void> {
     const width = Math.floor(_width);
     const height = Math.floor(_height);
 
-    logger.debug(`Setting viewport to: width=${width} height=${height} zoom=${zoom}`);
+    logger.debug(`Setting viewport to: width=${width} height=${height} scaleFactor=${zoom}`);
 
     await this.page.setViewport({
       width,

x-pack/plugins/screenshotting/server/browsers/chromium/driver_factory/index.test.ts

@@ -10,8 +10,8 @@ import * as Rx from 'rxjs';
 import { mergeMap, take } from 'rxjs/operators';
 import type { Logger } from 'src/core/server';
 import type { ScreenshotModePluginSetup } from 'src/plugins/screenshot_mode/server';
+import { DEFAULT_VIEWPORT, HeadlessChromiumDriverFactory } from '.';
 import { ConfigType } from '../../../config';
-import { HeadlessChromiumDriverFactory, DEFAULT_VIEWPORT } from '.';
 
 jest.mock('puppeteer');
 

x-pack/plugins/screenshotting/server/browsers/chromium/driver_factory/index.ts

@@ -7,43 +7,37 @@
 
 import { getDataPath } from '@kbn/utils';
 import { spawn } from 'child_process';
-import _ from 'lodash';
 import del from 'del';
 import fs from 'fs';
 import { uniq } from 'lodash';
 import path from 'path';
-import puppeteer, { Browser, ConsoleMessage, HTTPRequest, Page } from 'puppeteer';
+import puppeteer, { Browser, ConsoleMessage, HTTPRequest, Page, Viewport } from 'puppeteer';
 import { createInterface } from 'readline';
 import * as Rx from 'rxjs';
 import { InnerSubscriber } from 'rxjs/internal/InnerSubscriber';
 import {
   catchError,
+  concatMap,
   ignoreElements,
   map,
-  concatMap,
   mergeMap,
   reduce,
   takeUntil,
   tap,
 } from 'rxjs/operators';
 import type { Logger } from 'src/core/server';
 import type { ScreenshotModePluginSetup } from 'src/plugins/screenshot_mode/server';
-import { ConfigType } from '../../../config';
-import { errors } from '../../../../common';
 import { getChromiumDisconnectedError } from '../';
+import { errors } from '../../../../common';
+import { ConfigType } from '../../../config';
 import { safeChildProcess } from '../../safe_child_process';
 import { HeadlessChromiumDriver } from '../driver';
 import { args } from './args';
 import { getMetrics, PerformanceMetrics } from './metrics';
 
 interface CreatePageOptions {
   browserTimezone?: string;
-  defaultViewport: {
-    /** Size in pixels */
-    width?: number;
-    /** Size in pixels */
-    height?: number;
-  };
+  defaultViewport: { width?: number };
   openUrlTimeout: number;
 }
 
@@ -64,10 +58,16 @@ interface ClosePageResult {
   metrics?: PerformanceMetrics;
 }
 
-export const DEFAULT_VIEWPORT = {
-  width: 1950,
-  height: 1200,
-};
+/**
+ * Size of the desired initial viewport. This is needed to render the app before elements load into their
+ * layout. Once the elements are positioned, the viewport must be *resized* to include the entire element container.
+ */
+export const DEFAULT_VIEWPORT: Required<Pick<Viewport, 'width' | 'height' | 'deviceScaleFactor'>> =
+  {
+    width: 1950,
+    height: 1200,
+    deviceScaleFactor: 1,
+  };
 
 // Default args used by pptr
 // https://github.com/puppeteer/puppeteer/blob/13ea347/src/node/Launcher.ts#L168
@@ -142,6 +142,17 @@ export class HeadlessChromiumDriverFactory {
       const logger = pLogger.get('browser-driver');
       logger.info(`Creating browser page driver`);
 
+      // We set the viewport width using the client-side layout info to reduce the chances of
+      // browser reflow. Only the window height is expected to be adjusted dramatically
+      // before taking a screenshot, to ensure the elements to capture are contained in the viewport.
+      const viewport = {
+        ...DEFAULT_VIEWPORT,
+        width: defaultViewport.width ?? DEFAULT_VIEWPORT.width,
+      };
+
+      logger.debug(
+        `Launching with viewport: width=${viewport.width} height=${viewport.height} scaleFactor=${viewport.deviceScaleFactor}`
+      );
       const chromiumArgs = this.getChromiumArgs();
       logger.debug(`Chromium launch args set to: ${chromiumArgs}`);
 
@@ -154,13 +165,7 @@ export class HeadlessChromiumDriverFactory {
           ignoreHTTPSErrors: true,
           handleSIGHUP: false,
           args: chromiumArgs,
-
-          // We optionally set this at page creation to reduce the chances of
-          // browser reflow. In most cases only the height needs to be adjusted
-          // before taking a screenshot.
-          // NOTE: _.defaults assigns to the target object, so we copy it.
-          // NOTE NOTE: _.defaults is not the same as { ...DEFAULT_VIEWPORT, ...defaultViewport }
-          defaultViewport: _.defaults({ ...defaultViewport }, DEFAULT_VIEWPORT),
+          defaultViewport: viewport,
           env: {
             TZ: browserTimezone,
           },

x-pack/plugins/screenshotting/server/browsers/mock.ts

@@ -6,16 +6,17 @@
  */
 
 import { NEVER, of } from 'rxjs';
-import type { HeadlessChromiumDriver, HeadlessChromiumDriverFactory } from './chromium';
 import {
-  CONTEXT_SKIPTELEMETRY,
+  CONTEXT_DEBUG,
+  CONTEXT_ELEMENTATTRIBUTES,
   CONTEXT_GETNUMBEROFITEMS,
+  CONTEXT_GETRENDERERRORS,
+  CONTEXT_GETTIMERANGE,
   CONTEXT_INJECTCSS,
+  CONTEXT_SKIPTELEMETRY,
   CONTEXT_WAITFORRENDER,
-  CONTEXT_GETTIMERANGE,
-  CONTEXT_ELEMENTATTRIBUTES,
-  CONTEXT_GETRENDERERRORS,
 } from '../screenshots/constants';
+import type { HeadlessChromiumDriver, HeadlessChromiumDriverFactory } from './chromium';
 
 const selectors = {
   renderComplete: 'renderedSelector',
@@ -40,6 +41,7 @@ function getElementsPositionAndAttributes(title: string, description: string) {
 export function createMockBrowserDriver(): jest.Mocked<HeadlessChromiumDriver> {
   const evaluate = jest.fn(async (_, { context }) => {
     switch (context) {
+      case CONTEXT_DEBUG:
       case CONTEXT_SKIPTELEMETRY:
       case CONTEXT_INJECTCSS:
       case CONTEXT_WAITFORRENDER:

x-pack/plugins/screenshotting/server/layouts/base_layout.ts

@@ -48,9 +48,16 @@ export abstract class BaseLayout {
     pageSizeParams: PageSizeParams
   ): CustomPageSize | PredefinedPageSize;
 
-  // Return the dimensions unscaled dimensions (before multiplying the zoom factor)
-  // driver.setViewport() Adds a top and left margin to the viewport, and then multiplies by the scaling factor
-  public abstract getViewport(itemsCount: number): ViewZoomWidthHeight | null;
+  /**
+   * Return the unscaled dimensions (before multiplying the zoom factor)
+   *
+   * `itemsCount` is only needed for the `print` layout implementation, where the number of items to capture
+   * affects the viewport size
+   *
+   * @param {number} [itemsCount=1] - The number of items to capture. Default is 1.
+   * @returns ViewZoomWidthHeight - Viewport data
+   */
+  public abstract getViewport(itemsCount?: number): ViewZoomWidthHeight | null;
 
   public abstract getBrowserZoom(): number;
 

x-pack/plugins/screenshotting/server/layouts/create_layout.test.ts

@@ -61,6 +61,7 @@ describe('Create Layout', () => {
         },
         "useReportingBranding": true,
         "viewport": Object {
+          "deviceScaleFactor": 1,
           "height": 1200,
           "width": 1950,
         },

x-pack/plugins/screenshotting/server/layouts/print_layout.ts

@@ -6,10 +6,10 @@
  */
 
 import { PageOrientation, PredefinedPageSize } from 'pdfmake/interfaces';
-import type { LayoutParams, LayoutSelectorDictionary } from '../../common/layout';
-import { LayoutTypes } from '../../common';
 import type { Layout } from '.';
 import { DEFAULT_SELECTORS } from '.';
+import { LayoutTypes } from '../../common';
+import type { LayoutParams, LayoutSelectorDictionary } from '../../common/layout';
 import { DEFAULT_VIEWPORT } from '../browsers';
 import { BaseLayout } from './base_layout';
 
@@ -40,7 +40,7 @@ export class PrintLayout extends BaseLayout implements Layout {
     return this.zoom;
   }
 
-  public getViewport(itemsCount: number) {
+  public getViewport(itemsCount = 1) {
     return {
       zoom: this.zoom,
       width: this.viewport.width,

x-pack/plugins/screenshotting/server/screenshots/constants.ts

@@ -17,3 +17,4 @@ export const CONTEXT_ELEMENTATTRIBUTES = 'ElementPositionAndAttributes';
 export const CONTEXT_WAITFORELEMENTSTOBEINDOM = 'WaitForElementsToBeInDOM';
 export const CONTEXT_SKIPTELEMETRY = 'SkipTelemetry';
 export const CONTEXT_READMETADATA = 'ReadVisualizationsMetadata';
+export const CONTEXT_DEBUG = 'Debug';

x-pack/plugins/screenshotting/server/screenshots/get_element_position_data.test.ts

@@ -28,11 +28,12 @@ describe('getElementPositionAndAttributes', () => {
 
       elements.forEach((element) =>
         Object.assign(element, {
+          scrollIntoView: () => {},
           getBoundingClientRect: () => ({
             width: parseFloat(element.style.width),
             height: parseFloat(element.style.height),
-            top: parseFloat(element.style.top),
-            left: parseFloat(element.style.left),
+            y: parseFloat(element.style.top),
+            x: parseFloat(element.style.left),
           }),
         })
       );

x-pack/plugins/screenshotting/server/screenshots/get_element_position_data.ts

@@ -54,9 +54,8 @@ export const getElementPositionAndAttributes = async (
             results.push({
               position: {
                 boundingClientRect: {
-                  // modern browsers support x/y, but older ones don't
-                  top: boundingClientRect.y || boundingClientRect.top,
-                  left: boundingClientRect.x || boundingClientRect.left,
+                  top: boundingClientRect.y,
+                  left: boundingClientRect.x,
                   width: boundingClientRect.width,
                   height: boundingClientRect.height,
                 },

x-pack/plugins/screenshotting/server/screenshots/get_screenshots.test.ts

@@ -7,6 +7,8 @@
 
 import type { Logger } from 'src/core/server';
 import { createMockBrowserDriver } from '../browsers/mock';
+import { Layout } from '../layouts';
+import { createMockLayout } from '../layouts/mock';
 import { getScreenshots } from './get_screenshots';
 
 describe('getScreenshots', () => {
@@ -28,20 +30,22 @@ describe('getScreenshots', () => {
   ];
   let browser: ReturnType<typeof createMockBrowserDriver>;
   let logger: jest.Mocked<Logger>;
+  let layout: Layout;
 
   beforeEach(async () => {
     browser = createMockBrowserDriver();
     logger = { info: jest.fn() } as unknown as jest.Mocked<Logger>;
 
     browser.evaluate.mockImplementation(({ fn, args }) => (fn as Function)(...args));
+    layout = createMockLayout();
   });
 
   afterEach(() => {
     jest.clearAllMocks();
   });
 
   it('should return screenshots', async () => {
-    await expect(getScreenshots(browser, logger, elementsPositionAndAttributes)).resolves
+    await expect(getScreenshots(browser, logger, elementsPositionAndAttributes, layout)).resolves
       .toMatchInlineSnapshot(`
             Array [
               Object {
@@ -87,7 +91,7 @@ describe('getScreenshots', () => {
   });
 
   it('should forward elements positions', async () => {
-    await getScreenshots(browser, logger, elementsPositionAndAttributes);
+    await getScreenshots(browser, logger, elementsPositionAndAttributes, layout);
 
     expect(browser.screenshot).toHaveBeenCalledTimes(2);
     expect(browser.screenshot).toHaveBeenNthCalledWith(
@@ -104,7 +108,7 @@ describe('getScreenshots', () => {
     browser.screenshot.mockResolvedValue(Buffer.from(''));
 
     await expect(
-      getScreenshots(browser, logger, elementsPositionAndAttributes)
+      getScreenshots(browser, logger, elementsPositionAndAttributes, layout)
     ).rejects.toBeInstanceOf(Error);
   });
 });

x-pack/plugins/screenshotting/server/screenshots/get_screenshots.ts

@@ -8,6 +8,7 @@
 import apm from 'elastic-apm-node';
 import type { Logger } from 'src/core/server';
 import type { HeadlessChromiumDriver } from '../browsers';
+import { Layout } from '../layouts';
 import type { ElementsPositionAndAttribute } from './get_element_position_data';
 
 export interface Screenshot {
@@ -27,10 +28,52 @@ export interface Screenshot {
   description: string | null;
 }
 
+/**
+ * Resize the viewport to contain the element to capture.
+ *
+ * @async
+ * @param {HeadlessChromiumDriver} browser - used for its methods to control the page
+ * @param {ElementsPositionAndAttribute['position']} position - position data for the element to capture
+ * @param {Layout} layout - used for client-side layout data from the job params
+ * @param {Logger} logger
+ */
+const resizeViewport = async (
+  browser: HeadlessChromiumDriver,
+  position: ElementsPositionAndAttribute['position'],
+  layout: Layout,
+  logger: Logger
+) => {
+  const { boundingClientRect, scroll } = position;
+
+  // Using width from the layout is preferred, it avoids the elements moving around horizontally,
+  // which would invalidate the position data that was passed in.
+  const width = layout.width || boundingClientRect.left + scroll.x + boundingClientRect.width;
+
+  await browser.setViewport(
+    {
+      width,
+      height: boundingClientRect.top + scroll.y + boundingClientRect.height,
+      zoom: layout.getBrowserZoom(),
+    },
+    logger
+  );
+};
+
+/**
+ * Get screenshots of multiple areas of the page
+ *
+ * @async
+ * @param {HeadlessChromiumDriver} browser - used for its methods to control the page
+ * @param {Logger} logger
+ * @param {ElementsPositionAndAttribute[]} elementsPositionAndAttributes[] - position data about all the elements to capture
+ * @param {Layout} layout - used for client-side layout data from the job params
+ * @returns {Promise<Screenshot[]>}
+ */
 export const getScreenshots = async (
   browser: HeadlessChromiumDriver,
   logger: Logger,
-  elementsPositionAndAttributes: ElementsPositionAndAttribute[]
+  elementsPositionAndAttributes: ElementsPositionAndAttribute[],
+  layout: Layout
 ): Promise<Screenshot[]> => {
   logger.info(`taking screenshots`);
 
@@ -40,6 +83,8 @@ export const getScreenshots = async (
     const span = apm.startSpan('get_screenshots', 'read');
     const item = elementsPositionAndAttributes[i];
 
+    await resizeViewport(browser, item.position, layout, logger);
+
     const data = await browser.screenshot(item.position);
 
     if (!data?.byteLength) {