From 11f68bac54f7375c1ff0dc13f085f5ac9bdd81af Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Wed, 4 Mar 2020 19:15:01 -0800 Subject: [PATCH] feat(cr, wk): make clicks, input and evaluate await scheduled navigations (#1200) --- src/chromium/crPage.ts | 5 ++ src/dom.ts | 59 ++++++++++------- src/frames.ts | 33 ++++++++++ src/webkit/wkPage.ts | 5 ++ test/navigation.spec.js | 136 ++++++++++++++++++++++++++++++++++++++-- test/page.spec.js | 16 +---- 6 files changed, 214 insertions(+), 40 deletions(-) diff --git a/src/chromium/crPage.ts b/src/chromium/crPage.ts index c4d3702e70..32fd594cdf 100644 --- a/src/chromium/crPage.ts +++ b/src/chromium/crPage.ts @@ -80,6 +80,7 @@ export class CRPage implements PageDelegate { helper.addEventListener(this._client, 'Page.frameAttached', event => this._onFrameAttached(event.frameId, event.parentFrameId)), helper.addEventListener(this._client, 'Page.frameDetached', event => this._onFrameDetached(event.frameId)), helper.addEventListener(this._client, 'Page.frameNavigated', event => this._onFrameNavigated(event.frame, false)), + helper.addEventListener(this._client, 'Page.frameRequestedNavigation', event => this._onFrameRequestedNavigation(event)), helper.addEventListener(this._client, 'Page.frameStoppedLoading', event => this._onFrameStoppedLoading(event.frameId)), helper.addEventListener(this._client, 'Page.javascriptDialogOpening', event => this._onDialog(event)), helper.addEventListener(this._client, 'Page.lifecycleEvent', event => this._onLifecycleEvent(event)), @@ -174,6 +175,10 @@ export class CRPage implements PageDelegate { this._page._frameManager.frameCommittedNewDocumentNavigation(framePayload.id, framePayload.url, framePayload.name || '', framePayload.loaderId, initial); } + _onFrameRequestedNavigation(payload: Protocol.Page.frameRequestedNavigationPayload) { + this._page._frameManager.frameRequestedNavigation(payload.frameId); + } + async _ensureIsolatedWorld(name: string) { if (this._isolatedWorlds.has(name)) return; diff --git a/src/dom.ts b/src/dom.ts index e0c17abc73..a454a1d2d1 100644 --- a/src/dom.ts +++ b/src/dom.ts @@ -52,7 +52,9 @@ export class FrameExecutionContext extends js.ExecutionContext { if (!args.some(needsAdoption)) { // Only go through asynchronous calls if required. - return this._delegate.evaluate(this, returnByValue, pageFunction, ...args); + return await this.frame._page._frameManager.waitForNavigationsCreatedBy(async () => { + return this._delegate.evaluate(this, returnByValue, pageFunction, ...args); + }); } const toDispose: Promise[] = []; @@ -65,7 +67,9 @@ export class FrameExecutionContext extends js.ExecutionContext { })); let result; try { - result = await this._delegate.evaluate(this, returnByValue, pageFunction, ...adopted); + result = await this.frame._page._frameManager.waitForNavigationsCreatedBy(async () => { + return this._delegate.evaluate(this, returnByValue, pageFunction, ...adopted); + }); } finally { toDispose.map(handlePromise => handlePromise.then(handle => handle.dispose())); } @@ -248,12 +252,15 @@ export class ElementHandle extends js.JSHandle { const point = offset ? await this._offsetPoint(offset) : await this._clickablePoint(); if (waitFor) await this._waitForHitTargetAt(point, options); - let restoreModifiers: input.Modifier[] | undefined; - if (options && options.modifiers) - restoreModifiers = await this._page.keyboard._ensureModifiers(options.modifiers); - await action(point); - if (restoreModifiers) - await this._page.keyboard._ensureModifiers(restoreModifiers); + + await this._page._frameManager.waitForNavigationsCreatedBy(async () => { + let restoreModifiers: input.Modifier[] | undefined; + if (options && options.modifiers) + restoreModifiers = await this._page.keyboard._ensureModifiers(options.modifiers); + await action(point); + if (restoreModifiers) + await this._page.keyboard._ensureModifiers(restoreModifiers); + }); } hover(options?: PointerActionOptions & types.WaitForOptions): Promise { @@ -284,18 +291,22 @@ export class ElementHandle extends js.JSHandle { if (option.index !== undefined) assert(helper.isNumber(option.index), 'Indices must be numbers. Found index "' + option.index + '" of type "' + (typeof option.index) + '"'); } - return this._evaluateInUtility((injected, node, ...optionsToSelect) => injected.selectOptions(node, optionsToSelect), ...options); + return await this._page._frameManager.waitForNavigationsCreatedBy(async () => { + return this._evaluateInUtility((injected, node, ...optionsToSelect) => injected.selectOptions(node, optionsToSelect), ...options); + }); } async fill(value: string): Promise { assert(helper.isString(value), 'Value must be string. Found value "' + value + '" of type "' + (typeof value) + '"'); - const error = await this._evaluateInUtility((injected, node, value) => injected.fill(node, value), value); - if (error) - throw new Error(error); - if (value) - await this._page.keyboard.sendCharacters(value); - else - await this._page.keyboard.press('Delete'); + await this._page._frameManager.waitForNavigationsCreatedBy(async () => { + const error = await this._evaluateInUtility((injected, node, value) => injected.fill(node, value), value); + if (error) + throw new Error(error); + if (value) + await this._page.keyboard.sendCharacters(value); + else + await this._page.keyboard.press('Delete'); + }); } async setInputFiles(...files: (string | types.FilePayload)[]) { @@ -317,7 +328,9 @@ export class ElementHandle extends js.JSHandle { } return item; })); - await this._page._delegate.setInputFiles(this as any as ElementHandle, filePayloads); + await this._page._frameManager.waitForNavigationsCreatedBy(async () => { + await this._page._delegate.setInputFiles(this as any as ElementHandle, filePayloads); + }); } async focus() { @@ -332,13 +345,17 @@ export class ElementHandle extends js.JSHandle { } async type(text: string, options?: { delay?: number }) { - await this.focus(); - await this._page.keyboard.type(text, options); + await this._page._frameManager.waitForNavigationsCreatedBy(async () => { + await this.focus(); + await this._page.keyboard.type(text, options); + }); } async press(key: string, options?: { delay?: number, text?: string }) { - await this.focus(); - await this._page.keyboard.press(key, options); + await this._page._frameManager.waitForNavigationsCreatedBy(async () => { + await this.focus(); + await this._page.keyboard.press(key, options); + }); } async check(options?: types.WaitForOptions) { diff --git a/src/frames.ts b/src/frames.ts index 117e9b5939..9c9227dad3 100644 --- a/src/frames.ts +++ b/src/frames.ts @@ -59,6 +59,7 @@ export class FrameManager { private _mainFrame: Frame; readonly _lifecycleWatchers = new Set<() => void>(); readonly _consoleMessageTags = new Map(); + private _navigationRequestCollectors = new Set>(); constructor(page: Page) { this._page = page; @@ -107,7 +108,34 @@ export class FrameManager { } } + async waitForNavigationsCreatedBy(action: () => Promise): Promise { + const frameIds = new Set(); + this._navigationRequestCollectors.add(frameIds); + try { + const result = await action(); + if (!frameIds.size) + return result; + const frames = Array.from(frameIds.values()).map(frameId => this._frames.get(frameId)); + await Promise.all(frames.map(frame => frame!.waitForNavigation({ waitUntil: []}))).catch(e => {}); + await new Promise(platform.makeWaitForNextTask()); + return result; + } finally { + this._navigationRequestCollectors.delete(frameIds); + } + } + + frameRequestedNavigation(frameId: string) { + for (const frameIds of this._navigationRequestCollectors) + frameIds.add(frameId); + } + + _cancelFrameRequestedNavigation(frameId: string) { + for (const frameIds of this._navigationRequestCollectors) + frameIds.delete(frameId); + } + frameCommittedNewDocumentNavigation(frameId: string, url: string, name: string, documentId: string, initial: boolean) { + this._cancelFrameRequestedNavigation(frameId); const frame = this._frames.get(frameId)!; for (const child of frame.childFrames()) this._removeFramesRecursively(child); @@ -122,6 +150,7 @@ export class FrameManager { } frameCommittedSameDocumentNavigation(frameId: string, url: string) { + this._cancelFrameRequestedNavigation(frameId); const frame = this._frames.get(frameId); if (!frame) return; @@ -206,6 +235,7 @@ export class FrameManager { if (request._documentId && frame) { const isCurrentDocument = frame._lastDocumentId === request._documentId; if (!isCurrentDocument) { + this._cancelFrameRequestedNavigation(frame._id); let errorText = request.failure()!.errorText; if (canceled) errorText += '; maybe frame was detached?'; @@ -218,11 +248,13 @@ export class FrameManager { } provisionalLoadFailed(frame: Frame, documentId: string, error: string) { + this._cancelFrameRequestedNavigation(frame._id); for (const watcher of frame._documentWatchers) watcher(documentId, new Error(error)); } private _removeFramesRecursively(frame: Frame) { + this._cancelFrameRequestedNavigation(frame._id); for (const child of frame.childFrames()) this._removeFramesRecursively(child); frame._onDetached(); @@ -406,6 +438,7 @@ export class Frame { disposer.dispose(); if (error) throw error; + return request ? request._finalRequest._waitForResponse() : null; } diff --git a/src/webkit/wkPage.ts b/src/webkit/wkPage.ts index 7cf3379f6b..031525ca39 100644 --- a/src/webkit/wkPage.ts +++ b/src/webkit/wkPage.ts @@ -201,6 +201,7 @@ export class WKPage implements PageDelegate { helper.addEventListener(this._session, 'Page.navigatedWithinDocument', event => this._onFrameNavigatedWithinDocument(event.frameId, event.url)), helper.addEventListener(this._session, 'Page.frameAttached', event => this._onFrameAttached(event.frameId, event.parentFrameId)), helper.addEventListener(this._session, 'Page.frameDetached', event => this._onFrameDetached(event.frameId)), + helper.addEventListener(this._session, 'Page.frameScheduledNavigation', event => this._onFrameScheduledNavigation(event.frameId)), helper.addEventListener(this._session, 'Page.frameStoppedLoading', event => this._onFrameStoppedLoading(event.frameId)), helper.addEventListener(this._session, 'Page.loadEventFired', event => this._onLifecycleEvent(event.frameId, 'load')), helper.addEventListener(this._session, 'Page.domContentEventFired', event => this._onLifecycleEvent(event.frameId, 'domcontentloaded')), @@ -234,6 +235,10 @@ export class WKPage implements PageDelegate { await Promise.all(sessions.map(session => callback(session).catch(debugError))); } + private _onFrameScheduledNavigation(frameId: string) { + this._page._frameManager.frameRequestedNavigation(frameId); + } + private _onFrameStoppedLoading(frameId: string) { this._page._frameManager.frameStoppedLoading(frameId); } diff --git a/test/navigation.spec.js b/test/navigation.spec.js index 50a67b8255..cd99d8ff5a 100644 --- a/test/navigation.spec.js +++ b/test/navigation.spec.js @@ -800,6 +800,130 @@ module.exports.describe = function({testRunner, expect, playwright, MAC, WIN, FF }); }); + describe.fail(FFOX)('Page.automaticWaiting', () => { + it('clicking anchor should await navigation', async({page, server}) => { + const messages = []; + server.setRoute('/empty.html', async (req, res) => { + messages.push('route'); + res.end('done'); + }); + + await page.setContent(`empty.html`); + + await Promise.all([ + page.click('a').then(() => messages.push('click')), + page.waitForNavigation({ waitUntil: [] }).then(() => messages.push('waitForNavigation')) + ]); + expect(messages.join('|')).toBe('route|waitForNavigation|click'); + }); + it('clicking anchor should await cross-process navigation', async({page, server}) => { + const messages = []; + server.setRoute('/empty.html', async (req, res) => { + messages.push('route'); + res.end('done'); + }); + + await page.setContent(`empty.html`); + + await Promise.all([ + page.click('a').then(() => messages.push('click')), + page.waitForNavigation({ waitUntil: [] }).then(() => messages.push('waitForNavigation')) + ]); + expect(messages.join('|')).toBe('route|waitForNavigation|click'); + }); + it.fail(CHROMIUM)('should await form-get on click', async({page, server}) => { + const messages = []; + server.setRoute('/empty.html?foo=bar', async (req, res) => { + messages.push('route'); + res.end('done'); + }); + + await page.setContent(` +
+ + +
`); + + await Promise.all([ + page.click('input[type=submit]').then(() => messages.push('click')), + page.waitForNavigation({ waitUntil: [] }).then(() => messages.push('waitForNavigation')) + ]); + expect(messages.join('|')).toBe('route|waitForNavigation|click'); + }); + it.fail(CHROMIUM)('should await form-post on click', async({page, server}) => { + const messages = []; + server.setRoute('/empty.html', async (req, res) => { + messages.push('route'); + res.end('done'); + }); + + await page.setContent(` +
+ + +
`); + + await Promise.all([ + page.click('input[type=submit]').then(() => messages.push('click')), + page.waitForNavigation({ waitUntil: [] }).then(() => messages.push('waitForNavigation')) + ]); + expect(messages.join('|')).toBe('route|waitForNavigation|click'); + }); + it('assigning to location should await navigation', async({page, server}) => { + const messages = []; + server.setRoute('/empty.html', async (req, res) => { + messages.push('route'); + res.end('done'); + }); + await Promise.all([ + page.evaluate(`window.location.href = "${server.EMPTY_PAGE}"`).then(() => messages.push('evaluate')), + page.waitForNavigation({ waitUntil: [] }).then(() => messages.push('waitForNavigation')), + ]); + expect(messages.join('|')).toBe('route|waitForNavigation|evaluate'); + }); + it('should await navigating specified target', async({page, server, httpsServer}) => { + const messages = []; + server.setRoute('/empty.html', async (req, res) => { + messages.push('route'); + res.end('done'); + }); + + await page.setContent(` + empty.html + + `); + const frame = page.frame({ name: 'target' }); + await Promise.all([ + page.click('a').then(() => messages.push('click')), + frame.waitForNavigation({ waitUntil: [] }).then(() => messages.push('waitForNavigation')) + ]); + expect(frame.url()).toBe(server.EMPTY_PAGE); + expect(messages.join('|')).toBe('route|waitForNavigation|click'); + }); + }); + + describe('Page.automaticWaiting should not hang', () => { + it('should not throw when clicking on links which do not commit navigation', async({page, server, httpsServer}) => { + await page.goto(server.EMPTY_PAGE); + await page.setContent(`foobar`); + await page.click('a'); + }); + it('should not throw when clicking on download link', async({page, server, httpsServer}) => { + await page.setContent(`table2.wasm`); + await page.click('a'); + }); + it('should not hang on window.stop', async({page, server, httpsServer}) => { + server.setRoute('/empty.html', async (req, res) => {}); + await Promise.all([ + page.waitForNavigation().catch(e => {}), + page.evaluate((url) => { + window.location.href = url; + setTimeout(() => window.stop(), 100); + }, server.EMPTY_PAGE) + ]); + }); + }); + describe('Page.waitForLoadState', () => { it('should pick up ongoing navigation', async({page, server}) => { let response = null; @@ -950,13 +1074,13 @@ module.exports.describe = function({testRunner, expect, playwright, MAC, WIN, FF server.setRoute('/empty.html', () => {}); let error = null; - const navigationPromise = frame.waitForNavigation().catch(e => error = e); await Promise.all([ - server.waitForRequest('/empty.html'), - frame.evaluate(() => window.location = '/empty.html') - ]); - await page.$eval('iframe', frame => frame.remove()); - await navigationPromise; + frame.waitForNavigation().catch(e => error = e), + server.waitForRequest('/empty.html').then(() => { + page.$eval('iframe', frame => frame.remove()); + }), + frame.evaluate(() => window.location = '/empty.html'), + ]).catch(e => error = e); expect(error.message).toContain('frame was detached'); }); }); diff --git a/test/page.spec.js b/test/page.spec.js index 096832f539..53b13b746d 100644 --- a/test/page.spec.js +++ b/test/page.spec.js @@ -1123,24 +1123,14 @@ module.exports.describe = function({testRunner, expect, headless, playwright, FF describe('Page.frame', function() { it('should respect name', async function({page, server}) { - await page.setContent(` - empty.html - - `); + await page.setContent(``); expect(page.frame({ name: 'bogus' })).toBe(null); const frame = page.frame({ name: 'target' }); expect(frame).toBeTruthy(); - await Promise.all([ - frame.waitForNavigation(), - page.click('a') - ]); - expect(frame.url()).toBe(server.EMPTY_PAGE); + expect(frame === page.mainFrame().childFrames()[0]).toBeTruthy(); }); it('should respect url', async function({page, server}) { - await page.setContent(` - empty.html target=target>empty.html - - `); + await page.setContent(``); expect(page.frame({ url: /bogus/ })).toBe(null); expect(page.frame({ url: /empty/ }).url()).toBe(server.EMPTY_PAGE); });