Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: make noWaitAfter a default #34608

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions packages/playwright-core/src/server/dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -482,18 +482,15 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
if (restoreModifiers)
await this._page.keyboard.ensureModifiers(restoreModifiers);
if (hitTargetInterceptionHandle) {
// We do not want to accidentally stall on non-committed navigation blocking the evaluate.
const stopHitTargetInterception = this._frame.raceAgainstEvaluationStallingEvents(() => {
return hitTargetInterceptionHandle.evaluate(h => h.stop());
}).catch(e => 'done' as const).finally(() => {
hitTargetInterceptionHandle?.dispose();
});
if (options.waitAfter !== false) {
// When noWaitAfter is passed, we do not want to accidentally stall on
// non-committed navigation blocking the evaluate.
const hitTargetResult = await stopHitTargetInterception;
if (hitTargetResult !== 'done')
return hitTargetResult;
}
const hitTargetResult = await stopHitTargetInterception;
if (hitTargetResult !== 'done')
return hitTargetResult;
}
progress.log(` ${options.trial ? 'trial ' : ''}${actionName} action done`);
progress.log(' waiting for scheduled navigations to finish');
Expand Down
2 changes: 2 additions & 0 deletions packages/playwright-core/src/server/frames.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,8 @@ export class FrameManager {
}

async waitForSignalsCreatedBy<T>(progress: Progress | null, waitAfter: boolean, action: () => Promise<T>): Promise<T> {
if (!process.env.PLAYWRIGHT_WAIT_AFTER_CLICK)
waitAfter = false;
if (!waitAfter)
return action();
const barrier = new SignalBarrier(progress);
Expand Down
2 changes: 0 additions & 2 deletions packages/playwright-core/src/server/page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -473,8 +473,6 @@ export class Page extends SdkObject {
}

private async _performWaitForNavigationCheck(progress: Progress) {
if (process.env.PLAYWRIGHT_SKIP_NAVIGATION_CHECK)
return;
const mainFrame = this._frameManager.mainFrame();
if (!mainFrame || !mainFrame.pendingDocument())
return;
Expand Down
1 change: 1 addition & 0 deletions tests/library/chromium/bfcache.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.click('a');
await expect(page).toHaveURL('about:blank');

await page.goBack({ waitUntil: 'commit' });
await page.evaluate('window.didShow');
Expand Down
1 change: 1 addition & 0 deletions tests/library/har.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ it('should include form params', async ({ contextFactory, server }, testInfo) =>
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.click('input[type=submit]');
await expect(page).toHaveURL('**/post');
const log = await getLog();
expect(log.entries[1].request.postData).toEqual({
mimeType: 'application/x-www-form-urlencoded',
Expand Down
128 changes: 66 additions & 62 deletions tests/page/page-autowaiting-basic.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,82 +19,107 @@ import { stripAnsi } from 'tests/config/utils';
import type { TestServer } from '../config/testserver';
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 = [];
server.setRoute('/empty.html', async (req, res) => {
server.setRoute(url ?? '/empty.html', async (req, res) => {
messages.push('route');
route();
await releasePromise;
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 }) => {
const messages = initServer(server);
await page.setContent(`<a id="anchor" href="${server.EMPTY_PAGE}">empty.html</a>`);
await Promise.all([
page.click('a').then(() => messages.push('click')),
page.waitForEvent('framenavigated').then(() => messages.push('navigated')),
]);
expect(messages.join('|')).toBe('route|navigated|click');
it('should await navigation before clicking anchor', async ({ page, server }) => {
const { messages, release, routed } = initStallingServer(server);
await page.setContent(`<a href="${server.EMPTY_PAGE}">empty.html</a>`);

await page.click('a');
await routed;
expect(messages.join('|')).toBe('route');

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 }) => {
await page.setContent(`<a href="javascript:console.log(1)">console.log</a>`);
await page.click('a');
});

it('should await cross-process navigation when clicking anchor', async ({ page, server }) => {
const messages = initServer(server);
it('should await cross-process navigation before clicking anchor', async ({ page, server }) => {
const { messages, release, routed } = initStallingServer(server);
await page.setContent(`<a href="${server.CROSS_PROCESS_PREFIX + '/empty.html'}">empty.html</a>`);

await Promise.all([
page.click('a').then(() => messages.push('click')),
page.waitForEvent('framenavigated').then(() => messages.push('navigated')),
]);
expect(messages.join('|')).toBe('route|navigated|click');
});
await page.click('a');
await routed;
expect(messages.join('|')).toBe('route');

it('should await form-get on click', async ({ page, server }) => {
const messages = [];
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'>`);
});
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 navigation before click', async ({ page, server }) => {
const { messages, release, routed } = initStallingServer(server, '/empty.html?foo=bar');
await page.setContent(`
<form action="${server.EMPTY_PAGE}" method="get">
<input name="foo" value="bar">
<input type="submit" value="Submit">
</form>`);

await Promise.all([
page.click('input[type=submit]').then(() => messages.push('click')),
page.waitForEvent('framenavigated').then(() => messages.push('navigated')),
]);
expect(messages.join('|')).toBe('route|navigated|click');
await page.click('input[type=submit]');
await routed;
expect(messages.join('|')).toBe('route');

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 }) => {
const messages = initServer(server);
it('should await form-post navigation before click', async ({ page, server }) => {
const { messages, release, routed } = initStallingServer(server);
await page.setContent(`
<form action="${server.EMPTY_PAGE}" method="post">
<input name="foo" value="bar">
<input type="submit" value="Submit">
</form>`);

await Promise.all([
page.click('input[type=submit]').then(() => messages.push('click')),
page.waitForEvent('framenavigated').then(() => messages.push('navigated')),
]);
expect(messages.join('|')).toBe('route|navigated|click');
await page.click('input[type=submit]');
await routed;
expect(messages.join('|')).toBe('route');

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 () => {});
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 }) => {
Expand All @@ -103,16 +128,6 @@ it('should work with dblclick without noWaitAfter when navigation is stalled', 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 }) => {
server.setRoute('/login.html', async (req, res) => {
res.setHeader('Content-Type', 'text/html');
Expand All @@ -130,17 +145,6 @@ it('should work with goto following click', async ({ page, server }) => {
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 }) => {
await page.setContent(`<input id='checkbox' type='checkbox' style="visibility: hidden"></input>`);
const error = await page.locator('input').click({ timeout: 5000 }).catch(e => e);
Expand Down
1 change: 1 addition & 0 deletions tests/page/page-history.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.setContent(`<a href=${JSON.stringify(server.PREFIX + '/cached/bfcached.html?foo')}>click me</a>`);
await page.click('a');
await expect(page).toHaveURL(/.*foo$/);

let response = await page.goBack();
expect(response.url()).toBe(server.PREFIX + '/cached/bfcached.html');
Expand Down
5 changes: 2 additions & 3 deletions tests/page/page-network-request.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }) => {
await page.goto(server.EMPTY_PAGE);
server.setRoute('/post', (req, res) => res.end());
let request = null;
page.on('request', r => request = r);
const requestPromise = page.waitForRequest('**/post');
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]');
expect(request).toBeTruthy();
const request = await requestPromise;
expect(request.postDataJSON()).toEqual({ 'foo': 'bar', 'baz': '123' });
});

Expand Down
8 changes: 4 additions & 4 deletions tests/page/page-set-input-files.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -519,12 +519,12 @@ it('should accept single file', async ({ page, asset }) => {
});

it('should detect mime type', async ({ page, server, asset }) => {

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