From aac5bf24ec08912ba6f4261ef614d0a3f27ef6e5 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 22 May 2020 15:56:37 -0700 Subject: [PATCH] fix(popups): do not override popup size from window features (#2139) We usually force window size from the browser context. However, popups that have window features insist on a specific window size, so we respect that. --- src/chromium/crPage.ts | 18 ++++++++++++++++++ src/helper.ts | 10 ++++++++++ src/webkit/wkBrowser.ts | 8 ++++++++ src/webkit/wkPage.ts | 17 ++++++++++++++++- test/popup.spec.js | 20 ++++++++++++++++++++ 5 files changed, 72 insertions(+), 1 deletion(-) diff --git a/src/chromium/crPage.ts b/src/chromium/crPage.ts index 05ddb154ee..347f3f3cf9 100644 --- a/src/chromium/crPage.ts +++ b/src/chromium/crPage.ts @@ -56,6 +56,13 @@ export class CRPage implements PageDelegate { private readonly _pagePromise: Promise; _initializedPage: Page | null = null; + // Holds window features for the next popup being opened via window.open, + // until the popup target arrives. This could be racy if two oopifs + // simultaneously call window.open with window features: the order + // of their Page.windowOpen events is not guaranteed to match the order + // of new popup targets. + readonly _nextWindowOpenPopupFeatures: string[][] = []; + constructor(client: CRSession, targetId: string, browserContext: CRBrowserContext, opener: CRPage | null, hasUIWindow: boolean) { this._targetId = targetId; this._opener = opener; @@ -68,6 +75,12 @@ export class CRPage implements PageDelegate { this._mainFrameSession = new FrameSession(this, client, targetId, null); this._sessions.set(targetId, this._mainFrameSession); client.once(CRSessionEvents.Disconnected, () => this._page._didDisconnect()); + if (opener && browserContext._options.viewport !== null) { + const features = opener._nextWindowOpenPopupFeatures.shift() || []; + const viewportSize = helper.getViewportSizeFromWindowFeatures(features); + if (viewportSize) + this._page._state.viewportSize = viewportSize; + } this._pagePromise = this._mainFrameSession._initialize(hasUIWindow).then(() => this._initializedPage = this._page).catch(e => e); } @@ -372,6 +385,7 @@ class FrameSession { helper.addEventListener(this._client, 'Runtime.executionContextsCleared', event => this._onExecutionContextsCleared()), helper.addEventListener(this._client, 'Target.attachedToTarget', event => this._onAttachedToTarget(event)), helper.addEventListener(this._client, 'Target.detachedFromTarget', event => this._onDetachedFromTarget(event)), + helper.addEventListener(this._client, 'Page.windowOpen', event => this._onWindowOpen(event)), ]; } @@ -591,6 +605,10 @@ class FrameSession { this._page._removeWorker(event.sessionId); } + _onWindowOpen(event: Protocol.Page.windowOpenPayload) { + this._crPage._nextWindowOpenPopupFeatures.push(event.windowFeatures); + } + async _onConsoleAPI(event: Protocol.Runtime.consoleAPICalledPayload) { if (event.executionContextId === 0) { // DevTools protocol stores the last 1000 console messages. These diff --git a/src/helper.ts b/src/helper.ts index 9ef343cb82..92b3809509 100644 --- a/src/helper.ts +++ b/src/helper.ts @@ -346,6 +346,16 @@ class Helper { } } } + + static getViewportSizeFromWindowFeatures(features: string[]): types.Size | null { + const widthString = features.find(f => f.startsWith('width=')); + const heightString = features.find(f => f.startsWith('height=')); + const width = widthString ? parseInt(widthString.substring(6), 10) : NaN; + const height = heightString ? parseInt(heightString.substring(7), 10) : NaN; + if (!Number.isNaN(width) && !Number.isNaN(height)) + return { width, height }; + return null; + } } export function assert(value: any, message?: string): asserts value { diff --git a/src/webkit/wkBrowser.ts b/src/webkit/wkBrowser.ts index 8904d48044..5c66d08a01 100644 --- a/src/webkit/wkBrowser.ts +++ b/src/webkit/wkBrowser.ts @@ -57,6 +57,7 @@ export class WKBrowser extends BrowserBase { helper.addEventListener(this._browserSession, 'Playwright.pageProxyCreated', this._onPageProxyCreated.bind(this)), helper.addEventListener(this._browserSession, 'Playwright.pageProxyDestroyed', this._onPageProxyDestroyed.bind(this)), helper.addEventListener(this._browserSession, 'Playwright.provisionalLoadFailed', event => this._onProvisionalLoadFailed(event)), + helper.addEventListener(this._browserSession, 'Playwright.windowOpen', event => this._onWindowOpen(event)), helper.addEventListener(this._browserSession, 'Playwright.downloadCreated', this._onDownloadCreated.bind(this)), helper.addEventListener(this._browserSession, 'Playwright.downloadFilenameSuggested', this._onDownloadFilenameSuggested.bind(this)), helper.addEventListener(this._browserSession, 'Playwright.downloadFinished', this._onDownloadFinished.bind(this)), @@ -180,6 +181,13 @@ export class WKBrowser extends BrowserBase { wkPage.handleProvisionalLoadFailed(event); } + _onWindowOpen(event: Protocol.Playwright.windowOpenPayload) { + const wkPage = this._wkPages.get(event.pageProxyId); + if (!wkPage) + return; + wkPage.handleWindowOpen(event); + } + isConnected(): boolean { return !this._connection.isClosed(); } diff --git a/src/webkit/wkPage.ts b/src/webkit/wkPage.ts index 91e9735860..e06ef0e4c7 100644 --- a/src/webkit/wkPage.ts +++ b/src/webkit/wkPage.ts @@ -16,7 +16,7 @@ */ import * as frames from '../frames'; -import { helper, RegisteredListener, assert } from '../helper'; +import { helper, RegisteredListener, assert, debugAssert } from '../helper'; import * as dom from '../dom'; import * as network from '../network'; import { WKSession } from './wkConnection'; @@ -68,6 +68,10 @@ export class WKPage implements PageDelegate { _firstNonInitialNavigationCommittedReject = (e: Error) => {}; private _lastConsoleMessage: { derivedType: string, text: string, handles: JSHandle[]; count: number, location: ConsoleMessageLocation; } | null = null; + // Holds window features for the next popup being opened via window.open, + // until the popup page proxy arrives. + private _nextWindowOpenPopupFeatures?: string[]; + constructor(browserContext: WKBrowserContext, pageProxySession: WKSession, opener: WKPage | null) { this._pageProxySession = pageProxySession; this._opener = opener; @@ -90,6 +94,12 @@ export class WKPage implements PageDelegate { this._firstNonInitialNavigationCommittedFulfill = f; this._firstNonInitialNavigationCommittedReject = r; }); + if (opener && browserContext._options.viewport !== null && opener._nextWindowOpenPopupFeatures) { + const viewportSize = helper.getViewportSizeFromWindowFeatures(opener._nextWindowOpenPopupFeatures); + opener._nextWindowOpenPopupFeatures = undefined; + if (viewportSize) + this._page._state.viewportSize = viewportSize; + } } private async _initializePageProxySession() { @@ -243,6 +253,11 @@ export class WKPage implements PageDelegate { this._page._frameManager.provisionalLoadFailed(this._page.mainFrame(), event.loaderId, errorText); } + handleWindowOpen(event: Protocol.Playwright.windowOpenPayload) { + debugAssert(!this._nextWindowOpenPopupFeatures); + this._nextWindowOpenPopupFeatures = event.windowFeatures; + } + async pageOrError(): Promise { return this._pagePromise; } diff --git a/test/popup.spec.js b/test/popup.spec.js index db2d3ce14b..9516dd8308 100644 --- a/test/popup.spec.js +++ b/test/popup.spec.js @@ -138,6 +138,26 @@ describe('window.open', function() { await context.close(); expect(size).toEqual({width: 400, height: 500}); }); + it('should use viewport size from window features', async function({browser, server}) { + const context = await browser.newContext({ + viewport: { width: 700, height: 700 } + }); + const page = await context.newPage(); + await page.goto(server.EMPTY_PAGE); + const [size, popup] = await Promise.all([ + page.evaluate(() => { + const win = window.open(window.location.href, 'Title', 'toolbar=no,location=no,directories=no,status=no,menubar=no,scrollbars=yes,resizable=yes,width=600,height=300,top=0,left=0'); + return { width: win.innerWidth, height: win.innerHeight }; + }), + page.waitForEvent('popup'), + ]); + await popup.setViewportSize({ width: 500, height: 400 }); + await popup.waitForLoadState(); + const resized = await popup.evaluate(() => ({ width: window.innerWidth, height: window.innerHeight })); + await context.close(); + expect(size).toEqual({width: 600, height: 300}); + expect(resized).toEqual({width: 500, height: 400}); + }); it('should respect routes from browser context', async function({browser, server}) { const context = await browser.newContext(); const page = await context.newPage();