feat(cr, wk): make clicks, input and evaluate await scheduled navigations (#1200)

src/chromium/crPage.ts

@@ -80,6 +80,7 @@ export class CRPage implements PageDelegate {
       helper.addEventListener(this._client, 'Page.frameAttached', event => this._onFrameAttached(event.frameId, event.parentFrameId)),
       helper.addEventListener(this._client, 'Page.frameDetached', event => this._onFrameDetached(event.frameId)),
       helper.addEventListener(this._client, 'Page.frameNavigated', event => this._onFrameNavigated(event.frame, false)),
+      helper.addEventListener(this._client, 'Page.frameRequestedNavigation', event => this._onFrameRequestedNavigation(event)),
       helper.addEventListener(this._client, 'Page.frameStoppedLoading', event => this._onFrameStoppedLoading(event.frameId)),
       helper.addEventListener(this._client, 'Page.javascriptDialogOpening', event => this._onDialog(event)),
       helper.addEventListener(this._client, 'Page.lifecycleEvent', event => this._onLifecycleEvent(event)),
@@ -174,6 +175,10 @@ export class CRPage implements PageDelegate {
     this._page._frameManager.frameCommittedNewDocumentNavigation(framePayload.id, framePayload.url, framePayload.name || '', framePayload.loaderId, initial);
   }
 
+  _onFrameRequestedNavigation(payload: Protocol.Page.frameRequestedNavigationPayload) {
+    this._page._frameManager.frameRequestedNavigation(payload.frameId);
+  }
+
   async _ensureIsolatedWorld(name: string) {
     if (this._isolatedWorlds.has(name))
       return;

src/dom.ts

@@ -52,7 +52,9 @@ export class FrameExecutionContext extends js.ExecutionContext {
 
     if (!args.some(needsAdoption)) {
       // Only go through asynchronous calls if required.
-      return this._delegate.evaluate(this, returnByValue, pageFunction, ...args);
+      return await this.frame._page._frameManager.waitForNavigationsCreatedBy(async () => {
+        return this._delegate.evaluate(this, returnByValue, pageFunction, ...args);
+      });
     }
 
     const toDispose: Promise<ElementHandle>[] = [];
@@ -65,7 +67,9 @@ export class FrameExecutionContext extends js.ExecutionContext {
     }));
     let result;
     try {
-      result = await this._delegate.evaluate(this, returnByValue, pageFunction, ...adopted);
+      result = await this.frame._page._frameManager.waitForNavigationsCreatedBy(async () => {
+        return this._delegate.evaluate(this, returnByValue, pageFunction, ...adopted);
+      });
     } finally {
       toDispose.map(handlePromise => handlePromise.then(handle => handle.dispose()));
     }
@@ -248,12 +252,15 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
     const point = offset ? await this._offsetPoint(offset) : await this._clickablePoint();
     if (waitFor)
       await this._waitForHitTargetAt(point, options);
-    let restoreModifiers: input.Modifier[] | undefined;
+
-    if (options && options.modifiers)
+    await this._page._frameManager.waitForNavigationsCreatedBy(async () => {
-      restoreModifiers = await this._page.keyboard._ensureModifiers(options.modifiers);
+      let restoreModifiers: input.Modifier[] | undefined;
-    await action(point);
+      if (options && options.modifiers)
-    if (restoreModifiers)
+        restoreModifiers = await this._page.keyboard._ensureModifiers(options.modifiers);
-      await this._page.keyboard._ensureModifiers(restoreModifiers);
+      await action(point);
+      if (restoreModifiers)
+        await this._page.keyboard._ensureModifiers(restoreModifiers);
+    });
   }
 
   hover(options?: PointerActionOptions & types.WaitForOptions): Promise<void> {
@@ -284,18 +291,22 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
       if (option.index !== undefined)
         assert(helper.isNumber(option.index), 'Indices must be numbers. Found index "' + option.index + '" of type "' + (typeof option.index) + '"');
     }
-    return this._evaluateInUtility((injected, node, ...optionsToSelect) => injected.selectOptions(node, optionsToSelect), ...options);
+    return await this._page._frameManager.waitForNavigationsCreatedBy<string[]>(async () => {
+      return this._evaluateInUtility((injected, node, ...optionsToSelect) => injected.selectOptions(node, optionsToSelect), ...options);
+    });
   }
 
   async fill(value: string): Promise<void> {
     assert(helper.isString(value), 'Value must be string. Found value "' + value + '" of type "' + (typeof value) + '"');
-    const error = await this._evaluateInUtility((injected, node, value) => injected.fill(node, value), value);
+    await this._page._frameManager.waitForNavigationsCreatedBy(async () => {
-    if (error)
+      const error = await this._evaluateInUtility((injected, node, value) => injected.fill(node, value), value);
-      throw new Error(error);
+      if (error)
-    if (value)
+        throw new Error(error);
-      await this._page.keyboard.sendCharacters(value);
+      if (value)
-    else
+        await this._page.keyboard.sendCharacters(value);
-      await this._page.keyboard.press('Delete');
+      else
+        await this._page.keyboard.press('Delete');
+    });
   }
 
   async setInputFiles(...files: (string | types.FilePayload)[]) {
@@ -317,7 +328,9 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
       }
       return item;
     }));
