Skip to content

Commit

Permalink
fix(page): "load" event should fire before "waitForLoadState" resolves (
Browse files Browse the repository at this point in the history
#14897)

Currently, `loadstate` and `load` are two separate events in the protocol,
and are fired in this order. As a result, `waitForLoadState()` sometimes
resolves before the `'load'` event is fired, which is unexpected.

Also fixes a flaky test that assumed `load` event comes after `domcontentloaded`
for the empty page, which is not always a case in Chromium.
  • Loading branch information
dgozman committed Jun 16, 2022
1 parent 79378dd commit cdb8627
Show file tree
Hide file tree
Showing 9 changed files with 15 additions and 25 deletions.
4 changes: 4 additions & 0 deletions packages/playwright-core/src/client/frame.ts
Expand Up @@ -75,6 +75,10 @@ export class Frame extends ChannelOwner<channels.FrameChannel> implements api.Fr
}
if (event.remove)
this._loadStates.delete(event.remove);
if (!this._parentFrame && event.add === 'load' && this._page)
this._page.emit(Events.Page.Load, this._page);
if (!this._parentFrame && event.add === 'domcontentloaded' && this._page)
this._page.emit(Events.Page.DOMContentLoaded, this._page);
});
this._channel.on('navigated', event => {
this._url = event.url;
Expand Down
2 changes: 0 additions & 2 deletions packages/playwright-core/src/client/page.ts
Expand Up @@ -137,15 +137,13 @@ export class Page extends ChannelOwner<channels.PageChannel> implements api.Page
dialog.dismiss().catch(() => {});
}
});
this._channel.on('domcontentloaded', () => this.emit(Events.Page.DOMContentLoaded, this));
this._channel.on('download', ({ url, suggestedFilename, artifact }) => {
const artifactObject = Artifact.from(artifact);
this.emit(Events.Page.Download, new Download(this, url, suggestedFilename, artifactObject));
});
this._channel.on('fileChooser', ({ element, isMultiple }) => this.emit(Events.Page.FileChooser, new FileChooser(this, ElementHandle.from(element), isMultiple)));
this._channel.on('frameAttached', ({ frame }) => this._onFrameAttached(Frame.from(frame)));
this._channel.on('frameDetached', ({ frame }) => this._onFrameDetached(Frame.from(frame)));
this._channel.on('load', () => this.emit(Events.Page.Load, this));
this._channel.on('pageError', ({ error }) => this.emit(Events.Page.PageError, parseError(error)));
this._channel.on('route', ({ route, request }) => this._onRoute(Route.from(route), Request.from(request)));
this._channel.on('video', ({ artifact }) => {
Expand Down
6 changes: 0 additions & 6 deletions packages/playwright-core/src/protocol/channels.ts
Expand Up @@ -1331,11 +1331,9 @@ export interface PageEventTarget {
on(event: 'crash', callback: (params: PageCrashEvent) => void): this;
on(event: 'dialog', callback: (params: PageDialogEvent) => void): this;
on(event: 'download', callback: (params: PageDownloadEvent) => void): this;
on(event: 'domcontentloaded', callback: (params: PageDomcontentloadedEvent) => void): this;
on(event: 'fileChooser', callback: (params: PageFileChooserEvent) => void): this;
on(event: 'frameAttached', callback: (params: PageFrameAttachedEvent) => void): this;
on(event: 'frameDetached', callback: (params: PageFrameDetachedEvent) => void): this;
on(event: 'load', callback: (params: PageLoadEvent) => void): this;
on(event: 'pageError', callback: (params: PagePageErrorEvent) => void): this;
on(event: 'route', callback: (params: PageRouteEvent) => void): this;
on(event: 'video', callback: (params: PageVideoEvent) => void): this;
Expand Down Expand Up @@ -1396,7 +1394,6 @@ export type PageDownloadEvent = {
suggestedFilename: string,
artifact: ArtifactChannel,
};
export type PageDomcontentloadedEvent = {};
export type PageFileChooserEvent = {
element: ElementHandleChannel,
isMultiple: boolean,
Expand All @@ -1407,7 +1404,6 @@ export type PageFrameAttachedEvent = {
export type PageFrameDetachedEvent = {
frame: FrameChannel,
};
export type PageLoadEvent = {};
export type PagePageErrorEvent = {
error: SerializedError,
};
Expand Down Expand Up @@ -1836,11 +1832,9 @@ export interface PageEvents {
'crash': PageCrashEvent;
'dialog': PageDialogEvent;
'download': PageDownloadEvent;
'domcontentloaded': PageDomcontentloadedEvent;
'fileChooser': PageFileChooserEvent;
'frameAttached': PageFrameAttachedEvent;
'frameDetached': PageFrameDetachedEvent;
'load': PageLoadEvent;
'pageError': PagePageErrorEvent;
'route': PageRouteEvent;
'video': PageVideoEvent;
Expand Down
4 changes: 0 additions & 4 deletions packages/playwright-core/src/protocol/protocol.yml
Expand Up @@ -1318,8 +1318,6 @@ Page:
suggestedFilename: string
artifact: Artifact

domcontentloaded:

fileChooser:
parameters:
element: ElementHandle
Expand All @@ -1333,8 +1331,6 @@ Page:
parameters:
frame: Frame

load:

pageError:
parameters:
error: SerializedError
Expand Down
Expand Up @@ -65,7 +65,6 @@ export class PageDispatcher extends Dispatcher<Page, channels.PageChannel> imple
});
page.on(Page.Events.Console, message => this._dispatchEvent('console', { message: new ConsoleMessageDispatcher(this._scope, message) }));
page.on(Page.Events.Crash, () => this._dispatchEvent('crash'));
page.on(Page.Events.DOMContentLoaded, () => this._dispatchEvent('domcontentloaded'));
page.on(Page.Events.Dialog, dialog => this._dispatchEvent('dialog', { dialog: new DialogDispatcher(this._scope, dialog) }));
page.on(Page.Events.Download, (download: Download) => {
this._dispatchEvent('download', { url: download.url, suggestedFilename: download.suggestedFilename(), artifact: new ArtifactDispatcher(scope, download.artifact) });
Expand All @@ -76,7 +75,6 @@ export class PageDispatcher extends Dispatcher<Page, channels.PageChannel> imple
}));
page.on(Page.Events.FrameAttached, frame => this._onFrameAttached(frame));
page.on(Page.Events.FrameDetached, frame => this._onFrameDetached(frame));
page.on(Page.Events.Load, () => this._dispatchEvent('load'));
page.on(Page.Events.PageError, error => this._dispatchEvent('pageError', { error: serializeError(error) }));
page.on(Page.Events.WebSocket, webSocket => this._dispatchEvent('webSocket', { webSocket: new WebSocketDispatcher(this._scope, webSocket) }));
page.on(Page.Events.Worker, worker => this._dispatchEvent('worker', { worker: new WorkerDispatcher(this._scope, worker) }));
Expand Down
4 changes: 0 additions & 4 deletions packages/playwright-core/src/server/frames.ts
Expand Up @@ -577,10 +577,6 @@ export class Frame extends SdkObject {
this.emit(Frame.Events.AddLifecycle, event);
if (this === mainFrame && this._url !== 'about:blank')
debugLogger.log('api', ` "${event}" event fired`);
if (this === mainFrame && event === 'load')
this._page.emit(Page.Events.Load);
if (this === mainFrame && event === 'domcontentloaded')
this._page.emit(Page.Events.DOMContentLoaded);
}
}
for (const event of this._subtreeLifecycleEvents) {
Expand Down
12 changes: 9 additions & 3 deletions packages/playwright-core/src/server/har/harTracer.ts
Expand Up @@ -19,7 +19,7 @@ import type { APIRequestEvent, APIRequestFinishedEvent } from '../fetch';
import { APIRequestContext } from '../fetch';
import { helper } from '../helper';
import * as network from '../network';
import { Page } from '../page';
import type { Page } from '../page';
import type * as har from './har';
import { calculateSha1, monotonicTime } from '../../utils';
import type { RegisteredListener } from '../../utils/eventsHelper';
Expand All @@ -28,6 +28,8 @@ import { mime } from '../../utilsBundle';
import { ManualPromise } from '../../utils/manualPromise';
import { getPlaywrightVersion } from '../../common/userAgent';
import { urlMatches } from '../../common/netUtils';
import { Frame } from '../frames';
import type { LifecycleEvent } from '../types';

const FALLBACK_HTTP_VERSION = 'HTTP/1.1';

Expand Down Expand Up @@ -93,8 +95,12 @@ export class HarTracer {
private _ensurePageEntry(page: Page) {
let pageEntry = this._pageEntries.get(page);
if (!pageEntry) {
page.on(Page.Events.DOMContentLoaded, () => this._onDOMContentLoaded(page));
page.on(Page.Events.Load, () => this._onLoad(page));
page.mainFrame().on(Frame.Events.AddLifecycle, (event: LifecycleEvent) => {
if (event === 'load')
this._onLoad(page);
if (event === 'domcontentloaded')
this._onDOMContentLoaded(page);
});

pageEntry = {
startedDateTime: new Date(),
Expand Down
2 changes: 0 additions & 2 deletions packages/playwright-core/src/server/page.ts
Expand Up @@ -128,14 +128,12 @@ export class Page extends SdkObject {
Dialog: 'dialog',
Download: 'download',
FileChooser: 'filechooser',
DOMContentLoaded: 'domcontentloaded',
// Can't use just 'error' due to node.js special treatment of error events.
// @see https://nodejs.org/api/events.html#events_error_events
PageError: 'pageerror',
FrameAttached: 'frameattached',
FrameDetached: 'framedetached',
InternalFrameNavigatedToNewDocument: 'internalframenavigatedtonewdocument',
Load: 'load',
ScreencastFrame: 'screencastframe',
Video: 'video',
WebSocket: 'websocket',
Expand Down
4 changes: 2 additions & 2 deletions tests/page/page-autowaiting-basic.spec.ts
Expand Up @@ -193,9 +193,9 @@ it('should work with waitForLoadState(load)', async ({ page, 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('framenavigated').then(() => page.waitForLoadState('domcontentloaded')).then(() => messages.push('domcontentloaded')),
page.waitForEvent('load').then(() => messages.push('load')),
]);
expect(messages.join('|')).toBe('route|domcontentloaded|clickload');
expect(messages.join('|')).toBe('route|load|clickload');
});

it('should work with goto following click', async ({ page, server }) => {
Expand Down

0 comments on commit cdb8627

Please sign in to comment.