From 541fa95ce416c6b1e410145dc28d134332c54cec Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Mon, 27 Jan 2020 11:43:43 -0800 Subject: [PATCH] fix(ownerFrame): correctly handle adopted node usecase (#677) --- src/browserContext.ts | 5 +++++ src/chromium/crBrowser.ts | 9 +++++++++ src/chromium/crPage.ts | 8 ++++---- src/dom.ts | 11 ++++++++++- src/firefox/ffBrowser.ts | 9 +++++++++ src/firefox/ffPage.ts | 6 ++---- src/page.ts | 2 +- src/webkit/wkBrowser.ts | 12 ++++++++++++ src/webkit/wkPage.ts | 6 ++---- src/webkit/wkPageProxy.ts | 4 ++++ test/elementhandle.spec.js | 2 +- 11 files changed, 59 insertions(+), 15 deletions(-) diff --git a/src/browserContext.ts b/src/browserContext.ts index 1f7605aca3343..07824d5eac4c0 100644 --- a/src/browserContext.ts +++ b/src/browserContext.ts @@ -22,6 +22,7 @@ import { helper } from './helper'; export interface BrowserContextDelegate { pages(): Promise; + existingPages(): Page[]; newPage(): Promise; close(): Promise; @@ -69,6 +70,10 @@ export class BrowserContext { await this.setGeolocation(this._options.geolocation); } + _existingPages(): Page[] { + return this._delegate.existingPages(); + } + async pages(): Promise { return this._delegate.pages(); } diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index ed8b0ae180bd4..97034c165e274 100644 --- 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 => { 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 index 14e9f9696f38f..a1b8c6e2c6b95 100644 --- 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 { + async getOwnerFrame(handle: dom.ElementHandle): Promise { // 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 index afc63ab7f3bc8..f5772a4b5d017 100644 --- a/src/dom.ts +++ b/src/dom.ts @@ -135,7 +135,16 @@ export class ElementHandle extends js.JSHandle { } async ownerFrame(): Promise { - 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 { diff --git a/src/firefox/ffBrowser.ts b/src/firefox/ffBrowser.ts index 79cba88427858..c46814a76c227 100644 --- 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 => { const {targetId} = await this._connection.send('Target.newPage', { browserContextId: browserContextId || undefined diff --git a/src/firefox/ffPage.ts b/src/firefox/ffPage.ts index 3996d69716f69..726acc930aa79 100644 --- 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 { + async getOwnerFrame(handle: dom.ElementHandle): Promise { 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 index 951b66b95a1bf..84a3a551e6da8 100644 --- a/src/page.ts +++ b/src/page.ts @@ -62,7 +62,7 @@ export interface PageDelegate { isElementHandle(remoteObject: any): boolean; adoptElementHandle(handle: dom.ElementHandle, to: dom.FrameExecutionContext): Promise>; getContentFrame(handle: dom.ElementHandle): Promise; // Only called for frame owner elements. - getOwnerFrame(handle: dom.ElementHandle): Promise; + getOwnerFrame(handle: dom.ElementHandle): Promise; // Returns frameId. getContentQuads(handle: dom.ElementHandle): Promise; layoutViewport(): Promise<{ width: number, height: number }>; setInputFiles(handle: dom.ElementHandle, files: types.FilePayload[]): Promise; diff --git a/src/webkit/wkBrowser.ts b/src/webkit/wkBrowser.ts index deded6e18b6d1..d9ec96f3a1690 100644 --- 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 => { 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 index a47289d6fd029..730ff3301a596 100644 --- 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 { + async getOwnerFrame(handle: dom.ElementHandle): Promise { 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 index 7e1aa24a5514c..c1cda05b9632f 100644 --- 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 index 8cfd2b57ac314..d45a8bd42427e 100644 --- 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'),