[Reporting] set viewport to include clip area (#87253)

tsullivan · kibanamachine · tsullivan · commit 35ca9dd44daf · 2021-01-06T15:31:11.000-07:00
* [Reporting] set viewport to include clip area * remove getViewport * fix tests * simpler * fix 1 * revert * hacks * scope the logging variables * polish * hacky fix * quieter logging * make less hacky * fix functional test * revert lowering log level of browser console messages * revise comments * setViewport only to happen once * fix snapshot of layout type tests * fix comment text * Revert "setViewport only to happen once" This reverts commit 15977f9. * fix disgusting bug * use x/y ordering for width/height * fix fn test snapshots Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
diff --git a/x-pack/plugins/reporting/server/browsers/chromium/driver/chromium_driver.ts b/x-pack/plugins/reporting/server/browsers/chromium/driver/chromium_driver.ts
@@ -196,14 +196,17 @@ export class HeadlessChromiumDriver {
   }
 
   public async setViewport(
-    { width, height, zoom }: ViewZoomWidthHeight,
+    { width: _width, height: _height, zoom }: ViewZoomWidthHeight,
     logger: LevelLogger
   ): Promise<void> {
-    logger.debug(`Setting viewport to width: ${width}, height: ${height}, zoom: ${zoom}`);
+    const width = Math.floor(_width);
+    const height = Math.floor(_height);
+
+    logger.debug(`Setting viewport to: width=${width} height=${height} zoom=${zoom}`);
 
     await this.page.setViewport({
-      width: Math.floor(width / zoom),
-      height: Math.floor(height / zoom),
+      width,
+      height,
       deviceScaleFactor: zoom,
       isMobile: false,
     });
@@ -243,7 +246,7 @@ export class HeadlessChromiumDriver {
       }
 
       if (this._shouldUseCustomHeaders(conditionalHeaders.conditions, interceptedUrl)) {
-        logger.debug(`Using custom headers for ${interceptedUrl}`);
+        logger.trace(`Using custom headers for ${interceptedUrl}`);
         const headers = map(
           {
             ...interceptedRequest.request.headers,
@@ -270,7 +273,7 @@ export class HeadlessChromiumDriver {
         }
       } else {
         const loggedUrl = isData ? this.truncateUrl(interceptedUrl) : interceptedUrl;
-        logger.debug(`No custom headers for ${loggedUrl}`);
+        logger.trace(`No custom headers for ${loggedUrl}`);
         try {
           await client.send('Fetch.continueRequest', { requestId });
         } catch (err) {
diff --git a/x-pack/plugins/reporting/server/browsers/chromium/driver_factory/args.ts b/x-pack/plugins/reporting/server/browsers/chromium/driver_factory/args.ts
@@ -41,6 +41,9 @@ export const args = ({ userDataDir, viewport, disableSandbox, proxy: proxyConfig
     '--disable-gpu',
     '--headless',
     '--hide-scrollbars',
+    // NOTE: setting the window size does NOT set the viewport size: viewport and window size are different.
+    // The viewport may later need to be resized depending on the position of the clip area.
+    // These numbers come from the job parameters, so this is a close guess.
     `--window-size=${Math.floor(viewport.width)},${Math.floor(viewport.height)}`,
   ];
 
diff --git a/x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts b/x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts
@@ -65,6 +65,7 @@ export class HeadlessChromiumDriverFactory {
       logger.info(`Creating browser page driver`);
 
       const chromiumArgs = this.getChromiumArgs(viewport);
+      logger.debug(`Chromium launch args set to: ${chromiumArgs}`);
 
       let browser: puppeteer.Browser;
       let page: puppeteer.Page;
diff --git a/x-pack/plugins/reporting/server/export_types/printable_pdf/lib/generate_pdf.ts b/x-pack/plugins/reporting/server/export_types/printable_pdf/lib/generate_pdf.ts
@@ -43,6 +43,7 @@ export async function generatePdfObservableFactory(reporting: ReportingCore) {
     tracker.startLayout();
 
     const layout = createLayout(captureConfig, layoutParams);
+    logger.debug(`Layout: width=${layout.width} height=${layout.height}`);
     tracker.endLayout();
 
     tracker.startScreenshots();
diff --git a/x-pack/plugins/reporting/server/lib/layouts/canvas_layout.ts b/x-pack/plugins/reporting/server/lib/layouts/canvas_layout.ts
@@ -64,8 +64,8 @@ export class CanvasLayout extends Layout implements LayoutInstance {
 
   public getViewport() {
     return {
-      height: this.scaledHeight,
-      width: this.scaledWidth,
+      height: this.height,
+      width: this.width,
       zoom: ZOOM,
     };
   }
diff --git a/x-pack/plugins/reporting/server/lib/layouts/layout.ts b/x-pack/plugins/reporting/server/lib/layouts/layout.ts
@@ -33,6 +33,8 @@ export abstract class Layout {
     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;
 
   public abstract getBrowserZoom(): number;
diff --git a/x-pack/plugins/reporting/server/lib/layouts/preserve_layout.ts b/x-pack/plugins/reporting/server/lib/layouts/preserve_layout.ts
@@ -51,8 +51,8 @@ export class PreserveLayout extends Layout implements LayoutInstance {
 
   public getViewport() {
     return {
-      height: this.scaledHeight,
-      width: this.scaledWidth,
+      height: this.height,
+      width: this.width,
       zoom: ZOOM,
     };
   }
diff --git a/x-pack/plugins/reporting/server/lib/level_logger.ts b/x-pack/plugins/reporting/server/lib/level_logger.ts
@@ -49,6 +49,10 @@ export class LevelLogger implements GenericLevelLogger {
     this.getLogger(tags).debug(msg);
   }
 
+  public trace(msg: string, tags: string[] = []) {
+    this.getLogger(tags).trace(msg);
+  }
+
   public info(msg: string, tags: string[] = []) {
     this.getLogger(tags).info(trimStr(msg));
   }
diff --git a/x-pack/plugins/reporting/server/lib/screenshots/constants.ts b/x-pack/plugins/reporting/server/lib/screenshots/constants.ts
@@ -7,6 +7,7 @@
 export const DEFAULT_PAGELOAD_SELECTOR = '.application';
 
 export const CONTEXT_GETNUMBEROFITEMS = 'GetNumberOfItems';
+export const CONTEXT_GETBROWSERDIMENSIONS = 'GetBrowserDimensions';
 export const CONTEXT_INJECTCSS = 'InjectCss';
 export const CONTEXT_WAITFORRENDER = 'WaitForRender';
 export const CONTEXT_GETTIMERANGE = 'GetTimeRange';
diff --git a/x-pack/plugins/reporting/server/lib/screenshots/get_screenshots.ts b/x-pack/plugins/reporting/server/lib/screenshots/get_screenshots.ts
@@ -7,10 +7,60 @@
 import { i18n } from '@kbn/i18n';
 import { LevelLogger, startTrace } from '../';
 import { HeadlessChromiumDriver } from '../../browsers';
+import { LayoutInstance } from '../layouts';
 import { ElementsPositionAndAttribute, Screenshot } from './';
+import { CONTEXT_GETBROWSERDIMENSIONS } from './constants';
+
+// In Puppeteer 5.4+, the viewport size limits what the screenshot can take, even if a clip is specified. The clip area must
+// be visible in the viewport. This workaround resizes the viewport to the actual content height and width.
+// NOTE: this will fire a window resize event
+const resizeToClipArea = async (
+  item: ElementsPositionAndAttribute,
+  browser: HeadlessChromiumDriver,
+  zoom: number,
+  logger: LevelLogger
+) => {
+  // Check current viewport size
+  const { width, height, left, top } = item.position.boundingClientRect; // the "unscaled" pixel sizes
+  const [viewWidth, viewHeight] = await browser.evaluate(
+    {
+      fn: () => [document.body.clientWidth, document.body.clientHeight],
+      args: [],
+    },
+    { context: CONTEXT_GETBROWSERDIMENSIONS },
+    logger
+  );
+
+  logger.debug(`Browser viewport: width=${viewWidth} height=${viewHeight}`);
+
+  // Resize the viewport if the clip area is not visible
+  if (viewWidth < width + left || viewHeight < height + top) {
+    logger.debug(`Item's position is not within the viewport.`);
+
+    // add left and top margin to unscaled measurements
+    const newWidth = width + left;
+    const newHeight = height + top;
+
+    logger.debug(
+      `Resizing browser viewport to: width=${newWidth} height=${newHeight} zoom=${zoom}`
+    );
+
+    await browser.setViewport(
+      {
+        width: newWidth,
+        height: newHeight,
+        zoom,
+      },
+      logger
+    );
+  }
+
+  logger.debug(`Capturing item: width=${width} height=${height} left=${left} top=${top}`);
+};
 
 export const getScreenshots = async (
   browser: HeadlessChromiumDriver,
+  layout: LayoutInstance,
   elementsPositionAndAttributes: ElementsPositionAndAttribute[],
   logger: LevelLogger
 ): Promise<Screenshot[]> => {
@@ -25,6 +75,8 @@ export const getScreenshots = async (
   for (let i = 0; i < elementsPositionAndAttributes.length; i++) {
     const endTrace = startTrace('get_screenshots', 'read');
     const item = elementsPositionAndAttributes[i];
+
+    await resizeToClipArea(item, browser, layout.getBrowserZoom(), logger);
     const base64EncodedData = await browser.screenshot(item.position);
 
     screenshots.push({
diff --git a/x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts b/x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts
@@ -243,10 +243,10 @@ describe('Screenshot Observable Pipeline', () => {
                       "attributes": Object {},
                       "position": Object {
                         "boundingClientRect": Object {
-                          "height": 200,
+                          "height": 100,
                           "left": 0,
                           "top": 0,
-                          "width": 200,
+                          "width": 100,
                         },
                         "scroll": Object {
                           "x": 0,
@@ -271,10 +271,10 @@ describe('Screenshot Observable Pipeline', () => {
                       "attributes": Object {},
                       "position": Object {
                         "boundingClientRect": Object {
-                          "height": 200,
+                          "height": 100,
                           "left": 0,
                           "top": 0,
-                          "width": 200,
+                          "width": 100,
                         },
                         "scroll": Object {
                           "x": 0,
@@ -339,6 +339,8 @@ describe('Screenshot Observable Pipeline', () => {
 
         if (mockCall === contexts.CONTEXT_ELEMENTATTRIBUTES) {
           return Promise.resolve(null);
+        } else if (mockCall === contexts.CONTEXT_GETBROWSERDIMENSIONS) {
+          return Promise.resolve([800, 600]);
         } else {
           return Promise.resolve();
         }
diff --git a/x-pack/plugins/reporting/server/lib/screenshots/observable.ts b/x-pack/plugins/reporting/server/lib/screenshots/observable.ts
@@ -87,6 +87,7 @@ export function screenshotsObservableFactory(
               }),
               mergeMap(() => getNumberOfItems(captureConfig, driver, layout, logger)),
               mergeMap(async (itemsCount) => {
+                // set the viewport to the dimentions from the job, to allow elements to flow into the expected layout
                 const viewport = layout.getViewport(itemsCount) || getDefaultViewPort();
                 await Promise.all([
                   driver.setViewport(viewport, logger),
@@ -133,7 +134,7 @@ export function screenshotsObservableFactory(
                   const elements = data.elementsPositionAndAttributes
                     ? data.elementsPositionAndAttributes
                     : getDefaultElementPosition(layout.getViewport(1));
-                  const screenshots = await getScreenshots(driver, elements, logger);
+                  const screenshots = await getScreenshots(driver, layout, elements, logger);
                   const { timeRange, error: setupError } = data;
                   return {
                     timeRange,
diff --git a/x-pack/plugins/reporting/server/test_helpers/create_mock_browserdriverfactory.ts b/x-pack/plugins/reporting/server/test_helpers/create_mock_browserdriverfactory.ts
@@ -64,6 +64,9 @@ mockBrowserEvaluate.mockImplementation(() => {
   if (mockCall === contexts.CONTEXT_GETNUMBEROFITEMS) {
     return Promise.resolve(1);
   }
+  if (mockCall === contexts.CONTEXT_GETBROWSERDIMENSIONS) {
+    return Promise.resolve([600, 800]);
+  }
   if (mockCall === contexts.CONTEXT_INJECTCSS) {
     return Promise.resolve();
   }
diff --git a/x-pack/test/functional/apps/canvas/reports.ts b/x-pack/test/functional/apps/canvas/reports.ts
@@ -183,7 +183,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
           "
         `);
 
-        expectSnapshot(res.get('content-length')).toMatchInline(`"20726"`);
+        expect(res.get('content-length')).to.be('20725');
       });
 
       it('downloaded PDF base64 string is correct without borders and logo', async function () {
@@ -327,12 +327,12 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
         /Filter /FlateDecode
         /ColorSpace /DeviceRGB
         /SMask 14 0 R
-        /Length 18
+        /Length 17
         >>
         "
         `);
 
-        expectSnapshot(res.get('content-length')).toMatchInline(`"1599"`);
+        expect(res.get('content-length')).to.be('1598');
       });
     });
   });

Original file line number	Diff line number	Diff line change
`@@ -49,6 +49,10 @@ export class LevelLogger implements GenericLevelLogger {`
`49`	`49`	`this.getLogger(tags).debug(msg);`
`50`	`50`	`}`
`51`	`51`
	`52`	`+ public trace(msg: string, tags: string[] = []) {`
	`53`	`+ this.getLogger(tags).trace(msg);`
	`54`	`+ }`
	`55`	`+`
`52`	`56`	`public info(msg: string, tags: string[] = []) {`
`53`	`57`	`this.getLogger(tags).info(trimStr(msg));`
`54`	`58`	`}`