From 809002df60ace1c13fdad5e29eb074ac456af7f6 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 29 Jul 2022 12:44:04 -0700 Subject: [PATCH] fix(events): avoid firing lifecycle events twice (#16055) Previously, when some iframe started/finished a new navigation, we could have removed and then re-addded load/domcontentloaded on the main frame. Drive-by: unflake wheel test in Firefox. --- packages/playwright-core/src/server/frames.ts | 12 +++++++++--- tests/page/page-event-load.spec.ts | 2 +- tests/page/wheel.spec.ts | 3 +++ 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/playwright-core/src/server/frames.ts b/packages/playwright-core/src/server/frames.ts index 23542301fe..bc8acb9703 100644 --- a/packages/playwright-core/src/server/frames.ts +++ b/packages/playwright-core/src/server/frames.ts @@ -530,7 +530,7 @@ export class Frame extends SdkObject { _onClearLifecycle() { this._firedLifecycleEvents.clear(); // Recalculate subtree lifecycle for the whole tree - it should not be that big. - this._page.mainFrame()._recalculateLifecycle(); + this._page.mainFrame()._recalculateLifecycle(this); // Keep the current navigation request if any. this._inflightRequests = new Set(Array.from(this._inflightRequests).filter(request => request === this._currentDocument.request)); this._stopNetworkIdleTimer(); @@ -593,10 +593,10 @@ export class Frame extends SdkObject { }); } - _recalculateLifecycle() { + _recalculateLifecycle(frameThatAllowsRemovingLifecycleEvents?: Frame) { const events = new Set(this._firedLifecycleEvents); for (const child of this._childFrames) { - child._recalculateLifecycle(); + child._recalculateLifecycle(frameThatAllowsRemovingLifecycleEvents); // We require a particular lifecycle event to be fired in the whole // frame subtree, and then consider it done. for (const event of events) { @@ -604,6 +604,12 @@ export class Frame extends SdkObject { events.delete(event); } } + if (frameThatAllowsRemovingLifecycleEvents !== this) { + // Usually, lifecycle events are fired once and not removed after that, so we keep existing ones. + // However, when we clear them right before a new commit, this is allowed for a particular frame. + for (const event of this._subtreeLifecycleEvents) + events.add(event); + } const mainFrame = this._page.mainFrame(); for (const event of events) { // Checking whether we have already notified about this event. diff --git a/tests/page/page-event-load.spec.ts b/tests/page/page-event-load.spec.ts index 37181dbc20..ab81e9fc0d 100644 --- a/tests/page/page-event-load.spec.ts +++ b/tests/page/page-event-load.spec.ts @@ -30,7 +30,7 @@ it('should fire once', async ({ page, server, browserName }) => { it('should fire once with iframe navigation', async ({ page, server, browserName }) => { it.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/15086' }); - it.fail(); + it.fixme(browserName === 'firefox', 'Firefox sometimes double fires.'); let requestCount = 0; server.setRoute('/tracker', (_, res) => { diff --git a/tests/page/wheel.spec.ts b/tests/page/wheel.spec.ts index b6df5d909c..8628b12ed6 100644 --- a/tests/page/wheel.spec.ts +++ b/tests/page/wheel.spec.ts @@ -148,6 +148,9 @@ it('should work when the event is canceled', async ({ page }) => { await page.evaluate(() => { document.querySelector('div').addEventListener('wheel', e => e.preventDefault()); }); + // Give wheel listener a chance to propagate through all the layers in Firefox. + for (let i = 0; i < 10; i++) + await page.evaluate(() => new Promise(x => requestAnimationFrame(() => requestAnimationFrame(x)))); await page.mouse.wheel(0, 100); await expectEvent(page, { deltaX: 0,