-    await this._page._delegate.setInputFiles(this as any as ElementHandle<HTMLInputElement>, filePayloads);
+    await this._page._frameManager.waitForNavigationsCreatedBy(async () => {
+      await this._page._delegate.setInputFiles(this as any as ElementHandle<HTMLInputElement>, filePayloads);
+    });
   }
 
   async focus() {
@@ -332,13 +345,17 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
   }
 
   async type(text: string, options?: { delay?: number }) {
-    await this.focus();
+    await this._page._frameManager.waitForNavigationsCreatedBy(async () => {
-    await this._page.keyboard.type(text, options);
+      await this.focus();
+      await this._page.keyboard.type(text, options);
+    });
   }
 
   async press(key: string, options?: { delay?: number, text?: string }) {
-    await this.focus();
+    await this._page._frameManager.waitForNavigationsCreatedBy(async () => {
-    await this._page.keyboard.press(key, options);
+      await this.focus();
+      await this._page.keyboard.press(key, options);
+    });
   }
 
   async check(options?: types.WaitForOptions) {

src/frames.ts

@@ -59,6 +59,7 @@ export class FrameManager {
   private _mainFrame: Frame;
   readonly _lifecycleWatchers = new Set<() => void>();
   readonly _consoleMessageTags = new Map<string, ConsoleTagHandler>();
+  private _navigationRequestCollectors = new Set<Set<string>>();
 
   constructor(page: Page) {
     this._page = page;
@@ -107,7 +108,34 @@ export class FrameManager {
     }
   }
 
+  async waitForNavigationsCreatedBy<T>(action: () => Promise<T>): Promise<T> {
+    const frameIds = new Set<string>();
+    this._navigationRequestCollectors.add(frameIds);
+    try {
+      const result = await action();
+      if (!frameIds.size)
+        return result;
+      const frames = Array.from(frameIds.values()).map(frameId => this._frames.get(frameId));
+      await Promise.all(frames.map(frame => frame!.waitForNavigation({ waitUntil: []}))).catch(e => {});
+      await new Promise(platform.makeWaitForNextTask());
+      return result;
+    } finally {
+      this._navigationRequestCollectors.delete(frameIds);
+    }
+  }
+
+  frameRequestedNavigation(frameId: string) {
+    for (const frameIds of this._navigationRequestCollectors)
+      frameIds.add(frameId);
+  }
+
+  _cancelFrameRequestedNavigation(frameId: string) {
+    for (const frameIds of this._navigationRequestCollectors)
+      frameIds.delete(frameId);
+  }
+
   frameCommittedNewDocumentNavigation(frameId: string, url: string, name: string, documentId: string, initial: boolean) {
+    this._cancelFrameRequestedNavigation(frameId);
     const frame = this._frames.get(frameId)!;
     for (const child of frame.childFrames())
       this._removeFramesRecursively(child);
@@ -122,6 +150,7 @@ export class FrameManager {
   }
 
   frameCommittedSameDocumentNavigation(frameId: string, url: string) {
+    this._cancelFrameRequestedNavigation(frameId);
     const frame = this._frames.get(frameId);
     if (!frame)
       return;
@@ -206,6 +235,7 @@ export class FrameManager {
     if (request._documentId && frame) {
       const isCurrentDocument = frame._lastDocumentId === request._documentId;
       if (!isCurrentDocument) {
+        this._cancelFrameRequestedNavigation(frame._id);
         let errorText = request.failure()!.errorText;
         if (canceled)
           errorText += '; maybe frame was detached?';
@@ -218,11 +248,13 @@ export class FrameManager {
   }
 
   provisionalLoadFailed(frame: Frame, documentId: string, error: string) {
+    this._cancelFrameRequestedNavigation(frame._id);
     for (const watcher of frame._documentWatchers)
       watcher(documentId, new Error(error));
   }
 
   private _removeFramesRecursively(frame: Frame) {
+    this._cancelFrameRequestedNavigation(frame._id);
     for (const child of frame.childFrames())
       this._removeFramesRecursively(child);
     frame._onDetached();
@@ -406,6 +438,7 @@ export class Frame {
     disposer.dispose();
     if (error)
       throw error;
+
     return request ? request._finalRequest._waitForResponse() : null;
   }
 

src/webkit/wkPage.ts

@@ -201,6 +201,7 @@ export class WKPage implements PageDelegate {
       helper.addEventListener(this._session, 'Page.navigatedWithinDocument', event => this._onFrameNavigatedWithinDocument(event.frameId, event.url)),
       helper.addEventListener(this._session, 'Page.frameAttached', event => this._onFrameAttached(event.frameId, event.parentFrameId)),
       helper.addEventListener(this._session, 'Page.frameDetached', event => this._onFrameDetached(event.frameId)),
+      helper.addEventListener(this._session, 'Page.frameScheduledNavigation', event => this._onFrameScheduledNavigation(event.frameId)),
       helper.addEventListener(this._session, 'Page.frameStoppedLoading', event => this._onFrameStoppedLoading(event.frameId)),
       helper.addEventListener(this._session, 'Page.loadEventFired', event => this._onLifecycleEvent(event.frameId, 'load')),
       helper.addEventListener(this._session, 'Page.domContentEventFired', event => this._onLifecycleEvent(event.frameId, 'domcontentloaded')),
@@ -234,6 +235,10 @@ export class WKPage implements PageDelegate {
     await Promise.all(sessions.map(session => callback(session).catch(debugError)));
   }
 
+  private _onFrameScheduledNavigation(frameId: string) {
+    this._page._frameManager.frameRequestedNavigation(frameId);
+  }
+
   private _onFrameStoppedLoading(frameId: string) {
     this._page._frameManager.frameStoppedLoading(frameId);
   }

test/navigation.spec.js

@@ -800,6 +800,130 @@ module.exports.describe = function({testRunner, expect, playwright, MAC, WIN, FF
     });
   });
 
+  describe.fail(FFOX)('Page.automaticWaiting', () => {
+    it('clicking anchor should await navigation', async({page, server}) => {
+      const messages = [];
+      server.setRoute('/empty.html', async (req, res) => {
+        messages.push('route');
+        res.end('done');
+      });
+
+      await page.setContent(`<a href="${server.EMPTY_PAGE}">empty.html</a>`);
+
+      await Promise.all([
+        page.click('a').then(() => messages.push('click')),
+        page.waitForNavigation({ waitUntil: [] }).then(() => messages.push('waitForNavigation'))
+      ]);
+      expect(messages.join('|')).toBe('route|waitForNavigation|click');
+    });
+    it('clicking anchor should await cross-process navigation', async({page, server}) => {
+      const messages = [];
+      server.setRoute('/empty.html', async (req, res) => {
+        messages.push('route');
+        res.end('done');
+      });
+
+      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.waitForNavigation({ waitUntil: [] }).then(() => messages.push('waitForNavigation'))
+      ]);
+      expect(messages.join('|')).toBe('route|waitForNavigation|click');
+    });
+    it.fail(CHROMIUM)('should await form-get on click', async({page, server}) => {
+      const messages = [];
+      server.setRoute('/empty.html?foo=bar', async (req, res) => {
+        messages.push('route');
+        res.end('done');
+      });
+
+      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.waitForNavigation({ waitUntil: [] }).then(() => messages.push('waitForNavigation'))
+      ]);
+      expect(messages.join('|')).toBe('route|waitForNavigation|click');
+    });
+    it.fail(CHROMIUM)('should await form-post on click', async({page, server}) => {
+      const messages = [];
+      server.setRoute('/empty.html', async (req, res) => {
+        messages.push('route');
+        res.end('done');
+      });
+
+      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.waitForNavigation({ waitUntil: [] }).then(() => messages.push('waitForNavigation'))
+      ]);
+      expect(messages.join('|')).toBe('route|waitForNavigation|click');
+    });
+    it('assigning to location should await navigation', async({page, server}) => {
+      const messages = [];
+      server.setRoute('/empty.html', async (req, res) => {
+        messages.push('route');
+        res.end('done');
+      });
+      await Promise.all([
+        page.evaluate(`window.location.href = "${server.EMPTY_PAGE}"`).then(() => messages.push('evaluate')),
+        page.waitForNavigation({ waitUntil: [] }).then(() => messages.push('waitForNavigation')),
+      ]);
+      expect(messages.join('|')).toBe('route|waitForNavigation|evaluate');
+    });
+    it('should await navigating specified target', async({page, server, httpsServer}) => {
+      const messages = [];
+      server.setRoute('/empty.html', async (req, res) => {
+        messages.push('route');
+        res.end('done');
+      });
+
+      await page.setContent(`
+        <a href="${server.EMPTY_PAGE}" target=target>empty.html</a>
+        <iframe name=target></iframe>
+      `);
+      const frame = page.frame({ name: 'target' });
+      await Promise.all([
+        page.click('a').then(() => messages.push('click')),
+        frame.waitForNavigation({ waitUntil: [] }).then(() => messages.push('waitForNavigation'))
+      ]);
+      expect(frame.url()).toBe(server.EMPTY_PAGE);
+      expect(messages.join('|')).toBe('route|waitForNavigation|click');
+    });
+  });
+
+  describe('Page.automaticWaiting should not hang', () => {
+    it('should not throw when clicking on links which do not commit navigation', async({page, server, httpsServer}) => {
+      await page.goto(server.EMPTY_PAGE);
+      await page.setContent(`<a href='${httpsServer.EMPTY_PAGE}'>foobar</a>`);
+      await page.click('a');
+    });
+    it('should not throw when clicking on download link', async({page, server, httpsServer}) => {
+      await page.setContent(`<a href="${server.PREFIX}/wasm/table2.wasm" download=true>table2.wasm</a>`);
+      await page.click('a');
+    });
+    it('should not hang on window.stop', async({page, server, httpsServer}) => {
+      server.setRoute('/empty.html', async (req, res) => {});
+      await Promise.all([
+        page.waitForNavigation().catch(e => {}),
+        page.evaluate((url) => {
+          window.location.href = url;
+          setTimeout(() => window.stop(), 100);
+        }, server.EMPTY_PAGE)
+      ]);
+    });
+  });
+
   describe('Page.waitForLoadState', () => {
     it('should pick up ongoing navigation', async({page, server}) => {
       let response = null;
@@ -950,13 +1074,13 @@ module.exports.describe = function({testRunner, expect, playwright, MAC, WIN, FF
 
       server.setRoute('/empty.html', () => {});
       let error = null;
-      const navigationPromise = frame.waitForNavigation().catch(e => error = e);
       await Promise.all([
-        server.waitForRequest('/empty.html'),
+        frame.waitForNavigation().catch(e => error = e),
-        frame.evaluate(() => window.location = '/empty.html')
+        server.waitForRequest('/empty.html').then(() => {
-      ]);
+          page.$eval('iframe', frame => frame.remove());
-      await page.$eval('iframe', frame => frame.remove());
+        }),
-      await navigationPromise;
+        frame.evaluate(() => window.location = '/empty.html'),
+      ]).catch(e => error = e);
       expect(error.message).toContain('frame was detached');
     });
   });

test/page.spec.js

@@ -1123,24 +1123,14 @@ module.exports.describe = function({testRunner, expect, headless, playwright, FF
 
   describe('Page.frame', function() {
     it('should respect name', async function({page, server}) {
-      await page.setContent(`
+      await page.setContent(`<iframe name=target></iframe>`);
-        <a href="${server.EMPTY_PAGE}" target=target>empty.html</a>
-        <iframe name=target></iframe>
-      `);
       expect(page.frame({ name: 'bogus' })).toBe(null);
       const frame = page.frame({ name: 'target' });
       expect(frame).toBeTruthy();
-      await Promise.all([
+      expect(frame === page.mainFrame().childFrames()[0]).toBeTruthy();
-        frame.waitForNavigation(),
-        page.click('a')
-      ]);
-      expect(frame.url()).toBe(server.EMPTY_PAGE);
     });
     it('should respect url', async function({page, server}) {
-      await page.setContent(`
+      await page.setContent(`<iframe src="${server.EMPTY_PAGE}"></iframe>`);
-        <a href="${server.EMPTY_PAGE}">empty.html target=target>empty.html</a>
-        <iframe src="${server.EMPTY_PAGE}"></iframe>
-      `);
       expect(page.frame({ url: /bogus/ })).toBe(null);
       expect(page.frame({ url: /empty/ }).url()).toBe(server.EMPTY_PAGE);
     });