From 89d81eff88581c7353b23e585ebde2946bccccfe Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Thu, 16 May 2019 19:32:51 -0400 Subject: [PATCH 01/19] Bring over IFrameElements gatherer and add types. --- .../gather/gatherers/iframe-elements.js | 134 ++++++++++++++++++ types/artifacts.d.ts | 17 +++ 2 files changed, 151 insertions(+) create mode 100644 lighthouse-core/gather/gatherers/iframe-elements.js diff --git a/lighthouse-core/gather/gatherers/iframe-elements.js b/lighthouse-core/gather/gatherers/iframe-elements.js new file mode 100644 index 000000000000..0745cea60ea5 --- /dev/null +++ b/lighthouse-core/gather/gatherers/iframe-elements.js @@ -0,0 +1,134 @@ +/** + * @license Copyright 2019 Google Inc. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +const Gatherer = require('./gatherer.js'); +const pageFunctions = require('../../lib/page-functions.js'); + +/* eslint-env browser, node */ + +/** + * TODO(jburger): Move to page-functions.js once integrated with Lighthouse. + * @param {HTMLElement} element + * @param {String} attr + * @return {String} + */ +function getStyleAttrValue(element, attr) { + // Check style before computedStyle as computedStyle is expensive. + // @ts-ignore + return element.style[attr] || window.getComputedStyle(element)[attr]; +} + +/** + * TODO(jburger): Move to page-functions.js once integrated with Lighthouse. + * @param {HTMLElement} element + * @return {Boolean} + */ +function hasScrollableAncestor(element) { + let currentEl = element.parentElement; + while (currentEl) { + if (currentEl.scrollHeight > currentEl.clientHeight) { + const yScroll = getStyleAttrValue(currentEl, 'overflowY'); + if (yScroll) { + return true; + } + } + currentEl = currentEl.parentElement; + } + return false; +} + +/** + * TODO(jburger): Move to page-functions.js once integrated with Lighthouse. + * @param {?HTMLElement} element + * @return {Boolean} + */ +function isFixed(element) { + let currentEl = element; + while (currentEl) { + const position = getStyleAttrValue(currentEl, 'position'); + // Only truly fixed if an ancestor is scrollable. + if (position === 'fixed' && hasScrollableAncestor(currentEl)) { + return true; + } + currentEl = currentEl.parentElement; + } + return false; +} + +/** + * TODO(jburger): Move to page-functions.js once integrated with Lighthouse. + * @param {HTMLIFrameElement} element + * @return {DOMRect | ClientRect} + */ +function getClientRect(element) { + return element.getBoundingClientRect(); +} +/** + * @return {LH.Artifacts['IFrameElements']} + */ +function collectIFrameElements() { + // @ts-ignore - put into scope via stringification + const iFrameElements = getElementsInDocument('iframe'); // eslint-disable-line no-undef + return iFrameElements.map(/** @param {HTMLIFrameElement} node */ (node) => { + // @ts-ignore + const clientRect = getClientRect(node).toJSON(); + // Marking 1x1 as non-visible to ignore tracking pixels. + const isVisible = (clientRect.width > 1 && clientRect.height > 1); + return { + id: node.id, + src: node.src, + clientRect, + isVisible, + isFixed: isVisible && isFixed(node), + }; + }); +} + +/** @inheritdoc */ +class IFrameElements extends Gatherer { + /** + * @param {LH.Gatherer.PassContext} passContext + * @return {Promise} + * @override + */ + async afterPass(passContext) { + const driver = passContext.driver; + + const {frameTree} = await driver.sendCommand('Page.getFrameTree'); + const framesByDomId = new Map(); + if (frameTree.childFrames) { + for (const {frame} of frameTree.childFrames) { + if (framesByDomId.has(frame.name)) { + // DOM ID collision, mark it as null. + framesByDomId.set(frame.name, null); + } else { + framesByDomId.set(frame.name, frame); + } + } + } + + const expression = `(() => { + ${pageFunctions.getOuterHTMLSnippetString}; + ${pageFunctions.getElementsInDocumentString}; + ${getClientRect}; + ${getStyleAttrValue}; + ${hasScrollableAncestor}; + ${isFixed}; + return (${collectIFrameElements})(); + })()`; + + /** @type {LH.Artifacts['IFrameElements']} */ + const elements = + await driver.evaluateAsync(expression, {useIsolation: true}); + for (const el of elements) { + el.frame = framesByDomId.get(el.id); + } + return elements; + } +} + +module.exports = IFrameElements; diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index f28e7b1dc24b..92c8f9e6e351 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -59,6 +59,8 @@ declare global { export interface PublicGathererArtifacts { /** Console deprecation and intervention warnings logged by Chrome during page load. */ ConsoleMessages: Crdp.Log.EntryAddedEvent[]; + /** All the IFrame elements in the page.*/ + IFrameElements: Artifacts.IFrameElement[]; /** Information on size and loading for all the images in the page. Natural size information for `picture` and CSS images is only available if the image was one of the largest 50 images. */ ImageElements: Artifacts.ImageElement[]; /** All the link elements on the page or equivalently declared in `Link` headers. @see https://html.spec.whatwg.org/multipage/links.html */ @@ -179,6 +181,21 @@ declare global { params: {name: string; value: string}[]; } + export interface IFrameElement { + /** The `id` attribute of the IFrame. */ + id: string, + /** The `src` attribute of the IFrame. */ + src: string, + /** The IFrame's ClientRect. @see https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect */ + clientRect: ClientRect, + /** The visibility of the IFrame. */ + isVisible: boolean, + /** If the IFrame or an ancestor of the IFrame is fixed in position. */ + isFixed: boolean, + /** The Frame of the IFrame. @see https://chromedevtools.github.io/devtools-protocol/tot/Page#type-Frame */ + frame: Crdp.Page.Frame, + } + /** @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link#Attributes */ export interface LinkElement { /** The `rel` attribute of the link, normalized to lower case. @see https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types */ From 499c599d0d9f4eab971f454dc40d284d83b1df40 Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Fri, 17 May 2019 09:33:24 -0400 Subject: [PATCH 02/19] Move page functions to `.../lib/page-functions.js` --- .../gather/gatherers/iframe-elements.js | 66 ++----------------- lighthouse-core/lib/page-functions.js | 61 +++++++++++++++++ 2 files changed, 66 insertions(+), 61 deletions(-) diff --git a/lighthouse-core/gather/gatherers/iframe-elements.js b/lighthouse-core/gather/gatherers/iframe-elements.js index 0745cea60ea5..20b1ae02c6ee 100644 --- a/lighthouse-core/gather/gatherers/iframe-elements.js +++ b/lighthouse-core/gather/gatherers/iframe-elements.js @@ -10,63 +10,6 @@ const pageFunctions = require('../../lib/page-functions.js'); /* eslint-env browser, node */ -/** - * TODO(jburger): Move to page-functions.js once integrated with Lighthouse. - * @param {HTMLElement} element - * @param {String} attr - * @return {String} - */ -function getStyleAttrValue(element, attr) { - // Check style before computedStyle as computedStyle is expensive. - // @ts-ignore - return element.style[attr] || window.getComputedStyle(element)[attr]; -} - -/** - * TODO(jburger): Move to page-functions.js once integrated with Lighthouse. - * @param {HTMLElement} element - * @return {Boolean} - */ -function hasScrollableAncestor(element) { - let currentEl = element.parentElement; - while (currentEl) { - if (currentEl.scrollHeight > currentEl.clientHeight) { - const yScroll = getStyleAttrValue(currentEl, 'overflowY'); - if (yScroll) { - return true; - } - } - currentEl = currentEl.parentElement; - } - return false; -} - -/** - * TODO(jburger): Move to page-functions.js once integrated with Lighthouse. - * @param {?HTMLElement} element - * @return {Boolean} - */ -function isFixed(element) { - let currentEl = element; - while (currentEl) { - const position = getStyleAttrValue(currentEl, 'position'); - // Only truly fixed if an ancestor is scrollable. - if (position === 'fixed' && hasScrollableAncestor(currentEl)) { - return true; - } - currentEl = currentEl.parentElement; - } - return false; -} - -/** - * TODO(jburger): Move to page-functions.js once integrated with Lighthouse. - * @param {HTMLIFrameElement} element - * @return {DOMRect | ClientRect} - */ -function getClientRect(element) { - return element.getBoundingClientRect(); -} /** * @return {LH.Artifacts['IFrameElements']} */ @@ -83,6 +26,7 @@ function collectIFrameElements() { src: node.src, clientRect, isVisible, + // @ts-ignore isFixed: isVisible && isFixed(node), }; }); @@ -114,10 +58,10 @@ class IFrameElements extends Gatherer { const expression = `(() => { ${pageFunctions.getOuterHTMLSnippetString}; ${pageFunctions.getElementsInDocumentString}; - ${getClientRect}; - ${getStyleAttrValue}; - ${hasScrollableAncestor}; - ${isFixed}; + ${pageFunctions.getClientRectString}; + ${pageFunctions.getStyleAttrValueString}; + ${pageFunctions.hasScrollableAncestorString}; + ${pageFunctions.isFixedString}; return (${collectIFrameElements})(); })()`; diff --git a/lighthouse-core/lib/page-functions.js b/lighthouse-core/lib/page-functions.js index 9e9b79d69304..f393eeb971e0 100644 --- a/lighthouse-core/lib/page-functions.js +++ b/lighthouse-core/lib/page-functions.js @@ -218,6 +218,63 @@ function getNodeSelector(node) { return parts.join(' > '); } +/** + * @param {HTMLElement} element + * @param {String} attr + * @return {String} + */ +/* istanbul ignore next */ +function getStyleAttrValue(element, attr) { + // Check style before computedStyle as computedStyle is expensive. + return element.style[attr] || window.getComputedStyle(element)[attr]; +} + +/** + * @param {HTMLElement} element + * @return {Boolean} + */ +/* istanbul ignore next */ +function hasScrollableAncestor(element) { + let currentEl = element.parentElement; + while (currentEl) { + if (currentEl.scrollHeight > currentEl.clientHeight) { + const yScroll = getStyleAttrValue(currentEl, 'overflowY'); + if (yScroll) { + return true; + } + } + currentEl = currentEl.parentElement; + } + return false; +} + +/** + * @param {?HTMLElement} element + * @return {Boolean} + */ +/* istanbul ignore next */ +function isFixed(element) { + let currentEl = element; + while (currentEl) { + const position = getStyleAttrValue(currentEl, 'position'); + // Only truly fixed if an ancestor is scrollable. + if (position === 'fixed' && hasScrollableAncestor(currentEl)) { + return true; + } + currentEl = currentEl.parentElement; + } + return false; +} + +/** + * @param {HTMLIFrameElement} element + * @return {DOMRect | ClientRect} + */ +/* istanbul ignore next */ +function getClientRect(element) { + return element.getBoundingClientRect(); +} + module.exports = { wrapRuntimeEvalErrorInBrowserString: wrapRuntimeEvalErrorInBrowser.toString(), registerPerformanceObserverInPageString: registerPerformanceObserverInPage.toString(), @@ -230,4 +287,8 @@ module.exports = { getNodePathString: getNodePath.toString(), getNodeSelectorString: getNodeSelector.toString(), getNodeSelector: getNodeSelector, + getStyleAttrValueString: getStyleAttrValue.toString(), + hasScrollableAncestorString: hasScrollableAncestor.toString(), + isFixedString: isFixed.toString(), + getClientRectString: getClientRect.toString(), }; From 4abd4dc1e9f92f2fdc5b2e9b8ceb669ce449a208 Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Fri, 17 May 2019 09:59:28 -0400 Subject: [PATCH 03/19] Update default config to include IFrameElements gatherer in defaultPass. --- lighthouse-core/config/default-config.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lighthouse-core/config/default-config.js b/lighthouse-core/config/default-config.js index d309072f7aa8..e7d7d6b1d2bc 100644 --- a/lighthouse-core/config/default-config.js +++ b/lighthouse-core/config/default-config.js @@ -124,6 +124,7 @@ const defaultConfig = { 'link-elements', 'meta-elements', 'script-elements', + 'iframe-elements', 'dobetterweb/appcache', 'dobetterweb/doctype', 'dobetterweb/domstats', From 3ee62eebe6d4f1ad2719239dfe79bfb904d5d8cd Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Fri, 17 May 2019 10:44:42 -0400 Subject: [PATCH 04/19] Resolve no-undef lint errors. --- lighthouse-core/gather/gatherers/iframe-elements.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/gather/gatherers/iframe-elements.js b/lighthouse-core/gather/gatherers/iframe-elements.js index 20b1ae02c6ee..46811f2cea5c 100644 --- a/lighthouse-core/gather/gatherers/iframe-elements.js +++ b/lighthouse-core/gather/gatherers/iframe-elements.js @@ -13,12 +13,13 @@ const pageFunctions = require('../../lib/page-functions.js'); /** * @return {LH.Artifacts['IFrameElements']} */ +/* istanbul ignore next */ function collectIFrameElements() { // @ts-ignore - put into scope via stringification const iFrameElements = getElementsInDocument('iframe'); // eslint-disable-line no-undef return iFrameElements.map(/** @param {HTMLIFrameElement} node */ (node) => { // @ts-ignore - const clientRect = getClientRect(node).toJSON(); + const clientRect = getClientRect(node).toJSON(); // eslint-disable-line no-undef // Marking 1x1 as non-visible to ignore tracking pixels. const isVisible = (clientRect.width > 1 && clientRect.height > 1); return { @@ -27,7 +28,7 @@ function collectIFrameElements() { clientRect, isVisible, // @ts-ignore - isFixed: isVisible && isFixed(node), + isFixed: isVisible && isFixed(node), // eslint-disable-line no-undef }; }); } From ce2e280108eb333c78de3c2d6e025b1fb1b7fbe0 Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Fri, 17 May 2019 13:55:31 -0400 Subject: [PATCH 05/19] Address comments. --- .../gather/gatherers/iframe-elements.js | 14 ++-- lighthouse-core/lib/page-functions.js | 70 ++++++++----------- types/artifacts.d.ts | 2 +- 3 files changed, 34 insertions(+), 52 deletions(-) diff --git a/lighthouse-core/gather/gatherers/iframe-elements.js b/lighthouse-core/gather/gatherers/iframe-elements.js index 46811f2cea5c..de7233df60f0 100644 --- a/lighthouse-core/gather/gatherers/iframe-elements.js +++ b/lighthouse-core/gather/gatherers/iframe-elements.js @@ -18,8 +18,8 @@ function collectIFrameElements() { // @ts-ignore - put into scope via stringification const iFrameElements = getElementsInDocument('iframe'); // eslint-disable-line no-undef return iFrameElements.map(/** @param {HTMLIFrameElement} node */ (node) => { - // @ts-ignore - const clientRect = getClientRect(node).toJSON(); // eslint-disable-line no-undef + // @ts-ignore - put into scope via stringification + const clientRect = node.getBoundingClientRect().toJSON(); // eslint-disable-line no-undef // Marking 1x1 as non-visible to ignore tracking pixels. const isVisible = (clientRect.width > 1 && clientRect.height > 1); return { @@ -27,13 +27,12 @@ function collectIFrameElements() { src: node.src, clientRect, isVisible, - // @ts-ignore - isFixed: isVisible && isFixed(node), // eslint-disable-line no-undef + // @ts-ignore - put into scope via stringification + isPositionFixed: isVisible && isPositionFixed(node), // eslint-disable-line no-undef }; }); } -/** @inheritdoc */ class IFrameElements extends Gatherer { /** * @param {LH.Gatherer.PassContext} passContext @@ -59,10 +58,7 @@ class IFrameElements extends Gatherer { const expression = `(() => { ${pageFunctions.getOuterHTMLSnippetString}; ${pageFunctions.getElementsInDocumentString}; - ${pageFunctions.getClientRectString}; - ${pageFunctions.getStyleAttrValueString}; - ${pageFunctions.hasScrollableAncestorString}; - ${pageFunctions.isFixedString}; + ${pageFunctions.isPositionFixedString}; return (${collectIFrameElements})(); })()`; diff --git a/lighthouse-core/lib/page-functions.js b/lighthouse-core/lib/page-functions.js index f393eeb971e0..7ed373ef342f 100644 --- a/lighthouse-core/lib/page-functions.js +++ b/lighthouse-core/lib/page-functions.js @@ -219,41 +219,39 @@ function getNodeSelector(node) { } /** - * @param {HTMLElement} element - * @param {String} attr - * @return {String} - */ -/* istanbul ignore next */ -function getStyleAttrValue(element, attr) { - // Check style before computedStyle as computedStyle is expensive. - return element.style[attr] || window.getComputedStyle(element)[attr]; -} - -/** - * @param {HTMLElement} element - * @return {Boolean} + * @param {?HTMLElement} element + * @return {boolean} */ /* istanbul ignore next */ -function hasScrollableAncestor(element) { - let currentEl = element.parentElement; - while (currentEl) { - if (currentEl.scrollHeight > currentEl.clientHeight) { - const yScroll = getStyleAttrValue(currentEl, 'overflowY'); - if (yScroll) { - return true; +function isPositionFixed(element) { + /** + * @param {HTMLElement} element + * @return {boolean} + */ + function hasScrollableAncestor(element) { + let currentEl = element.parentElement; + while (currentEl) { + if (currentEl.scrollHeight > currentEl.clientHeight) { + const yScroll = getStyleAttrValue(currentEl, 'overflowY'); + if (yScroll) { + return true; + } } + currentEl = currentEl.parentElement; } - currentEl = currentEl.parentElement; + return false; + } + + /** + * @param {HTMLElement} element + * @param {string} attr + * @return {string} + */ + function getStyleAttrValue(element, attr) { + // Check style before computedStyle as computedStyle is expensive. + return element.style[attr] || window.getComputedStyle(element)[attr]; } - return false; -} -/** - * @param {?HTMLElement} element - * @return {Boolean} - */ -/* istanbul ignore next */ -function isFixed(element) { let currentEl = element; while (currentEl) { const position = getStyleAttrValue(currentEl, 'position'); @@ -266,15 +264,6 @@ function isFixed(element) { return false; } -/** - * @param {HTMLIFrameElement} element - * @return {DOMRect | ClientRect} - */ -/* istanbul ignore next */ -function getClientRect(element) { - return element.getBoundingClientRect(); -} - module.exports = { wrapRuntimeEvalErrorInBrowserString: wrapRuntimeEvalErrorInBrowser.toString(), registerPerformanceObserverInPageString: registerPerformanceObserverInPage.toString(), @@ -287,8 +276,5 @@ module.exports = { getNodePathString: getNodePath.toString(), getNodeSelectorString: getNodeSelector.toString(), getNodeSelector: getNodeSelector, - getStyleAttrValueString: getStyleAttrValue.toString(), - hasScrollableAncestorString: hasScrollableAncestor.toString(), - isFixedString: isFixed.toString(), - getClientRectString: getClientRect.toString(), + isPositionFixedString: isPositionFixed.toString(), }; diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 92c8f9e6e351..05468c3879e2 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -191,7 +191,7 @@ declare global { /** The visibility of the IFrame. */ isVisible: boolean, /** If the IFrame or an ancestor of the IFrame is fixed in position. */ - isFixed: boolean, + isPositionFixed: boolean, /** The Frame of the IFrame. @see https://chromedevtools.github.io/devtools-protocol/tot/Page#type-Frame */ frame: Crdp.Page.Frame, } From 2642fa987f95178895a24a44ac4e8d204615920f Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Tue, 21 May 2019 10:27:15 -0400 Subject: [PATCH 06/19] Update `index-test.js.snap` snapshot. --- lighthouse-cli/test/cli/__snapshots__/index-test.js.snap | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap b/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap index d2aa1fbba196..555e3ddbd67a 100644 --- a/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap +++ b/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap @@ -1129,6 +1129,9 @@ Object { Object { "path": "script-elements", }, + Object { + "path": "iframe-elements", + }, Object { "path": "dobetterweb/appcache", }, From 76e3e1d7ef3befc41e353cdb2acd9852af98532e Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Tue, 21 May 2019 17:05:31 -0400 Subject: [PATCH 07/19] Address numerous comments. --- .../gather/gatherers/iframe-elements.js | 17 ++++++------- lighthouse-core/lib/page-functions.js | 7 ++++-- types/artifacts.d.ts | 25 ++++++++++++------- 3 files changed, 29 insertions(+), 20 deletions(-) diff --git a/lighthouse-core/gather/gatherers/iframe-elements.js b/lighthouse-core/gather/gatherers/iframe-elements.js index de7233df60f0..f077028b0d2d 100644 --- a/lighthouse-core/gather/gatherers/iframe-elements.js +++ b/lighthouse-core/gather/gatherers/iframe-elements.js @@ -18,15 +18,15 @@ function collectIFrameElements() { // @ts-ignore - put into scope via stringification const iFrameElements = getElementsInDocument('iframe'); // eslint-disable-line no-undef return iFrameElements.map(/** @param {HTMLIFrameElement} node */ (node) => { - // @ts-ignore - put into scope via stringification - const clientRect = node.getBoundingClientRect().toJSON(); // eslint-disable-line no-undef + const clientRect = node.getBoundingClientRect(); // Marking 1x1 as non-visible to ignore tracking pixels. - const isVisible = (clientRect.width > 1 && clientRect.height > 1); + + const {top, bottom, left, right, width, height} = clientRect; return { id: node.id, src: node.src, - clientRect, - isVisible, + clientRect: {top, bottom, left, right, width, height}, + pixelArea: width * height, // @ts-ignore - put into scope via stringification isPositionFixed: isVisible && isPositionFixed(node), // eslint-disable-line no-undef }; @@ -63,12 +63,11 @@ class IFrameElements extends Gatherer { })()`; /** @type {LH.Artifacts['IFrameElements']} */ - const elements = - await driver.evaluateAsync(expression, {useIsolation: true}); - for (const el of elements) { + const iframeElements = await driver.evaluateAsync(expression, {useIsolation: true}); + for (const el of iframeElements) { el.frame = framesByDomId.get(el.id); } - return elements; + return iframeElements; } } diff --git a/lighthouse-core/lib/page-functions.js b/lighthouse-core/lib/page-functions.js index 7ed373ef342f..35320ecf7498 100644 --- a/lighthouse-core/lib/page-functions.js +++ b/lighthouse-core/lib/page-functions.js @@ -219,7 +219,10 @@ function getNodeSelector(node) { } /** - * @param {?HTMLElement} element + * This function checks if an element or an ancestor of an element is `position:fixed`. + * In addition we ensure that the element is capable of behaving as a `position:fixed` + * element, checking that it lives within a scrollable ancestor. + * @param {HTMLElement} element * @return {boolean} */ /* istanbul ignore next */ @@ -256,7 +259,7 @@ function isPositionFixed(element) { while (currentEl) { const position = getStyleAttrValue(currentEl, 'position'); // Only truly fixed if an ancestor is scrollable. - if (position === 'fixed' && hasScrollableAncestor(currentEl)) { + if ((position === 'fixed' || position === 'sticky') && hasScrollableAncestor(currentEl)) { return true; } currentEl = currentEl.parentElement; diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 05468c3879e2..3ac00493c451 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -59,7 +59,7 @@ declare global { export interface PublicGathererArtifacts { /** Console deprecation and intervention warnings logged by Chrome during page load. */ ConsoleMessages: Crdp.Log.EntryAddedEvent[]; - /** All the IFrame elements in the page.*/ + /** All the iframe elements in the page.*/ IFrameElements: Artifacts.IFrameElement[]; /** Information on size and loading for all the images in the page. Natural size information for `picture` and CSS images is only available if the image was one of the largest 50 images. */ ImageElements: Artifacts.ImageElement[]; @@ -182,17 +182,24 @@ declare global { } export interface IFrameElement { - /** The `id` attribute of the IFrame. */ + /** The `id` attribute of the iframe. */ id: string, - /** The `src` attribute of the IFrame. */ + /** The `src` attribute of the iframe. */ src: string, - /** The IFrame's ClientRect. @see https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect */ - clientRect: ClientRect, - /** The visibility of the IFrame. */ - isVisible: boolean, - /** If the IFrame or an ancestor of the IFrame is fixed in position. */ + /** The iframe's ClientRect. @see https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect */ + clientRect: { + top: number; + bottom: number; + left: number; + right: number; + width: number; + height: number; + }, + /** The pixel area iframe. */ + pixelArea: number, + /** If the iframe or an ancestor of the iframe is fixed in position. */ isPositionFixed: boolean, - /** The Frame of the IFrame. @see https://chromedevtools.github.io/devtools-protocol/tot/Page#type-Frame */ + /** The Frame of the iframe. @see https://chromedevtools.github.io/devtools-protocol/tot/Page#type-Frame */ frame: Crdp.Page.Frame, } From 861e3edade0233c60ced54d778d5bff830f4747d Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Mon, 1 Jul 2019 13:15:21 -0400 Subject: [PATCH 08/19] Address comments and update snapshot. --- types/artifacts.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 56356b073bb7..539e3161164d 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -203,7 +203,7 @@ declare global { /** If the iframe or an ancestor of the iframe is fixed in position. */ isPositionFixed: boolean, /** The Frame of the iframe. @see https://chromedevtools.github.io/devtools-protocol/tot/Page#type-Frame */ - frame: Crdp.Page.Frame, + frame?: Crdp.Page.Frame, } /** @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link#Attributes */ From 0c8ba7f39fbfdd6f105076f441d5f08f7cce5c85 Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Mon, 1 Jul 2019 14:06:21 -0400 Subject: [PATCH 09/19] Recurse frameTree. --- .../gather/gatherers/iframe-elements.js | 37 +++++++++++++++---- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/lighthouse-core/gather/gatherers/iframe-elements.js b/lighthouse-core/gather/gatherers/iframe-elements.js index f077028b0d2d..6d9d10443f58 100644 --- a/lighthouse-core/gather/gatherers/iframe-elements.js +++ b/lighthouse-core/gather/gatherers/iframe-elements.js @@ -28,7 +28,7 @@ function collectIFrameElements() { clientRect: {top, bottom, left, right, width, height}, pixelArea: width * height, // @ts-ignore - put into scope via stringification - isPositionFixed: isVisible && isPositionFixed(node), // eslint-disable-line no-undef + isPositionFixed: isPositionFixed(node), // eslint-disable-line no-undef }; }); } @@ -44,17 +44,38 @@ class IFrameElements extends Gatherer { const {frameTree} = await driver.sendCommand('Page.getFrameTree'); const framesByDomId = new Map(); - if (frameTree.childFrames) { - for (const {frame} of frameTree.childFrames) { - if (framesByDomId.has(frame.name)) { - // DOM ID collision, mark it as null. - framesByDomId.set(frame.name, null); - } else { - framesByDomId.set(frame.name, frame); + let toVisit = [frameTree]; + + while(toVisit.length) { + const tempFrameTree = toVisit.shift(); + if(tempFrameTree) { + for (const childFrameTree of tempFrameTree.childFrames || []) { + if (childFrameTree.childFrames) { + toVisit.concat(childFrameTree.childFrames) + } + if (framesByDomId.has(childFrameTree.frame.name)) { + // DOM ID collision, mark it as null. + framesByDomId.set(childFrameTree.frame.name, null); + } else { + framesByDomId.set(childFrameTree.frame.name, childFrameTree.frame); + } } } } + // const {frameTree} = await driver.sendCommand('Page.getFrameTree'); + // const framesByDomId = new Map(); + // if (frameTree.childFrames) { + // for (const {frame} of frameTree.childFrames || []) { + // if (framesByDomId.has(frame.name)) { + // // DOM ID collision, mark it as null. + // framesByDomId.set(frame.name, null); + // } else { + // framesByDomId.set(frame.name, frame); + // } + // } + // } + const expression = `(() => { ${pageFunctions.getOuterHTMLSnippetString}; ${pageFunctions.getElementsInDocumentString}; From c9887c6e2d3be65b742547aa3c5beac920341b85 Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Mon, 1 Jul 2019 14:11:31 -0400 Subject: [PATCH 10/19] Fix lint errors. --- .../gather/gatherers/iframe-elements.js | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/lighthouse-core/gather/gatherers/iframe-elements.js b/lighthouse-core/gather/gatherers/iframe-elements.js index 6d9d10443f58..1feec9ff5802 100644 --- a/lighthouse-core/gather/gatherers/iframe-elements.js +++ b/lighthouse-core/gather/gatherers/iframe-elements.js @@ -44,14 +44,14 @@ class IFrameElements extends Gatherer { const {frameTree} = await driver.sendCommand('Page.getFrameTree'); const framesByDomId = new Map(); - let toVisit = [frameTree]; + const toVisit = [frameTree]; - while(toVisit.length) { + while (toVisit.length) { const tempFrameTree = toVisit.shift(); - if(tempFrameTree) { + if (tempFrameTree) { for (const childFrameTree of tempFrameTree.childFrames || []) { if (childFrameTree.childFrames) { - toVisit.concat(childFrameTree.childFrames) + toVisit.concat(childFrameTree.childFrames); } if (framesByDomId.has(childFrameTree.frame.name)) { // DOM ID collision, mark it as null. @@ -63,19 +63,6 @@ class IFrameElements extends Gatherer { } } - // const {frameTree} = await driver.sendCommand('Page.getFrameTree'); - // const framesByDomId = new Map(); - // if (frameTree.childFrames) { - // for (const {frame} of frameTree.childFrames || []) { - // if (framesByDomId.has(frame.name)) { - // // DOM ID collision, mark it as null. - // framesByDomId.set(frame.name, null); - // } else { - // framesByDomId.set(frame.name, frame); - // } - // } - // } - const expression = `(() => { ${pageFunctions.getOuterHTMLSnippetString}; ${pageFunctions.getElementsInDocumentString}; From 0ca3b25bed5406be9ab0fe08af8207c6f27c463c Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Mon, 1 Jul 2019 14:28:18 -0400 Subject: [PATCH 11/19] Declare type for framesByDomId map. --- lighthouse-core/gather/gatherers/iframe-elements.js | 3 ++- types/artifacts.d.ts | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/gather/gatherers/iframe-elements.js b/lighthouse-core/gather/gatherers/iframe-elements.js index 1feec9ff5802..b9fe45b342bb 100644 --- a/lighthouse-core/gather/gatherers/iframe-elements.js +++ b/lighthouse-core/gather/gatherers/iframe-elements.js @@ -43,8 +43,9 @@ class IFrameElements extends Gatherer { const driver = passContext.driver; const {frameTree} = await driver.sendCommand('Page.getFrameTree'); - const framesByDomId = new Map(); const toVisit = [frameTree]; + /** @type {Map} */ + const framesByDomId = new Map(); while (toVisit.length) { const tempFrameTree = toVisit.shift(); diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 539e3161164d..b39d0fc70a23 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -203,7 +203,7 @@ declare global { /** If the iframe or an ancestor of the iframe is fixed in position. */ isPositionFixed: boolean, /** The Frame of the iframe. @see https://chromedevtools.github.io/devtools-protocol/tot/Page#type-Frame */ - frame?: Crdp.Page.Frame, + frame?: Crdp.Page.Frame | null, } /** @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link#Attributes */ From d2223fd38e834b6b9f266bb072c27c6fa48a40ac Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Tue, 2 Jul 2019 10:36:46 -0400 Subject: [PATCH 12/19] Address comments. --- lighthouse-core/gather/gatherers/iframe-elements.js | 8 +++----- types/artifacts.d.ts | 2 +- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/lighthouse-core/gather/gatherers/iframe-elements.js b/lighthouse-core/gather/gatherers/iframe-elements.js index b9fe45b342bb..8eca353d306e 100644 --- a/lighthouse-core/gather/gatherers/iframe-elements.js +++ b/lighthouse-core/gather/gatherers/iframe-elements.js @@ -19,8 +19,6 @@ function collectIFrameElements() { const iFrameElements = getElementsInDocument('iframe'); // eslint-disable-line no-undef return iFrameElements.map(/** @param {HTMLIFrameElement} node */ (node) => { const clientRect = node.getBoundingClientRect(); - // Marking 1x1 as non-visible to ignore tracking pixels. - const {top, bottom, left, right, width, height} = clientRect; return { id: node.id, @@ -44,7 +42,7 @@ class IFrameElements extends Gatherer { const {frameTree} = await driver.sendCommand('Page.getFrameTree'); const toVisit = [frameTree]; - /** @type {Map} */ + /** @type {Map} */ const framesByDomId = new Map(); while (toVisit.length) { @@ -55,8 +53,8 @@ class IFrameElements extends Gatherer { toVisit.concat(childFrameTree.childFrames); } if (framesByDomId.has(childFrameTree.frame.name)) { - // DOM ID collision, mark it as null. - framesByDomId.set(childFrameTree.frame.name, null); + // DOM ID collision, mark as undefined. + framesByDomId.set(childFrameTree.frame.name, undefined); } else { framesByDomId.set(childFrameTree.frame.name, childFrameTree.frame); } diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index b39d0fc70a23..539e3161164d 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -203,7 +203,7 @@ declare global { /** If the iframe or an ancestor of the iframe is fixed in position. */ isPositionFixed: boolean, /** The Frame of the iframe. @see https://chromedevtools.github.io/devtools-protocol/tot/Page#type-Frame */ - frame?: Crdp.Page.Frame | null, + frame?: Crdp.Page.Frame, } /** @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link#Attributes */ From a1a6f21722fd255f640ab034a10bcf00d3b5448b Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Fri, 12 Jul 2019 14:12:21 -0400 Subject: [PATCH 13/19] Resolve comments. --- lighthouse-core/gather/gatherers/iframe-elements.js | 5 ++--- lighthouse-core/lib/page-functions.js | 6 +++++- types/artifacts.d.ts | 2 -- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/lighthouse-core/gather/gatherers/iframe-elements.js b/lighthouse-core/gather/gatherers/iframe-elements.js index 8eca353d306e..9a31a2b66cbe 100644 --- a/lighthouse-core/gather/gatherers/iframe-elements.js +++ b/lighthouse-core/gather/gatherers/iframe-elements.js @@ -24,7 +24,6 @@ function collectIFrameElements() { id: node.id, src: node.src, clientRect: {top, bottom, left, right, width, height}, - pixelArea: width * height, // @ts-ignore - put into scope via stringification isPositionFixed: isPositionFixed(node), // eslint-disable-line no-undef }; @@ -41,7 +40,7 @@ class IFrameElements extends Gatherer { const driver = passContext.driver; const {frameTree} = await driver.sendCommand('Page.getFrameTree'); - const toVisit = [frameTree]; + let toVisit = [frameTree]; /** @type {Map} */ const framesByDomId = new Map(); @@ -50,7 +49,7 @@ class IFrameElements extends Gatherer { if (tempFrameTree) { for (const childFrameTree of tempFrameTree.childFrames || []) { if (childFrameTree.childFrames) { - toVisit.concat(childFrameTree.childFrames); + toVisit = toVisit.concat(childFrameTree.childFrames); } if (framesByDomId.has(childFrameTree.frame.name)) { // DOM ID collision, mark as undefined. diff --git a/lighthouse-core/lib/page-functions.js b/lighthouse-core/lib/page-functions.js index e9cf7d7fe642..ee4c707186bd 100644 --- a/lighthouse-core/lib/page-functions.js +++ b/lighthouse-core/lib/page-functions.js @@ -243,7 +243,11 @@ function isPositionFixed(element) { while (currentEl) { if (currentEl.scrollHeight > currentEl.clientHeight) { const yScroll = getStyleAttrValue(currentEl, 'overflowY'); - if (yScroll) { + // In case where we reach outermost element (``), default computed value of visible + // will be scrollable. + if ((yScroll === 'scroll' || yScroll === 'auto') || + (yScroll === 'visible' && currentEl.parentElement === null) + ) { return true; } } diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 539e3161164d..57ad6bc1c2f7 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -198,8 +198,6 @@ declare global { width: number; height: number; }, - /** The pixel area iframe. */ - pixelArea: number, /** If the iframe or an ancestor of the iframe is fixed in position. */ isPositionFixed: boolean, /** The Frame of the iframe. @see https://chromedevtools.github.io/devtools-protocol/tot/Page#type-Frame */ From a4bacac8140418c191adc50096409f9e75daac85 Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Mon, 15 Jul 2019 13:51:22 -0400 Subject: [PATCH 14/19] Address comments. --- .../gather/gatherers/iframe-elements.js | 26 ++++++++--------- lighthouse-core/lib/page-functions.js | 29 +++++-------------- 2 files changed, 20 insertions(+), 35 deletions(-) diff --git a/lighthouse-core/gather/gatherers/iframe-elements.js b/lighthouse-core/gather/gatherers/iframe-elements.js index 9a31a2b66cbe..e2453faae845 100644 --- a/lighthouse-core/gather/gatherers/iframe-elements.js +++ b/lighthouse-core/gather/gatherers/iframe-elements.js @@ -45,19 +45,19 @@ class IFrameElements extends Gatherer { const framesByDomId = new Map(); while (toVisit.length) { - const tempFrameTree = toVisit.shift(); - if (tempFrameTree) { - for (const childFrameTree of tempFrameTree.childFrames || []) { - if (childFrameTree.childFrames) { - toVisit = toVisit.concat(childFrameTree.childFrames); - } - if (framesByDomId.has(childFrameTree.frame.name)) { - // DOM ID collision, mark as undefined. - framesByDomId.set(childFrameTree.frame.name, undefined); - } else { - framesByDomId.set(childFrameTree.frame.name, childFrameTree.frame); - } - } + const frameTree = toVisit.shift(); + // Should never be undefined, but needed for tsc. + if (!frameTree) continue; + if (framesByDomId.has(frameTree.frame.name)) { + // DOM ID collision, mark as undefined. + framesByDomId.set(frameTree.frame.name, undefined); + } else { + framesByDomId.set(frameTree.frame.name, frameTree.frame); + } + + // Add children to queue. + if (frameTree.childFrames) { + toVisit = toVisit.concat(frameTree.childFrames); } } diff --git a/lighthouse-core/lib/page-functions.js b/lighthouse-core/lib/page-functions.js index ee4c707186bd..5bd2ebb0c8ae 100644 --- a/lighthouse-core/lib/page-functions.js +++ b/lighthouse-core/lib/page-functions.js @@ -234,28 +234,6 @@ function getNodeSelector(node) { */ /* istanbul ignore next */ function isPositionFixed(element) { - /** - * @param {HTMLElement} element - * @return {boolean} - */ - function hasScrollableAncestor(element) { - let currentEl = element.parentElement; - while (currentEl) { - if (currentEl.scrollHeight > currentEl.clientHeight) { - const yScroll = getStyleAttrValue(currentEl, 'overflowY'); - // In case where we reach outermost element (``), default computed value of visible - // will be scrollable. - if ((yScroll === 'scroll' || yScroll === 'auto') || - (yScroll === 'visible' && currentEl.parentElement === null) - ) { - return true; - } - } - currentEl = currentEl.parentElement; - } - return false; - } - /** * @param {HTMLElement} element * @param {string} attr @@ -266,6 +244,13 @@ function isPositionFixed(element) { return element.style[attr] || window.getComputedStyle(element)[attr]; } + // Position fixed/sticky has no effect in case when document does not scroll. + const htmlEl = document.querySelector('html'); + if (htmlEl.scrollHeight <= htmlEl.clientHeight || + ['scroll', 'auto', 'visible'].includes(getStyleAttrValue(htmlEl, 'overflowY'))) { + return false; + } + let currentEl = element; while (currentEl) { const position = getStyleAttrValue(currentEl, 'position'); From 54f9793fc3f8e20f51f04663ace8bb04f0e57a31 Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Tue, 16 Jul 2019 14:07:23 -0400 Subject: [PATCH 15/19] Update oopif to ensure functionality of IFrameElements gatherer. Update getElementsInDocument function to find child elements within IFrames. --- lighthouse-cli/test/fixtures/online-only.html | 1 + lighthouse-cli/test/fixtures/oopif.html | 8 ++-- .../test/smokehouse/oopif-config.js | 5 --- .../test/smokehouse/oopif-expectations.js | 43 +++++++++++++++++++ lighthouse-core/lib/page-functions.js | 13 ++++-- types/artifacts.d.ts | 2 +- 6 files changed, 58 insertions(+), 14 deletions(-) diff --git a/lighthouse-cli/test/fixtures/online-only.html b/lighthouse-cli/test/fixtures/online-only.html index 720d19d5bd6b..702c7ee02df8 100644 --- a/lighthouse-cli/test/fixtures/online-only.html +++ b/lighthouse-cli/test/fixtures/online-only.html @@ -4,5 +4,6 @@ There was nothing special about this site, nothing was left to be seen, not even a kite. + diff --git a/lighthouse-cli/test/fixtures/oopif.html b/lighthouse-cli/test/fixtures/oopif.html index df3cb0c1b9cf..c1b9e100c202 100644 --- a/lighthouse-cli/test/fixtures/oopif.html +++ b/lighthouse-cli/test/fixtures/oopif.html @@ -1,10 +1,8 @@ - - Where is my iframe? -

Hello frames

- + + - + \ No newline at end of file diff --git a/lighthouse-cli/test/smokehouse/oopif-config.js b/lighthouse-cli/test/smokehouse/oopif-config.js index ebc5fa22bfe6..bc069999c34f 100644 --- a/lighthouse-cli/test/smokehouse/oopif-config.js +++ b/lighthouse-cli/test/smokehouse/oopif-config.js @@ -10,9 +10,4 @@ */ module.exports = { extends: 'lighthouse:default', - settings: { - onlyAudits: [ - 'network-requests', - ], - }, }; diff --git a/lighthouse-cli/test/smokehouse/oopif-expectations.js b/lighthouse-cli/test/smokehouse/oopif-expectations.js index a73509e061d7..6ca5bb0d5655 100644 --- a/lighthouse-cli/test/smokehouse/oopif-expectations.js +++ b/lighthouse-cli/test/smokehouse/oopif-expectations.js @@ -27,5 +27,48 @@ module.exports = [ }, }, }, + artifacts: { + IFrameElements: [ + { + id: 'oopif', + src: 'https://www.paulirish.com/2012/why-moving-elements-with-translate-is-better-than-posabs-topleft/', + clientRect: { + width: '>0', + height: '>0', + }, + isPositionFixed: false, + }, + { + id: 'outer-iframe', + src: 'http://localhost:10200/online-only.html', + clientRect: { + width: '>0', + height: '>0', + }, + isPositionFixed: true, + frame: { + name: 'outer-iframe', + url: 'http://localhost:10200/online-only.html', + securityOrigin: 'http://localhost:10200', + mimeType: 'text/html', + }, + }, + { + id: 'inner-iframe', + src: '', + clientRect: { + width: '>0', + height: '>0', + }, + isPositionFixed: false, + frame: { + name: 'inner-iframe', + url: 'about:blank', + securityOrigin: '://', + mimeType: 'text/html', + }, + }, + ], + }, }, ]; diff --git a/lighthouse-core/lib/page-functions.js b/lighthouse-core/lib/page-functions.js index 5bd2ebb0c8ae..4d2db073b42b 100644 --- a/lighthouse-core/lib/page-functions.js +++ b/lighthouse-core/lib/page-functions.js @@ -98,6 +98,13 @@ function getElementsInDocument(selector) { if (el.shadowRoot) { _findAllElements(el.shadowRoot.querySelectorAll('*')); } + // If the element has a contentWindow (IFrame), dig deeper. + // Try/Catch needed in case of cross-origin frame. + try { + if (el.contentWindow) { + _findAllElements(el.contentWindow.document.querySelectorAll('*')); + } + } catch (e) {} } }; _findAllElements(document.querySelectorAll('*')); @@ -246,8 +253,8 @@ function isPositionFixed(element) { // Position fixed/sticky has no effect in case when document does not scroll. const htmlEl = document.querySelector('html'); - if (htmlEl.scrollHeight <= htmlEl.clientHeight || - ['scroll', 'auto', 'visible'].includes(getStyleAttrValue(htmlEl, 'overflowY'))) { + if (htmlEl.scrollHeight <= htmlEl.clientHeight || + !['scroll', 'auto', 'visible'].includes(getStyleAttrValue(htmlEl, 'overflowY'))) { return false; } @@ -255,7 +262,7 @@ function isPositionFixed(element) { while (currentEl) { const position = getStyleAttrValue(currentEl, 'position'); // Only truly fixed if an ancestor is scrollable. - if ((position === 'fixed' || position === 'sticky') && hasScrollableAncestor(currentEl)) { + if ((position === 'fixed' || position === 'sticky')) { return true; } currentEl = currentEl.parentElement; diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 57ad6bc1c2f7..937ea6dadb69 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -200,7 +200,7 @@ declare global { }, /** If the iframe or an ancestor of the iframe is fixed in position. */ isPositionFixed: boolean, - /** The Frame of the iframe. @see https://chromedevtools.github.io/devtools-protocol/tot/Page#type-Frame */ + /** The Frame of the iframe, undefined if cross-origin. @see https://chromedevtools.github.io/devtools-protocol/tot/Page#type-Frame */ frame?: Crdp.Page.Frame, } From 13e6f6683b3a88fcec5110a31cb689778da838f9 Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Tue, 16 Jul 2019 17:59:20 -0400 Subject: [PATCH 16/19] Address comments. --- lighthouse-cli/test/fixtures/oopif.html | 2 +- lighthouse-cli/test/smokehouse/oopif-expectations.js | 1 + lighthouse-core/lib/page-functions.js | 12 ++++-------- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/lighthouse-cli/test/fixtures/oopif.html b/lighthouse-cli/test/fixtures/oopif.html index c1b9e100c202..d3bdb265cd21 100644 --- a/lighthouse-cli/test/fixtures/oopif.html +++ b/lighthouse-cli/test/fixtures/oopif.html @@ -5,4 +5,4 @@

Hello frames

- \ No newline at end of file + diff --git a/lighthouse-cli/test/smokehouse/oopif-expectations.js b/lighthouse-cli/test/smokehouse/oopif-expectations.js index 6ca5bb0d5655..9df12f2f0cdb 100644 --- a/lighthouse-cli/test/smokehouse/oopif-expectations.js +++ b/lighthouse-cli/test/smokehouse/oopif-expectations.js @@ -37,6 +37,7 @@ module.exports = [ height: '>0', }, isPositionFixed: false, + frame: undefined, }, { id: 'outer-iframe', diff --git a/lighthouse-core/lib/page-functions.js b/lighthouse-core/lib/page-functions.js index 4d2db073b42b..6fba8d506176 100644 --- a/lighthouse-core/lib/page-functions.js +++ b/lighthouse-core/lib/page-functions.js @@ -98,13 +98,10 @@ function getElementsInDocument(selector) { if (el.shadowRoot) { _findAllElements(el.shadowRoot.querySelectorAll('*')); } - // If the element has a contentWindow (IFrame), dig deeper. - // Try/Catch needed in case of cross-origin frame. - try { - if (el.contentWindow) { - _findAllElements(el.contentWindow.document.querySelectorAll('*')); - } - } catch (e) {} + // If the element has a contentDocument (IFrame), dig deeper. + if (el.contentDocument) { + _findAllElements(el.contentDocument.querySelectorAll('*')); + } } }; _findAllElements(document.querySelectorAll('*')); @@ -261,7 +258,6 @@ function isPositionFixed(element) { let currentEl = element; while (currentEl) { const position = getStyleAttrValue(currentEl, 'position'); - // Only truly fixed if an ancestor is scrollable. if ((position === 'fixed' || position === 'sticky')) { return true; } From e674bc921f41f655c01aaf62d3dcd4a2991d215e Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Thu, 12 Sep 2019 10:54:42 -0700 Subject: [PATCH 17/19] Remove `frame` object from IFrameElement. --- .../test/smokehouse/oopif-expectations.js | 13 ---------- .../gather/gatherers/iframe-elements.js | 25 ------------------- types/artifacts.d.ts | 2 -- 3 files changed, 40 deletions(-) diff --git a/lighthouse-cli/test/smokehouse/oopif-expectations.js b/lighthouse-cli/test/smokehouse/oopif-expectations.js index 9df12f2f0cdb..61f6d60234b9 100644 --- a/lighthouse-cli/test/smokehouse/oopif-expectations.js +++ b/lighthouse-cli/test/smokehouse/oopif-expectations.js @@ -37,7 +37,6 @@ module.exports = [ height: '>0', }, isPositionFixed: false, - frame: undefined, }, { id: 'outer-iframe', @@ -47,12 +46,6 @@ module.exports = [ height: '>0', }, isPositionFixed: true, - frame: { - name: 'outer-iframe', - url: 'http://localhost:10200/online-only.html', - securityOrigin: 'http://localhost:10200', - mimeType: 'text/html', - }, }, { id: 'inner-iframe', @@ -62,12 +55,6 @@ module.exports = [ height: '>0', }, isPositionFixed: false, - frame: { - name: 'inner-iframe', - url: 'about:blank', - securityOrigin: '://', - mimeType: 'text/html', - }, }, ], }, diff --git a/lighthouse-core/gather/gatherers/iframe-elements.js b/lighthouse-core/gather/gatherers/iframe-elements.js index e2453faae845..97aea7d0bdff 100644 --- a/lighthouse-core/gather/gatherers/iframe-elements.js +++ b/lighthouse-core/gather/gatherers/iframe-elements.js @@ -39,28 +39,6 @@ class IFrameElements extends Gatherer { async afterPass(passContext) { const driver = passContext.driver; - const {frameTree} = await driver.sendCommand('Page.getFrameTree'); - let toVisit = [frameTree]; - /** @type {Map} */ - const framesByDomId = new Map(); - - while (toVisit.length) { - const frameTree = toVisit.shift(); - // Should never be undefined, but needed for tsc. - if (!frameTree) continue; - if (framesByDomId.has(frameTree.frame.name)) { - // DOM ID collision, mark as undefined. - framesByDomId.set(frameTree.frame.name, undefined); - } else { - framesByDomId.set(frameTree.frame.name, frameTree.frame); - } - - // Add children to queue. - if (frameTree.childFrames) { - toVisit = toVisit.concat(frameTree.childFrames); - } - } - const expression = `(() => { ${pageFunctions.getOuterHTMLSnippetString}; ${pageFunctions.getElementsInDocumentString}; @@ -70,9 +48,6 @@ class IFrameElements extends Gatherer { /** @type {LH.Artifacts['IFrameElements']} */ const iframeElements = await driver.evaluateAsync(expression, {useIsolation: true}); - for (const el of iframeElements) { - el.frame = framesByDomId.get(el.id); - } return iframeElements; } } diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 937ea6dadb69..f62430558a18 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -200,8 +200,6 @@ declare global { }, /** If the iframe or an ancestor of the iframe is fixed in position. */ isPositionFixed: boolean, - /** The Frame of the iframe, undefined if cross-origin. @see https://chromedevtools.github.io/devtools-protocol/tot/Page#type-Frame */ - frame?: Crdp.Page.Frame, } /** @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link#Attributes */ From 8d9354e673f8e90a995d707b8f6f8bf2e6840c33 Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Thu, 12 Sep 2019 11:03:37 -0700 Subject: [PATCH 18/19] Remove recursive iframe gathering functionality from _findAllElements() and updated relevant smoketest. --- lighthouse-cli/test/smokehouse/oopif-expectations.js | 9 --------- lighthouse-core/lib/page-functions.js | 4 ---- 2 files changed, 13 deletions(-) diff --git a/lighthouse-cli/test/smokehouse/oopif-expectations.js b/lighthouse-cli/test/smokehouse/oopif-expectations.js index 61f6d60234b9..5c624c9999e4 100644 --- a/lighthouse-cli/test/smokehouse/oopif-expectations.js +++ b/lighthouse-cli/test/smokehouse/oopif-expectations.js @@ -47,15 +47,6 @@ module.exports = [ }, isPositionFixed: true, }, - { - id: 'inner-iframe', - src: '', - clientRect: { - width: '>0', - height: '>0', - }, - isPositionFixed: false, - }, ], }, }, diff --git a/lighthouse-core/lib/page-functions.js b/lighthouse-core/lib/page-functions.js index 6fba8d506176..d54fc4070ebe 100644 --- a/lighthouse-core/lib/page-functions.js +++ b/lighthouse-core/lib/page-functions.js @@ -98,10 +98,6 @@ function getElementsInDocument(selector) { if (el.shadowRoot) { _findAllElements(el.shadowRoot.querySelectorAll('*')); } - // If the element has a contentDocument (IFrame), dig deeper. - if (el.contentDocument) { - _findAllElements(el.contentDocument.querySelectorAll('*')); - } } }; _findAllElements(document.querySelectorAll('*')); From 82ddb3b8b6e9b595142679717fe4757737d6d98d Mon Sep 17 00:00:00 2001 From: Jon Burger Date: Thu, 12 Sep 2019 11:08:13 -0700 Subject: [PATCH 19/19] Remove unused `inner-iframe` from online-only fixture. --- lighthouse-cli/test/fixtures/online-only.html | 1 - 1 file changed, 1 deletion(-) diff --git a/lighthouse-cli/test/fixtures/online-only.html b/lighthouse-cli/test/fixtures/online-only.html index 702c7ee02df8..720d19d5bd6b 100644 --- a/lighthouse-cli/test/fixtures/online-only.html +++ b/lighthouse-cli/test/fixtures/online-only.html @@ -4,6 +4,5 @@ There was nothing special about this site, nothing was left to be seen, not even a kite. -