core(dom): introduce safelySetHref (#12796)

Author: paulirish

lighthouse-core/test/lib/page-functions-test.js

@@ -15,33 +15,32 @@ let assert;
 let jsdom;
 /** @type {import('../../lib/page-functions.js')} */
 let pageFunctions;
-/** @type {import('../../../report/renderer/dom.js').DOM} */
-let DOM;
 
 /* eslint-env jest */
 
+/* global document */
+
 describe('Page Functions', () => {
   const url = 'http://www.example.com';
-  let dom;
 
   beforeAll(async () => {
     assert = (await import('assert')).strict;
     jsdom = await import('jsdom');
     pageFunctions = (await import('../../lib/page-functions.js')).default;
-    DOM = (await import('../../../report/renderer/dom.js')).DOM;
 
     const {document, ShadowRoot, Node, HTMLElement} = new jsdom.JSDOM('', {url}).window;
     global.ShadowRoot = ShadowRoot;
     global.Node = Node;
     global.HTMLElement = HTMLElement;
+    global.document = document;
     global.window = {};
-    dom = new DOM(document);
   });
 
   afterAll(() => {
     global.ShadowRoot = undefined;
     global.Node = undefined;
     global.window = undefined;
+    global.document = undefined;
   });
 
   describe('wrapRuntimeEvalErrorInBrowser()', () => {
@@ -85,124 +84,146 @@ describe('Page Functions', () => {
 
   describe('get outer HTML snippets', () => {
     it('gets full HTML snippet', () => {
-      assert.equal(pageFunctions.getOuterHTMLSnippet(
-        dom.createElement('div', '', {id: '1', style: 'style'})), '<div id="1" style="style">');
+      const elem = document.createElement('div');
+      elem.id = '1';
+      elem.style = 'width: 1px;';
+      assert.equal(pageFunctions.getOuterHTMLSnippet(elem), '<div id="1" style="width: 1px;">');
     });
 
     it('replaces img.src with img.currentSrc', () => {
-      const el = dom.createElement('img', '', {id: '1', src: 'no'});
+      const el = document.createElement('img');
+      el.id = '1';
+      el.src = 'no';
       Object.defineProperty(el, 'currentSrc', {value: 'yes'});
       assert.equal(pageFunctions.getOuterHTMLSnippet(el), '<img id="1" src="yes">');
     });
 
     it('does not replace img.src with img.currentSrc if resolve to same URL', () => {
-      const el = dom.createElement('img', '', {id: '1', src: './a.png'});
+      const el = document.createElement('img');
+      el.id = '1';
+      el.src = './a.png';
       Object.defineProperty(el, 'currentSrc', {value: `${url}/a.png`});
       assert.equal(pageFunctions.getOuterHTMLSnippet(el), '<img id="1" src="./a.png">');
     });
 
     it('removes a specific attribute', () => {
-      assert.equal(pageFunctions.getOuterHTMLSnippet(
-        dom.createElement('div', '', {id: '1', style: 'style'}), ['style']), '<div id="1">');
+      const elem = document.createElement('div');
+      elem.id = '1';
+      elem.style = 'width: 1px;';
+      assert.equal(pageFunctions.getOuterHTMLSnippet(elem, ['style']), '<div id="1">');
     });
 
     it('removes multiple attributes', () => {
-      assert.equal(pageFunctions.getOuterHTMLSnippet(
-        dom.createElement('div', '', {'id': '1', 'style': 'style', 'aria-label': 'label'}),
-        ['style', 'aria-label']
-      ), '<div id="1">');
+      const elem = document.createElement('div');
+      elem.id = '1';
+      elem.style = 'width: 1px;';
+      elem.setAttribute('aria-label', 'label');
+      assert.equal(
+        pageFunctions.getOuterHTMLSnippet(elem, ['style', 'aria-label']),
+        '<div id="1">');
     });
 
     it('should handle dom nodes that cannot be cloned', () => {
-      const element = dom.createElement('div');
+      const element = document.createElement('div');
       element.cloneNode = () => {
         throw new Error('oops!');
       };
       assert.equal(pageFunctions.getOuterHTMLSnippet(element), '<div>');
     });
     it('ignores when attribute not found', () => {
+      const elem = document.createElement('div');
+      elem.id = '1';
+      elem.style = 'width: 1px;';
+      elem.setAttribute('aria-label', 'label');
       assert.equal(pageFunctions.getOuterHTMLSnippet(
-        dom.createElement('div', '', {'id': '1', 'style': 'style', 'aria-label': 'label'}),
+        elem,
         ['style-missing', 'aria-label-missing']
-      ), '<div id="1" style="style" aria-label="label">');
+      ), '<div id="1" style="width: 1px;" aria-label="label">');
     });
 
     it('works if attribute values contain line breaks', () => {
-      assert.equal(pageFunctions.getOuterHTMLSnippet(
-        dom.createElement('div', '', {style: 'style1\nstyle2'})), '<div style="style1\nstyle2">');
+      const elem = document.createElement('div');
+      elem.style = 'width: 1px;\nheight: 2px;';
+      assert.equal(pageFunctions.getOuterHTMLSnippet(elem),
+        '<div style="width: 1px; height: 2px;">');
     });
 
     it('truncates attribute values that are too long', () => {
-      const longClass = 'a'.repeat(200);
+      const elem = document.createElement('div');
+      elem.className = 'a'.repeat(200);
       const truncatedExpectation = 'a'.repeat(74) + '…';
-      assert.equal(pageFunctions.getOuterHTMLSnippet(
-        dom.createElement('div', '', {class: longClass})), `<div class="${truncatedExpectation}">`
-      );
+      assert.equal(
+        pageFunctions.getOuterHTMLSnippet(elem),
+        `<div class="${truncatedExpectation}">`);
     });
 
     it('removes attributes if the length of the attribute name + value is too long', () => {
       const longValue = 'a'.repeat(200);
       const truncatedValue = 'a'.repeat(74) + '…';
-      const element = dom.createElement('div', '', {
-        class: longValue,
-        id: longValue,
-        att1: 'shouldn\'t see this',
-        att2: 'shouldn\'t see this either',
-      });
-      const snippet = pageFunctions.getOuterHTMLSnippet(element, [], 150);
+      const elem = document.createElement('div');
+      elem.className = longValue;
+      elem.id = longValue;
+      elem.setAttribute('att1', 'shouldn\'t see this');
+      elem.setAttribute('att2', 'shouldn\'t see this either');
+
+      const snippet = pageFunctions.getOuterHTMLSnippet(elem, [], 150);
       assert.equal(snippet, `<div class="${truncatedValue}" id="${truncatedValue}" …>`
       );
     });
   });
 
   describe('getNodeSelector', () => {
     it('Uses IDs where available and otherwise falls back to classes', () => {
-      const parentEl = dom.createElement('div', '', {id: 'wrapper', class: 'dont-use-this'});
-      const childEl = dom.createElement('div', '', {class: 'child'});
+      const parentEl = document.createElement('div');
+      parentEl.id = 'wrapper';
+      parentEl.className = 'dont-use-this';
+      const childEl = document.createElement('div');
+      childEl.className = 'child';
       parentEl.appendChild(childEl);
       assert.equal(pageFunctions.getNodeSelector(childEl), 'div#wrapper > div.child');
     });
   });
 
   describe('getNodeLabel', () => {
     it('Returns innerText if element has visible text', () => {
-      const el = dom.createElement('div');
+      const el = document.createElement('div');
       el.innerText = 'Hello';
       assert.equal(pageFunctions.getNodeLabel(el), 'Hello');
     });
 
     it('Falls back to children and alt/aria-label if a title can\'t be determined', () => {
-      const el = dom.createElement('div');
-      const childEl = dom.createElement('div', '', {'aria-label': 'Something'});
+      const el = document.createElement('div');
+      const childEl = document.createElement('div');
+      childEl.setAttribute('aria-label', 'Something');
       el.appendChild(childEl);
       assert.equal(pageFunctions.getNodeLabel(el), 'Something');
     });
 
     it('Truncates long text', () => {
-      const el = dom.createElement('div');
+      const el = document.createElement('div');
       el.setAttribute('alt', Array(100).fill('a').join(''));
       assert.equal(pageFunctions.getNodeLabel(el).length, 80);
     });
 
     it('Truncates long text containing unicode surrogate pairs', () => {
-      const el = dom.createElement('div');
+      const el = document.createElement('div');
       // `getNodeLabel` truncates to 80 characters internally.
       // We want to test a unicode character on the boundary.
       el.innerText = Array(78).fill('a').join('') + '💡💡💡';
       assert.equal(pageFunctions.getNodeLabel(el), Array(78).fill('a').join('') + '💡…');
     });
 
     it('Returns null if there is no better label', () => {
-      const el = dom.createElement('div');
-      const childEl = dom.createElement('span');
+      const el = document.createElement('div');
+      const childEl = document.createElement('span');
       el.appendChild(childEl);
       assert.equal(pageFunctions.getNodeLabel(el), null);
     });
   });
 
   describe('getNodePath', () => {
     it('returns basic node path', () => {
-      const el = dom.createElement('div');
+      const el = document.createElement('div');
       el.innerHTML = `
         <section>
           <span>Sup</span>
@@ -215,11 +236,11 @@ describe('Page Functions', () => {
     });
 
     it('returns node path through shadow root', () => {
-      const el = dom.createElement('div');
-      const main = el.appendChild(dom.createElement('main'));
+      const el = document.createElement('div');
+      const main = el.appendChild(document.createElement('main'));
       const shadowRoot = main.attachShadow({mode: 'open'});
-      const sectionEl = dom.createElement('section');
-      const img = dom.createElement('img');
+      const sectionEl = document.createElement('section');
+      const img = document.createElement('img');
       img.src = '#';
       sectionEl.append(img);
       shadowRoot.append(sectionEl);
@@ -230,8 +251,12 @@ describe('Page Functions', () => {
 
   describe('getNodeDetails', () => {
     it('Returns selector as fallback if nodeLabel equals html tag name', () => {
-      const el = dom.createElement('div', '', {id: 'parent', class: 'parent-el'});
-      const childEl = dom.createElement('p', '', {id: 'child', class: 'child-el'});
+      const el = document.createElement('div');
+      el.id = 'parent';
+      el.className = 'parent-el';
+      const childEl = document.createElement('p');
+      childEl.id = 'child';
+      childEl.className = 'child-el';
       el.appendChild(childEl);
       const {nodeLabel} = pageFunctions.getNodeDetails(el);
       assert.equal(nodeLabel, 'div#parent');

lighthouse-treemap/app/src/main.js

@@ -646,16 +646,14 @@ function renderViewModeButtons(viewModes) {
     if (!viewMode.enabled) viewModeEl.classList.add('view-mode--disabled');
     viewModeEl.id = `view-mode--${viewMode.id}`;
 
-    const inputEl = TreemapUtil.createChildOf(viewModeEl, 'input', 'view-mode__button', {
-      id: `view-mode--${viewMode.id}__label`,
-      type: 'radio',
-      name: 'view-mode',
-      disabled: viewMode.enabled ? undefined : '',
-    });
-
-    const labelEl = TreemapUtil.createChildOf(viewModeEl, 'label', undefined, {
-      for: inputEl.id,
-    });
+    const inputEl = TreemapUtil.createChildOf(viewModeEl, 'input', 'view-mode__button');
+    inputEl.id = `view-mode--${viewMode.id}__label`;
+    inputEl.type = 'radio';
+    inputEl.name = 'view-mode';
+    inputEl.disabled = !viewMode.enabled;
+
+    const labelEl = TreemapUtil.createChildOf(viewModeEl, 'label');
+    labelEl.htmlFor = inputEl.id;
     TreemapUtil.createChildOf(labelEl, 'span', 'view-mode__label').textContent = viewMode.label;
     TreemapUtil.createChildOf(labelEl, 'span', 'view-mode__sublabel lh-text-dim').textContent =
       ` (${viewMode.subLabel})`;

lighthouse-treemap/app/src/util.js

@@ -75,22 +75,13 @@ class TreemapUtil {
    * @template {string} T
    * @param {T} name
    * @param {string=} className
-   * @param {Object<string, (string|undefined)>=} attrs Attribute key/val pairs.
-   *     Note: if an attribute key has an undefined value, this method does not
-   *     set the attribute on the node.
    * @return {HTMLElementByTagName[T]}
    */
-  static createElement(name, className, attrs = {}) {
+  static createElement(name, className) {
     const element = document.createElement(name);
     if (className) {
       element.className = className;
     }
-    Object.keys(attrs).forEach(key => {
-      const value = attrs[key];
-      if (typeof value !== 'undefined') {
-        element.setAttribute(key, value);
-      }
-    });
     return element;
   }
 
@@ -99,13 +90,10 @@ class TreemapUtil {
    * @param {Element} parentElem
    * @param {T} elementName
    * @param {string=} className
-   * @param {Object<string, (string|undefined)>=} attrs Attribute key/val pairs.
-   *     Note: if an attribute key has an undefined value, this method does not
-   *     set the attribute on the node.
    * @return {HTMLElementByTagName[T]}
    */
-  static createChildOf(parentElem, elementName, className, attrs) {
-    const element = this.createElement(elementName, className, attrs);
+  static createChildOf(parentElem, elementName, className) {
+    const element = this.createElement(elementName, className);
     parentElem.appendChild(element);
     return element;
   }

report/renderer/category-renderer.js

@@ -82,9 +82,8 @@ export class CategoryRenderer {
     descEl.appendChild(this.dom.convertMarkdownLinkSnippets(audit.result.description));
 
     for (const relevantMetric of audit.relevantMetrics || []) {
-      const adornEl = this.dom.createChildOf(descEl, 'span', 'lh-audit__adorn', {
-        title: `Relevant to ${relevantMetric.result.title}`,
-      });
+      const adornEl = this.dom.createChildOf(descEl, 'span', 'lh-audit__adorn');
+      adornEl.title = `Relevant to ${relevantMetric.result.title}`;
       adornEl.textContent = relevantMetric.acronym || relevantMetric.id;
     }
 
@@ -332,7 +331,7 @@ export class CategoryRenderer {
   renderScoreGauge(category, groupDefinitions) { // eslint-disable-line no-unused-vars
     const tmpl = this.dom.cloneTemplate('#tmpl-lh-gauge', this.templateContext);
     const wrapper = this.dom.find('a.lh-gauge__wrapper', tmpl);
-    wrapper.href = `#${category.id}`;
+    this.dom.safelySetHref(wrapper, `#${category.id}`);
 
     if (Util.isPluginCategory(category.id)) {
       wrapper.classList.add('lh-gauge__wrapper--plugin');

report/renderer/details-renderer.js

@@ -151,24 +151,19 @@ export class DetailsRenderer {
    * @return {HTMLElement}
    */
   _renderLink(details) {
-    const allowedProtocols = ['https:', 'http:'];
-    let url;
-    try {
-      url = new URL(details.url);
-    } catch (_) {}
+    const a = this._dom.createElement('a');
+    this._dom.safelySetHref(a, details.url);
 
-    if (!url || !allowedProtocols.includes(url.protocol)) {
+    if (!a.href) {
       // Fall back to just the link text if invalid or protocol not allowed.
       const element = this._renderText(details.text);
       element.classList.add('lh-link');
       return element;
     }
 
-    const a = this._dom.createElement('a');
     a.rel = 'noopener';
     a.target = '_blank';
     a.textContent = details.text;
-    a.href = url.href;
     a.classList.add('lh-link');
     return a;
   }
@@ -599,10 +594,9 @@ export class DetailsRenderer {
 
     for (const thumbnail of details.items) {
       const frameEl = this._dom.createChildOf(filmstripEl, 'div', 'lh-filmstrip__frame');
-      this._dom.createChildOf(frameEl, 'img', 'lh-filmstrip__thumbnail', {
-        src: thumbnail.data,
-        alt: `Screenshot`,
-      });
+      const imgEl = this._dom.createChildOf(frameEl, 'img', 'lh-filmstrip__thumbnail');
+      imgEl.src = thumbnail.data;
+      imgEl.alt = `Screenshot`;
     }
     return filmstripEl;
   }