From 072dcba94a7188bc6bbb56a50efdce31658b87f6 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 12 May 2020 18:31:17 -0700 Subject: [PATCH] api(viewport): do not allow isMobile and deviceScaleFactor for null viewports (#2190) Also make sure we do not alter viewport when passed null. --- src/browserContext.ts | 4 ++++ src/chromium/crPage.ts | 20 ++++++++++---------- src/page.ts | 9 +-------- src/webkit/wkPage.ts | 18 ++++++++---------- test/browsercontext.spec.js | 8 ++++++++ test/emulation.spec.js | 7 +++++++ test/headful.spec.js | 18 ++++++++++++++++++ 7 files changed, 56 insertions(+), 28 deletions(-) diff --git a/src/browserContext.ts b/src/browserContext.ts index 479a85bada..1446459969 100644 --- a/src/browserContext.ts +++ b/src/browserContext.ts @@ -186,6 +186,10 @@ export function assertBrowserContextIsNotOwned(context: BrowserContextBase) { export function validateBrowserContextOptions(options: BrowserContextOptions): BrowserContextOptions { const result = { ...options }; + if (result.viewport === null && result.deviceScaleFactor !== undefined) + throw new Error(`"deviceScaleFactor" option is not supported with null "viewport"`); + if (result.viewport === null && result.isMobile !== undefined) + throw new Error(`"isMobile" option is not supported with null "viewport"`); if (!result.viewport && result.viewport !== null) result.viewport = { width: 1280, height: 720 }; if (result.viewport) diff --git a/src/chromium/crPage.ts b/src/chromium/crPage.ts index 820644a388..81a4a1c3c7 100644 --- a/src/chromium/crPage.ts +++ b/src/chromium/crPage.ts @@ -429,7 +429,7 @@ class FrameSession { promises.push(this._client.send('Page.setBypassCSP', { enabled: true })); if (options.ignoreHTTPSErrors) promises.push(this._client.send('Security.setIgnoreCertificateErrors', { ignore: true })); - if (this._isMainFrame() && options.viewport) + if (this._isMainFrame()) promises.push(this._updateViewport()); if (options.hasTouch) promises.push(this._client.send('Emulation.setTouchEmulationEnabled', { enabled: true })); @@ -706,23 +706,23 @@ class FrameSession { async _updateViewport(): Promise { assert(this._isMainFrame()); const options = this._crPage._browserContext._options; - let viewport = options.viewport || { width: 0, height: 0 }; const viewportSize = this._page._state.viewportSize; - if (viewportSize) - viewport = { ...viewport, ...viewportSize }; - const isLandscape = viewport.width > viewport.height; + if (viewportSize === null) + return; + const isLandscape = viewportSize.width > viewportSize.height; const promises = [ this._client.send('Emulation.setDeviceMetricsOverride', { mobile: !!options.isMobile, - width: viewport.width, - height: viewport.height, - screenWidth: viewport.width, - screenHeight: viewport.height, + width: viewportSize.width, + height: viewportSize.height, + screenWidth: viewportSize.width, + screenHeight: viewportSize.height, deviceScaleFactor: options.deviceScaleFactor || 1, screenOrientation: isLandscape ? { angle: 90, type: 'landscapePrimary' } : { angle: 0, type: 'portraitPrimary' }, }), ]; if (this._windowId) { + // TODO: popup windows have their own insets. let insets = { width: 24, height: 88 }; if (process.platform === 'win32') insets = { width: 16, height: 88 }; @@ -733,7 +733,7 @@ class FrameSession { promises.push(this._client.send('Browser.setWindowBounds', { windowId: this._windowId, - bounds: { width: viewport.width + insets.width, height: viewport.height + insets.height } + bounds: { width: viewportSize.width + insets.width, height: viewportSize.height + insets.height } })); } await Promise.all(promises); diff --git a/src/page.ts b/src/page.ts index 9d885f96ff..3004593db2 100644 --- a/src/page.ts +++ b/src/page.ts @@ -120,15 +120,8 @@ export class Page extends ExtendedEventEmitter implements InnerLogger { this._disconnectedCallback = () => {}; this._disconnectedPromise = new Promise(f => this._disconnectedCallback = f); this._browserContext = browserContext; - let viewportSize: types.Size | null = null; - if (browserContext._options.viewport) { - viewportSize = { - width: browserContext._options.viewport.width, - height: browserContext._options.viewport.height, - }; - } this._state = { - viewportSize, + viewportSize: browserContext._options.viewport ? { ...browserContext._options.viewport } : null, mediaType: null, colorScheme: null, extraHTTPHeaders: null, diff --git a/src/webkit/wkPage.ts b/src/webkit/wkPage.ts index 5e34230794..caa17250c6 100644 --- a/src/webkit/wkPage.ts +++ b/src/webkit/wkPage.ts @@ -100,8 +100,7 @@ export class WKPage implements PageDelegate { const contextOptions = this._browserContext._options; if (contextOptions.javaScriptEnabled === false) promises.push(this._pageProxySession.send('Emulation.setJavaScriptEnabled', { enabled: false })); - if (this._page._state.viewportSize || contextOptions.viewport) - promises.push(this._updateViewport()); + promises.push(this._updateViewport()); promises.push(this.updateHttpCredentials()); if (this._browserContext._permissions.size) { for (const [key, value] of this._browserContext._permissions) @@ -573,24 +572,23 @@ export class WKPage implements PageDelegate { async _updateViewport(): Promise { const options = this._browserContext._options; - let viewport = options.viewport || { width: 0, height: 0 }; const viewportSize = this._page._state.viewportSize; - if (viewportSize) - viewport = { ...viewport, ...viewportSize }; + if (viewportSize === null) + return; const promises: Promise[] = [ this._pageProxySession.send('Emulation.setDeviceMetricsOverride', { - width: viewport.width, - height: viewport.height, + width: viewportSize.width, + height: viewportSize.height, fixedLayout: !!options.isMobile, deviceScaleFactor: options.deviceScaleFactor || 1 }), this._session.send('Page.setScreenSizeOverride', { - width: viewport.width, - height: viewport.height, + width: viewportSize.width, + height: viewportSize.height, }), ]; if (options.isMobile) { - const angle = viewport.width > viewport.height ? 90 : 0; + const angle = viewportSize.width > viewportSize.height ? 90 : 0; promises.push(this._session.send('Page.setOrientationOverride', { angle })); } await Promise.all(promises); diff --git a/test/browsercontext.spec.js b/test/browsercontext.spec.js index dd3e58f43c..76d8a4168b 100644 --- a/test/browsercontext.spec.js +++ b/test/browsercontext.spec.js @@ -102,6 +102,14 @@ describe('BrowserContext', function() { expect(await page.evaluate('window.innerHeight')).toBe(789); await context.close(); }); + it('should not allow deviceScaleFactor with null viewport', async({ browser }) => { + const error = await browser.newContext({ viewport: null, deviceScaleFactor: 1 }).catch(e => e); + expect(error.message).toBe('"deviceScaleFactor" option is not supported with null "viewport"'); + }); + it('should not allow isMobile with null viewport', async({ browser }) => { + const error = await browser.newContext({ viewport: null, isMobile: true }).catch(e => e); + expect(error.message).toBe('"isMobile" option is not supported with null "viewport"'); + }); it('close() should work for empty context', async({ browser }) => { const context = await browser.newContext(); await context.close(); diff --git a/test/emulation.spec.js b/test/emulation.spec.js index 1b95369f99..f6a659e110 100644 --- a/test/emulation.spec.js +++ b/test/emulation.spec.js @@ -77,6 +77,13 @@ describe('BrowserContext({viewport})', function() { await page.goto(server.PREFIX + '/detect-touch.html'); expect(await page.evaluate(() => document.body.textContent.trim())).toBe('NO'); }); + it('should support touch with null viewport', async({browser, server}) => { + const context = await browser.newContext({ viewport: null, hasTouch: true }); + const page = await context.newPage(); + await page.goto(server.PREFIX + '/mobile.html'); + expect(await page.evaluate(() => 'ontouchstart' in window)).toBe(true); + await context.close(); + }); }); describe.skip(FFOX)('viewport.isMobile', () => { diff --git a/test/headful.spec.js b/test/headful.spec.js index 87ca758e6d..a63e5df166 100644 --- a/test/headful.spec.js +++ b/test/headful.spec.js @@ -114,4 +114,22 @@ describe('Headful', function() { } await browser.close(); }); + it.fail(WEBKIT)('should not override viewport size when passed null', async function({browserType, defaultBrowserOptions, server}) { + // Our WebKit embedder does not respect window features. + const browser = await browserType.launch({...defaultBrowserOptions, headless: false }); + const context = await browser.newContext({ viewport: null }); + const page = await context.newPage(); + await page.goto(server.EMPTY_PAGE); + const [popup] = await Promise.all([ + page.waitForEvent('popup'), + 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=780,height=200,top=0,left=0'); + win.resizeTo(500, 450); + }), + ]); + await popup.waitForLoadState(); + const size = await popup.evaluate(() => ({ width: window.outerWidth, height: window.outerHeight })); + await context.close(); + expect(size).toEqual({width: 500, height: 450}); + }); });