From ace1230e41aad6168dc85b9bc1f7c04d9dce5527 Mon Sep 17 00:00:00 2001
From: jrandolf <101637635+jrandolf@users.noreply.github.com>
Date: Mon, 25 Sep 2023 15:05:20 +0200
Subject: [PATCH] fix: set defaults in screenshot (#11021)

---
 docs/api/puppeteer.screenshotoptions.md  |  2 +-
 packages/puppeteer-core/src/api/Page.ts  | 22 ++++++--
 packages/puppeteer-core/src/bidi/Page.ts | 67 +++++++++---------------
 packages/puppeteer-core/src/cdp/Page.ts  |  4 +-
 4 files changed, 46 insertions(+), 49 deletions(-)

diff --git a/docs/api/puppeteer.screenshotoptions.md b/docs/api/puppeteer.screenshotoptions.md
index 568a9bf3..225b832c 100644
--- a/docs/api/puppeteer.screenshotoptions.md
+++ b/docs/api/puppeteer.screenshotoptions.md
@@ -17,7 +17,7 @@ export interface ScreenshotOptions
 | captureBeyondViewport | <code>optional</code> | boolean                                         | Capture the screenshot beyond the viewport.                                                                                                                                                                                                        | <code>true</code>     |
 | clip                  | <code>optional</code> | [ScreenshotClip](./puppeteer.screenshotclip.md) | Specifies the region of the page to clip.                                                                                                                                                                                                          |                       |
 | encoding              | <code>optional</code> | 'base64' \| 'binary'                            | Encoding of the image.                                                                                                                                                                                                                             | <code>'binary'</code> |
-| fromSurface           | <code>optional</code> | boolean                                         | Capture the screenshot from the surface, rather than the view.                                                                                                                                                                                     | <code>false</code>    |
+| fromSurface           | <code>optional</code> | boolean                                         | Capture the screenshot from the surface, rather than the view.                                                                                                                                                                                     | <code>true</code>     |
 | fullPage              | <code>optional</code> | boolean                                         | When <code>true</code>, takes a screenshot of the full page.                                                                                                                                                                                       | <code>false</code>    |
 | omitBackground        | <code>optional</code> | boolean                                         | Hides default white background and allows capturing screenshots with transparency.                                                                                                                                                                 | <code>false</code>    |
 | optimizeForSpeed      | <code>optional</code> | boolean                                         |                                                                                                                                                                                                                                                    | <code>false</code>    |
diff --git a/packages/puppeteer-core/src/api/Page.ts b/packages/puppeteer-core/src/api/Page.ts
index 1f1a8dd2..da7b75fa 100644
--- a/packages/puppeteer-core/src/api/Page.ts
+++ b/packages/puppeteer-core/src/api/Page.ts
@@ -242,7 +242,7 @@ export interface ScreenshotOptions {
   /**
    * Capture the screenshot from the surface, rather than the view.
    *
-   * @defaultValue `false`
+   * @defaultValue `true`
    */
   fromSurface?: boolean;
   /**
@@ -495,6 +495,20 @@ export interface NewDocumentScriptEvaluation {
   identifier: string;
 }
 
+/**
+ * @internal
+ */
+export function setDefaultScreenshotOptions(options: ScreenshotOptions): void {
+  options.optimizeForSpeed ??= false;
+  options.type ??= 'png';
+  options.fromSurface ??= true;
+  options.fullPage ??= false;
+  options.omitBackground ??= false;
+  options.encoding ??= 'binary';
+  options.captureBeyondViewport ??= true;
+  options.allowViewportExpansion ??= options.captureBeyondViewport;
+}
+
 /**
  * Page provides methods to interact with a single tab or
  * {@link https://developer.chrome.com/extensions/background_pages | extension background page}
@@ -2285,7 +2299,7 @@ export abstract class Page extends EventEmitter<PageEvents> {
             ...userOptions.clip,
           }
         : undefined,
-    } as ScreenshotOptions;
+    };
     if (options.type === undefined && options.path !== undefined) {
       const filePath = options.path;
       // Note we cannot use Node.js here due to browser compatability.
@@ -2358,8 +2372,8 @@ export abstract class Page extends EventEmitter<PageEvents> {
       );
     }
 
-    options.captureBeyondViewport ??= true;
-    options.allowViewportExpansion ??= options.captureBeyondViewport;
+    setDefaultScreenshotOptions(options);
+
     options.clip = options.clip && roundClip(normalizeClip(options.clip));
 
     await using stack = new AsyncDisposableStack();
diff --git a/packages/puppeteer-core/src/bidi/Page.ts b/packages/puppeteer-core/src/bidi/Page.ts
index b0f7ad36..c15b97d0 100644
--- a/packages/puppeteer-core/src/bidi/Page.ts
+++ b/packages/puppeteer-core/src/bidi/Page.ts
@@ -647,52 +647,33 @@ export class BidiPage extends Page {
     }
   }
 
-  override async screenshot(
-    options: Readonly<ScreenshotOptions> & {encoding: 'base64'}
-  ): Promise<string>;
-  override async screenshot(
-    options?: Readonly<ScreenshotOptions>
-  ): Promise<Buffer>;
-  override async screenshot(
-    options: Readonly<ScreenshotOptions> = {}
-  ): Promise<Buffer | string> {
-    const {
-      clip,
-      type,
-      captureBeyondViewport,
-      allowViewportExpansion = true,
-    } = options;
-    if (captureBeyondViewport) {
-      throw new Error(`BiDi does not support 'captureBeyondViewport'.`);
-    }
-    const invalidOption = Object.keys(options).find(option => {
-      return [
-        'fromSurface',
-        'omitBackground',
-        'optimizeForSpeed',
-        'quality',
-      ].includes(option);
-    });
-    if (invalidOption !== undefined) {
-      throw new Error(`BiDi does not support ${invalidOption}.`);
-    }
-    if ((type ?? 'png') !== 'png') {
-      throw new Error(`BiDi only supports 'png' type.`);
-    }
-    if (clip?.scale !== undefined) {
-      throw new Error(`BiDi does not support 'scale' in 'clip'.`);
-    }
-    return await super.screenshot({
-      ...options,
-      captureBeyondViewport,
-      allowViewportExpansion: captureBeyondViewport ?? allowViewportExpansion,
-    });
-  }
-
   override async _screenshot(
     options: Readonly<ScreenshotOptions>
   ): Promise<string> {
-    const {clip} = options;
+    const {clip, type, captureBeyondViewport, allowViewportExpansion} = options;
+    if (captureBeyondViewport && !allowViewportExpansion) {
+      throw new Error(
+        `BiDi does not support 'captureBeyondViewport'. Use 'allowViewportExpansion'.`
+      );
+    }
+    if (options.omitBackground !== undefined && options.omitBackground) {
+      throw new Error(`BiDi does not support 'omitBackground'.`);
+    }
+    if (options.optimizeForSpeed !== undefined && options.optimizeForSpeed) {
+      throw new Error(`BiDi does not support 'optimizeForSpeed'.`);
+    }
+    if (options.fromSurface !== undefined && !options.fromSurface) {
+      throw new Error(`BiDi does not support 'fromSurface'.`);
+    }
+    if (options.quality !== undefined) {
+      throw new Error(`BiDi does not support 'quality'.`);
+    }
+    if (type === 'webp' || type === 'jpeg') {
+      throw new Error(`BiDi only supports 'png' type.`);
+    }
+    if (clip !== undefined && clip.scale !== undefined && clip.scale !== 1) {
+      throw new Error(`BiDi does not support 'scale' in 'clip'.`);
+    }
     const {
       result: {data},
     } = await this.#connection.send('browsingContext.captureScreenshot', {
diff --git a/packages/puppeteer-core/src/cdp/Page.ts b/packages/puppeteer-core/src/cdp/Page.ts
index 3a91186f..22886670 100644
--- a/packages/puppeteer-core/src/cdp/Page.ts
+++ b/packages/puppeteer-core/src/cdp/Page.ts
@@ -1039,7 +1039,9 @@ export class CdpPage extends Page {
     await this.#frameManager.networkManager.setCacheEnabled(enabled);
   }
 
-  async _screenshot(options: Readonly<ScreenshotOptions>): Promise<string> {
+  override async _screenshot(
+    options: Readonly<ScreenshotOptions>
+  ): Promise<string> {
     const {
       fromSurface,
       omitBackground,