From a468c73db4cff59b960c1fae03e7ea9cbe421f57 Mon Sep 17 00:00:00 2001 From: Randolf Date: Thu, 15 Feb 2024 21:22:11 +0100 Subject: [PATCH] chore: implement improved `waitForNavigation` --- packages/puppeteer-core/src/bidi/Frame.ts | 183 ++++++++++-------- .../src/bidi/core/BrowsingContext.ts | 35 ++-- .../src/bidi/core/Navigation.ts | 48 +---- .../puppeteer-core/src/bidi/core/Session.ts | 13 -- .../puppeteer-core/third_party/rxjs/rxjs.ts | 1 - test/TestExpectations.json | 7 - 6 files changed, 139 insertions(+), 148 deletions(-) diff --git a/packages/puppeteer-core/src/bidi/Frame.ts b/packages/puppeteer-core/src/bidi/Frame.ts index 2cc259cff69..ab7a9fbaa0f 100644 --- a/packages/puppeteer-core/src/bidi/Frame.ts +++ b/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}) => { - navigation.once('fragment', () => { - this.page().trustedEmitter.emit(PageEvent.FrameNavigated, this); - }); + this.browsingContext.on('fragment', () => { + 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 { - const [response] = await Promise.all([ - this.waitForNavigation(options), - // Some implementations currently only report errors when the - // readiness=interactive. - // - // Related: https://bugzilla.mozilla.org/show_bug.cgi?id=1846601 - this.browsingContext.navigate( - url, - Bidi.BrowsingContext.ReadinessState.Interactive - ), - ]).catch( + return await firstValueFrom( + this.#waitForNavigation$({ + ...options, + // Some implementations currently only report errors when the + // readiness=interactive. + // + // Related: https://bugzilla.mozilla.org/show_bug.cgi?id=1846601 + completion$: from( + this.browsingContext.navigate( + url, + 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 { - const {timeout: ms = this.timeoutSettings.navigationTimeout()} = 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.'); - }) - ) - ) - ) - ); + return await firstValueFrom(this.#waitForNavigation$(options)); } 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; + } = {} + ): Observable { + const { + timeout: ms = this.timeoutSettings.navigationTimeout(), + completion$, + } = options; + + const enum State { + WaitingForRequest, + WaitingForResponse, + } + + // Wait for the first navigation of any type. + let navigations$: Observable = 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 { let {waitUntil = 'load'} = options; diff --git a/packages/puppeteer-core/src/bidi/core/BrowsingContext.ts b/packages/puppeteer-core/src/bidi/core/BrowsingContext.ts index 6ba600c0e56..ede15ac9816 100644 --- a/packages/puppeteer-core/src/bidi/core/BrowsingContext.ts +++ b/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 && !this.#navigation.disposed) { - return; + + if (this.#navigation !== undefined) { + this.#navigation.dispose(); } - // Note the navigation ID is null for this event. - this.#navigation = Navigation.from(this); + this.#navigation = Navigation.from(this, info.url); const navigationEmitter = this.#disposables.use( new EventEmitter(this.#navigation) ); - for (const eventName of ['fragment', 'failed', 'aborted'] as const) { - navigationEmitter.once(eventName, ({url}) => { + for (const eventName of ['failed', 'aborted'] as const) { + 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; diff --git a/packages/puppeteer-core/src/bidi/core/Navigation.ts b/packages/puppeteer-core/src/bidi/core/Navigation.ts index 50040164a5d..2855e161ce5 100644 --- a/packages/puppeteer-core/src/bidi/core/Navigation.ts +++ b/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 { - const navigation = new Navigation(context); + static from(context: BrowsingContext, url: string): Navigation { + 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(); + 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 { - return this.#navigation; + get url(): string { + return this.#url; } // keep-sorted end @inertIfDisposed - private dispose(): void { + dispose(): void { this[disposeSymbol](); } diff --git a/packages/puppeteer-core/src/bidi/core/Session.ts b/packages/puppeteer-core/src/bidi/core/Session.ts index ffd39769e70..dd90f8190c2 100644 --- a/packages/puppeteer-core/src/bidi/core/Session.ts +++ b/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 diff --git a/packages/puppeteer-core/third_party/rxjs/rxjs.ts b/packages/puppeteer-core/third_party/rxjs/rxjs.ts index 6f4f844f5d0..4cb3556de9c 100644 --- a/packages/puppeteer-core/third_party/rxjs/rxjs.ts +++ b/packages/puppeteer-core/third_party/rxjs/rxjs.ts @@ -14,7 +14,6 @@ export { first, firstValueFrom, forkJoin, - delayWhen, from, fromEvent, identity, diff --git a/test/TestExpectations.json b/test/TestExpectations.json index 0a0756efc49..06e0ca37fa2 100644 --- a/test/TestExpectations.json +++ b/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"],