From 05c1fab981989739435c04e7f1cc2ef393db2ae6 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 1 Oct 2024 14:13:58 +0100 Subject: [PATCH] feat: wait for pending navigation to resolve before many actions This includes all actions that perform locator handler check, and also `reload()`, `goBack()` and `goForward()`. Setting `PLAYWRIGHT_SKIP_NAVIGATION_CHECK` environment variable disables this behavior. --- packages/playwright-core/src/server/dom.ts | 10 +-- packages/playwright-core/src/server/frames.ts | 48 ++++++------ packages/playwright-core/src/server/page.ts | 30 ++++++- tests/page/page-autowaiting-no-hang.spec.ts | 78 ++++++++++++++++++- 4 files changed, 135 insertions(+), 31 deletions(-) diff --git a/packages/playwright-core/src/server/dom.ts b/packages/playwright-core/src/server/dom.ts index 05a8b4fda28e18..0fe2c9aa2aaf16 100644 --- a/packages/playwright-core/src/server/dom.ts +++ b/packages/playwright-core/src/server/dom.ts @@ -292,7 +292,7 @@ export class ElementHandle extends js.JSHandle { }; } - async _retryAction(progress: Progress, actionName: string, action: (retry: number) => Promise, options: { trial?: boolean, force?: boolean, skipLocatorHandlersCheckpoint?: boolean }): Promise<'error:notconnected' | 'done'> { + async _retryAction(progress: Progress, actionName: string, action: (retry: number) => Promise, options: { trial?: boolean, force?: boolean, skipActionPreChecls?: boolean }): Promise<'error:notconnected' | 'done'> { let retry = 0; // We progressively wait longer between retries, up to 500ms. const waitTime = [0, 20, 100, 100, 500]; @@ -310,8 +310,8 @@ export class ElementHandle extends js.JSHandle { } else { progress.log(`attempting ${actionName} action${options.trial ? ' (trial run)' : ''}`); } - if (!options.skipLocatorHandlersCheckpoint && !options.force) - await this._frame._page.performLocatorHandlersCheckpoint(progress); + if (!options.skipActionPreChecls && !options.force) + await this._frame._page.performActionPreChecks(progress); const result = await action(retry); ++retry; if (result === 'error:notvisible') { @@ -346,7 +346,7 @@ export class ElementHandle extends js.JSHandle { async _retryPointerAction(progress: Progress, actionName: ActionName, waitForEnabled: boolean, action: (point: types.Point) => Promise, options: { waitAfter: boolean | 'disabled' } & types.PointerActionOptions & types.PointerActionWaitOptions): Promise<'error:notconnected' | 'done'> { // Note: do not perform locator handlers checkpoint to avoid moving the mouse in the middle of a drag operation. - const skipLocatorHandlersCheckpoint = actionName === 'move and up'; + const skipActionPreChecls = actionName === 'move and up'; return await this._retryAction(progress, actionName, async retry => { // By default, we scroll with protocol method to reveal the action point. // However, that might not work to scroll from under position:sticky elements @@ -360,7 +360,7 @@ export class ElementHandle extends js.JSHandle { ]; const forceScrollOptions = scrollOptions[retry % scrollOptions.length]; return await this._performPointerAction(progress, actionName, waitForEnabled, action, forceScrollOptions, options); - }, { ...options, skipLocatorHandlersCheckpoint }); + }, { ...options, skipActionPreChecls }); } async _performPointerAction( diff --git a/packages/playwright-core/src/server/frames.ts b/packages/playwright-core/src/server/frames.ts index b7b626d09fa9a8..f0ca5b46ada94d 100644 --- a/packages/playwright-core/src/server/frames.ts +++ b/packages/playwright-core/src/server/frames.ts @@ -786,11 +786,11 @@ export class Frame extends SdkObject { }, this._page._timeoutSettings.timeout(options)); } - async waitForSelectorInternal(progress: Progress, selector: string, performLocatorHandlersCheckpoint: boolean, options: types.WaitForElementOptions, scope?: dom.ElementHandle): Promise | null> { + async waitForSelectorInternal(progress: Progress, selector: string, performActionPreChecks: boolean, options: types.WaitForElementOptions, scope?: dom.ElementHandle): Promise | null> { const { state = 'visible' } = options; const promise = this.retryWithProgressAndTimeouts(progress, [0, 20, 50, 100, 100, 500], async continuePolling => { - if (performLocatorHandlersCheckpoint) - await this._page.performLocatorHandlersCheckpoint(progress); + if (performActionPreChecks) + await this._page.performActionPreChecks(progress); const resolved = await this.selectors.resolveInjectedForSelector(selector, options, scope); progress.throwIfAborted(); @@ -1113,12 +1113,12 @@ export class Frame extends SdkObject { progress: Progress, selector: string, strict: boolean | undefined, - performLocatorHandlersCheckpoint: boolean, + performActionPreChecks: boolean, action: (handle: dom.ElementHandle) => Promise): Promise { progress.log(`waiting for ${this._asLocator(selector)}`); return this.retryWithProgressAndTimeouts(progress, [0, 20, 50, 100, 100, 500], async continuePolling => { - if (performLocatorHandlersCheckpoint) - await this._page.performLocatorHandlersCheckpoint(progress); + if (performActionPreChecks) + await this._page.performActionPreChecks(progress); const resolved = await this.selectors.resolveInjectedForSelector(selector, { strict }); progress.throwIfAborted(); @@ -1160,7 +1160,7 @@ export class Frame extends SdkObject { } async rafrafTimeoutScreenshotElementWithProgress(progress: Progress, selector: string, timeout: number, options: ScreenshotOptions): Promise { - return await this._retryWithProgressIfNotConnected(progress, selector, true /* strict */, true /* performLocatorHandlersCheckpoint */, async handle => { + return await this._retryWithProgressIfNotConnected(progress, selector, true /* strict */, true /* performActionPreChecks */, async handle => { await handle._frame.rafrafTimeout(timeout); return await this._page._screenshotter.screenshotElement(progress, handle, options); }); @@ -1169,21 +1169,21 @@ export class Frame extends SdkObject { async click(metadata: CallMetadata, selector: string, options: { noWaitAfter?: boolean } & types.MouseClickOptions & types.PointerActionWaitOptions) { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performLocatorHandlersCheckpoint */, handle => handle._click(progress, { ...options, waitAfter: !options.noWaitAfter }))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._click(progress, { ...options, waitAfter: !options.noWaitAfter }))); }, this._page._timeoutSettings.timeout(options)); } async dblclick(metadata: CallMetadata, selector: string, options: types.MouseMultiClickOptions & types.PointerActionWaitOptions = {}) { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performLocatorHandlersCheckpoint */, handle => handle._dblclick(progress, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._dblclick(progress, options))); }, this._page._timeoutSettings.timeout(options)); } async dragAndDrop(metadata: CallMetadata, source: string, target: string, options: types.DragActionOptions & types.PointerActionWaitOptions = {}) { const controller = new ProgressController(metadata, this); await controller.run(async progress => { - dom.assertDone(await this._retryWithProgressIfNotConnected(progress, source, options.strict, !options.force /* performLocatorHandlersCheckpoint */, async handle => { + dom.assertDone(await this._retryWithProgressIfNotConnected(progress, source, options.strict, !options.force /* performActionPreChecks */, async handle => { return handle._retryPointerAction(progress, 'move and down', false, async point => { await this._page.mouse.move(point.x, point.y); await this._page.mouse.down(); @@ -1195,7 +1195,7 @@ export class Frame extends SdkObject { }); })); // Note: do not perform locator handlers checkpoint to avoid moving the mouse in the middle of a drag operation. - dom.assertDone(await this._retryWithProgressIfNotConnected(progress, target, options.strict, false /* performLocatorHandlersCheckpoint */, async handle => { + dom.assertDone(await this._retryWithProgressIfNotConnected(progress, target, options.strict, false /* performActionPreChecks */, async handle => { return handle._retryPointerAction(progress, 'move and up', false, async point => { await this._page.mouse.move(point.x, point.y); await this._page.mouse.up(); @@ -1214,28 +1214,28 @@ export class Frame extends SdkObject { throw new Error('The page does not support tap. Use hasTouch context option to enable touch support.'); const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performLocatorHandlersCheckpoint */, handle => handle._tap(progress, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._tap(progress, options))); }, this._page._timeoutSettings.timeout(options)); } async fill(metadata: CallMetadata, selector: string, value: string, options: types.TimeoutOptions & types.StrictOptions & { force?: boolean }) { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performLocatorHandlersCheckpoint */, handle => handle._fill(progress, value, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._fill(progress, value, options))); }, this._page._timeoutSettings.timeout(options)); } async focus(metadata: CallMetadata, selector: string, options: types.TimeoutOptions & types.StrictOptions = {}) { const controller = new ProgressController(metadata, this); await controller.run(async progress => { - dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performLocatorHandlersCheckpoint */, handle => handle._focus(progress))); + dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performActionPreChecks */, handle => handle._focus(progress))); }, this._page._timeoutSettings.timeout(options)); } async blur(metadata: CallMetadata, selector: string, options: types.TimeoutOptions & types.StrictOptions = {}) { const controller = new ProgressController(metadata, this); await controller.run(async progress => { - dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performLocatorHandlersCheckpoint */, handle => handle._blur(progress))); + dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performActionPreChecks */, handle => handle._blur(progress))); }, this._page._timeoutSettings.timeout(options)); } @@ -1342,14 +1342,14 @@ export class Frame extends SdkObject { async hover(metadata: CallMetadata, selector: string, options: types.PointerActionOptions & types.PointerActionWaitOptions = {}) { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performLocatorHandlersCheckpoint */, handle => handle._hover(progress, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._hover(progress, options))); }, this._page._timeoutSettings.timeout(options)); } async selectOption(metadata: CallMetadata, selector: string, elements: dom.ElementHandle[], values: types.SelectOption[], options: types.CommonActionOptions = {}): Promise { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performLocatorHandlersCheckpoint */, handle => handle._selectOption(progress, elements, values, options)); + return await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._selectOption(progress, elements, values, options)); }, this._page._timeoutSettings.timeout(options)); } @@ -1357,35 +1357,35 @@ export class Frame extends SdkObject { const inputFileItems = await prepareFilesForUpload(this, params); const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, params.strict, true /* performLocatorHandlersCheckpoint */, handle => handle._setInputFiles(progress, inputFileItems))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, params.strict, true /* performActionPreChecks */, handle => handle._setInputFiles(progress, inputFileItems))); }, this._page._timeoutSettings.timeout(params)); } async type(metadata: CallMetadata, selector: string, text: string, options: { delay?: number } & types.TimeoutOptions & types.StrictOptions = {}) { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performLocatorHandlersCheckpoint */, handle => handle._type(progress, text, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performActionPreChecks */, handle => handle._type(progress, text, options))); }, this._page._timeoutSettings.timeout(options)); } async press(metadata: CallMetadata, selector: string, key: string, options: { delay?: number, noWaitAfter?: boolean } & types.TimeoutOptions & types.StrictOptions = {}) { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performLocatorHandlersCheckpoint */, handle => handle._press(progress, key, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performActionPreChecks */, handle => handle._press(progress, key, options))); }, this._page._timeoutSettings.timeout(options)); } async check(metadata: CallMetadata, selector: string, options: types.PointerActionWaitOptions = {}) { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performLocatorHandlersCheckpoint */, handle => handle._setChecked(progress, true, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._setChecked(progress, true, options))); }, this._page._timeoutSettings.timeout(options)); } async uncheck(metadata: CallMetadata, selector: string, options: types.PointerActionWaitOptions = {}) { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performLocatorHandlersCheckpoint */, handle => handle._setChecked(progress, false, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._setChecked(progress, false, options))); }, this._page._timeoutSettings.timeout(options)); } @@ -1414,7 +1414,7 @@ export class Frame extends SdkObject { await (new ProgressController(metadata, this)).run(async progress => { progress.log(`${metadata.apiName}${timeout ? ` with timeout ${timeout}ms` : ''}`); progress.log(`waiting for ${this._asLocator(selector)}`); - await this._page.performLocatorHandlersCheckpoint(progress); + await this._page.performActionPreChecks(progress); }, timeout); // Step 2: perform one-shot expect check without a timeout. @@ -1441,7 +1441,7 @@ export class Frame extends SdkObject { // Step 3: auto-retry expect with increasing timeouts. Bounded by the total remaining time. return await (new ProgressController(metadata, this)).run(async progress => { return await this.retryWithProgressAndTimeouts(progress, [100, 250, 500, 1000], async continuePolling => { - await this._page.performLocatorHandlersCheckpoint(progress); + await this._page.performActionPreChecks(progress); const { matches, received } = await this._expectInternal(progress, selector, options, lastIntermediateResult); if (matches === options.isNot) { // Keep waiting in these cases: diff --git a/packages/playwright-core/src/server/page.ts b/packages/playwright-core/src/server/page.ts index 3fb72bb0c3d5b8..7987b1623009bf 100644 --- a/packages/playwright-core/src/server/page.ts +++ b/packages/playwright-core/src/server/page.ts @@ -31,7 +31,7 @@ import * as accessibility from './accessibility'; import { FileChooser } from './fileChooser'; import type { Progress } from './progress'; import { ProgressController } from './progress'; -import { LongStandingScope, assert, createGuid } from '../utils'; +import { LongStandingScope, assert, createGuid, trimStringWithEllipsis } from '../utils'; import { ManualPromise } from '../utils/manualPromise'; import { debugLogger } from '../utils/debugLogger'; import type { ImageComparatorOptions } from '../utils/comparators'; @@ -45,6 +45,7 @@ import { parseEvaluationResultValue, source } from './isomorphic/utilityScriptSe import type { SerializedValue } from './isomorphic/utilityScriptSerializers'; import { TargetClosedError } from './errors'; import { asLocator } from '../utils'; +import { helper } from './helper'; export interface PageDelegate { readonly rawMouse: input.RawMouse; @@ -380,6 +381,7 @@ export class Page extends SdkObject { async reload(metadata: CallMetadata, options: types.NavigateOptions): Promise { const controller = new ProgressController(metadata, this); return controller.run(progress => this.mainFrame().raceNavigationAction(progress, options, async () => { + await this.performWaitForNavigationCheck(progress); // Note: waitForNavigation may fail before we get response to reload(), // so we should await it immediately. const [response] = await Promise.all([ @@ -394,6 +396,7 @@ export class Page extends SdkObject { async goBack(metadata: CallMetadata, options: types.NavigateOptions): Promise { const controller = new ProgressController(metadata, this); return controller.run(progress => this.mainFrame().raceNavigationAction(progress, options, async () => { + await this.performWaitForNavigationCheck(progress); // Note: waitForNavigation may fail before we get response to goBack, // so we should catch it immediately. let error: Error | undefined; @@ -414,6 +417,7 @@ export class Page extends SdkObject { async goForward(metadata: CallMetadata, options: types.NavigateOptions): Promise { const controller = new ProgressController(metadata, this); return controller.run(progress => this.mainFrame().raceNavigationAction(progress, options, async () => { + await this.performWaitForNavigationCheck(progress); // Note: waitForNavigation may fail before we get response to goForward, // so we should catch it immediately. let error: Error | undefined; @@ -455,6 +459,30 @@ export class Page extends SdkObject { this._locatorHandlers.delete(uid); } + async performActionPreChecks(progress: Progress) { + await this.performWaitForNavigationCheck(progress); + progress.throwIfAborted(); + await this.performLocatorHandlersCheckpoint(progress); + } + + async performWaitForNavigationCheck(progress: Progress) { + if (process.env.PLAYWRIGHT_SKIP_NAVIGATION_CHECK) + return; + const mainFrame = this._frameManager.mainFrame(); + if (!mainFrame || !mainFrame.pendingDocument()) + return; + const url = mainFrame.pendingDocument()?.request?.url(); + const toUrl = url ? `" ${trimStringWithEllipsis(url, 200)}"` : ''; + progress.log(` waiting for${toUrl} navigation to finish...`); + await helper.waitForEvent(progress, mainFrame, frames.Frame.Events.InternalNavigation, (e: frames.NavigationEvent) => { + if (!e.isPublic) + return false; + if (!e.error) + progress.log(` navigated to "${trimStringWithEllipsis(mainFrame.url(), 200)}"`); + return true; + }).promise; + } + async performLocatorHandlersCheckpoint(progress: Progress) { // Do not run locator handlers from inside locator handler callbacks to avoid deadlocks. if (this._locatorHandlerRunningCounter) diff --git a/tests/page/page-autowaiting-no-hang.spec.ts b/tests/page/page-autowaiting-no-hang.spec.ts index f580b8629a142a..0bb0703122fc01 100644 --- a/tests/page/page-autowaiting-no-hang.spec.ts +++ b/tests/page/page-autowaiting-no-hang.spec.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { test as it } from './pageTest'; +import { test as it, expect } from './pageTest'; it('clicking on links which do not commit navigation', async ({ page, server, httpsServer }) => { await page.goto(server.EMPTY_PAGE); @@ -65,3 +65,79 @@ it('opening a popup', async function({ page, server }) { page.evaluate(() => window.open(window.location.href) && 1), ]); }); + +it('clicking in the middle of navigation that aborts', async ({ page, server }) => { + let abortCallback; + const abortPromise = new Promise(f => abortCallback = f); + + let stallCallback; + const stallPromise = new Promise(f => stallCallback = f); + + server.setRoute('/stall.html', async (req, res) => { + stallCallback(); + await abortPromise; + req.socket.destroy(); + }); + + await page.goto(server.PREFIX + '/one-style.html'); + page.goto(server.PREFIX + '/stall.html').catch(() => {}); + await stallPromise; + + const clickPromise = page.click('body'); + await page.waitForTimeout(1000); + abortCallback(); + + await clickPromise; +}); + +it('clicking in the middle of navigation that commits', async ({ page, server }) => { + let commitCallback; + const abortPromise = new Promise(f => commitCallback = f); + + let stallCallback; + const stallPromise = new Promise(f => stallCallback = f); + + server.setRoute('/stall.html', async (req, res) => { + stallCallback(); + await abortPromise; + res.writeHead(200, { 'Content-Type': 'text/html' }); + res.end('hello world'); + }); + + await page.goto(server.PREFIX + '/one-style.html'); + page.goto(server.PREFIX + '/stall.html').catch(() => {}); + await stallPromise; + + const clickPromise = page.click('body'); + await page.waitForTimeout(1000); + commitCallback(); + + await clickPromise; + await expect(page.locator('body')).toContainText('hello world'); +}); + +it('goBack in the middle of navigation that commits', async ({ page, server }) => { + let commitCallback; + const abortPromise = new Promise(f => commitCallback = f); + + let stallCallback; + const stallPromise = new Promise(f => stallCallback = f); + + server.setRoute('/stall.html', async (req, res) => { + stallCallback(); + await abortPromise; + res.writeHead(200, { 'Content-Type': 'text/html' }); + res.end('hello world'); + }); + + await page.goto(server.PREFIX + '/one-style.html'); + page.goto(server.PREFIX + '/stall.html').catch(() => {}); + await stallPromise; + + const goBackPromise = page.goBack(); + await page.waitForTimeout(1000); + commitCallback(); + + await goBackPromise; + await expect(page.locator('body')).toContainText('hello'); +});