From 75ed251c9e75e869ad7f8304fbe4240dd952b17e Mon Sep 17 00:00:00 2001 From: Max Schmitt Date: Thu, 17 Aug 2023 10:57:28 +0200 Subject: [PATCH] fix: download of attachments in UI Mode (#26407) Fixes https://github.com/microsoft/playwright/issues/26326. --- .../src/client/browserContext.ts | 9 ++++ .../playwright-core/src/client/electron.ts | 3 +- packages/playwright-core/src/client/types.ts | 3 +- .../playwright-core/src/protocol/validator.ts | 10 ++-- .../src/server/browserContext.ts | 4 +- .../playwright-core/src/server/browserType.ts | 3 +- .../src/server/chromium/crBrowser.ts | 4 +- .../playwright-core/src/server/download.ts | 2 +- .../src/server/firefox/ffBrowser.ts | 16 ++++--- .../src/server/webkit/wkBrowser.ts | 2 +- packages/protocol/src/channels.ts | 20 ++++---- packages/protocol/src/protocol.yml | 14 +++++- packages/trace-viewer/src/sw.ts | 22 +++++++-- packages/trace-viewer/src/traceModel.ts | 7 +++ .../trace-viewer/src/ui/attachmentsTab.tsx | 4 +- packages/trace/src/trace.ts | 16 ++++--- .../ui-mode-test-attachments.spec.ts | 48 +++++++++++++++---- 17 files changed, 132 insertions(+), 55 deletions(-) diff --git a/packages/playwright-core/src/client/browserContext.ts b/packages/playwright-core/src/client/browserContext.ts index 1284ff6625..30144a1aaf 100644 --- a/packages/playwright-core/src/client/browserContext.ts +++ b/packages/playwright-core/src/client/browserContext.ts @@ -451,6 +451,7 @@ export async function prepareBrowserContextParams(options: BrowserContextOptions colorScheme: options.colorScheme === null ? 'no-override' : options.colorScheme, reducedMotion: options.reducedMotion === null ? 'no-override' : options.reducedMotion, forcedColors: options.forcedColors === null ? 'no-override' : options.forcedColors, + acceptDownloads: toAcceptDownloadsProtocol(options.acceptDownloads), }; if (!contextParams.recordVideo && options.videosPath) { contextParams.recordVideo = { @@ -460,3 +461,11 @@ export async function prepareBrowserContextParams(options: BrowserContextOptions } return contextParams; } + +function toAcceptDownloadsProtocol(acceptDownloads?: boolean) { + if (acceptDownloads === undefined) + return undefined; + if (acceptDownloads === true) + return 'accept'; + return 'deny'; +} diff --git a/packages/playwright-core/src/client/electron.ts b/packages/playwright-core/src/client/electron.ts index 9442d61576..0c59db7b15 100644 --- a/packages/playwright-core/src/client/electron.ts +++ b/packages/playwright-core/src/client/electron.ts @@ -29,11 +29,12 @@ import type { Page } from './page'; import type { Env, WaitForEventOptions, Headers, BrowserContextOptions } from './types'; import { Waiter } from './waiter'; -type ElectronOptions = Omit & { +type ElectronOptions = Omit & { env?: Env, extraHTTPHeaders?: Headers, recordHar?: BrowserContextOptions['recordHar'], colorScheme?: 'dark' | 'light' | 'no-preference' | null, + acceptDownloads?: boolean, }; type ElectronAppType = typeof import('electron'); diff --git a/packages/playwright-core/src/client/types.ts b/packages/playwright-core/src/client/types.ts index 9f8845aded..4884d89cc2 100644 --- a/packages/playwright-core/src/client/types.ts +++ b/packages/playwright-core/src/client/types.ts @@ -47,7 +47,7 @@ export type SetStorageState = { export type LifecycleEvent = channels.LifecycleEvent; export const kLifecycleEvents: Set = new Set(['load', 'domcontentloaded', 'networkidle', 'commit']); -export type BrowserContextOptions = Omit & { +export type BrowserContextOptions = Omit & { viewport?: Size | null; extraHTTPHeaders?: Headers; logger?: Logger; @@ -69,6 +69,7 @@ export type BrowserContextOptions = Omit page._browserContext === this)); const promises: Promise[] = [super._initialize()]; - if (this._browser.options.name !== 'electron' && this._browser.options.name !== 'clank') { + if (this._browser.options.name !== 'electron' && this._browser.options.name !== 'clank' && this._options.acceptDownloads !== 'internal-browser-default') { promises.push(this._browser._session.send('Browser.setDownloadBehavior', { - behavior: this._options.acceptDownloads ? 'allowAndName' : 'deny', + behavior: this._options.acceptDownloads === 'accept' ? 'allowAndName' : 'deny', browserContextId: this._browserContextId, downloadPath: this._browser.options.downloadsPath, eventsEnabled: true, diff --git a/packages/playwright-core/src/server/download.ts b/packages/playwright-core/src/server/download.ts index 1e33c65e8b..f7a92c8c7d 100644 --- a/packages/playwright-core/src/server/download.ts +++ b/packages/playwright-core/src/server/download.ts @@ -26,7 +26,7 @@ export class Download { private _suggestedFilename: string | undefined; constructor(page: Page, downloadsPath: string, uuid: string, url: string, suggestedFilename?: string) { - const unaccessibleErrorMessage = !page._browserContext._options.acceptDownloads ? 'Pass { acceptDownloads: true } when you are creating your browser context.' : undefined; + const unaccessibleErrorMessage = page._browserContext._options.acceptDownloads === 'deny' ? 'Pass { acceptDownloads: true } when you are creating your browser context.' : undefined; this.artifact = new Artifact(page, path.join(downloadsPath, uuid), unaccessibleErrorMessage, () => { return this._page._browserContext.cancelDownload(uuid); }); diff --git a/packages/playwright-core/src/server/firefox/ffBrowser.ts b/packages/playwright-core/src/server/firefox/ffBrowser.ts index d6d22b3eae..9db93752fe 100644 --- a/packages/playwright-core/src/server/firefox/ffBrowser.ts +++ b/packages/playwright-core/src/server/firefox/ffBrowser.ts @@ -174,13 +174,15 @@ export class FFBrowserContext extends BrowserContext { assert(!this._ffPages().length); const browserContextId = this._browserContextId; const promises: Promise[] = [super._initialize()]; - promises.push(this._browser._connection.send('Browser.setDownloadOptions', { - browserContextId, - downloadOptions: { - behavior: this._options.acceptDownloads ? 'saveToDisk' : 'cancel', - downloadsDir: this._browser.options.downloadsPath, - }, - })); + if (this._options.acceptDownloads !== 'internal-browser-default') { + promises.push(this._browser._connection.send('Browser.setDownloadOptions', { + browserContextId, + downloadOptions: { + behavior: this._options.acceptDownloads === 'accept' ? 'saveToDisk' : 'cancel', + downloadsDir: this._browser.options.downloadsPath, + }, + })); + } if (this._options.viewport) { const viewport = { viewportSize: { width: this._options.viewport.width, height: this._options.viewport.height }, diff --git a/packages/playwright-core/src/server/webkit/wkBrowser.ts b/packages/playwright-core/src/server/webkit/wkBrowser.ts index 826415077c..492eaa2929 100644 --- a/packages/playwright-core/src/server/webkit/wkBrowser.ts +++ b/packages/playwright-core/src/server/webkit/wkBrowser.ts @@ -222,7 +222,7 @@ export class WKBrowserContext extends BrowserContext { const browserContextId = this._browserContextId; const promises: Promise[] = [super._initialize()]; promises.push(this._browser._browserSession.send('Playwright.setDownloadBehavior', { - behavior: this._options.acceptDownloads ? 'allow' : 'deny', + behavior: this._options.acceptDownloads === 'accept' ? 'allow' : 'deny', downloadPath: this._browser.options.downloadsPath, browserContextId })); diff --git a/packages/protocol/src/channels.ts b/packages/protocol/src/channels.ts index 91a19123f0..3ffa80dc60 100644 --- a/packages/protocol/src/channels.ts +++ b/packages/protocol/src/channels.ts @@ -965,7 +965,7 @@ export type BrowserTypeLaunchPersistentContextParams = { colorScheme?: 'dark' | 'light' | 'no-preference' | 'no-override', reducedMotion?: 'reduce' | 'no-preference' | 'no-override', forcedColors?: 'active' | 'none' | 'no-override', - acceptDownloads?: boolean, + acceptDownloads?: 'accept' | 'deny' | 'internal-browser-default', baseURL?: string, recordVideo?: { dir: string, @@ -1036,7 +1036,7 @@ export type BrowserTypeLaunchPersistentContextOptions = { colorScheme?: 'dark' | 'light' | 'no-preference' | 'no-override', reducedMotion?: 'reduce' | 'no-preference' | 'no-override', forcedColors?: 'active' | 'none' | 'no-override', - acceptDownloads?: boolean, + acceptDownloads?: 'accept' | 'deny' | 'internal-browser-default', baseURL?: string, recordVideo?: { dir: string, @@ -1138,7 +1138,7 @@ export type BrowserNewContextParams = { colorScheme?: 'dark' | 'light' | 'no-preference' | 'no-override', reducedMotion?: 'reduce' | 'no-preference' | 'no-override', forcedColors?: 'active' | 'none' | 'no-override', - acceptDownloads?: boolean, + acceptDownloads?: 'accept' | 'deny' | 'internal-browser-default', baseURL?: string, recordVideo?: { dir: string, @@ -1196,7 +1196,7 @@ export type BrowserNewContextOptions = { colorScheme?: 'dark' | 'light' | 'no-preference' | 'no-override', reducedMotion?: 'reduce' | 'no-preference' | 'no-override', forcedColors?: 'active' | 'none' | 'no-override', - acceptDownloads?: boolean, + acceptDownloads?: 'accept' | 'deny' | 'internal-browser-default', baseURL?: string, recordVideo?: { dir: string, @@ -1257,7 +1257,7 @@ export type BrowserNewContextForReuseParams = { colorScheme?: 'dark' | 'light' | 'no-preference' | 'no-override', reducedMotion?: 'reduce' | 'no-preference' | 'no-override', forcedColors?: 'active' | 'none' | 'no-override', - acceptDownloads?: boolean, + acceptDownloads?: 'accept' | 'deny' | 'internal-browser-default', baseURL?: string, recordVideo?: { dir: string, @@ -1315,7 +1315,7 @@ export type BrowserNewContextForReuseOptions = { colorScheme?: 'dark' | 'light' | 'no-preference' | 'no-override', reducedMotion?: 'reduce' | 'no-preference' | 'no-override', forcedColors?: 'active' | 'none' | 'no-override', - acceptDownloads?: boolean, + acceptDownloads?: 'accept' | 'deny' | 'internal-browser-default', baseURL?: string, recordVideo?: { dir: string, @@ -3986,7 +3986,7 @@ export type ElectronLaunchParams = { cwd?: string, env?: NameValue[], timeout?: number, - acceptDownloads?: boolean, + acceptDownloads?: 'accept' | 'deny' | 'internal-browser-default', bypassCSP?: boolean, colorScheme?: 'dark' | 'light' | 'no-preference' | 'no-override', extraHTTPHeaders?: NameValue[], @@ -4021,7 +4021,7 @@ export type ElectronLaunchOptions = { cwd?: string, env?: NameValue[], timeout?: number, - acceptDownloads?: boolean, + acceptDownloads?: 'accept' | 'deny' | 'internal-browser-default', bypassCSP?: boolean, colorScheme?: 'dark' | 'light' | 'no-preference' | 'no-override', extraHTTPHeaders?: NameValue[], @@ -4412,7 +4412,7 @@ export type AndroidDeviceLaunchBrowserParams = { colorScheme?: 'dark' | 'light' | 'no-preference' | 'no-override', reducedMotion?: 'reduce' | 'no-preference' | 'no-override', forcedColors?: 'active' | 'none' | 'no-override', - acceptDownloads?: boolean, + acceptDownloads?: 'accept' | 'deny' | 'internal-browser-default', baseURL?: string, recordVideo?: { dir: string, @@ -4468,7 +4468,7 @@ export type AndroidDeviceLaunchBrowserOptions = { colorScheme?: 'dark' | 'light' | 'no-preference' | 'no-override', reducedMotion?: 'reduce' | 'no-preference' | 'no-override', forcedColors?: 'active' | 'none' | 'no-override', - acceptDownloads?: boolean, + acceptDownloads?: 'accept' | 'deny' | 'internal-browser-default', baseURL?: string, recordVideo?: { dir: string, diff --git a/packages/protocol/src/protocol.yml b/packages/protocol/src/protocol.yml index c1d15602e4..bbc4009c4b 100644 --- a/packages/protocol/src/protocol.yml +++ b/packages/protocol/src/protocol.yml @@ -474,7 +474,12 @@ ContextOptions: - active - none - no-override - acceptDownloads: boolean? + acceptDownloads: + type: enum? + literals: + - accept + - deny + - internal-browser-default baseURL: string? recordVideo: type: object? @@ -3136,7 +3141,12 @@ Electron: type: array? items: NameValue timeout: number? - acceptDownloads: boolean? + acceptDownloads: + type: enum? + literals: + - accept + - deny + - internal-browser-default bypassCSP: boolean? colorScheme: type: enum? diff --git a/packages/trace-viewer/src/sw.ts b/packages/trace-viewer/src/sw.ts index 13c21d745e..ce83a027bb 100644 --- a/packages/trace-viewer/src/sw.ts +++ b/packages/trace-viewer/src/sw.ts @@ -122,9 +122,7 @@ async function doFetch(event: FetchEvent): Promise { // We will accept explicit ?trace= value as well as the clientId associated with the trace. if (traceUrl !== trace && !traceUrls.includes(trace)) continue; - const blob = await traceModel!.resourceForSha1(relativePath.slice('/sha1/'.length)); - if (blob) - return new Response(blob, { status: 200 }); + return await serveResource(traceModel, relativePath.slice('/sha1/'.length)); } return new Response(null, { status: 404 }); } @@ -145,6 +143,24 @@ async function doFetch(event: FetchEvent): Promise { return snapshotServer.serveResource(lookupUrls, request.method, snapshotUrl); } +async function serveResource(traceModel: TraceModel, sha1: string): Promise { + const blob = await traceModel!.resourceForSha1(sha1); + if (blob) + return new Response(blob, { status: 200, headers: headersForResource(traceModel, sha1) }); + return new Response(null, { status: 404 }); +} + +function headersForResource(traceModel: TraceModel, sha1: string): Headers | undefined { + const attachment = traceModel.attachmentForSha1(sha1); + if (!attachment) + return; + const headers = new Headers(); + headers.set('Content-Disposition', `attachment; filename="${attachment.name}"`); + if (attachment.contentType) + headers.set('Content-Type', attachment.contentType); + return headers; +} + async function gc() { const clients = await self.clients.matchAll(); const usedTraces = new Set(); diff --git a/packages/trace-viewer/src/traceModel.ts b/packages/trace-viewer/src/traceModel.ts index 208a6705f5..2726bc6ece 100644 --- a/packages/trace-viewer/src/traceModel.ts +++ b/packages/trace-viewer/src/traceModel.ts @@ -35,6 +35,7 @@ export class TraceModel { private _snapshotStorage: SnapshotStorage | undefined; private _version: number | undefined; private _backend!: TraceModelBackend; + private _attachments = new Map(); constructor() { } @@ -112,6 +113,10 @@ export class TraceModel { return this._backend.readBlob('resources/' + sha1); } + attachmentForSha1(sha1: string): trace.AfterActionTraceEventAttachment | undefined { + return this._attachments.get(sha1); + } + storage(): SnapshotStorage { return this._snapshotStorage!; } @@ -169,6 +174,8 @@ export class TraceModel { existing!.result = event.result; existing!.error = event.error; existing!.attachments = event.attachments; + for (const attachment of event.attachments?.filter(a => a.sha1) || []) + this._attachments.set(attachment.sha1!, attachment); break; } case 'action': { diff --git a/packages/trace-viewer/src/ui/attachmentsTab.tsx b/packages/trace-viewer/src/ui/attachmentsTab.tsx index 597ee60c36..c95c224569 100644 --- a/packages/trace-viewer/src/ui/attachmentsTab.tsx +++ b/packages/trace-viewer/src/ui/attachmentsTab.tsx @@ -56,13 +56,13 @@ export const AttachmentsSection: React.FunctionComponent<{ {[...screenshots].map((a, i) => { return ; })} {otherAttachments.size ?
Attachments
: undefined} {[...otherAttachments].map((a, i) => { return ; })} ; diff --git a/packages/trace/src/trace.ts b/packages/trace/src/trace.ts index 891d7be97e..afd101d1f2 100644 --- a/packages/trace/src/trace.ts +++ b/packages/trace/src/trace.ts @@ -73,6 +73,14 @@ export type InputActionTraceEvent = { point?: Point; }; +export type AfterActionTraceEventAttachment = { + name: string; + contentType: string; + path?: string; + sha1?: string; + base64?: string; +}; + export type AfterActionTraceEvent = { type: 'after', callId: string; @@ -80,13 +88,7 @@ export type AfterActionTraceEvent = { afterSnapshot?: string; log: string[]; error?: SerializedError['error']; - attachments?: { - name: string; - contentType: string; - path?: string; - sha1?: string; - base64?: string; - }[]; + attachments?: AfterActionTraceEventAttachment[]; result?: any; }; diff --git a/tests/playwright-test/ui-mode-test-attachments.spec.ts b/tests/playwright-test/ui-mode-test-attachments.spec.ts index 59aefdb70b..5dee257eda 100644 --- a/tests/playwright-test/ui-mode-test-attachments.spec.ts +++ b/tests/playwright-test/ui-mode-test-attachments.spec.ts @@ -14,6 +14,7 @@ * limitations under the License. */ +import fs from 'fs'; import { test, expect, retries } from './ui-mode-fixtures'; test.describe.configure({ mode: 'parallel', retries }); @@ -32,12 +33,11 @@ test('should contain file attachment', async ({ runUITest }) => { await expect(page.getByTestId('status-line')).toHaveText('1/1 passed (100%)'); await page.getByText('Attachments').click(); await page.getByText('attach "note"', { exact: true }).click(); - const popupPromise = page.waitForEvent('popup'); + const downloadPromise = page.waitForEvent('download'); await page.getByRole('link', { name: 'note' }).click(); - const popup = await popupPromise; - await popup.waitForLoadState(); - const content = await popup.content(); - expect(content).toContain('attach test'); + const download = await downloadPromise; + expect(download.suggestedFilename()).toBe('note'); + expect((await readAllFromStream(await download.createReadStream())).toString()).toContain('attach test'); }); test('should contain string attachment', async ({ runUITest }) => { @@ -54,10 +54,38 @@ test('should contain string attachment', async ({ runUITest }) => { await expect(page.getByTestId('status-line')).toHaveText('1/1 passed (100%)'); await page.getByText('Attachments').click(); await page.getByText('attach "note"', { exact: true }).click(); - const popupPromise = page.waitForEvent('popup'); + const downloadPromise = page.waitForEvent('download'); await page.getByRole('link', { name: 'note' }).click(); - const popup = await popupPromise; - await popup.waitForLoadState(); - const content = await popup.content(); - expect(content).toContain('text42'); + const download = await downloadPromise; + expect(download.suggestedFilename()).toBe('note'); + expect((await readAllFromStream(await download.createReadStream())).toString()).toEqual('text42'); }); + +test('should contain attachment with filename and extension', async ({ runUITest, asset }) => { + const { page } = await runUITest({ + 'a.test.ts': ` + import { test } from '@playwright/test'; + test('attach test', async () => { + await test.info().attach('screenshot.png', { path: ${JSON.stringify(asset('pptr.png'))} }); + }); + `, + }); + await page.getByText('attach test').click(); + await page.getByTitle('Run all').click(); + await expect(page.getByTestId('status-line')).toHaveText('1/1 passed (100%)'); + await page.getByText('Attachments').click(); + await page.getByText('attach "screenshot.png"', { exact: true }).click(); + const downloadPromise = page.waitForEvent('download'); + await page.getByRole('link', { name: 'screenshot.png' }).click(); + const download = await downloadPromise; + expect(download.suggestedFilename()).toBe('screenshot.png'); + expect(await readAllFromStream(await download.createReadStream())).toEqual(fs.readFileSync(asset('pptr.png'))); +}); + +function readAllFromStream(stream: NodeJS.ReadableStream): Promise { + return new Promise(resolve => { + const chunks: Buffer[] = []; + stream.on('data', chunk => chunks.push(chunk)); + stream.on('end', () => resolve(Buffer.concat(chunks))); + }); +}