chore: implement improved waitForNavigation

packages/puppeteer-core/src/bidi/Frame.ts

@@ -10,14 +10,18 @@ import type {Observable} from '../../third_party/rxjs/rxjs.js';
 import {
   combineLatest,
   defer,
-  delayWhen,
   filter,
   first,
   firstValueFrom,
+  from,
   map,
+  merge,
+  mergeMap,
   of,
   raceWith,
-  switchMap,
+  startWith,
+  take,
+  zip,
 } from '../../third_party/rxjs/rxjs.js';
 import type {CDPSession} from '../api/CDPSession.js';
 import type {ElementHandle} from '../api/ElementHandle.js';
@@ -121,10 +125,8 @@ export class BidiFrame extends Frame {
       });
     });
 
-    this.browsingContext.on('navigation', ({navigation}) => {
+    this.browsingContext.on('fragment', () => {
-      navigation.once('fragment', () => {
+      this.page().trustedEmitter.emit(PageEvent.FrameNavigated, this);
-        this.page().trustedEmitter.emit(PageEvent.FrameNavigated, this);
-      });
     });
     this.browsingContext.on('load', () => {
       this.page().trustedEmitter.emit(PageEvent.Load, undefined);
@@ -292,23 +294,26 @@ export class BidiFrame extends Frame {
     url: string,
     options: GoToOptions = {}
   ): Promise<BidiHTTPResponse | null> {
-    const [response] = await Promise.all([
+    return await firstValueFrom(
-      this.waitForNavigation(options),
+      this.#waitForNavigation$({
-      // Some implementations currently only report errors when the
+        ...options,
-      // readiness=interactive.
+        // Some implementations currently only report errors when the
-      //
+        // readiness=interactive.
-      // Related: https://bugzilla.mozilla.org/show_bug.cgi?id=1846601
+        //
-      this.browsingContext.navigate(
+        // Related: https://bugzilla.mozilla.org/show_bug.cgi?id=1846601
-        url,
+        completion$: from(
-        Bidi.BrowsingContext.ReadinessState.Interactive
+          this.browsingContext.navigate(
-      ),
+            url,
-    ]).catch(
+            Bidi.BrowsingContext.ReadinessState.Interactive
+          )
+        ),
+      })
+    ).catch(
       rewriteNavigationError(
         url,
         options.timeout ?? this.timeoutSettings.navigationTimeout()
       )
     );
-    return response;
   }
 
   @throwIfDetached
@@ -331,64 +336,7 @@ export class BidiFrame extends Frame {
   override async waitForNavigation(
     options: WaitForOptions = {}
   ): Promise<BidiHTTPResponse | null> {
-    const {timeout: ms = this.timeoutSettings.navigationTimeout()} = options;
+    return await firstValueFrom(this.#waitForNavigation$(options));
-
-    const frames = this.childFrames().map(frame => {
-      return frame.#detached$();
-    });
-    return await firstValueFrom(
-      combineLatest([
-        fromEmitterEvent(this.browsingContext, 'navigation').pipe(
-          switchMap(({navigation}) => {
-            return this.#waitForLoad$(options).pipe(
-              delayWhen(() => {
-                if (frames.length === 0) {
-                  return of(undefined);
-                }
-                return combineLatest(frames);
-              }),
-              raceWith(
-                fromEmitterEvent(navigation, 'fragment'),
-                fromEmitterEvent(navigation, 'failed').pipe(
-                  map(({url}) => {
-                    throw new Error(`Navigation failed: ${url}`);
-                  })
-                ),
-                fromEmitterEvent(navigation, 'aborted').pipe(
-                  map(({url}) => {
-                    throw new Error(`Navigation aborted: ${url}`);
-                  })
-                )
-              ),
-              map(() => {
-                return navigation;
-              })
-            );
-          })
-        ),
-        this.#waitForNetworkIdle$(options),
-      ]).pipe(
-        map(([navigation]) => {
-          const request = navigation.request;
-          if (!request) {
-            return null;
-          }
-          const httpRequest = requests.get(request)!;
-          const lastRedirect = httpRequest.redirectChain().at(-1);
-          return (
-            lastRedirect !== undefined ? lastRedirect : httpRequest
-          ).response();
-        }),
-        raceWith(
-          timeout(ms),
-          this.#detached$().pipe(
-            map(() => {
-              throw new TargetCloseError('Frame detached.');
-            })
-          )
-        )
-      )
-    );
   }
 
   override waitForDevicePrompt(): never {
@@ -446,6 +394,89 @@ export class BidiFrame extends Frame {
     return new BidiCdpSession(this, sessionId);
   }
 
+  @throwIfDetached
+  #waitForNavigation$(
+    options: WaitForOptions & {
+      /**
+       * If defined, waitForNavigation$ will wait for this condition and a
+       * navigation.
+       */
+      completion$?: Observable<unknown>;
+    } = {}
+  ): Observable<BidiHTTPResponse | null> {
+    const {
+      timeout: ms = this.timeoutSettings.navigationTimeout(),
+      completion$,
+    } = options;
+
+    const enum State {
+      WaitingForRequest,
+      WaitingForResponse,
+    }
+
+    // Wait for the first navigation of any type.
+    let navigations$: Observable<unknown> = merge(
+      fromEmitterEvent(this.browsingContext, 'navigation'),
+      fromEmitterEvent(this.browsingContext, 'fragment')
+    ).pipe(first());
+
+    // Wait for the completion condition to complete if defined.
+    if (completion$ !== undefined) {
+      navigations$ = zip(navigations$, completion$);
+    }
+
+    // An observable that returns the response to the first navigation request,
+    // if any.
+    const frames = this.childFrames();
+    const response$ = fromEmitterEvent(this.browsingContext, 'navigation').pipe(
+      take(1),
+      mergeMap(({navigation}) => {
+        // Wait for conditions before returning the response.
+        return combineLatest([
+          this.#waitForNetworkIdle$(options),
+          this.#waitForLoad$(options),
+          ...frames.map(frame => {
+            return frame.#detached$();
+          }),
+        ]).pipe(
+          map(() => {
+            if (!navigation.request) {
+              return null;
+            }
+            const httpRequest = requests.get(navigation.request)!;
+            const redirect = httpRequest.redirectChain().at(-1);
+            return (redirect !== undefined ? redirect : httpRequest).response();
+          }),
+          startWith(State.WaitingForResponse)
+        );
+      }),
+      startWith(State.WaitingForRequest)
+    );
+
+    return combineLatest([response$, navigations$]).pipe(
+      mergeMap(([stateOrResponse]) => {
+        switch (stateOrResponse) {
+          // If we are waiting for the navigation request, give up.
+          case State.WaitingForRequest:
+            return of(null);
+          // We shall also wait for the response.
+          case State.WaitingForResponse:
+            return of();
+          default:
+            return of(stateOrResponse);
+        }
+      }),
+      raceWith(
+        timeout(ms),
+        this.#detached$().pipe(
+          map(() => {
+            throw new TargetCloseError('Frame detached.');
+          })
+        )
+      )
+    );
+  }
+
   @throwIfDetached
   #waitForLoad$(options: WaitForOptions = {}): Observable<void> {
     let {waitUntil = 'load'} = options;

packages/puppeteer-core/src/bidi/core/BrowsingContext.ts

@@ -109,6 +109,13 @@ export class BrowsingContext extends EventEmitter<{
     /** The realm for the new dedicated worker */
     realm: DedicatedWorkerRealm;
   };
+  /** Emitted whenever the browsing context fragment navigates */
+  fragment: {
+    /** The new url */
+    url: string;
+    /** The timestamp of the navigation */
+    timestamp: Date;
+  };
 }> {
   static from(
     userContext: UserContext,
@@ -214,35 +221,41 @@ export class BrowsingContext extends EventEmitter<{
       if (info.context !== this.id) {
         return;
       }
-      this.#url = info.url;
 
       for (const [id, request] of this.#requests) {
         if (request.disposed) {
           this.#requests.delete(id);
         }
       }
-      // If the navigation hasn't finished, then this is nested navigation. The
+
-      // current navigation will handle this.
+      if (this.#navigation !== undefined) {
-      if (this.#navigation !== undefined && !this.#navigation.disposed) {
+        this.#navigation.dispose();
-        return;
       }
 
-      // Note the navigation ID is null for this event.
+      this.#navigation = Navigation.from(this, info.url);
-      this.#navigation = Navigation.from(this);
 
       const navigationEmitter = this.#disposables.use(
         new EventEmitter(this.#navigation)
       );
-      for (const eventName of ['fragment', 'failed', 'aborted'] as const) {
+      for (const eventName of ['failed', 'aborted'] as const) {
-        navigationEmitter.once(eventName, ({url}) => {
+        navigationEmitter.once(eventName, () => {
           navigationEmitter[disposeSymbol]();
-
-          this.#url = url;
         });
       }
 
       this.emit('navigation', {navigation: this.#navigation});
     });
+    sessionEmitter.on('browsingContext.fragmentNavigated', info => {
+      if (info.context !== this.id) {
+        return;
+      }
+      this.#url = info.url;
+
+      this.emit('fragment', {
+        url: info.url,
+        timestamp: new Date(info.timestamp),
+      });
+    });
     sessionEmitter.on('network.beforeRequestSent', event => {
       if (event.context !== this.id) {
         return;

packages/puppeteer-core/src/bidi/core/Navigation.ts

@@ -26,31 +26,30 @@ export interface NavigationInfo {
 export class Navigation extends EventEmitter<{
   /** Emitted when navigation has a request associated with it. */
   request: Request;
-  /** Emitted when fragment navigation occurred. */
-  fragment: NavigationInfo;
   /** Emitted when navigation failed. */
   failed: NavigationInfo;
   /** Emitted when navigation was aborted. */
   aborted: NavigationInfo;
 }> {
-  static from(context: BrowsingContext): Navigation {
+  static from(context: BrowsingContext, url: string): Navigation {
-    const navigation = new Navigation(context);
+    const navigation = new Navigation(context, url);
     navigation.#initialize();
     return navigation;
   }
 
   // keep-sorted start
   #request: Request | undefined;
-  #navigation: Navigation | undefined;
   readonly #browsingContext: BrowsingContext;
   readonly #disposables = new DisposableStack();
   readonly #id = new Deferred<string | null>();
+  readonly #url: string;
   // keep-sorted end
 
-  private constructor(context: BrowsingContext) {
+  private constructor(context: BrowsingContext, url: string) {
     super();
     // keep-sorted start
     this.#browsingContext = context;
+    this.#url = url;
     // keep-sorted end
   }
 
@@ -84,35 +83,7 @@ export class Navigation extends EventEmitter<{
     const sessionEmitter = this.#disposables.use(
       new EventEmitter(this.#session)
     );
-    sessionEmitter.on('browsingContext.navigationStarted', info => {
-      if (
-        info.context !== this.#browsingContext.id ||
-        this.#navigation !== undefined
-      ) {
-        return;
-      }
-      this.#navigation = Navigation.from(this.#browsingContext);
-    });
-
-    for (const eventName of [
-      'browsingContext.domContentLoaded',
-      'browsingContext.load',
-    ] as const) {
-      sessionEmitter.on(eventName, info => {
-        if (
-          info.context !== this.#browsingContext.id ||
-          info.navigation === null ||
-          !this.#matches(info.navigation)
-        ) {
-          return;
-        }
-
-        this.dispose();
-      });
-    }
-
     for (const [eventName, event] of [
-      ['browsingContext.fragmentNavigated', 'fragment'],
       ['browsingContext.navigationFailed', 'failed'],
       ['browsingContext.navigationAborted', 'aborted'],
     ] as const) {
@@ -136,9 +107,6 @@ export class Navigation extends EventEmitter<{
   }
 
   #matches(navigation: string | null): boolean {
-    if (this.#navigation !== undefined && !this.#navigation.disposed) {
-      return false;
-    }
     if (!this.#id.resolved()) {
       this.#id.resolve(navigation);
       return true;
@@ -156,13 +124,13 @@ export class Navigation extends EventEmitter<{
   get request(): Request | undefined {
     return this.#request;
   }
-  get navigation(): Navigation | undefined {
+  get url(): string {
-    return this.#navigation;
+    return this.#url;
   }
   // keep-sorted end
 
   @inertIfDisposed
-  private dispose(): void {
+  dispose(): void {
     this[disposeSymbol]();
   }
 

packages/puppeteer-core/src/bidi/core/Session.ts

@@ -105,19 +105,6 @@ export class Session
     browserEmitter.once('closed', ({reason}) => {
       this.dispose(reason);
     });
-
-    // TODO: Currently, some implementations do not emit navigationStarted event
-    // for fragment navigations (as per spec) and some do. This could emits a
-    // synthetic navigationStarted to work around this inconsistency.
-    const seen = new WeakSet();
-    this.on('browsingContext.fragmentNavigated', info => {
-      if (seen.has(info)) {
-        return;
-      }
-      seen.add(info);
-      this.emit('browsingContext.navigationStarted', info);
-      this.emit('browsingContext.fragmentNavigated', info);
-    });
   }
 
   // keep-sorted start block=yes

packages/puppeteer-core/third_party/rxjs/rxjs.ts

@@ -14,7 +14,6 @@ export {
   first,
   firstValueFrom,
   forkJoin,
-  delayWhen,
   from,
   fromEvent,
   identity,

test/TestExpectations.json

@@ -2241,13 +2241,6 @@
     "parameters": ["cdp", "firefox"],
     "expectations": ["FAIL"]
   },
-  {
-    "testIdPattern": "[navigation.spec] navigation Page.goto should work when page calls history API in beforeunload",
-    "platforms": ["darwin", "linux", "win32"],
-    "parameters": ["firefox", "webDriverBiDi"],
-    "expectations": ["FAIL"],
-    "comment": "History navigation is breaking the Puppeteer expecation about navigation."
-  },
   {
     "testIdPattern": "[navigation.spec] navigation Page.goto should work with anchor navigation",
     "platforms": ["darwin", "linux", "win32"],