feat: wait for pending navigation to resolve before many actions (#32899)

This includes all actions that perform locator handler check.
    
Note this makes it impossible to interact with the page while a main
frame navigation is ongoing. This was already the case for Chromium, but
now WebKit and Firefox align with it.

Setting `PLAYWRIGHT_SKIP_NAVIGATION_CHECK` environment variable disables
this behavior.
This commit is contained in:
Dmitry Gozman 2024-10-04 07:25:18 -07:00 committed by GitHub
parent 1b457b1ff8
commit 84b4fd4e40
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 136 additions and 49 deletions

View File

@ -292,7 +292,7 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
};
}
async _retryAction(progress: Progress, actionName: string, action: (retry: number) => Promise<PerformActionResult>, options: { trial?: boolean, force?: boolean, skipLocatorHandlersCheckpoint?: boolean }): Promise<'error:notconnected' | 'done'> {
async _retryAction(progress: Progress, actionName: string, action: (retry: number) => Promise<PerformActionResult>, options: { trial?: boolean, force?: boolean, skipActionPreChecks?: 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<T extends Node = Node> extends js.JSHandle<T> {
} else {
progress.log(`attempting ${actionName} action${options.trial ? ' (trial run)' : ''}`);
}
if (!options.skipLocatorHandlersCheckpoint && !options.force)
await this._frame._page.performLocatorHandlersCheckpoint(progress);
if (!options.skipActionPreChecks && !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<T extends Node = Node> extends js.JSHandle<T> {
async _retryPointerAction(progress: Progress, actionName: ActionName, waitForEnabled: boolean, action: (point: types.Point) => Promise<void>,
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 skipActionPreChecks = 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<T extends Node = Node> extends js.JSHandle<T> {
];
const forceScrollOptions = scrollOptions[retry % scrollOptions.length];
return await this._performPointerAction(progress, actionName, waitForEnabled, action, forceScrollOptions, options);
}, { ...options, skipLocatorHandlersCheckpoint });
}, { ...options, skipActionPreChecks });
}
async _performPointerAction(

View File

@ -791,11 +791,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<dom.ElementHandle<Element> | null> {
async waitForSelectorInternal(progress: Progress, selector: string, performActionPreChecks: boolean, options: types.WaitForElementOptions, scope?: dom.ElementHandle): Promise<dom.ElementHandle<Element> | 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();
@ -1118,12 +1118,12 @@ export class Frame extends SdkObject {
progress: Progress,
selector: string,
strict: boolean | undefined,
performLocatorHandlersCheckpoint: boolean,
performActionPreChecks: boolean,
action: (handle: dom.ElementHandle<Element>) => Promise<R | 'error:notconnected'>): Promise<R> {
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();
@ -1167,7 +1167,7 @@ export class Frame extends SdkObject {
}
async rafrafTimeoutScreenshotElementWithProgress(progress: Progress, selector: string, timeout: number, options: ScreenshotOptions): Promise<Buffer> {
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);
});
@ -1176,21 +1176,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();
@ -1202,7 +1202,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();
@ -1221,28 +1221,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));
}
@ -1349,14 +1349,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<string[]> {
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));
}
@ -1364,35 +1364,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));
}
@ -1421,7 +1421,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.
@ -1448,7 +1448,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:

View File

@ -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;
@ -455,7 +456,34 @@ export class Page extends SdkObject {
this._locatorHandlers.delete(uid);
}
async performLocatorHandlersCheckpoint(progress: Progress) {
async performActionPreChecks(progress: Progress) {
await this._performWaitForNavigationCheck(progress);
progress.throwIfAborted();
await this._performLocatorHandlersCheckpoint(progress);
progress.throwIfAborted();
// Wait once again, just in case a locator handler caused a navigation.
await this._performWaitForNavigationCheck(progress);
}
private 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;
}
private async _performLocatorHandlersCheckpoint(progress: Progress) {
// Do not run locator handlers from inside locator handler callbacks to avoid deadlocks.
if (this._locatorHandlerRunningCounter)
return;
@ -559,7 +587,7 @@ export class Page extends SdkObject {
const rafrafScreenshot = locator ? async (progress: Progress, timeout: number) => {
return await locator.frame.rafrafTimeoutScreenshotElementWithProgress(progress, locator.selector, timeout, options || {});
} : async (progress: Progress, timeout: number) => {
await this.performLocatorHandlersCheckpoint(progress);
await this.performActionPreChecks(progress);
await this.mainFrame().rafrafTimeout(timeout);
return await this._screenshotter.screenshotPage(progress, options || {});
};

View File

@ -184,19 +184,3 @@ it('should work with Ctrl-clicking', async ({ browser, server, browserName }) =>
expect(await popup.opener()).toBe(null);
await context.close();
});
it('should not hang on ctrl-click during provisional load', async ({ context, page, server, isWindows, browserName, isLinux }) => {
it.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/11595' });
it.skip(browserName === 'chromium', 'Chromium does not dispatch renderer messages while navigation is provisional.');
it.fixme(browserName === 'webkit' && isWindows, 'Timesout while trying to click');
it.fixme(browserName === 'webkit' && isLinux, 'Timesout while trying to click');
await page.goto(server.EMPTY_PAGE);
await page.setContent('<a href="/one-style.html">yo</a>');
server.setRoute('/slow.html', () => {});
const [popup] = await Promise.all([
context.waitForEvent('page'),
server.waitForRequest('/slow.html').then(() => page.click('a', { modifiers: ['ControlOrMeta'] })),
page.evaluate(url => setTimeout(() => location.href = url, 0), server.CROSS_PROCESS_PREFIX + '/slow.html'),
]);
expect(popup).toBeTruthy();
});

View File

@ -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,78 @@ 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().catch(() => {});
await page.waitForTimeout(1000);
commitCallback();
await goBackPromise;
});