From 2bf28ea1e59445731cfe228e692eae78c1e0b03a Mon Sep 17 00:00:00 2001 From: jrandolf <101637635+jrandolf@users.noreply.github.com> Date: Thu, 16 Nov 2023 13:46:28 +0100 Subject: [PATCH] chore: implement document screenshots in BiDi (#11398) --- .../api/puppeteer.elementscreenshotoptions.md | 6 +- docs/api/puppeteer.screenshotoptions.md | 24 ++-- .../puppeteer-core/src/api/ElementHandle.ts | 19 +--- packages/puppeteer-core/src/api/Page.ts | 106 ++---------------- packages/puppeteer-core/src/bidi/Page.ts | 40 +++++-- test/TestExpectations.json | 32 ++++-- .../screenshot-offscreen-clip.png | Bin 266 -> 346 bytes .../screenshot-offscreen-clip.png | Bin 873 -> 459 bytes 8 files changed, 78 insertions(+), 149 deletions(-) diff --git a/docs/api/puppeteer.elementscreenshotoptions.md b/docs/api/puppeteer.elementscreenshotoptions.md index 120b2a00013..0f2af382236 100644 --- a/docs/api/puppeteer.elementscreenshotoptions.md +++ b/docs/api/puppeteer.elementscreenshotoptions.md @@ -14,6 +14,6 @@ export interface ElementScreenshotOptions extends ScreenshotOptions ## Properties -| Property | Modifiers | Type | Description | Default | -| -------------- | --------------------- | ------- | ----------- | ------- | -| scrollIntoView | optional | boolean | | true | +| Property | Modifiers | Type | Description | Default | +| -------------- | --------------------- | ------- | ----------- | ----------------- | +| scrollIntoView | optional | boolean | | true | diff --git a/docs/api/puppeteer.screenshotoptions.md b/docs/api/puppeteer.screenshotoptions.md index 225b832c952..c8b0a3ca5b9 100644 --- a/docs/api/puppeteer.screenshotoptions.md +++ b/docs/api/puppeteer.screenshotoptions.md @@ -12,15 +12,15 @@ export interface ScreenshotOptions ## Properties -| Property | Modifiers | Type | Description | Default | -| --------------------- | --------------------- | ----------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------- | -| captureBeyondViewport | optional | boolean | Capture the screenshot beyond the viewport. | true | -| clip | optional | [ScreenshotClip](./puppeteer.screenshotclip.md) | Specifies the region of the page to clip. | | -| encoding | optional | 'base64' \| 'binary' | Encoding of the image. | 'binary' | -| fromSurface | optional | boolean | Capture the screenshot from the surface, rather than the view. | true | -| fullPage | optional | boolean | When true, takes a screenshot of the full page. | false | -| omitBackground | optional | boolean | Hides default white background and allows capturing screenshots with transparency. | false | -| optimizeForSpeed | optional | boolean | | false | -| path | optional | string | The file path to save the image to. The screenshot type will be inferred from file extension. If path is a relative path, then it is resolved relative to current working directory. If no path is provided, the image won't be saved to the disk. | | -| quality | optional | number | Quality of the image, between 0-100. Not applicable to png images. | | -| type | optional | 'png' \| 'jpeg' \| 'webp' | | 'png' | +| Property | Modifiers | Type | Description | Default | +| --------------------- | --------------------- | ----------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------- | +| captureBeyondViewport | optional | boolean | Capture the screenshot beyond the viewport. | false if there is no clip. true otherwise. | +| clip | optional | [ScreenshotClip](./puppeteer.screenshotclip.md) | Specifies the region of the page to clip. | | +| encoding | optional | 'base64' \| 'binary' | Encoding of the image. | 'binary' | +| fromSurface | optional | boolean | Capture the screenshot from the surface, rather than the view. | true | +| fullPage | optional | boolean | When true, takes a screenshot of the full page. | false | +| omitBackground | optional | boolean | Hides default white background and allows capturing screenshots with transparency. | false | +| optimizeForSpeed | optional | boolean | | false | +| path | optional | string | The file path to save the image to. The screenshot type will be inferred from file extension. If path is a relative path, then it is resolved relative to current working directory. If no path is provided, the image won't be saved to the disk. | | +| quality | optional | number | Quality of the image, between 0-100. Not applicable to png images. | | +| type | optional | 'png' \| 'jpeg' \| 'webp' | | 'png' | diff --git a/packages/puppeteer-core/src/api/ElementHandle.ts b/packages/puppeteer-core/src/api/ElementHandle.ts index 19b0f259254..1ccb0817043 100644 --- a/packages/puppeteer-core/src/api/ElementHandle.ts +++ b/packages/puppeteer-core/src/api/ElementHandle.ts @@ -109,7 +109,7 @@ export interface Point { */ export interface ElementScreenshotOptions extends ScreenshotOptions { /** - * @defaultValue true + * @defaultValue `true` */ scrollIntoView?: boolean; } @@ -1348,21 +1348,12 @@ export abstract class ElementHandle< this: ElementHandle, options: Readonly = {} ): Promise { - const { - scrollIntoView = true, - captureBeyondViewport = true, - allowViewportExpansion = captureBeyondViewport, - } = options; + const {scrollIntoView = true} = options; let clip = await this.#nonEmptyVisibleBoundingBox(); const page = this.frame.page(); - await using _ = - allowViewportExpansion && clip - ? await page._createTemporaryViewportContainingBox(clip) - : null; - if (scrollIntoView) { await this.scrollIntoViewIfNeeded(); @@ -1382,11 +1373,7 @@ export abstract class ElementHandle< clip.x += pageLeft; clip.y += pageTop; - return await page.screenshot({ - ...options, - captureBeyondViewport: false, - clip, - }); + return await page.screenshot({...options, clip}); } async #nonEmptyVisibleBoundingBox() { diff --git a/packages/puppeteer-core/src/api/Page.ts b/packages/puppeteer-core/src/api/Page.ts index 4d1baf93d4e..26229a062d4 100644 --- a/packages/puppeteer-core/src/api/Page.ts +++ b/packages/puppeteer-core/src/api/Page.ts @@ -85,7 +85,6 @@ import type {ScreenRecorder} from '../node/ScreenRecorder.js'; import {assert} from '../util/assert.js'; import {guarded} from '../util/decorators.js'; import { - AsyncDisposableStack, asyncDisposeSymbol, DisposableStack, disposeSymbol, @@ -279,17 +278,9 @@ export interface ScreenshotOptions { /** * Capture the screenshot beyond the viewport. * - * @defaultValue `true` + * @defaultValue `false` if there is no `clip`. `true` otherwise. */ captureBeyondViewport?: boolean; - /** - * TODO(jrandolf): Investigate whether viewport expansion is a better - * alternative for cross-browser screenshots as opposed to - * `captureBeyondViewport`. - * - * @internal - */ - allowViewportExpansion?: boolean; } /** @@ -555,7 +546,6 @@ export function setDefaultScreenshotOptions(options: ScreenshotOptions): void { options.omitBackground ??= false; options.encoding ??= 'binary'; options.captureBeyondViewport ??= true; - options.allowViewportExpansion ??= options.captureBeyondViewport; } /** @@ -2439,7 +2429,7 @@ export abstract class Page extends EventEmitter { ): Promise { await this.bringToFront(); - // TODO: use structuredClone after Node 16 support is dropped.« + // TODO: use structuredClone after Node 16 support is dropped. const options = { ...userOptions, clip: userOptions.clip @@ -2482,10 +2472,6 @@ export abstract class Page extends EventEmitter { ); } } - assert( - !options.clip || !options.fullPage, - "'clip' and 'fullPage' are exclusive" - ); if (options.clip) { if (options.clip.width <= 0) { throw new Error("'width' in 'clip' must be positive."); @@ -2500,32 +2486,15 @@ export abstract class Page extends EventEmitter { options.clip = options.clip && roundRectangle(normalizeRectangle(options.clip)); - await using stack = new AsyncDisposableStack(); - if (options.allowViewportExpansion || options.captureBeyondViewport) { - if (options.fullPage) { - const dimensions = await this.mainFrame() - .isolatedRealm() - .evaluate(() => { - const {scrollHeight, scrollWidth} = document.documentElement; - const {height: viewportHeight, width: viewportWidth} = - window.visualViewport!; - return { - height: Math.max(scrollHeight, viewportHeight), - width: Math.max(scrollWidth, viewportWidth), - }; - }); - options.clip = {...dimensions, x: 0, y: 0}; - stack.use( - await this._createTemporaryViewportContainingBox(options.clip) - ); - } else if (options.clip && !options.captureBeyondViewport) { - stack.use( - options.clip && - (await this._createTemporaryViewportContainingBox(options.clip)) - ); - } else if (!options.clip) { - options.captureBeyondViewport = false; + if (options.fullPage) { + if (options.clip) { + throw new Error("'clip' and 'fullPage' are exclusive"); } + } else if ( + !options.clip && + userOptions.captureBeyondViewport === undefined + ) { + options.captureBeyondViewport = false; } const data = await this._screenshot(options); @@ -2542,61 +2511,6 @@ export abstract class Page extends EventEmitter { */ abstract _screenshot(options: Readonly): Promise; - /** - * @internal - */ - async _createTemporaryViewportContainingBox( - clip: ScreenshotClip - ): Promise { - const viewport = await this.mainFrame() - .isolatedRealm() - .evaluate(() => { - return { - pageLeft: window.visualViewport!.pageLeft, - pageTop: window.visualViewport!.pageTop, - width: window.visualViewport!.width, - height: window.visualViewport!.height, - }; - }); - await using stack = new AsyncDisposableStack(); - if (clip.x < viewport.pageLeft || clip.y < viewport.pageTop) { - await this.evaluate( - (left, top) => { - window.scroll({left, top, behavior: 'instant'}); - }, - Math.floor(clip.x), - Math.floor(clip.y) - ); - stack.defer(async () => { - await this.evaluate( - (left, top) => { - window.scroll({left, top, behavior: 'instant'}); - }, - viewport.pageLeft, - viewport.pageTop - ).catch(debugError); - }); - } - if ( - clip.width + clip.x > viewport.width || - clip.height + clip.y > viewport.height - ) { - const originalViewport = this.viewport() ?? { - width: 0, - height: 0, - }; - // We add 1 for fractional x and y. - await this.setViewport({ - width: Math.max(viewport.width, Math.ceil(clip.width + clip.x)), - height: Math.max(viewport.height, Math.ceil(clip.height + clip.y)), - }); - stack.defer(async () => { - await this.setViewport(originalViewport).catch(debugError); - }); - } - return stack.move(); - } - /** * @internal */ diff --git a/packages/puppeteer-core/src/bidi/Page.ts b/packages/puppeteer-core/src/bidi/Page.ts index a61d169e23a..325b544f7c4 100644 --- a/packages/puppeteer-core/src/bidi/Page.ts +++ b/packages/puppeteer-core/src/bidi/Page.ts @@ -29,6 +29,7 @@ import { raceWith, } from '../../third_party/rxjs/rxjs.js'; import type {CDPSession} from '../api/CDPSession.js'; +import type {BoundingBox} from '../api/ElementHandle.js'; import type {WaitForOptions} from '../api/Frame.js'; import { Page, @@ -647,13 +648,7 @@ export class BidiPage extends Page { override async _screenshot( options: Readonly ): Promise { - const {clip, type, captureBeyondViewport, allowViewportExpansion, quality} = - options; - if (captureBeyondViewport && !allowViewportExpansion) { - throw new UnsupportedOperation( - `BiDi does not support 'captureBeyondViewport'. Use 'allowViewportExpansion'.` - ); - } + const {clip, type, captureBeyondViewport, quality} = options; if (options.omitBackground !== undefined && options.omitBackground) { throw new UnsupportedOperation(`BiDi does not support 'omitBackground'.`); } @@ -665,6 +660,29 @@ export class BidiPage extends Page { if (options.fromSurface !== undefined && !options.fromSurface) { throw new UnsupportedOperation(`BiDi does not support 'fromSurface'.`); } + + let box: BoundingBox | undefined; + if (clip) { + if (captureBeyondViewport) { + box = clip; + } else { + const [pageLeft, pageTop] = await this.evaluate(() => { + if (!window.visualViewport) { + throw new Error('window.visualViewport is not supported.'); + } + return [ + window.visualViewport.pageLeft, + window.visualViewport.pageTop, + ] as const; + }); + box = { + ...clip, + x: clip.x - pageLeft, + y: clip.y - pageTop, + }; + } + } + if (clip !== undefined && clip.scale !== undefined && clip.scale !== 1) { throw new UnsupportedOperation( `BiDi does not support 'scale' in 'clip'.` @@ -675,14 +693,12 @@ export class BidiPage extends Page { result: {data}, } = await this.#connection.send('browsingContext.captureScreenshot', { context: this.mainFrame()._id, + origin: captureBeyondViewport ? 'document' : 'viewport', format: { type: `image/${type}`, - quality: quality ? quality / 100 : undefined, - }, - clip: clip && { - type: 'box', - ...clip, + ...(quality !== undefined ? {quality: quality / 100} : {}), }, + ...(box ? {clip: {type: 'box', ...box}} : {}), }); return data; } diff --git a/test/TestExpectations.json b/test/TestExpectations.json index 08803c1511f..953ee308bad 100644 --- a/test/TestExpectations.json +++ b/test/TestExpectations.json @@ -1166,7 +1166,7 @@ { "testIdPattern": "[screenshot.spec] Screenshots Page.screenshot should get screenshot bigger than the viewport", "platforms": ["darwin", "linux", "win32"], - "parameters": ["webDriverBiDi"], + "parameters": ["cdp"], "expectations": ["FAIL"] }, { @@ -3348,13 +3348,19 @@ "expectations": ["FAIL", "PASS"] }, { - "testIdPattern": "[screenshot.spec] Screenshots ElementHandle.screenshot should capture full element when larger than viewport", + "testIdPattern": "[screenshot.spec] Screenshots ElementHandle.screenshot should work for an element with an offset", + "platforms": ["darwin", "linux", "win32"], + "parameters": ["cdp", "firefox"], + "expectations": ["FAIL"] + }, + { + "testIdPattern": "[screenshot.spec] Screenshots ElementHandle.screenshot should work for an element with an offset", "platforms": ["darwin", "linux", "win32"], "parameters": ["firefox", "webDriverBiDi"], "expectations": ["FAIL"] }, { - "testIdPattern": "[screenshot.spec] Screenshots ElementHandle.screenshot should work for an element with an offset", + "testIdPattern": "[screenshot.spec] Screenshots ElementHandle.screenshot should work with a rotated element", "platforms": ["darwin", "linux", "win32"], "parameters": ["cdp", "firefox"], "expectations": ["FAIL"] @@ -3362,7 +3368,7 @@ { "testIdPattern": "[screenshot.spec] Screenshots ElementHandle.screenshot should work with a rotated element", "platforms": ["darwin", "linux", "win32"], - "parameters": ["cdp", "firefox"], + "parameters": ["firefox", "webDriverBiDi"], "expectations": ["FAIL"] }, { @@ -3383,12 +3389,24 @@ "parameters": ["firefox", "webDriverBiDi"], "expectations": ["FAIL"] }, + { + "testIdPattern": "[screenshot.spec] Screenshots Page.screenshot should take fullPage screenshots", + "platforms": ["darwin", "linux", "win32"], + "parameters": ["cdp", "firefox"], + "expectations": ["FAIL"] + }, { "testIdPattern": "[screenshot.spec] Screenshots Page.screenshot should work", "platforms": ["darwin", "linux", "win32"], "parameters": ["firefox", "webDriverBiDi"], "expectations": ["FAIL"] }, + { + "testIdPattern": "[screenshot.spec] Screenshots Page.screenshot should work with odd clip size on Retina displays", + "platforms": ["darwin", "linux", "win32"], + "parameters": ["firefox", "webDriverBiDi"], + "expectations": ["FAIL"] + }, { "testIdPattern": "[stacktrace.spec] Stack trace should work for none error objects", "platforms": ["darwin", "linux", "win32"], @@ -3713,12 +3731,6 @@ "parameters": ["cdp", "firefox", "headless"], "expectations": ["FAIL"] }, - { - "testIdPattern": "[screenshot.spec] Screenshots Page.screenshot should take fullPage screenshots", - "platforms": ["darwin", "linux", "win32"], - "parameters": ["cdp", "firefox", "headless"], - "expectations": ["FAIL"] - }, { "testIdPattern": "[screenshot.spec] Screenshots Page.screenshot should use scale for clip", "platforms": ["darwin", "linux", "win32"], diff --git a/test/golden-chrome/screenshot-offscreen-clip.png b/test/golden-chrome/screenshot-offscreen-clip.png index e503f801ec6a112abf23373dca38891a34056607..d7637631b7d49d8e94a5db2fa01cc59a79034350 100644 GIT binary patch literal 346 zcmeAS@N?(olHy`uVBq!ia0vp^l0clo!3HFYLuc0jDaPU;cPEB*=VV?2*&?1Ujv*P1 zZ*Lf8H778zJ`gVyvAXKe_aI7Rhu9K<6EEU4M0Qm+#vBY}$yb;wqSSiFVc~-yjz3G9 zMXoG#-w%09oj&WQEDx6h1{Q;-tDnm{r-UW|C@+kY literal 266 zcmeAS@N?(olHy`uVBq!ia0vp^DImwh4)bmss0Gyl(=*}He| z?%lgLZQ8VY_3D)?S5BKYt*@^yIXT(a*Voh2)6~?|$jC@fPftrrOG!ydOiT=D24B0v z79iE;>EaktaqI03Pre2N4u?R|B2Zu+ahi$k7SbmAbl6mK2XLEo#D%MYA xWG1Xwe0ayZvy73p6J9jP+`6s2>qh={Ze1TK!)u2gJ_fpo!PC{xWt~$(6988PWRw5^ diff --git a/test/golden-firefox/screenshot-offscreen-clip.png b/test/golden-firefox/screenshot-offscreen-clip.png index 0298d3cf4400e56bbbe77238536c6d3b7770b896..4c34e47fbd48a3b3a6541dfd5d286334646d875c 100644 GIT binary patch literal 459 zcmeAS@N?(olHy`uVBq!ia0vp^l0clo!3HFYLuc18FfdN?ba4#HaDF>W(f@FONNfIo z2S)*;)+J3#7M+waY3$@m5l!Q2JaTrC+U*6W;v-liozD6Qf0;Eg!NWsM?8=jG>~giT z+TY?XT{n3W5<2I*-~Dp`Ti0i&uUr&xezj<8qQ_HP)7qt{RU>ab$+TO3F~eoHP5!Os ztjU`nPS|!^x7K%gnopBLz~-Ahn{*bxjyvDuX}Q{KQp>wve~rV{I#-tfUM@cOgyyKuAtEKZ{3y&dU~7dr|G?Eb!}zWd+Z%WwSDo9S^n~-E8W%~N7)zs9P z=XS-GSv8kU;PGsCEW3U-X`_d{zu%3MKDQi=Tx}EH>UPT}ZfQ1%=Q~t$GLwCxb{aFa zSx1X@PN{3zU2pv_F04+3{baeQlh|4QkJDx9e$Vb+vZPrcnsH4ti-(#ppHmL&20kH$ znLZXA8$}OTc{HRLJybXn#_(E!ani{aVimevd|5K<_xt1ZO^5fJZ~Vj_vc$beV2L-s zb;mu!r~*=0zx%tZj%OTnoSMu8SAego%SUp6$qkX_um=%;4gqC5Kbfqf!{0!i8p zr4txVrW|tYSj%wDkx6ATC%fV{rU-V<2|j8vEjPFlWK|rDW)={uP~+F%I^)@AFTMP{ z{7H=2ZeW<)T=y;Ecl|41s@bG-`DVAC)f{GPkG`eLqnHtKW_y}f6-xW xOAbe0d$()m`R6Y$zmzUBnp`%)Cy@U}{P~I3n