fix: only check loading iframe in lifecycling (#8348)

puppeteer · May 17, 2022 · 7438030 · 7438030
1 parent 3ef02a3
commit 7438030
Show file tree

Hide file tree

Showing 6 changed files with 61 additions and 2 deletions.
diff --git a/src/common/FrameManager.ts b/src/common/FrameManager.ts
@@ -108,6 +108,9 @@ export class FrameManager extends EventEmitter {
         );
       }
     );
+    session.on('Page.frameStartedLoading', (event) => {
+      this._onFrameStartedLoading(event.frameId);
+    });
     session.on('Page.frameStoppedLoading', (event) => {
       this._onFrameStoppedLoading(event.frameId);
     });
@@ -287,6 +290,12 @@ export class FrameManager extends EventEmitter {
     this.emit(FrameManagerEmittedEvents.LifecycleEvent, frame);
   }
 
+  _onFrameStartedLoading(frameId: string): void {
+    const frame = this._frames.get(frameId);
+    if (!frame) return;
+    frame._onLoadingStarted();
+  }
+
   _onFrameStoppedLoading(frameId: string): void {
     const frame = this._frames.get(frameId);
     if (!frame) return;
@@ -650,6 +659,10 @@ export class Frame {
    * @internal
    */
   _name?: string;
+  /**
+   * @internal
+   */
+  _hasStartedLoading = false;
 
   /**
    * @internal
@@ -1416,6 +1429,13 @@ export class Frame {
     this._lifecycleEvents.add('load');
   }
 
+  /**
+   * @internal
+   */
+  _onLoadingStarted(): void {
+    this._hasStartedLoading = true;
+  }
+
   /**
    * @internal
    */

diff --git a/src/common/LifecycleWatcher.ts b/src/common/LifecycleWatcher.ts
@@ -255,7 +255,12 @@ export class LifecycleWatcher {
         if (!frame._lifecycleEvents.has(event)) return false;
       }
       for (const child of frame.childFrames()) {
-        if (!checkLifecycle(child, expectedLifecycle)) return false;
+        if (
+          child._hasStartedLoading &&
+          !checkLifecycle(child, expectedLifecycle)
+        ) {
+          return false;
+        }
       }
       return true;
     }

diff --git a/test/assets/frames/lazy-frame.html b/test/assets/frames/lazy-frame.html
@@ -0,0 +1,3 @@
+<iframe width="100%" height="300" src="about:blank"></iframe>
+<div style="height: 800vh"></div>
+<iframe width="100%" height="300" src='./frame.html' loading="lazy"></iframe>
diff --git a/test/assets/lazy-oopif-frame.html b/test/assets/lazy-oopif-frame.html
@@ -0,0 +1,3 @@
+<iframe width="100%" height="300" src="about:blank"></iframe>
+<div style="height: 800vh"></div>
+<iframe width="100%" height="300" src='www.example.com' loading="lazy"></iframe>
diff --git a/test/frame.spec.ts b/test/frame.spec.ts
@@ -278,6 +278,18 @@ describe('Frame specs', function () {
         server.PREFIX + '/frames/frame.html?param=value#fragment'
       );
     });
+    itFailsFirefox('should support lazy frames', async () => {
+      const { page, server } = getTestState();
+
+      await page.setViewport({ width: 1000, height: 1000 });
+      await page.goto(server.PREFIX + '/frames/lazy-frame.html');
+
+      expect(page.frames().map((frame) => frame._hasStartedLoading)).toEqual([
+        true,
+        true,
+        false,
+      ]);
+    });
   });
 
   describe('Frame.client', function () {

diff --git a/test/oopif.spec.ts b/test/oopif.spec.ts
@@ -16,7 +16,11 @@
 
 import utils from './utils.js';
 import expect from 'expect';
-import { getTestState, describeChromeOnly } from './mocha-utils'; // eslint-disable-line import/extensions
+import {
+  getTestState,
+  describeChromeOnly,
+  itFailsFirefox,
+} from './mocha-utils'; // eslint-disable-line import/extensions
 
 describeChromeOnly('OOPIF', function () {
   /* We use a special browser for this test as we need the --site-per-process flag */
@@ -386,6 +390,18 @@ describeChromeOnly('OOPIF', function () {
     await target.page();
     browser1.disconnect();
   });
+  itFailsFirefox('should support lazy OOP frames', async () => {
+    const { server } = getTestState();
+
+    await page.goto(server.PREFIX + '/lazy-oopif-frame.html');
+    await page.setViewport({ width: 1000, height: 1000 });
+
+    expect(page.frames().map((frame) => frame._hasStartedLoading)).toEqual([
+      true,
+      true,
+      false,
+    ]);
+  });
 
   describe('waitForFrame', () => {
     it('should resolve immediately if the frame already exists', async () => {