From 5c3f483659c8446d0c5402657eb20c01d96bad17 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Wed, 13 Jan 2021 14:25:42 -0800 Subject: [PATCH] fix(cli): do not extend injected script on same-document navigations (#5002) Otherwise, the injected script has to be ready for reentrancy. --- src/cli/injected/recorder.ts | 2 +- src/server/browserContext.ts | 2 +- src/server/frames.ts | 3 +-- src/server/page.ts | 6 ++--- test/browsercontext-expose-function.spec.ts | 23 +++++++++++++++++ test/cli/cli-codegen.spec.ts | 28 +++++++++++++++++++++ 6 files changed, 57 insertions(+), 7 deletions(-) diff --git a/src/cli/injected/recorder.ts b/src/cli/injected/recorder.ts index 32ca7e698a9cb..5ec1bb7288154 100644 --- a/src/cli/injected/recorder.ts +++ b/src/cli/injected/recorder.ts @@ -372,7 +372,7 @@ export class Recorder { if (event.key === '@' && event.code === 'KeyL') return false; // Allow and ignore common used shortcut for pasting. - if (process.platform === 'darwin') { + if (navigator.platform.includes('Mac')) { if (event.key === 'v' && event.metaKey) return false; } else { diff --git a/src/server/browserContext.ts b/src/server/browserContext.ts index 24acdedca5402..2f13802f712ed 100644 --- a/src/server/browserContext.ts +++ b/src/server/browserContext.ts @@ -382,7 +382,7 @@ export abstract class BrowserContext extends EventEmitter { async extendInjectedScript(source: string, arg?: any) { const installInFrame = (frame: frames.Frame) => frame.extendInjectedScript(source, arg).catch(e => {}); const installInPage = (page: Page) => { - page.on(Page.Events.FrameNavigated, installInFrame); + page.on(Page.Events.InternalFrameNavigatedToNewDocument, installInFrame); return Promise.all(page.frames().map(installInFrame)); }; this.on(BrowserContext.Events.Page, installInPage); diff --git a/src/server/frames.ts b/src/server/frames.ts index fa9f73a3cc344..b522ade64b45a 100644 --- a/src/server/frames.ts +++ b/src/server/frames.ts @@ -199,7 +199,7 @@ export class FrameManager { frame.emit(Frame.Events.Navigation, navigationEvent); if (!initial) { debugLogger.log('api', ` navigated to "${url}"`); - this._page.frameNavigated(frame); + this._page.frameNavigatedToNewDocument(frame); } // Restore pending if any - see comments above about keepPending. frame._pendingDocument = keepPending; @@ -213,7 +213,6 @@ export class FrameManager { const navigationEvent: NavigationEvent = { url, name: frame._name }; frame.emit(Frame.Events.Navigation, navigationEvent); debugLogger.log('api', ` navigated to "${url}"`); - this._page.frameNavigated(frame); } frameAbortedNavigation(frameId: string, errorText: string, documentId?: string) { diff --git a/src/server/page.ts b/src/server/page.ts index cad5db0979876..29a01cd3a36a7 100644 --- a/src/server/page.ts +++ b/src/server/page.ts @@ -110,7 +110,7 @@ export class Page extends EventEmitter { RequestFinished: 'requestfinished', FrameAttached: 'frameattached', FrameDetached: 'framedetached', - FrameNavigated: 'framenavigated', + InternalFrameNavigatedToNewDocument: 'internalframenavigatedtonewdocument', Load: 'load', Popup: 'popup', WebSocket: 'websocket', @@ -457,8 +457,8 @@ export class Page extends EventEmitter { this.emit(Page.Events.VideoStarted, video); } - frameNavigated(frame: frames.Frame) { - this.emit(Page.Events.FrameNavigated, frame); + frameNavigatedToNewDocument(frame: frames.Frame) { + this.emit(Page.Events.InternalFrameNavigatedToNewDocument, frame); const url = frame.url(); if (!url.startsWith('http')) return; diff --git a/test/browsercontext-expose-function.spec.ts b/test/browsercontext-expose-function.spec.ts index 96a74a5299066..a26d2717353dd 100644 --- a/test/browsercontext-expose-function.spec.ts +++ b/test/browsercontext-expose-function.spec.ts @@ -82,3 +82,26 @@ it('exposeBindingHandle should work', async ({context}) => { expect(await target.evaluate(x => x.foo)).toBe(42); expect(result).toEqual(17); }); + +it('extendInjectedScript should work', async ({ context, server }) => { + await (context as any)._extendInjectedScript(`var pwExport = (() => { + class Foo { + constructor() { + window._counter = (window._counter || 0) + 1; + } + } + return Foo; + })()`); + + const page = await context.newPage(); + await page.waitForFunction(() => (window as any)._counter === 1); + + await page.goto(server.EMPTY_PAGE); + await page.waitForFunction(() => (window as any)._counter === 1); + + await Promise.all([ + page.waitForNavigation(), + page.evaluate(() => history.pushState({}, '', '/url.html')) + ]); + expect(await page.evaluate(() => (window as any)._counter)).toBe(1); +}); diff --git a/test/cli/cli-codegen.spec.ts b/test/cli/cli-codegen.spec.ts index a5e44453e8255..b83ea5d19742f 100644 --- a/test/cli/cli-codegen.spec.ts +++ b/test/cli/cli-codegen.spec.ts @@ -40,6 +40,34 @@ describe('cli codegen', (test, { browserName, headful }) => { expect(message.text()).toBe('click'); }); + it('should click after same-document navigation', async ({ page, recorder, httpServer }) => { + httpServer.setHandler((req: http.IncomingMessage, res: http.ServerResponse) => { + res.setHeader('Content-Type', 'text/html; charset=utf-8'); + res.end(''); + }); + await recorder.setContentAndWait(``, httpServer.PREFIX + '/foo.html'); + await Promise.all([ + page.waitForNavigation(), + page.evaluate(() => history.pushState({}, '', '/url.html')), + ]); + // This is the only way to give recorder a chance to install + // the second unnecessary copy of the recorder script. + await page.waitForTimeout(1000); + + const selector = await recorder.hoverOverElement('button'); + expect(selector).toBe('text="Submit"'); + + const [message] = await Promise.all([ + page.waitForEvent('console'), + recorder.waitForOutput('click'), + page.dispatchEvent('button', 'click', { detail: 1 }) + ]); + expect(recorder.output()).toContain(` + // Click text="Submit" + await page.click('text="Submit"');`); + expect(message.text()).toBe('click'); + }); + it('should not target selector preview by text regexp', async ({ page, recorder }) => { await recorder.setContentAndWait(`dummy`);