From 719ff97d0af96ae2df7eeb18f355b9cf710c8563 Mon Sep 17 00:00:00 2001 From: Eric Bidelman Date: Fri, 7 Apr 2017 13:59:13 -0700 Subject: [PATCH 1/7] Generate DOM for markdown links in help text --- lighthouse-core/report/v2/report-renderer.js | 33 +++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/lighthouse-core/report/v2/report-renderer.js b/lighthouse-core/report/v2/report-renderer.js index 531bd2ffa412..1e0a2f61119d 100644 --- a/lighthouse-core/report/v2/report-renderer.js +++ b/lighthouse-core/report/v2/report-renderer.js @@ -120,7 +120,8 @@ class ReportRenderer { `lighthouse-score__value--${scoringMode}`); element.querySelector('.lighthouse-score__title').textContent = title; - element.querySelector('.lighthouse-score__description').textContent = description; + element.querySelector('.lighthouse-score__description') + .appendChild(this._convertMarkdownLinksToElement(description)); return element; } @@ -180,6 +181,36 @@ class ReportRenderer { } } + /** + * @param {string} text + * @return {!Element} + */ + _convertMarkdownLinksToElement(text) { + const element = this._createElement('span'); + + // Split on markdown links (e.g. [some link](https://...)). + const parts = text.split(/(\[(.*?)\]\((https?:\/\/.*?)\))/g); + + while (parts.length) { + // Remove the same number of elements as there are capture groups. + // eslint-disable-next-line no-unused-vars + const [preambleText, fullMarkdownLink, linkText, linkHref] = parts.splice(0, 4); + element.appendChild(document.createTextNode(preambleText)); + + // Append link if there are any. + if (linkText && linkHref) { + const a = this._createElement('a'); + a.rel = 'noopener'; + a.target = '_blank'; + a.href = linkHref; + a.textContent = linkText; + element.appendChild(a); + } + } + + return element; + } + /** * @param {!DetailsJSON} text * @return {!Element} From 2cb25c487ee18798fb997a8240cd02cfe542f73f Mon Sep 17 00:00:00 2001 From: Eric Bidelman Date: Fri, 7 Apr 2017 17:38:31 -0700 Subject: [PATCH 2/7] Add test --- lighthouse-core/report/v2/report-renderer.js | 2 +- .../test/report/v2/report-renderer-test.js | 84 +++++++++++-------- 2 files changed, 51 insertions(+), 35 deletions(-) diff --git a/lighthouse-core/report/v2/report-renderer.js b/lighthouse-core/report/v2/report-renderer.js index 1e0a2f61119d..7d1afc06e4ac 100644 --- a/lighthouse-core/report/v2/report-renderer.js +++ b/lighthouse-core/report/v2/report-renderer.js @@ -195,7 +195,7 @@ class ReportRenderer { // Remove the same number of elements as there are capture groups. // eslint-disable-next-line no-unused-vars const [preambleText, fullMarkdownLink, linkText, linkHref] = parts.splice(0, 4); - element.appendChild(document.createTextNode(preambleText)); + element.appendChild(this._document.createTextNode(preambleText)); // Append link if there are any. if (linkText && linkHref) { diff --git a/lighthouse-core/test/report/v2/report-renderer-test.js b/lighthouse-core/test/report/v2/report-renderer-test.js index 8d3590bb3b1e..8b5057a245f5 100644 --- a/lighthouse-core/test/report/v2/report-renderer-test.js +++ b/lighthouse-core/test/report/v2/report-renderer-test.js @@ -26,10 +26,55 @@ const sampleResults = require('../../results/sample_v2.json'); const TEMPLATE_FILE = fs.readFileSync(__dirname + '/../../../report/v2/templates.html', 'utf8'); describe('ReportRenderer V2', () => { - describe('renderReport', () => { - const document = jsdom.jsdom(TEMPLATE_FILE); - const renderer = new ReportRenderer(document); + const document = jsdom.jsdom(TEMPLATE_FILE); + const renderer = new ReportRenderer(document); + + describe('createElement', () => { + it('creates a simple element using default values', () => { + const el = renderer._createElement('div'); + assert.equal(el.localName, 'div'); + assert.equal(el.className, ''); + assert.equal(el.className, el.attributes.length); + }); + + it('creates an element from parameters', () => { + const el = renderer._createElement( + 'div', 'class1 class2', {title: 'title attr', tabindex: 0}); + assert.equal(el.localName, 'div'); + assert.equal(el.className, 'class1 class2'); + assert.equal(el.getAttribute('title'), 'title attr'); + assert.equal(el.getAttribute('tabindex'), '0'); + }); + }); + + describe('cloneTemplate', () => { + it('should clone a template', () => { + const clone = renderer._cloneTemplate('#tmpl-lighthouse-audit-score'); + assert.ok(clone.querySelector('.lighthouse-score')); + }); + + it('fails when template cannot be found', () => { + assert.throws(() => renderer._cloneTemplate('#unknown-selector')); + }); + }); + + describe('_convertMarkdownLinksToElement', () => { + it('converts links', () => { + const result = renderer._convertMarkdownLinksToElement( + 'Some [link](https://example.com/foo). [Learn more](http://example.com).'); + assert.equal(result.innerHTML, + 'Some link. ' + + 'Learn more.'); + }); + + it('ignores links that do not start with http', () => { + const text = 'Sentence with [link](/local/path).'; + const result = renderer._convertMarkdownLinksToElement(text); + assert.equal(result.innerHTML, text); + }); + }); + describe('renderReport', () => { it('should render a report', () => { const output = renderer.renderReport(sampleResults); assert.ok(output.classList.contains('lighthouse-report')); @@ -44,35 +89,6 @@ describe('ReportRenderer V2', () => { assert.ok(output.classList.contains('lighthouse-exception')); }); - describe('createElement', () => { - it('creates a simple element using default values', () => { - const el = renderer._createElement('div'); - assert.equal(el.localName, 'div'); - assert.equal(el.className, ''); - assert.equal(el.className, el.attributes.length); - }); - - it('creates an element from parameters', () => { - const el = renderer._createElement( - 'div', 'class1 class2', {title: 'title attr', tabindex: 0}); - assert.equal(el.localName, 'div'); - assert.equal(el.className, 'class1 class2'); - assert.equal(el.getAttribute('title'), 'title attr'); - assert.equal(el.getAttribute('tabindex'), '0'); - }); - }); - - describe('cloneTemplate', () => { - it('should clone a template', () => { - const clone = renderer._cloneTemplate('#tmpl-lighthouse-audit-score'); - assert.ok(clone.querySelector('.lighthouse-score')); - }); - - it('fails when template cannot be found', () => { - assert.throws(() => renderer._cloneTemplate('#unknown-selector')); - }); - }); - it('renders an audit', () => { const audit = sampleResults.reportCategories[0].audits[0]; const auditDOM = renderer._renderAudit(audit); @@ -82,7 +98,7 @@ describe('ReportRenderer V2', () => { const score = auditDOM.querySelector('.lighthouse-score__value'); assert.equal(title.textContent, audit.result.description); - assert.equal(description.textContent, audit.result.helpText); + assert.ok(description.querySelector('a'), 'audit help text contains coverted markdown links'); assert.equal(score.textContent, '0'); assert.ok(score.classList.contains('lighthouse-score__value--fail')); assert.ok(score.classList.contains(`lighthouse-score__value--${audit.result.scoringMode}`)); @@ -102,7 +118,7 @@ describe('ReportRenderer V2', () => { 'category score is numeric'); assert.equal(value.textContent, Math.round(category.score), 'category score is rounded'); assert.equal(title.textContent, category.name, 'title is set'); - assert.equal(description.textContent, category.description, 'description is set'); + assert.ok(description.querySelector('a'), 'description contains converted markdown links'); const audits = categoryDOM.querySelectorAll('.lighthouse-category > .lighthouse-audit'); assert.equal(audits.length, category.audits.length, 'renders correct number of audits'); From f3ad0be1cee22b300b296e63139e4afcafb2b408 Mon Sep 17 00:00:00 2001 From: Eric Bidelman Date: Mon, 10 Apr 2017 17:17:03 -0700 Subject: [PATCH 3/7] Feedback --- lighthouse-core/report/v2/report-renderer.js | 9 ++++----- .../test/report/v2/report-renderer-test.js | 15 +++++++++++++-- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/lighthouse-core/report/v2/report-renderer.js b/lighthouse-core/report/v2/report-renderer.js index 7d1afc06e4ac..a8fc9a816ba3 100644 --- a/lighthouse-core/report/v2/report-renderer.js +++ b/lighthouse-core/report/v2/report-renderer.js @@ -183,18 +183,17 @@ class ReportRenderer { /** * @param {string} text - * @return {!Element} + * @return {!HTMLSpanElement} */ _convertMarkdownLinksToElement(text) { const element = this._createElement('span'); // Split on markdown links (e.g. [some link](https://...)). - const parts = text.split(/(\[(.*?)\]\((https?:\/\/.*?)\))/g); + const parts = text.split(/\[(.*?)\]\((https?:\/\/.*?)\)/g); while (parts.length) { - // Remove the same number of elements as there are capture groups. - // eslint-disable-next-line no-unused-vars - const [preambleText, fullMarkdownLink, linkText, linkHref] = parts.splice(0, 4); + // Pop off the same number of elements as there are capture groups. + const [preambleText, linkText, linkHref] = parts.splice(0, 3); element.appendChild(this._document.createTextNode(preambleText)); // Append link if there are any. diff --git a/lighthouse-core/test/report/v2/report-renderer-test.js b/lighthouse-core/test/report/v2/report-renderer-test.js index 8b5057a245f5..77dc77a95c77 100644 --- a/lighthouse-core/test/report/v2/report-renderer-test.js +++ b/lighthouse-core/test/report/v2/report-renderer-test.js @@ -59,12 +59,23 @@ describe('ReportRenderer V2', () => { }); describe('_convertMarkdownLinksToElement', () => { - it('converts links', () => { - const result = renderer._convertMarkdownLinksToElement( + it('correctly converts links', () => { + let result = renderer._convertMarkdownLinksToElement( 'Some [link](https://example.com/foo). [Learn more](http://example.com).'); assert.equal(result.innerHTML, 'Some link. ' + 'Learn more.'); + + result = renderer._convertMarkdownLinksToElement('[link](https://example.com/foo)'); + assert.equal(result.innerHTML, + 'link', + 'just a link'); + + result = renderer._convertMarkdownLinksToElement( + '[ Link ](https://example.com/foo) and some text afterwards.'); + assert.equal(result.innerHTML, + ' Link ' + + 'and some text afterwards.', 'link with spaces in brackets'); }); it('ignores links that do not start with http', () => { From 23715f837de9748dfe05fe9300869f670059c1bb Mon Sep 17 00:00:00 2001 From: Eric Bidelman Date: Mon, 10 Apr 2017 17:37:51 -0700 Subject: [PATCH 4/7] Check for valid url --- lighthouse-core/report/v2/report-renderer.js | 8 ++++++-- lighthouse-core/test/report/v2/report-renderer-test.js | 10 +++++++++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/lighthouse-core/report/v2/report-renderer.js b/lighthouse-core/report/v2/report-renderer.js index a8fc9a816ba3..b268f0fd2e23 100644 --- a/lighthouse-core/report/v2/report-renderer.js +++ b/lighthouse-core/report/v2/report-renderer.js @@ -201,9 +201,13 @@ class ReportRenderer { const a = this._createElement('a'); a.rel = 'noopener'; a.target = '_blank'; - a.href = linkHref; a.textContent = linkText; - element.appendChild(a); + try { + a.href = (new URL(linkHref)).href; + element.appendChild(a); + } catch (e) { + element.appendChild(this._document.createTextNode(linkText)); + } } } diff --git a/lighthouse-core/test/report/v2/report-renderer-test.js b/lighthouse-core/test/report/v2/report-renderer-test.js index 77dc77a95c77..593b0b6d8d25 100644 --- a/lighthouse-core/test/report/v2/report-renderer-test.js +++ b/lighthouse-core/test/report/v2/report-renderer-test.js @@ -22,6 +22,7 @@ const fs = require('fs'); const jsdom = require('jsdom'); const ReportRenderer = require('../../../report/v2/report-renderer.js'); const sampleResults = require('../../results/sample_v2.json'); +const URL = window.URL = require('../../../lib/url-shim'); // eslint-disable-line no-unused-vars const TEMPLATE_FILE = fs.readFileSync(__dirname + '/../../../report/v2/templates.html', 'utf8'); @@ -29,6 +30,7 @@ describe('ReportRenderer V2', () => { const document = jsdom.jsdom(TEMPLATE_FILE); const renderer = new ReportRenderer(document); + describe('createElement', () => { it('creates a simple element using default values', () => { const el = renderer._createElement('div'); @@ -64,7 +66,7 @@ describe('ReportRenderer V2', () => { 'Some [link](https://example.com/foo). [Learn more](http://example.com).'); assert.equal(result.innerHTML, 'Some link. ' + - 'Learn more.'); + 'Learn more.'); result = renderer._convertMarkdownLinksToElement('[link](https://example.com/foo)'); assert.equal(result.innerHTML, @@ -78,6 +80,12 @@ describe('ReportRenderer V2', () => { 'and some text afterwards.', 'link with spaces in brackets'); }); + it('handles invalid urls', () => { + const text = 'Text has [bad](https:///) link.'; + const result = renderer._convertMarkdownLinksToElement(text); + assert.equal(result.innerHTML, 'Text has bad link.'); + }); + it('ignores links that do not start with http', () => { const text = 'Sentence with [link](/local/path).'; const result = renderer._convertMarkdownLinksToElement(text); From 692cdd0532eb71b38d5eefac59e22866fcba4d47 Mon Sep 17 00:00:00 2001 From: Eric Bidelman Date: Mon, 10 Apr 2017 18:38:10 -0700 Subject: [PATCH 5/7] Node doesnt have URL --- .../test/report/v2/report-renderer-test.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/test/report/v2/report-renderer-test.js b/lighthouse-core/test/report/v2/report-renderer-test.js index 593b0b6d8d25..d1c4996317e0 100644 --- a/lighthouse-core/test/report/v2/report-renderer-test.js +++ b/lighthouse-core/test/report/v2/report-renderer-test.js @@ -20,17 +20,24 @@ const assert = require('assert'); const fs = require('fs'); const jsdom = require('jsdom'); +const URL = require('../../../lib/url-shim'); const ReportRenderer = require('../../../report/v2/report-renderer.js'); const sampleResults = require('../../results/sample_v2.json'); -const URL = window.URL = require('../../../lib/url-shim'); // eslint-disable-line no-unused-vars const TEMPLATE_FILE = fs.readFileSync(__dirname + '/../../../report/v2/templates.html', 'utf8'); describe('ReportRenderer V2', () => { + before(() => { + global.URL = URL; + }); + + after(() => { + global.URL = undefined; + }); + const document = jsdom.jsdom(TEMPLATE_FILE); const renderer = new ReportRenderer(document); - describe('createElement', () => { it('creates a simple element using default values', () => { const el = renderer._createElement('div'); From ec93a12645e25a5bb8d54381b5163de0ebbc8864 Mon Sep 17 00:00:00 2001 From: Eric Bidelman Date: Tue, 11 Apr 2017 16:51:58 -0700 Subject: [PATCH 6/7] Allow to throw if invalid url --- lighthouse-core/report/v2/report-renderer.js | 8 ++------ lighthouse-core/test/report/v2/report-renderer-test.js | 6 ------ 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/lighthouse-core/report/v2/report-renderer.js b/lighthouse-core/report/v2/report-renderer.js index b268f0fd2e23..5a6c1e3c27f9 100644 --- a/lighthouse-core/report/v2/report-renderer.js +++ b/lighthouse-core/report/v2/report-renderer.js @@ -202,12 +202,8 @@ class ReportRenderer { a.rel = 'noopener'; a.target = '_blank'; a.textContent = linkText; - try { - a.href = (new URL(linkHref)).href; - element.appendChild(a); - } catch (e) { - element.appendChild(this._document.createTextNode(linkText)); - } + a.href = (new URL(linkHref)).href; + element.appendChild(a); } } diff --git a/lighthouse-core/test/report/v2/report-renderer-test.js b/lighthouse-core/test/report/v2/report-renderer-test.js index d1c4996317e0..c9cdf0ab4e9a 100644 --- a/lighthouse-core/test/report/v2/report-renderer-test.js +++ b/lighthouse-core/test/report/v2/report-renderer-test.js @@ -87,12 +87,6 @@ describe('ReportRenderer V2', () => { 'and some text afterwards.', 'link with spaces in brackets'); }); - it('handles invalid urls', () => { - const text = 'Text has [bad](https:///) link.'; - const result = renderer._convertMarkdownLinksToElement(text); - assert.equal(result.innerHTML, 'Text has bad link.'); - }); - it('ignores links that do not start with http', () => { const text = 'Sentence with [link](/local/path).'; const result = renderer._convertMarkdownLinksToElement(text); From 56b96e5562b64d980f27d51d4e4f7297f8ebbb31 Mon Sep 17 00:00:00 2001 From: Eric Bidelman Date: Tue, 11 Apr 2017 16:56:17 -0700 Subject: [PATCH 7/7] Verify invalid url throws --- lighthouse-core/test/report/v2/report-renderer-test.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lighthouse-core/test/report/v2/report-renderer-test.js b/lighthouse-core/test/report/v2/report-renderer-test.js index c9cdf0ab4e9a..5cd581e61e30 100644 --- a/lighthouse-core/test/report/v2/report-renderer-test.js +++ b/lighthouse-core/test/report/v2/report-renderer-test.js @@ -87,6 +87,13 @@ describe('ReportRenderer V2', () => { 'and some text afterwards.', 'link with spaces in brackets'); }); + it('handles invalid urls', () => { + const text = 'Text has [bad](https:///) link.'; + assert.throws(() => { + renderer._convertMarkdownLinksToElement(text); + }); + }); + it('ignores links that do not start with http', () => { const text = 'Sentence with [link](/local/path).'; const result = renderer._convertMarkdownLinksToElement(text);