chore: make noWaitAfter a default

dgozman · dgozman · commit af40eae13e8a · 2025-02-04T15:17:43.000Z
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.
diff --git a/packages/playwright-core/src/server/dom.ts b/packages/playwright-core/src/server/dom.ts
@@ -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');
diff --git a/packages/playwright-core/src/server/frames.ts b/packages/playwright-core/src/server/frames.ts
@@ -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);
diff --git a/packages/playwright-core/src/server/page.ts b/packages/playwright-core/src/server/page.ts
@@ -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;
diff --git a/tests/library/chromium/bfcache.spec.ts b/tests/library/chromium/bfcache.spec.ts
@@ -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');
diff --git a/tests/library/har.spec.ts b/tests/library/har.spec.ts
@@ -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',
diff --git a/tests/page/page-autowaiting-basic.spec.ts b/tests/page/page-autowaiting-basic.spec.ts
@@ -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 }) => {
@@ -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');
@@ -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);
diff --git a/tests/page/page-history.spec.ts b/tests/page/page-history.spec.ts
@@ -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');
diff --git a/tests/page/page-network-request.spec.ts b/tests/page/page-network-request.spec.ts
@@ -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' });
 });
 
diff --git a/tests/page/page-set-input-files.spec.ts b/tests/page/page-set-input-files.spec.ts
@@ -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();
     });
   });
@@ -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(

Original file line number	Diff line number	Diff line change
`@@ -161,6 +161,8 @@ export class FrameManager {`
`161`	`161`	`}`
`162`	`162`
`163`	`163`	`async waitForSignalsCreatedBy<T>(progress: Progress \| null, waitAfter: boolean, action: () => Promise<T>): Promise<T> {`
	`164`	`+ if (!process.env.PLAYWRIGHT_WAIT_AFTER_CLICK)`
	`165`	`+ waitAfter = false;`
`164`	`166`	`if (!waitAfter)`
`165`	`167`	`return action();`
`166`	`168`	`const barrier = new SignalBarrier(progress);`
Original file line number	Diff line number	Diff line change
`@@ -473,8 +473,6 @@ export class Page extends SdkObject {`
`473`	`473`	`}`
`474`	`474`
`475`	`475`	`private async _performWaitForNavigationCheck(progress: Progress) {`
`476`		`- if (process.env.PLAYWRIGHT_SKIP_NAVIGATION_CHECK)`
`477`		`- return;`
`478`	`476`	`const mainFrame = this._frameManager.mainFrame();`
`479`	`477`	`if (!mainFrame \|\| !mainFrame.pendingDocument())`
`480`	`478`	`return;`