core(full-page-screenshot): resolve node rects during emulation (#11536)

GoogleChrome · Nov 30, 2020 · 26074a7 · 26074a7
1 parent 29b0eb0
commit 26074a7
Show file tree

Hide file tree

Showing 23 changed files with 322 additions and 86 deletions.
diff --git a/lighthouse-cli/test/fixtures/screenshot.html b/lighthouse-cli/test/fixtures/screenshot.html
@@ -14,11 +14,19 @@
 </style>
 
 <body>
+  <p id="textEl">Screenshot tester.</p>
+
   <script>
     const params = new URLSearchParams(document.location.search);
     if (params.has('width')) document.body.style.width = params.get('width');
     if (params.has('height')) document.body.style.height = params.get('height');
-  </script>
-
-  <p>Screenshot tester.</p>
+
+    textEl.style.color = 'gray';
+    textEl.style.backgroundColor = 'lightgrey';
+    let padding = 1;
+    setInterval(() => {
+      textEl.style.paddingTop = padding + 'px';
+      padding += 1;
+    }, 50);
+  </script>  
 </body>
diff --git a/lighthouse-cli/test/smokehouse/test-definitions/screenshot/expectations.js b/lighthouse-cli/test/smokehouse/test-definitions/screenshot/expectations.js
@@ -12,9 +12,42 @@ const expectations = [
   {
     artifacts: {
       FullPageScreenshot: {
-        width: '>1000',
-        height: '>1000',
-        data: /data:image\/jpeg;base64,.{10000,}$/,
+        screenshot: {
+          width: '>1000',
+          height: '>1000',
+          data: /data:image\/jpeg;base64,.{10000,}$/,
+        },
+        nodes: {
+          'page-0-BODY': {
+            top: 8,
+            bottom: 1008,
+            left: 8,
+            right: 1008,
+            width: 1000,
+            height: 1000,
+          },
+          // The following 2 are the same element (from different JS contexts). This element
+          // starts with height ~18 and grows over time. See screenshot.html.
+          'page-1-P': {
+            top: 8,
+            left: 8,
+            height: '>40',
+          },
+          '5-1-P': {
+            top: 8,
+            left: 8,
+            height: '>40',
+          },
+          '5-2-BODY': {
+            top: 8,
+            bottom: 1008,
+            left: 8,
+            right: 1008,
+            width: 1000,
+            height: 1000,
+          },
+          '5-3-HTML': {},
+        },
       },
     },
     lhr: {

diff --git a/lighthouse-cli/test/smokehouse/test-definitions/screenshot/screenshot-config.js b/lighthouse-cli/test/smokehouse/test-definitions/screenshot/screenshot-config.js
@@ -11,7 +11,6 @@ const experimentalConfig = require('../../../../../lighthouse-core/config/experi
 module.exports = {
   ...experimentalConfig,
   settings: {
-    onlyAudits: ['full-page-screenshot'],
     emulatedFormFactor: 'desktop',
   },
 };
diff --git a/lighthouse-core/audits/accessibility/axe-audit.js b/lighthouse-core/audits/accessibility/axe-audit.js
@@ -77,6 +77,7 @@ class AxeAudit extends Audit {
       items = rule.nodes.map(node => ({
         node: /** @type {LH.Audit.Details.NodeValue} */ ({
           type: 'node',
+          lhId: node.lhId,
           selector: node.selector,
           path: node.devtoolsNodePath,
           snippet: node.snippet,

diff --git a/lighthouse-core/audits/full-page-screenshot.js b/lighthouse-core/audits/full-page-screenshot.js
@@ -34,7 +34,7 @@ class FullPageScreenshot extends Audit {
       score: 1,
       details: {
         type: 'full-page-screenshot',
-        ...artifacts.FullPageScreenshot,
+        fullPageScreenshot: artifacts.FullPageScreenshot,
       },
     };
   }

diff --git a/lighthouse-core/audits/largest-contentful-paint-element.js b/lighthouse-core/audits/largest-contentful-paint-element.js
@@ -44,6 +44,7 @@ class LargestContentfulPaintElement extends Audit {
       lcpElementDetails.push({
         node: /** @type {LH.Audit.Details.NodeValue} */ ({
           type: 'node',
+          lhId: lcpElement.lhId,
           path: lcpElement.devtoolsNodePath,
           selector: lcpElement.selector,
           nodeLabel: lcpElement.nodeLabel,

diff --git a/lighthouse-core/audits/layout-shift-elements.js b/lighthouse-core/audits/layout-shift-elements.js
@@ -29,7 +29,7 @@ class LayoutShiftElements extends Audit {
       title: str_(UIStrings.title),
       description: str_(UIStrings.description),
       scoreDisplayMode: Audit.SCORING_MODES.INFORMATIVE,
-      requiredArtifacts: ['TraceElements'],
+      requiredArtifacts: ['traces', 'TraceElements'],
     };
   }
 
@@ -45,6 +45,7 @@ class LayoutShiftElements extends Audit {
       return {
         node: /** @type {LH.Audit.Details.NodeValue} */ ({
           type: 'node',
+          lhId: element.lhId,
           path: element.devtoolsNodePath,
           selector: element.selector,
           nodeLabel: element.nodeLabel,

diff --git a/lighthouse-core/audits/seo/tap-targets.js b/lighthouse-core/audits/seo/tap-targets.js
@@ -249,6 +249,7 @@ function targetToTableNode(target) {
 
   return {
     type: 'node',
+    lhId: target.lhId,
     snippet: target.snippet,
     path: target.devtoolsNodePath,
     selector: target.selector,

diff --git a/lighthouse-core/gather/driver/execution-context.js b/lighthouse-core/gather/driver/execution-context.js
@@ -85,6 +85,7 @@ class ExecutionContext {
       expression: `(function wrapInNativePromise() {
         const __nativePromise = window.__nativePromise || Promise;
         const URL = window.__nativeURL || window.URL;
+        window.__lighthouseExecutionContextId = ${contextId};
         return new __nativePromise(function (resolve) {
           return __nativePromise.resolve()
             .then(_ => ${expression})

diff --git a/lighthouse-core/gather/gatherers/full-page-screenshot.js b/lighthouse-core/gather/gatherers/full-page-screenshot.js
@@ -5,7 +5,10 @@
  */
 'use strict';
 
+/* globals window getBoundingClientRect */
+
 const Gatherer = require('./gatherer.js');
+const pageFunctions = require('../../lib/page-functions.js');
 
 // JPEG quality setting
 // Exploration and examples of reports using different quality settings: https://docs.google.com/document/d/1ZSffucIca9XDW2eEwfoevrk-OTl7WQFeMf0CgeJAA8M/edit#
@@ -23,7 +26,7 @@ function snakeCaseToCamelCase(str) {
 class FullPageScreenshot extends Gatherer {
   /**
    * @param {LH.Gatherer.PassContext} passContext
-   * @return {Promise<LH.Artifacts.FullPageScreenshot>}
+   * @return {Promise<LH.Artifacts.FullPageScreenshot['screenshot']>}
    */
   async _takeScreenshot(passContext) {
     const driver = passContext.driver;
@@ -64,6 +67,46 @@ class FullPageScreenshot extends Gatherer {
     };
   }
 
+  /**
+   * Gatherers can collect details about DOM nodes, including their position on the page.
+   * Layout shifts occuring after a gatherer runs can cause these positions to be incorrect,
+   * resulting in a poor experience for element screenshots.
+   * `getNodeDetails` maintains a collection of DOM objects in the page, which we can iterate
+   * to re-collect the bounding client rectangle.
+   * @see pageFunctions.getNodeDetails
+   * @param {LH.Gatherer.PassContext} passContext
+   * @return {Promise<LH.Artifacts.FullPageScreenshot['nodes']>}
+   */
+  async _resolveNodes(passContext) {
+    function resolveNodes() {
+      /** @type {LH.Artifacts.FullPageScreenshot['nodes']} */
+      const nodes = {};
+      if (!window.__lighthouseNodesDontTouchOrAllVarianceGoesAway) return nodes;
+
+      const lhIdToElements = window.__lighthouseNodesDontTouchOrAllVarianceGoesAway;
+      for (const [node, id] of lhIdToElements.entries()) {
+        // @ts-expect-error - getBoundingClientRect put into scope via stringification
+        const rect = getBoundingClientRect(node);
+        if (rect.width || rect.height) nodes[id] = rect;
+      }
+
+      return nodes;
+    }
+    const expression = `(function () {
+      ${pageFunctions.getBoundingClientRectString};
+      return (${resolveNodes.toString()}());
+    })()`;
+
+    // Collect nodes with the page context (`useIsolation: false`) and with our own, reused
+    // context (useIsolation: false). Gatherers use both modes when collecting node details,
+    // so we must do the same here too.
+    const pageContextResult =
+      await passContext.driver.evaluateAsync(expression, {useIsolation: false});
+    const isolatedContextResult =
+      await passContext.driver.evaluateAsync(expression, {useIsolation: true});
+    return {...pageContextResult, ...isolatedContextResult};
+  }
+
   /**
    * @param {LH.Gatherer.PassContext} passContext
    * @return {Promise<LH.Artifacts['FullPageScreenshot']>}
@@ -77,8 +120,10 @@ class FullPageScreenshot extends Gatherer {
       !passContext.settings.internalDisableDeviceScreenEmulation;
 
     try {
-      const screenshot = await this._takeScreenshot(passContext);
-      return screenshot;
+      return {
+        screenshot: await this._takeScreenshot(passContext),
+        nodes: await this._resolveNodes(passContext),
+      };
     } finally {
       // Revert resized page.
       if (lighthouseControlsEmulation) {

diff --git a/lighthouse-core/lib/page-functions.js b/lighthouse-core/lib/page-functions.js
@@ -446,20 +446,55 @@ function wrapRequestIdleCallback(cpuSlowdownMultiplier) {
   };
 }
 
-const getNodeDetailsString = `function getNodeDetails(elem) {
+/**
+ * @param {HTMLElement} element
+ */
+function getNodeDetailsImpl(element) {
+  // This bookkeeping is for the FullPageScreenshot gatherer.
+  if (!window.__lighthouseNodesDontTouchOrAllVarianceGoesAway) {
+    window.__lighthouseNodesDontTouchOrAllVarianceGoesAway = new Map();
+  }
+
+  // Create an id that will be unique across all execution contexts.
+  // The id could be any arbitrary string, the exact value is not important.
+  // For example, tagName is added only because it might be useful for debugging.
+  // But execution id and map size are added to ensure uniqueness.
+  // We also dedupe this id so that details collected for an element within the same
+  // pass and execution context will share the same id. Not technically important, but
+  // cuts down on some duplication.
+  let lhId = window.__lighthouseNodesDontTouchOrAllVarianceGoesAway.get(element);
+  if (!lhId) {
+    lhId = [
+      window.__lighthouseExecutionContextId !== undefined ?
+        window.__lighthouseExecutionContextId :
+        'page',
+      window.__lighthouseNodesDontTouchOrAllVarianceGoesAway.size,
+      element.tagName,
+    ].join('-');
+    window.__lighthouseNodesDontTouchOrAllVarianceGoesAway.set(element, lhId);
+  }
+
+  const htmlElement = element instanceof ShadowRoot ? element.host : element;
+  const details = {
+    lhId,
+    devtoolsNodePath: getNodePath(element),
+    selector: getNodeSelector(htmlElement),
+    boundingRect: getBoundingClientRect(htmlElement),
+    snippet: getOuterHTMLSnippet(element),
+    nodeLabel: getNodeLabel(htmlElement),
+  };
+
+  return details;
+}
+
+const getNodeDetailsString = `function getNodeDetails(element) {
   ${getNodePath.toString()};
   ${getNodeSelector.toString()};
   ${getBoundingClientRect.toString()};
   ${getOuterHTMLSnippet.toString()};
   ${getNodeLabel.toString()};
-  const htmlElem = elem instanceof ShadowRoot ? elem.host : elem;
-  return {
-    devtoolsNodePath: getNodePath(elem),
-    selector: getNodeSelector(htmlElem),
-    boundingRect: getBoundingClientRect(htmlElem),
-    snippet: getOuterHTMLSnippet(elem),
-    nodeLabel: getNodeLabel(htmlElem),
-  };
+  ${getNodeDetailsImpl.toString()};
+  return getNodeDetailsImpl(element);
 }`;
 
 module.exports = {

diff --git a/lighthouse-core/report/html/renderer/details-renderer.js b/lighthouse-core/report/html/renderer/details-renderer.js
@@ -32,7 +32,7 @@ const URL_PREFIXES = ['http://', 'https://', 'data:'];
 class DetailsRenderer {
   /**
    * @param {DOM} dom
-   * @param {{fullPageScreenshot?: LH.Audit.Details.FullPageScreenshot}} [options]
+   * @param {{fullPageScreenshot?: LH.Artifacts.FullPageScreenshot}} [options]
    */
   constructor(dom, options = {}) {
     this._dom = dom;
@@ -521,16 +521,18 @@ class DetailsRenderer {
     if (item.selector) element.setAttribute('data-selector', item.selector);
     if (item.snippet) element.setAttribute('data-snippet', item.snippet);
 
-    if (!item.boundingRect || !this._fullPageScreenshot) {
-      return element;
-    }
+    if (!this._fullPageScreenshot) return element;
+
+    const rect =
+      (item.lhId ? this._fullPageScreenshot.nodes[item.lhId] : null) || item.boundingRect;
+    if (!rect) return element;
 
     const maxThumbnailSize = {width: 147, height: 100};
     const elementScreenshot = ElementScreenshotRenderer.render(
       this._dom,
       this._templateContext,
-      this._fullPageScreenshot,
-      item.boundingRect,
+      this._fullPageScreenshot.screenshot,
+      rect,
       maxThumbnailSize
     );
     if (elementScreenshot) element.prepend(elementScreenshot);