core(root-causes): grab from trace insights rather than use protocol

core/audits/layout-shifts.js

@@ -29,8 +29,6 @@ const UIStrings = {
   rootCauseFontChanges: 'Web font loaded',
   /** A possible reason why that the layout shift occured. */
   rootCauseInjectedIframe: 'Injected iframe',
-  /** A possible reason why that the layout shift occured. */
-  rootCauseRenderBlockingRequest: 'A late network request adjusted the page layout',
   /** Label shown per-audit to show how many layout shifts are present. The `{# shifts found}` placeholder will be replaced with the number of layout shifts. */
   displayValueShiftsFound: `{shiftCount, plural, =1 {1 layout shift found} other {# layout shifts found}}`,
 };
@@ -82,41 +80,30 @@ class LayoutShifts extends Audit {
       const biggestImpactElement = traceElements.find(t => t.nodeId === biggestImpactNodeId);
 
       // Turn root causes into sub-items.
-      const index = layoutShiftEvents.indexOf(event);
-      const rootCauses = artifacts.RootCauses.layoutShifts[index];
+      const rootCauses = artifacts.RootCauses.layoutShifts.get(event);
       /** @type {SubItem[]} */
       const subItems = [];
       if (rootCauses) {
-        for (const cause of rootCauses.unsizedMedia) {
+        for (const backendNodeId of rootCauses.unsizedImages) {
           const element = artifacts.TraceElements.find(
-            t => t.traceEventType === 'layout-shift' && t.nodeId === cause.node.backendNodeId);
+            t => t.traceEventType === 'trace-engine' && t.nodeId === backendNodeId);
           subItems.push({
             extra: element ? Audit.makeNodeItem(element.node) : undefined,
             cause: str_(UIStrings.rootCauseUnsizedMedia),
           });
         }
-        for (const cause of rootCauses.fontChanges) {
-          const url = cause.request.args.data.url;
+        for (const request of rootCauses.fontRequests) {
+          const url = request.args.data.url;
           subItems.push({
             extra: {type: 'url', value: url},
             cause: str_(UIStrings.rootCauseFontChanges),
           });
         }
-        for (const cause of rootCauses.iframes) {
-          const element = artifacts.TraceElements.find(
-            t => t.traceEventType === 'layout-shift' && t.nodeId === cause.iframe.backendNodeId);
+        if (rootCauses.iframeIds.length) {
           subItems.push({
-            extra: element ? Audit.makeNodeItem(element.node) : undefined,
             cause: str_(UIStrings.rootCauseInjectedIframe),
           });
         }
-        for (const cause of rootCauses.renderBlockingRequests) {
-          const url = cause.request.args.data.url;
-          subItems.push({
-            extra: {type: 'url', value: url},
-            cause: str_(UIStrings.rootCauseRenderBlockingRequest),
-          });
-        }
       }
 
       items.push({

core/gather/gatherers/root-causes.js

@@ -6,7 +6,6 @@
 
 import BaseGatherer from '../base-gatherer.js';
 import Trace from './trace.js';
-import * as TraceEngine from '../../lib/trace-engine.js';
 import {TraceEngineResult} from '../../computed/trace-engine-result.js';
 
 class RootCauses extends BaseGatherer {
@@ -20,125 +19,25 @@ class RootCauses extends BaseGatherer {
   };
 
   /**
-   * @param {LH.Gatherer.Driver} driver
-   * @param {LH.Artifacts.TraceEngineResult['data']} traceParsedData
+   * @param {LH.Gatherer.Context<'Trace'>} context
    * @return {Promise<LH.Artifacts.TraceEngineRootCauses>}
    */
-  static async runRootCauseAnalysis(driver, traceParsedData) {
-    await driver.defaultSession.sendCommand('DOM.enable');
-    await driver.defaultSession.sendCommand('CSS.enable');
-
-    // DOM.getDocument is necessary for pushNodesByBackendIdsToFrontend to properly retrieve
-    // nodeIds if the DOM domain was enabled before this gatherer, invoke it to be safe.
-    await driver.defaultSession.sendCommand('DOM.getDocument', {depth: -1, pierce: true});
-
-    /** @type {import('@paulirish/trace_engine').RootCauses.RootCauses.RootCauseProtocolInterface} */
-    const protocolInterface = {
-      /** @param {string} url */
-      // eslint-disable-next-line no-unused-vars
-      getInitiatorForRequest(url) {
-        return null;
-      },
-      /** @param {import('@paulirish/trace_engine/generated/protocol.js').DOM.BackendNodeId[]} backendNodeIds */
-      async pushNodesByBackendIdsToFrontend(backendNodeIds) {
-        const response = await driver.defaultSession.sendCommand(
-          'DOM.pushNodesByBackendIdsToFrontend', {backendNodeIds});
-        const nodeIds =
-          /** @type {import('@paulirish/trace_engine/generated/protocol.js').DOM.NodeId[]} */(
-            response.nodeIds);
-        return nodeIds;
-      },
-      /** @param {import('@paulirish/trace_engine/generated/protocol.js').DOM.NodeId} nodeId */
-      async getNode(nodeId) {
-        try {
-          const response = await driver.defaultSession.sendCommand('DOM.describeNode', {nodeId});
-          // This always zero, so let's fix it here.
-          // https://bugs.chromium.org/p/chromium/issues/detail?id=1515175
-          response.node.nodeId = nodeId;
-          const node =
-            /** @type {import('@paulirish/trace_engine/generated/protocol.js').DOM.Node} */(
-              response.node);
-          return node;
-        } catch (err) {
-          if (err.message.includes('Could not find node with given id')) {
-            // TODO: when injecting an iframe, the engine gets the node of that frame's document element.
-            // But we don't have a way to access that frame. We just have our default session.
-            // Ex:
-            // node cli http://localhost:10503/shift-attribution.html --quiet --only-audits layout-shifts
-            // To fix we must:
-            // - Change trace engine `getNode` protocol interface to also give frame id
-            // - Expand our driver.targetManager to know how to talk to a session connected to a specific frame
-            // When this is fixed, remove this try/catch.
-            // Note: this could be buggy by giving the wrong node detail if a node id meant for a non-main frame
-            // happens to match one from the main frame ... which is pretty likely...
-            // TODO: fix trace engine type to allow returning null.
-            return /** @type {any} */(null);
-          }
-          throw err;
-        }
-      },
-      /** @param {number} nodeId */
-      async getComputedStyleForNode(nodeId) {
-        try {
-          const response = await driver.defaultSession.sendCommand(
-            'CSS.getComputedStyleForNode', {nodeId});
-          return response.computedStyle;
-        } catch {
-          return [];
-        }
-      },
-      /** @param {import('@paulirish/trace_engine/generated/protocol.js').DOM.NodeId} nodeId */
-      async getMatchedStylesForNode(nodeId) {
-        try {
-          const response = await driver.defaultSession.sendCommand(
-            'CSS.getMatchedStylesForNode', {nodeId});
-          return {...response, getError() {}};
-        } catch (err) {
-          return /** @type {any} */({getError() {
-            return err.toString();
-          }});
-        }
-      },
-      /** @param {string} url */
-      // @ts-expect-error not using, dont care about type error.
-      // eslint-disable-next-line no-unused-vars
-      async fontFaceForSource(url) {
-        return null;
-      },
-    };
+  async getArtifact(context) {
+    const trace = context.dependencies.Trace;
+    const traceEngineResult = await TraceEngineResult.request({trace}, context);
 
     /** @type {LH.Artifacts.TraceEngineRootCauses} */
     const rootCauses = {
-      layoutShifts: {},
+      layoutShifts: new Map(),
     };
-    const rootCausesEngine = new TraceEngine.RootCauses(protocolInterface);
-    const layoutShiftEvents = traceParsedData.LayoutShifts.clusters.flatMap(c => c.events);
-    for (const event of layoutShiftEvents) {
-      const r = await rootCausesEngine.layoutShifts.rootCausesForEvent(traceParsedData, event);
-      if (!r) continue;
-
-      for (const cause of r.fontChanges) {
-        // TODO: why isn't trace engine unwrapping this promise ...
-        cause.fontFace = await cause.fontFace;
+    for (const insightSet of traceEngineResult.insights.values()) {
+      for (const [shift, reasons] of insightSet.model.CLSCulprits.shifts) {
+        rootCauses.layoutShifts.set(shift, reasons);
       }
-      rootCauses.layoutShifts[layoutShiftEvents.indexOf(event)] = r;
     }
 
-    await driver.defaultSession.sendCommand('DOM.disable');
-    await driver.defaultSession.sendCommand('CSS.disable');
-
     return rootCauses;
   }
-
-  /**
-   * @param {LH.Gatherer.Context<'Trace'>} context
-   * @return {Promise<LH.Artifacts.TraceEngineRootCauses>}
-   */
-  async getArtifact(context) {
-    const trace = context.dependencies.Trace;
-    const traceEngineResult = await TraceEngineResult.request({trace}, context);
-    return RootCauses.runRootCauseAnalysis(context.driver, traceEngineResult.data);
-  }
 }
 
 export default RootCauses;

core/gather/gatherers/trace-elements.js

@@ -103,13 +103,23 @@ class TraceElements extends BaseGatherer {
       seen.add(obj);
 
       if (obj && typeof obj === 'object' && !Array.isArray(obj)) {
-        Object.keys(obj).forEach(key => {
-          if (typeof obj[key] === 'object') {
-            recursiveObjectEnumerate(obj[key], cb, seen);
-          } else {
-            cb(obj, key);
+        if (obj instanceof Map) {
+          for (const [key, val] of obj) {
+            if (typeof val === 'object') {
+              recursiveObjectEnumerate(val, cb, seen);
+            } else {
+              cb(val, key);
+            }
           }
-        });
+        } else {
+          Object.keys(obj).forEach(key => {
+            if (typeof obj[key] === 'object') {
+              recursiveObjectEnumerate(obj[key], cb, seen);
+            } else {
+              cb(obj[key], key);
+            }
+          });
+        }
       } else if (Array.isArray(obj)) {
         obj.forEach(item => {
           if (typeof item === 'object' || Array.isArray(item)) {
@@ -121,9 +131,10 @@ class TraceElements extends BaseGatherer {
 
     /** @type {number[]} */
     const nodeIds = [];
-    recursiveObjectEnumerate(insightSet.model, (obj, key) => {
-      if (typeof obj[key] === 'number' && (key === 'nodeId' || key === 'node_id')) {
-        nodeIds.push(obj[key]);
+    recursiveObjectEnumerate(insightSet.model, (val, key) => {
+      const keys = ['nodeId', 'node_id', 'unsizedImages'];
+      if (typeof val === 'number' && keys.includes(key)) {
+        nodeIds.push(val);
       }
     }, new Set());
 
@@ -212,14 +223,6 @@ class TraceElements extends BaseGatherer {
           nodeIds.push(biggestImpactedNodeId);
         }
 
-        const index = layoutShiftEvents.indexOf(event);
-        const shiftRootCauses = rootCauses.layoutShifts[index];
-        if (shiftRootCauses) {
-          for (const cause of shiftRootCauses.unsizedMedia) {
-            nodeIds.push(cause.node.backendNodeId);
-          }
-        }
-
         return nodeIds.map(nodeId => ({nodeId}));
       });
   }

core/test/fixtures/user-flows/reports/sample-flow-result.json

@@ -10230,56 +10230,10 @@
             "id": "layout-shifts",
             "title": "Avoid large layout shifts",
             "description": "These are the largest layout shifts observed on the page. Each table item represents a single layout shift, and shows the element that shifted the most. Below each item are possible root causes that led to the layout shift. Some of these layout shifts may not be included in the CLS metric value due to [windowing](https://web.dev/articles/cls#what_is_cls). [Learn how to improve CLS](https://web.dev/articles/optimize-cls)",
-            "score": 0,
-            "scoreDisplayMode": "metricSavings",
-            "displayValue": "1 layout shift found",
-            "metricSavings": {
-              "CLS": 0.10200000000000001
-            },
-            "details": {
-              "type": "table",
-              "headings": [
-                {
-                  "key": "node",
-                  "valueType": "node",
-                  "subItemsHeading": {
-                    "key": "extra"
-                  },
-                  "label": "Element"
-                },
-                {
-                  "key": "score",
-                  "valueType": "numeric",
-                  "subItemsHeading": {
-                    "key": "cause",
-                    "valueType": "text"
-                  },
-                  "granularity": 0.001,
-                  "label": "Layout shift score"
-                }
-              ],
-              "items": [
-                {
-                  "node": {
-                    "type": "node",
-                    "lhId": "page-1-FOOTER",
-                    "path": "1,HTML,1,BODY,0,DIV,0,DIV,5,FOOTER",
-                    "selector": "body > div#__next > div.flex > footer.w-full",
-                    "boundingRect": {
-                      "top": 1580,
-                      "bottom": 1664,
-                      "left": 0,
-                      "right": 412,
-                      "width": 412,
-                      "height": 84
-                    },
-                    "snippet": "<footer class=\"w-full flex flex-row flex-wrap p-2 text-center text-gray-400 sm:text-left …\">",
-                    "nodeLabel": "Code © 2020-2021 Mike's Cereal Shack Authors\nContent © 2005-2013 NBCUniversal M…"
-                  },
-                  "score": 0.10206561360874848
-                }
-              ]
-            },
+            "score": null,
+            "scoreDisplayMode": "error",
+            "errorMessage": "artifacts.RootCauses.layoutShifts.get is not a function",
+            "errorStack": "TypeError: artifacts.RootCauses.layoutShifts.get is not a function\n    at LayoutShifts.audit (file:///core/audits/layout-shifts.js)\n    at async Runner._runAudit (file:///core/runner.js)\n    at async Runner._runAudits (file:///core/runner.js)\n    at async Runner.audit (file:///core/runner.js)\n    at async auditGatherSteps (file:///core/user-flow.js)\n    at async Module.auditFlowArtifacts (file:///core/index.js)\n    at async generateFlowResult (file:///core/scripts/update-flow-fixtures.js)\n    at async file:///core/scripts/update-flow-fixtures.js",
             "guidanceLevel": 2
           },
           "long-tasks": {
@@ -13100,21 +13054,6 @@
             "core/audits/layout-shifts.js | description": [
               "audits[layout-shifts].description"
             ],
-            "core/audits/layout-shifts.js | displayValueShiftsFound": [
-              {
-                "values": {
-                  "shiftCount": 1
-                },
-                "path": "audits[layout-shifts].displayValue"
-              }
-            ],
-            "core/lib/i18n/i18n.js | columnElement": [
-              "audits[layout-shifts].details.headings[0].label",
-              "audits[dom-size-insight].details.headings[1].label"
-            ],
-            "core/audits/layout-shifts.js | columnScore": [
-              "audits[layout-shifts].details.headings[1].label"
-            ],
             "core/audits/long-tasks.js | title": [
               "audits[long-tasks].title"
             ],
@@ -13310,6 +13249,9 @@
             "node_modules/@paulirish/trace_engine/models/trace/insights/DOMSize.js | statistic": [
               "audits[dom-size-insight].details.headings[0].label"
             ],
+            "core/lib/i18n/i18n.js | columnElement": [
+              "audits[dom-size-insight].details.headings[1].label"
+            ],
             "node_modules/@paulirish/trace_engine/models/trace/insights/DOMSize.js | value": [
               "audits[dom-size-insight].details.headings[2].label"
             ],