[Screenshotting] Add captureBeyondViewport: false to workaround a res…

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

@@ -35,12 +35,6 @@ export interface ElementPosition {
   };
 }
 
-export interface Viewport {
-  zoom: number;
-  width: number;
-  height: number;
-}
-
 interface OpenOptions {
   context?: Context;
   headers: Headers;
@@ -203,7 +197,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;
@@ -214,6 +208,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)) {
@@ -263,14 +258,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

@@ -5,13 +5,13 @@
  * 2.0.
  */
 
+import type { Logger } from '@kbn/core/server';
+import type { ScreenshotModePluginSetup } from '@kbn/screenshot-mode-plugin/server';
 import puppeteer from 'puppeteer';
 import * as Rx from 'rxjs';
 import { mergeMap, take } from 'rxjs/operators';
-import type { Logger } from '@kbn/core/server';
-import type { ScreenshotModePluginSetup } from '@kbn/screenshot-mode-plugin/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

@@ -5,44 +5,38 @@
  * 2.0.
  */
 
+import type { Logger } from '@kbn/core/server';
+import type { ScreenshotModePluginSetup } from '@kbn/screenshot-mode-plugin/server';
 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 {
   catchError,
+  concatMap,
   ignoreElements,
   map,
-  concatMap,
   mergeMap,
   reduce,
   takeUntil,
   tap,
 } from 'rxjs/operators';
-import type { Logger } from '@kbn/core/server';
-import type { ScreenshotModePluginSetup } from '@kbn/screenshot-mode-plugin/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;
 }
 
@@ -63,10 +57,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
@@ -138,6 +138,19 @@ export class HeadlessChromiumDriverFactory {
 
       const chromiumArgs = this.getChromiumArgs();
       logger.debug(`Chromium launch args set to: ${chromiumArgs}`);
+
+      // 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}`
+      );
+
       (async () => {
         let browser: Browser | undefined;
         try {
@@ -148,13 +161,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

@@ -8,6 +8,7 @@
 import { APP_WRAPPER_CLASS } from '@kbn/core/server';
 export const DEFAULT_PAGELOAD_SELECTOR = `.${APP_WRAPPER_CLASS}`;
 
+// FIXME: cleanup: remove this file and use the EventLogger's Actions enum instead
 export const CONTEXT_GETNUMBEROFITEMS = 'GetNumberOfItems';
 export const CONTEXT_INJECTCSS = 'InjectCss';
 export const CONTEXT_WAITFORRENDER = 'WaitForRender';
@@ -17,3 +18,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

@@ -32,11 +32,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

@@ -60,9 +60,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

@@ -8,6 +8,8 @@
 import { loggingSystemMock } from '@kbn/core/server/mocks';
 import { createMockBrowserDriver } from '../browsers/mock';
 import { ConfigType } from '../config';
+import { Layout } from '../layouts';
+import { createMockLayout } from '../layouts/mock';
 import { EventLogger } from './event_logger';
 import { getScreenshots } from './get_screenshots';
 
@@ -31,21 +33,23 @@ describe('getScreenshots', () => {
   let browser: ReturnType<typeof createMockBrowserDriver>;
   let eventLogger: EventLogger;
   let config = {} as ConfigType;
+  let layout: Layout;
 
   beforeEach(async () => {
     browser = createMockBrowserDriver();
     config = { capture: { zoom: 2 } } as ConfigType;
     eventLogger = new EventLogger(loggingSystemMock.createLogger(), config);
     browser.evaluate.mockImplementation(({ fn, args }) => (fn as Function)(...args));
+    layout = createMockLayout();
   });
 
   afterEach(() => {
     jest.clearAllMocks();
   });
 
   it('should return screenshots', async () => {
-    await expect(getScreenshots(browser, eventLogger, elementsPositionAndAttributes)).resolves
-      .toMatchInlineSnapshot(`
+    await expect(getScreenshots(browser, eventLogger, elementsPositionAndAttributes, layout))
+      .resolves.toMatchInlineSnapshot(`
             Array [
               Object {
                 "data": Object {
@@ -90,7 +94,7 @@ describe('getScreenshots', () => {
   });
 
   it('should forward elements positions', async () => {
-    await getScreenshots(browser, eventLogger, elementsPositionAndAttributes);
+    await getScreenshots(browser, eventLogger, elementsPositionAndAttributes, layout);
 
     expect(browser.screenshot).toHaveBeenCalledTimes(2);
     expect(browser.screenshot).toHaveBeenNthCalledWith(
@@ -107,7 +111,7 @@ describe('getScreenshots', () => {
     browser.screenshot.mockResolvedValue(Buffer.from(''));
 
     await expect(
-      getScreenshots(browser, eventLogger, elementsPositionAndAttributes)
+      getScreenshots(browser, eventLogger, elementsPositionAndAttributes, layout)
     ).rejects.toBeInstanceOf(Error);
   });
 });