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.

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

[Reporting] set viewport to include clip area #87253

Merged
merged 26 commits into from
Jan 6, 2021
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
a45e22c
[Reporting] set viewport to include clip area
tsullivan Jan 4, 2021
c418bad
remove getViewport
tsullivan Jan 4, 2021
71c3799
fix tests
tsullivan Jan 5, 2021
0bfb3da
Merge branch 'master' into fix/reporting-viewport-regression
kibanamachine Jan 5, 2021
7737cf8
Merge remote-tracking branch 'elastic/master' into fix/reporting-view…
tsullivan Jan 5, 2021
889ead8
simpler
tsullivan Jan 5, 2021
55bdc0d
fix 1
tsullivan Jan 5, 2021
0944141
revert
tsullivan Jan 5, 2021
fd7b1b6
hacks
tsullivan Jan 5, 2021
6f66ee6
scope the logging variables
tsullivan Jan 5, 2021
dcbedf0
polish
tsullivan Jan 5, 2021
bb4c7fb
hacky fix
tsullivan Jan 5, 2021
e40e75d
quieter logging
tsullivan Jan 5, 2021
a8932ef
make less hacky
tsullivan Jan 5, 2021
24ac875
fix functional test
tsullivan Jan 6, 2021
d7da344
revert lowering log level of browser console messages
tsullivan Jan 6, 2021
9bc4239
revise comments
tsullivan Jan 6, 2021
15977f9
setViewport only to happen once
tsullivan Jan 6, 2021
c88ca13
Merge branch 'master' into fix/reporting-viewport-regression
kibanamachine Jan 6, 2021
653c301
fix snapshot of layout type tests
tsullivan Jan 6, 2021
9dbb13b
fix comment text
tsullivan Jan 6, 2021
baba969
Revert "setViewport only to happen once"
tsullivan Jan 6, 2021
82d2662
fix disgusting bug
tsullivan Jan 6, 2021
2de92b8
use x/y ordering for width/height
tsullivan Jan 6, 2021
fa3921d
fix fn test snapshots
tsullivan Jan 6, 2021
968e03a
Merge branch 'master' into fix/reporting-viewport-regression
kibanamachine Jan 6, 2021
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
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
Expand Down Expand Up @@ -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,
Expand All @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)}`,
];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export async function generatePdfObservableFactory(reporting: ReportingCore) {
tracker.startLayout();

const layout = createLayout(captureConfig, layoutParams);
logger.debug(`Layout: height=${layout.height} width=${layout.width}`);
tracker.endLayout();

tracker.startScreenshots();
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/reporting/server/lib/layouts/canvas_layout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
}
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/reporting/server/lib/layouts/layout.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
}
Expand Down
4 changes: 4 additions & 0 deletions x-pack/plugins/reporting/server/lib/level_logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
52 changes: 52 additions & 0 deletions x-pack/plugins/reporting/server/lib/screenshots/get_screenshots.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Copy link
Member Author

Choose a reason for hiding this comment

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

Sometimes it's hard to visually check that we assign height values to height values, and width values to width values. I use a plugin to highlight different strings different colors to give it a color pattern check, and that's what I did here to see that there was a bug.

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 top and left 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: top:${top} left:${left} height:${height} width:${width}`);
};

export const getScreenshots = async (
browser: HeadlessChromiumDriver,
layout: LayoutInstance,
elementsPositionAndAttributes: ElementsPositionAndAttribute[],
logger: LevelLogger
): Promise<Screenshot[]> => {
Expand All @@ -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({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
2 changes: 1 addition & 1 deletion x-pack/test/functional/apps/canvas/reports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down