fix(ownerFrame): correctly handle adopted node usecase (#677)

microsoft · Jan 27, 2020 · 541fa95 · 541fa95
1 parent b3cd7a4
commit 541fa95
Show file tree

Hide file tree

Showing 11 changed files with 59 additions and 15 deletions.
diff --git a/src/browserContext.ts b/src/browserContext.ts
@@ -22,6 +22,7 @@ import { helper } from './helper';
 
 export interface BrowserContextDelegate {
   pages(): Promise<Page[]>;
+  existingPages(): Page[];
   newPage(): Promise<Page>;
   close(): Promise<void>;
 
@@ -69,6 +70,10 @@ export class BrowserContext {
       await this.setGeolocation(this._options.geolocation);
   }
 
+  _existingPages(): Page[] {
+    return this._delegate.existingPages();
+  }
+
   async pages(): Promise<Page[]> {
     return this._delegate.pages();
   }

diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts
@@ -74,6 +74,15 @@ export class CRBrowser extends platform.EventEmitter implements Browser {
         return pages.filter(page => !!page) as Page[];
       },
 
+      existingPages: (): Page[] => {
+        const pages: Page[] = [];
+        for (const target of this._allTargets()) {
+          if (target.browserContext() === context && target._crPage)
+            pages.push(target._crPage.page());
+        }
+        return pages;
+      },
+
       newPage: async (): Promise<Page> => {
         const { targetId } = await this._client.send('Target.createTarget', { url: 'about:blank', browserContextId: contextId || undefined });
         const target = this._targets.get(targetId)!;

diff --git a/src/chromium/crPage.ts b/src/chromium/crPage.ts
@@ -424,7 +424,7 @@ export class CRPage implements PageDelegate {
     return this._page._frameManager.frame(nodeInfo.node.frameId);
   }
 
-  async getOwnerFrame(handle: dom.ElementHandle): Promise<frames.Frame | null> {
+  async getOwnerFrame(handle: dom.ElementHandle): Promise<string | null> {
     // document.documentElement has frameId of the owner frame.
     const documentElement = await handle.evaluateHandle(node => {
       const doc = node as Document;
@@ -440,10 +440,10 @@ export class CRPage implements PageDelegate {
     const nodeInfo = await this._client.send('DOM.describeNode', {
       objectId: remoteObject.objectId
     });
-    const frame = nodeInfo && typeof nodeInfo.node.frameId === 'string' ?
-      this._page._frameManager.frame(nodeInfo.node.frameId) : null;
+    const frameId = nodeInfo && typeof nodeInfo.node.frameId === 'string' ?
+      nodeInfo.node.frameId : null;
     await documentElement.dispose();
-    return frame;
+    return frameId;
   }
 
   isElementHandle(remoteObject: any): boolean {

diff --git a/src/dom.ts b/src/dom.ts
@@ -135,7 +135,16 @@ export class ElementHandle<T extends Node = Node> extends js.JSHandle<T> {
   }
 
   async ownerFrame(): Promise<frames.Frame | null> {
-    return this._page._delegate.getOwnerFrame(this);
+    const frameId = await this._page._delegate.getOwnerFrame(this);
+    if (!frameId)
+      return null;
+    const pages = this._page.browserContext()._existingPages();
+    for (const page of pages) {
+      const frame = page._frameManager.frame(frameId);
+      if (frame)
+        return frame;
+    }
+    return null;
   }
 
   async contentFrame(): Promise<frames.Frame | null> {

diff --git a/src/firefox/ffBrowser.ts b/src/firefox/ffBrowser.ts
@@ -164,6 +164,15 @@ export class FFBrowser extends platform.EventEmitter implements Browser {
         return pages.filter(page => !!page);
       },
 
+      existingPages: (): Page[] => {
+        const pages: Page[] = [];
+        for (const target of this._allTargets()) {
+          if (target.browserContext() === context && target._ffPage)
+            pages.push(target._ffPage._page);
+        }
+        return pages;
+      },
+
       newPage: async (): Promise<Page> => {
         const {targetId} = await this._connection.send('Target.newPage', {
           browserContextId: browserContextId || undefined

diff --git a/src/firefox/ffPage.ts b/src/firefox/ffPage.ts
@@ -373,14 +373,12 @@ export class FFPage implements PageDelegate {
     return this._page._frameManager.frame(contentFrameId);
   }
 
-  async getOwnerFrame(handle: dom.ElementHandle): Promise<frames.Frame | null> {
+  async getOwnerFrame(handle: dom.ElementHandle): Promise<string | null> {
     const { ownerFrameId } = await this._session.send('Page.describeNode', {
       frameId: handle._context.frame._id,
       objectId: toRemoteObject(handle).objectId!,
     });
-    if (!ownerFrameId)
-      return null;
-    return this._page._frameManager.frame(ownerFrameId);
+    return ownerFrameId || null;
   }
 
   isElementHandle(remoteObject: any): boolean {

diff --git a/src/page.ts b/src/page.ts
@@ -62,7 +62,7 @@ export interface PageDelegate {
   isElementHandle(remoteObject: any): boolean;
   adoptElementHandle<T extends Node>(handle: dom.ElementHandle<T>, to: dom.FrameExecutionContext): Promise<dom.ElementHandle<T>>;
   getContentFrame(handle: dom.ElementHandle): Promise<frames.Frame | null>;  // Only called for frame owner elements.
-  getOwnerFrame(handle: dom.ElementHandle): Promise<frames.Frame | null>;
+  getOwnerFrame(handle: dom.ElementHandle): Promise<string | null>; // Returns frameId.
   getContentQuads(handle: dom.ElementHandle): Promise<types.Quad[] | null>;
   layoutViewport(): Promise<{ width: number, height: number }>;
   setInputFiles(handle: dom.ElementHandle<HTMLInputElement>, files: types.FilePayload[]): Promise<void>;

diff --git a/src/webkit/wkBrowser.ts b/src/webkit/wkBrowser.ts
@@ -168,6 +168,18 @@ export class WKBrowser extends platform.EventEmitter implements Browser {
         return await Promise.all(pageProxies.map(proxy => proxy.page()));
       },
 
+      existingPages: (): Page[] => {
+        const pages: Page[] = [];
+        for (const pageProxy of this._pageProxies.values()) {
+          if (pageProxy._browserContext !== context)
+            continue;
+          const page = pageProxy.existingPage();
+          if (page)
+            pages.push(page);
+        }
+        return pages;
+      },
+
       newPage: async (): Promise<Page> => {
         const { pageProxyId } = await this._browserSession.send('Browser.createPage', { browserContextId });
         const pageProxy = this._pageProxies.get(pageProxyId)!;

diff --git a/src/webkit/wkPage.ts b/src/webkit/wkPage.ts
@@ -484,16 +484,14 @@ export class WKPage implements PageDelegate {
     return this._page._frameManager.frame(nodeInfo.contentFrameId);
   }
 
-  async getOwnerFrame(handle: dom.ElementHandle): Promise<frames.Frame | null> {
+  async getOwnerFrame(handle: dom.ElementHandle): Promise<string | null> {
     const remoteObject = toRemoteObject(handle);
     if (!remoteObject.objectId)
       return null;
     const nodeInfo = await this._session.send('DOM.describeNode', {
       objectId: remoteObject.objectId
     });
-    if (!nodeInfo.ownerFrameId)
-      return null;
-    return this._page._frameManager.frame(nodeInfo.ownerFrameId);
+    return nodeInfo.ownerFrameId || null;
   }
 
   isElementHandle(remoteObject: any): boolean {

diff --git a/src/webkit/wkPageProxy.ts b/src/webkit/wkPageProxy.ts
@@ -99,6 +99,10 @@ export class WKPageProxy {
     return this._pagePromise;
   }
 
+  existingPage(): Page | undefined {
+    return this._wkPage ? this._wkPage._page : undefined;
+  }
+
   onPopupCreated(popupPageProxy: WKPageProxy) {
     const wkPage = this._wkPage;
     if (!wkPage || !wkPage._page.listenerCount(Events.Page.Popup))

diff --git a/test/elementhandle.spec.js b/test/elementhandle.spec.js
@@ -155,7 +155,7 @@ module.exports.describe = function({testRunner, expect, FFOX, CHROMIUM, WEBKIT})
       });
       expect(await divHandle.ownerFrame()).toBe(page.mainFrame());
     });
-    xit('should work for adopted elements', async({page,server}) => {
+    it.skip(FFOX)('should work for adopted elements', async({page,server}) => {
       await page.goto(server.EMPTY_PAGE);
       const [popup] = await Promise.all([
         page.waitForEvent('popup'),