report: source-location for linkifying

lighthouse-core/audits/seo/font-size.js

@@ -121,64 +121,70 @@ function nodeToTableNode(node) {
  * @param {string} baseURL
  * @param {FailingNodeData['cssRule']} styleDeclaration
  * @param {FailingNodeData['node']} node
- * @returns {{source: string, selector: string | {type: 'node', selector: string, snippet: string}}}
+ * @returns {{source: {type: 'url', value: string} | {type: 'ui-location', sourceURL?: string, url: string, line: number, column: number} | {type: 'code', value: string}, selector: string | {type: 'node', selector: string, snippet: string}}}
  */
 function findStyleRuleSource(baseURL, styleDeclaration, node) {
-  if (
-    !styleDeclaration ||
+  if (!styleDeclaration ||
     styleDeclaration.type === 'Attributes' ||
     styleDeclaration.type === 'Inline'
   ) {
     return {
+      source: {type: 'url', value: baseURL},
       selector: nodeToTableNode(node),
-      source: baseURL,
     };
   }
 
   if (styleDeclaration.parentRule &&
     styleDeclaration.parentRule.origin === 'user-agent') {
     return {
+      source: {type: 'code', value: 'User Agent Stylesheet'},
       selector: styleDeclaration.parentRule.selectors.map(item => item.text).join(', '),
-      source: 'User Agent Stylesheet',
     };
   }
 
-  if (styleDeclaration.type === 'Regular' && styleDeclaration.parentRule) {
+  let selector = '';
+  if (styleDeclaration.parentRule) {
     const rule = styleDeclaration.parentRule;
-    const stylesheet = styleDeclaration.stylesheet;
-
-    if (stylesheet) {
-      let source;
-      const selector = rule.selectors.map(item => item.text).join(', ');
-
-      if (stylesheet.sourceURL) {
-        const url = new URL(stylesheet.sourceURL, baseURL);
-        const range = styleDeclaration.range;
-        source = `${url.href}`;
+    selector = rule.selectors.map(item => item.text).join(', ');
+  }
 
-        if (range) {
-          // `stylesheet` can be either an external file (stylesheet.startLine will always be 0),
-          // or a <style> block (stylesheet.startLine will vary)
-          const absoluteStartLine = range.startLine + stylesheet.startLine + 1;
-          const absoluteStartColumn = range.startColumn + stylesheet.startColumn + 1;
+  if (styleDeclaration.stylesheet && !styleDeclaration.stylesheet.sourceURL) {
+    return {
+      source: {type: 'code', value: 'dynamic'},
+      selector,
+    };
+  }
 
-          source += `:${absoluteStartLine}:${absoluteStartColumn}`;
-        }
-      } else {
-        // dynamically injected to page
-        source = 'dynamic';
+  if (styleDeclaration.range) {
+    // The magic comment sourceURL, if any. Used to resolve a url relative to the baseUrl.
+    // Note, URLs resolved from a magic comment aren't expected to _actually_ exist ...
+    const sourceURL = styleDeclaration.stylesheet ? styleDeclaration.stylesheet.sourceURL : '';
+    const url = new URL(sourceURL, baseURL).href;
+    // TODO when is `styleDeclaration.range` null ???
+    let {startLine, startColumn} = styleDeclaration.range ? styleDeclaration.range : {startLine: 0, startColumn: 0};
+
+    // Inline elements need to add the startLine/startColumn of the <script> element to the ui location, so that the location
+    // is relevant to the HTML file the stylesheet is within.
+    // But, if an inline stylesheet has a sourceURL magic comment, `hasSourceURL` is true and thus `styleDeclaration.range` is sufficient.
+    if (styleDeclaration.stylesheet && styleDeclaration.stylesheet.isInline && !styleDeclaration.stylesheet.hasSourceURL) {
+      startLine += styleDeclaration.stylesheet.startLine;
+      // The column the stylesheet begins on is only relevant if the rule is declared on the same line.
+      // `<div>...</div><style>.some-rule{}`
+      if (styleDeclaration.range && styleDeclaration.range.startLine === 0) {
+        startColumn += styleDeclaration.stylesheet.startColumn;
       }
-
-      return {
-        selector,
-        source,
-      };
     }
+
+    const magicCommentSourceUrl = styleDeclaration.stylesheet && styleDeclaration.stylesheet.hasSourceURL ? sourceURL : undefined;
+    return {
+      source: {type: 'ui-location', sourceURL: magicCommentSourceUrl, url, line: startLine, column: startColumn},
+      selector,
+    };
   }
 
   return {
-    selector: '',
-    source: 'Unknown',
+    selector,
+    source: {type: 'code', value: 'Unknown'},
   };
 }
 
@@ -254,7 +260,7 @@ class FontSize extends Audit {
 
     /** @type {LH.Audit.Details.Table['headings']} */
     const headings = [
-      {key: 'source', itemType: 'url', text: 'Source'},
+      {key: 'source', itemType: 'ui-location', text: 'Source'},
       {key: 'selector', itemType: 'code', text: 'Selector'},
       {key: 'coverage', itemType: 'text', text: '% of Page Text'},
       {key: 'fontSize', itemType: 'text', text: 'Font Size'},
@@ -279,7 +285,7 @@ class FontSize extends Audit {
         (failingTextLength - analyzedFailingTextLength) / visitedTextLength * 100;
 
       tableData.push({
-        source: 'Add\'l illegible text',
+        source: {type: 'code', value: 'Add\'l illegible text'},
         selector: '',
         coverage: `${percentageOfUnanalyzedFailingText.toFixed(2)}%`,
         fontSize: '< 12px',
@@ -288,7 +294,7 @@ class FontSize extends Audit {
 
     if (percentageOfPassingText > 0) {
       tableData.push({
-        source: 'Legible text',
+        source: {type: 'code', value: 'Legible text'},
         selector: '',
         coverage: `${percentageOfPassingText.toFixed(2)}%`,
         fontSize: '≥ 12px',

lighthouse-core/report/html/renderer/details-renderer.js

@@ -95,7 +95,6 @@ class DetailsRenderer {
 
   /**
    * @param {string} text
-   * @return {HTMLElement}
    */
   renderTextURL(text) {
     const url = text;
@@ -214,6 +213,9 @@ class DetailsRenderer {
         case 'node': {
           return this.renderNode(value);
         }
+        case 'ui-location': {
+          return this.renderUILocation(value);
+        }
         case 'url': {
           return this.renderTextURL(value.value);
         }
@@ -377,6 +379,22 @@ class DetailsRenderer {
     return element;
   }
 
+  /**
+   * @param {LH.Audit.Details.UILocationValue} item
+   * @return {Element}
+   * @protected
+   */
+  renderUILocation(item) {
+    // TODO use source map
+    const element = this.renderTextURL(`${item.url}:${item.line + 1}:${item.column}`);
+    element.classList.add('lh-ui-location__location');
+    // TODO: remove sourceURL. see comment on UILocationValue doc.
+    element.setAttribute('data-url', item.sourceURL || item.url);
+    element.setAttribute('data-line', String(item.line));
+    element.setAttribute('data-column', String(item.column));
+    return element;
+  }
+
   /**
    * @param {LH.Audit.Details.Filmstrip} details
    * @return {Element}

lighthouse-core/report/html/renderer/report-ui-features.js

@@ -224,7 +224,7 @@ class ReportUIFeatures {
     /** @type {Array<HTMLTableElement>} */
     const tables = Array.from(this._document.querySelectorAll('.lh-table'));
     const tablesWithUrls = tables
-      .filter(el => el.querySelector('td.lh-table-column--url'))
+      .filter(el => el.querySelector('td.lh-table-column--url, td.lh-table-column--ui-location'))
       .filter(el => {
         const containingAudit = el.closest('.lh-audit');
         if (!containingAudit) throw new Error('.lh-table not within audit');
@@ -292,8 +292,13 @@ class ReportUIFeatures {
     for (const urlItem of urlItems) {
       const datasetUrl = urlItem.dataset.url;
       if (!datasetUrl) continue;
-      const isThirdParty = Util.getRootDomain(datasetUrl) !== finalUrlRootDomain;
-      if (!isThirdParty) continue;
+      try {
+        const isThirdParty = Util.getRootDomain(datasetUrl) !== finalUrlRootDomain;
+        if (!isThirdParty) continue;
+      } catch (err) {
+        // Invalid url (from sourceURL magic comment). Assume 1p.
+        continue;
+      }
 
       const urlRowEl = urlItem.closest('tr');
       if (urlRowEl) {

lighthouse-core/report/html/report-styles.css

@@ -973,7 +973,8 @@
   font-size: large;
 }
 
-.lh-code {
+.lh-code,
+.lh-ui-location {
   white-space: normal;
   margin-top: 0;
   font-size: 85%;
@@ -1330,6 +1331,7 @@
 
 /* Looks unnecessary, but mostly for keeping the <th>s left-aligned */
 .lh-table-column--text,
+.lh-table-column--ui-location,
 .lh-table-column--url,
 /* .lh-table-column--thumbnail, */
 /* .lh-table-column--empty,*/

lighthouse-core/scripts/roll-to-devtools.sh

@@ -21,7 +21,7 @@ else
   frontend_dir="$chromium_dir/third_party/blink/renderer/devtools/front_end"
 fi
 
-tests_dir="$frontend_dir/../../../web_tests/http/tests/devtools/audits2"
+tests_dir="$frontend_dir/../../../web_tests/http/tests/devtools/audits"
 
 if [[ ! -d "$frontend_dir" || ! -a "$frontend_dir/Runtime.js" ]]; then
   echo -e "\033[31m✖ Error!\033[39m"
@@ -43,10 +43,10 @@ cp -pPR "$lh_bg_js" "$lh_worker_dir/lighthouse-dt-bundle.js"
 echo -e "\033[96m ✓\033[39m (Potentially stale) lighthouse-dt-bundle copied."
 
 # copy report generator + cached resources into $fe_lh_dir
-cp -r dist/dt-report-resources/ $fe_lh_dir
+cp -r dist/dt-report-resources/* $fe_lh_dir
 
 # update expected version string in tests
 VERSION=$(node -e "console.log(require('./package.json').version)")
 sed -i '' -e "s/Version:.*/Version: $VERSION/g" "$tests_dir"/*-expected.txt
 
-echo "Done. To rebase the test expectations, run: yarn --cwd ~/chromium/src/third_party/blink/renderer/devtools test 'http/tests/devtools/audits2/*' --reset-results"
+echo "Done. To rebase the test expectations, run: yarn --cwd ~/chromium/src/third_party/blink/renderer/devtools test 'http/tests/devtools/audits/*' --reset-results"

types/audit-details.d.ts

@@ -82,7 +82,7 @@ declare global {
       }
 
       /** Possible types of values found within table items. */
-      type ItemValueTypes = 'bytes' | 'code' | 'link' | 'ms' | 'node' | 'numeric' | 'text' | 'thumbnail' | 'timespanMs' | 'url';
+      type ItemValueTypes = 'bytes' | 'code' | 'link' | 'ms' | 'node' | 'ui-location' | 'numeric' | 'text' | 'thumbnail' | 'timespanMs' | 'url';
 
       // TODO(bckenny): unify Table/Opportunity headings and items on next breaking change.
 
@@ -104,7 +104,7 @@ declare global {
 
       export type TableItem = {
         debugData?: DebugData;
-        [p: string]: string | number | boolean | undefined | DebugData | NodeValue | LinkValue | UrlValue | CodeValue;
+        [p: string]: string | number | boolean | undefined | DebugData | NodeValue | UILocationValue | LinkValue | UrlValue | CodeValue;
       }
 
       export interface OpportunityColumnHeading {
@@ -167,6 +167,23 @@ declare global {
         nodeLabel?: string;
       }
 
+      export interface UILocationValue {
+        type: 'ui-location';
+        /** TODO: remove `sourceURL`.
+         * It's only necessary for the case where a file has a magic sourceURL comment.
+         * Since `url` is made with the surrounding source file's URL as the base URL (new URL(sourceURL, baseURL).href),
+         * the url will be `http://localhost:8000/filenameFromSourceURLComment.css` instead of `filenameFromSourceURLComment.css`.
+         * When DevTools tries to linkify the former, it does so as if it's an actual URL and fails to link to the file in Sources.
+         * The latter works fine.
+         *
+         * Workaround: use `sourceURL` if defined for the data-url, else use `url`.
+        */
+        sourceURL: string;
+        url: string;
+        line: number;
+        column: number;
+      }
+
       /**
        * A value used within a details object, intended to be displayed as a
        * linkified URL, regardless of the controlling heading's valueType.