chore: make noWaitAfter a default

This changes the last actions, namely `click` and `press`, to not wait
for the ongoing navigations after the action.

Maintaining this behavior becomes tricky, because browsers move away from
guaranteed synchronous navigations to optimized performance. See #34377
for more details.

This is technically a breaking change. Most of the time, this should
not be noticeable, because the next action will auto-wait the navigation
and for any required conditions anyway. However, there are some patterns
revealed by our tests that are affected:
- Calling `goBack/goForward` immediately after an action. This pattern
  requires `expect(page).toHaveURL()` or a similar check inbetween.
- Listening for network events during the action, and immediately
  asserting after the action. This pattern requires `waitForRequest()`
  or a similar promise-based waiter as recommended in best practices.

We maintain the opt-out `env.PLAYWRIGHT_WAIT_AFTER_CLICK` that reverts
to the old behavior for now.

Additionally, previous opt-out option `env.PLAYWRIGHT_SKIP_NAVIGATION_CHECK`
has been removed, because there have been just a single issue with it,
that was immediately addressed in a patch release.
This commit is contained in:
Dmitry Gozman 2025-02-04 12:51:03 +00:00
parent 96d4dc1eda
commit af40eae13e
9 changed files with 81 additions and 78 deletions

View file

@ -482,18 +482,15 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
if (restoreModifiers) if (restoreModifiers)
await this._page.keyboard.ensureModifiers(restoreModifiers); await this._page.keyboard.ensureModifiers(restoreModifiers);
if (hitTargetInterceptionHandle) { if (hitTargetInterceptionHandle) {
// We do not want to accidentally stall on non-committed navigation blocking the evaluate.
const stopHitTargetInterception = this._frame.raceAgainstEvaluationStallingEvents(() => { const stopHitTargetInterception = this._frame.raceAgainstEvaluationStallingEvents(() => {
return hitTargetInterceptionHandle.evaluate(h => h.stop()); return hitTargetInterceptionHandle.evaluate(h => h.stop());
}).catch(e => 'done' as const).finally(() => { }).catch(e => 'done' as const).finally(() => {
hitTargetInterceptionHandle?.dispose(); hitTargetInterceptionHandle?.dispose();
}); });
if (options.waitAfter !== false) { const hitTargetResult = await stopHitTargetInterception;
// When noWaitAfter is passed, we do not want to accidentally stall on if (hitTargetResult !== 'done')
// non-committed navigation blocking the evaluate. return hitTargetResult;
const hitTargetResult = await stopHitTargetInterception;
if (hitTargetResult !== 'done')
return hitTargetResult;
}
} }
progress.log(` ${options.trial ? 'trial ' : ''}${actionName} action done`); progress.log(` ${options.trial ? 'trial ' : ''}${actionName} action done`);
progress.log(' waiting for scheduled navigations to finish'); progress.log(' waiting for scheduled navigations to finish');

View file

@ -161,6 +161,8 @@ export class FrameManager {
} }
async waitForSignalsCreatedBy<T>(progress: Progress | null, waitAfter: boolean, action: () => Promise<T>): Promise<T> { async waitForSignalsCreatedBy<T>(progress: Progress | null, waitAfter: boolean, action: () => Promise<T>): Promise<T> {
if (!process.env.PLAYWRIGHT_WAIT_AFTER_CLICK)
waitAfter = false;
if (!waitAfter) if (!waitAfter)
return action(); return action();
const barrier = new SignalBarrier(progress); const barrier = new SignalBarrier(progress);

View file

@ -473,8 +473,6 @@ export class Page extends SdkObject {
} }
private async _performWaitForNavigationCheck(progress: Progress) { private async _performWaitForNavigationCheck(progress: Progress) {
if (process.env.PLAYWRIGHT_SKIP_NAVIGATION_CHECK)
return;
const mainFrame = this._frameManager.mainFrame(); const mainFrame = this._frameManager.mainFrame();
if (!mainFrame || !mainFrame.pendingDocument()) if (!mainFrame || !mainFrame.pendingDocument())
return; return;

View file

@ -30,6 +30,7 @@ test('bindings should work after restoring from bfcache', async ({ page, server
await page.setContent(`<a href='about:blank'}>click me</a>`); await page.setContent(`<a href='about:blank'}>click me</a>`);
await page.click('a'); await page.click('a');
await expect(page).toHaveURL('about:blank');
await page.goBack({ waitUntil: 'commit' }); await page.goBack({ waitUntil: 'commit' });
await page.evaluate('window.didShow'); await page.evaluate('window.didShow');

View file

@ -181,6 +181,7 @@ it('should include form params', async ({ contextFactory, server }, testInfo) =>
await page.goto(server.EMPTY_PAGE); await page.goto(server.EMPTY_PAGE);
await page.setContent(`<form method='POST' action='/post'><input type='text' name='foo' value='bar'><input type='number' name='baz' value='123'><input type='submit'></form>`); await page.setContent(`<form method='POST' action='/post'><input type='text' name='foo' value='bar'><input type='number' name='baz' value='123'><input type='submit'></form>`);
await page.click('input[type=submit]'); await page.click('input[type=submit]');
await expect(page).toHaveURL('**/post');
const log = await getLog(); const log = await getLog();
expect(log.entries[1].request.postData).toEqual({ expect(log.entries[1].request.postData).toEqual({
mimeType: 'application/x-www-form-urlencoded', mimeType: 'application/x-www-form-urlencoded',

View file

@ -19,24 +19,37 @@ import { stripAnsi } from 'tests/config/utils';
import type { TestServer } from '../config/testserver'; import type { TestServer } from '../config/testserver';
import { test as it, expect } from './pageTest'; import { test as it, expect } from './pageTest';
function initServer(server: TestServer): string[] { function initStallingServer(server: TestServer, url?: string) {
let release: () => void;
const releasePromise = new Promise<void>(r => release = r);
let route: () => void;
const routePromise = new Promise<void>(r => route = r);
const messages = []; const messages = [];
server.setRoute('/empty.html', async (req, res) => { server.setRoute(url ?? '/empty.html', async (req, res) => {
messages.push('route'); messages.push('route');
route();
await releasePromise;
res.setHeader('Content-Type', 'text/html'); res.setHeader('Content-Type', 'text/html');
res.end(`<link rel='stylesheet' href='./one-style.css'>`); res.end(`<button onclick="window.__clicked=true">click me</button>`);
}); });
return messages; return { messages, release, routed: routePromise };
} }
it('should await navigation when clicking anchor', async ({ page, server }) => { it('should await navigation before clicking anchor', async ({ page, server }) => {
const messages = initServer(server); const { messages, release, routed } = initStallingServer(server);
await page.setContent(`<a id="anchor" href="${server.EMPTY_PAGE}">empty.html</a>`); await page.setContent(`<a href="${server.EMPTY_PAGE}">empty.html</a>`);
await Promise.all([
page.click('a').then(() => messages.push('click')), await page.click('a');
page.waitForEvent('framenavigated').then(() => messages.push('navigated')), await routed;
]); expect(messages.join('|')).toBe('route');
expect(messages.join('|')).toBe('route|navigated|click');
const click2 = page.click('button').then(() => messages.push('click2'));
await page.waitForTimeout(1000);
expect(messages.join('|')).toBe('route');
release();
await click2;
expect(messages.join('|')).toBe('route|click2');
}); });
it('should not stall on JS navigation link', async ({ page, browserName }) => { it('should not stall on JS navigation link', async ({ page, browserName }) => {
@ -44,57 +57,69 @@ it('should not stall on JS navigation link', async ({ page, browserName }) => {
await page.click('a'); await page.click('a');
}); });
it('should await cross-process navigation when clicking anchor', async ({ page, server }) => { it('should await cross-process navigation before clicking anchor', async ({ page, server }) => {
const messages = initServer(server); const { messages, release, routed } = initStallingServer(server);
await page.setContent(`<a href="${server.CROSS_PROCESS_PREFIX + '/empty.html'}">empty.html</a>`); await page.setContent(`<a href="${server.CROSS_PROCESS_PREFIX + '/empty.html'}">empty.html</a>`);
await Promise.all([ await page.click('a');
page.click('a').then(() => messages.push('click')), await routed;
page.waitForEvent('framenavigated').then(() => messages.push('navigated')), expect(messages.join('|')).toBe('route');
]);
expect(messages.join('|')).toBe('route|navigated|click'); const click2 = page.click('button').then(() => messages.push('click2'));
await page.waitForTimeout(1000);
expect(messages.join('|')).toBe('route');
release();
await click2;
expect(messages.join('|')).toBe('route|click2');
}); });
it('should await form-get on click', async ({ page, server }) => { it('should await form-get navigation before click', async ({ page, server }) => {
const messages = []; const { messages, release, routed } = initStallingServer(server, '/empty.html?foo=bar');
server.setRoute('/empty.html?foo=bar', async (req, res) => {
messages.push('route');
res.setHeader('Content-Type', 'text/html');
res.end(`<link rel='stylesheet' href='./one-style.css'>`);
});
await page.setContent(` await page.setContent(`
<form action="${server.EMPTY_PAGE}" method="get"> <form action="${server.EMPTY_PAGE}" method="get">
<input name="foo" value="bar"> <input name="foo" value="bar">
<input type="submit" value="Submit"> <input type="submit" value="Submit">
</form>`); </form>`);
await Promise.all([ await page.click('input[type=submit]');
page.click('input[type=submit]').then(() => messages.push('click')), await routed;
page.waitForEvent('framenavigated').then(() => messages.push('navigated')), expect(messages.join('|')).toBe('route');
]);
expect(messages.join('|')).toBe('route|navigated|click'); const click2 = page.click('button').then(() => messages.push('click2'));
await page.waitForTimeout(1000);
expect(messages.join('|')).toBe('route');
release();
await click2;
expect(messages.join('|')).toBe('route|click2');
}); });
it('should await form-post on click', async ({ page, server }) => { it('should await form-post navigation before click', async ({ page, server }) => {
const messages = initServer(server); const { messages, release, routed } = initStallingServer(server);
await page.setContent(` await page.setContent(`
<form action="${server.EMPTY_PAGE}" method="post"> <form action="${server.EMPTY_PAGE}" method="post">
<input name="foo" value="bar"> <input name="foo" value="bar">
<input type="submit" value="Submit"> <input type="submit" value="Submit">
</form>`); </form>`);
await Promise.all([ await page.click('input[type=submit]');
page.click('input[type=submit]').then(() => messages.push('click')), await routed;
page.waitForEvent('framenavigated').then(() => messages.push('navigated')), expect(messages.join('|')).toBe('route');
]);
expect(messages.join('|')).toBe('route|navigated|click'); const click2 = page.click('button').then(() => messages.push('click2'));
await page.waitForTimeout(1000);
expect(messages.join('|')).toBe('route');
release();
await click2;
expect(messages.join('|')).toBe('route|click2');
}); });
it('should work with noWaitAfter: true', async ({ page, server }) => { it('should work without noWaitAfter when navigation is stalled', async ({ page, server }) => {
server.setRoute('/empty.html', async () => {}); server.setRoute('/empty.html', async () => {});
await page.setContent(`<a id="anchor" href="${server.EMPTY_PAGE}">empty.html</a>`); await page.setContent(`<a id="anchor" href="${server.EMPTY_PAGE}">empty.html</a>`);
await page.click('a', { noWaitAfter: true }); await page.click('a');
}); });
it('should work with dblclick without noWaitAfter when navigation is stalled', async ({ page, server }) => { it('should work with dblclick without noWaitAfter when navigation is stalled', async ({ page, server }) => {
@ -103,16 +128,6 @@ it('should work with dblclick without noWaitAfter when navigation is stalled', a
await page.dblclick('a'); await page.dblclick('a');
}); });
it('should work with waitForLoadState(load)', async ({ page, server }) => {
const messages = initServer(server);
await page.setContent(`<a id="anchor" href="${server.EMPTY_PAGE}">empty.html</a>`);
await Promise.all([
page.click('a').then(() => page.waitForLoadState('load')).then(() => messages.push('clickload')),
page.waitForEvent('load').then(() => messages.push('load')),
]);
expect(messages.join('|')).toBe('route|load|clickload');
});
it('should work with goto following click', async ({ page, server }) => { it('should work with goto following click', async ({ page, server }) => {
server.setRoute('/login.html', async (req, res) => { server.setRoute('/login.html', async (req, res) => {
res.setHeader('Content-Type', 'text/html'); res.setHeader('Content-Type', 'text/html');
@ -130,17 +145,6 @@ it('should work with goto following click', async ({ page, server }) => {
await page.goto(server.EMPTY_PAGE); await page.goto(server.EMPTY_PAGE);
}); });
it('should report navigation in the log when clicking anchor', async ({ page, server, mode }) => {
it.skip(mode !== 'default');
await page.setContent(`<a href="${server.PREFIX + '/frames/one-frame.html'}">click me</a>`);
const __testHookAfterPointerAction = () => new Promise(f => setTimeout(f, 6000));
const error = await page.click('a', { timeout: 5000, __testHookAfterPointerAction } as any).catch(e => e);
expect(error.message).toContain('page.click: Timeout 5000ms exceeded.');
expect(error.message).toContain('waiting for scheduled navigations to finish');
expect(error.message).toContain(`navigated to "${server.PREFIX + '/frames/one-frame.html'}"`);
});
it('should report and collapse log in action', async ({ page, server, mode }) => { it('should report and collapse log in action', async ({ page, server, mode }) => {
await page.setContent(`<input id='checkbox' type='checkbox' style="visibility: hidden"></input>`); await page.setContent(`<input id='checkbox' type='checkbox' style="visibility: hidden"></input>`);
const error = await page.locator('input').click({ timeout: 5000 }).catch(e => e); const error = await page.locator('input').click({ timeout: 5000 }).catch(e => e);

View file

@ -95,6 +95,7 @@ it('goBack/goForward should work with bfcache-able pages', async ({ page, server
await page.goto(server.PREFIX + '/cached/bfcached.html'); await page.goto(server.PREFIX + '/cached/bfcached.html');
await page.setContent(`<a href=${JSON.stringify(server.PREFIX + '/cached/bfcached.html?foo')}>click me</a>`); await page.setContent(`<a href=${JSON.stringify(server.PREFIX + '/cached/bfcached.html?foo')}>click me</a>`);
await page.click('a'); await page.click('a');
await expect(page).toHaveURL(/.*foo$/);
let response = await page.goBack(); let response = await page.goBack();
expect(response.url()).toBe(server.PREFIX + '/cached/bfcached.html'); expect(response.url()).toBe(server.PREFIX + '/cached/bfcached.html');

View file

@ -288,11 +288,10 @@ it('should parse the json post data', async ({ page, server }) => {
it('should parse the data if content-type is application/x-www-form-urlencoded', async ({ page, server }) => { it('should parse the data if content-type is application/x-www-form-urlencoded', async ({ page, server }) => {
await page.goto(server.EMPTY_PAGE); await page.goto(server.EMPTY_PAGE);
server.setRoute('/post', (req, res) => res.end()); server.setRoute('/post', (req, res) => res.end());
let request = null; const requestPromise = page.waitForRequest('**/post');
page.on('request', r => request = r);
await page.setContent(`<form method='POST' action='/post'><input type='text' name='foo' value='bar'><input type='number' name='baz' value='123'><input type='submit'></form>`); await page.setContent(`<form method='POST' action='/post'><input type='text' name='foo' value='bar'><input type='number' name='baz' value='123'><input type='submit'></form>`);
await page.click('input[type=submit]'); await page.click('input[type=submit]');
expect(request).toBeTruthy(); const request = await requestPromise;
expect(request.postDataJSON()).toEqual({ 'foo': 'bar', 'baz': '123' }); expect(request.postDataJSON()).toEqual({ 'foo': 'bar', 'baz': '123' });
}); });

View file

@ -519,12 +519,12 @@ it('should accept single file', async ({ page, asset }) => {
}); });
it('should detect mime type', async ({ page, server, asset }) => { it('should detect mime type', async ({ page, server, asset }) => {
let resolveFiles;
let files: Record<string, formidable.File>; const files = new Promise<Record<string, formidable.File>>(r => resolveFiles = r);
server.setRoute('/upload', async (req, res) => { server.setRoute('/upload', async (req, res) => {
const form = new formidable.IncomingForm(); const form = new formidable.IncomingForm();
form.parse(req, function(err, fields, f) { form.parse(req, function(err, fields, f) {
files = f as Record<string, formidable.File>; resolveFiles(f);
res.end(); res.end();
}); });
}); });
@ -541,7 +541,7 @@ it('should detect mime type', async ({ page, server, asset }) => {
page.click('input[type=submit]'), page.click('input[type=submit]'),
server.waitForRequest('/upload'), server.waitForRequest('/upload'),
]); ]);
const { file1, file2 } = files; const { file1, file2 } = await files;
expect(file1.originalFilename).toBe('file-to-upload.txt'); expect(file1.originalFilename).toBe('file-to-upload.txt');
expect(file1.mimetype).toBe('text/plain'); expect(file1.mimetype).toBe('text/plain');
expect(fs.readFileSync(file1.filepath).toString()).toBe( expect(fs.readFileSync(file1.filepath).toString()).toBe(