Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report v2: check/x boolean audits, expand details failing audits, optimal val #1963

Merged
merged 10 commits into from
Apr 7, 2017
4 changes: 4 additions & 0 deletions lighthouse-core/report/v2/report-generator.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const REPORT_TEMPLATE = fs.readFileSync(path.join(__dirname, './report-template.
// TODO: Setup a gulp pipeline to concat and minify the renderer files?
const REPORT_JAVASCRIPT = fs.readFileSync(path.join(__dirname, './report-renderer.js'), 'utf8');
const REPORT_CSS = fs.readFileSync(path.join(__dirname, './report-styles.css'), 'utf8');
const REPORT_TEMPLATES = fs.readFileSync(path.join(__dirname, './templates.html'), 'utf8');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

want to clean up our paths here while you're at this? :D fs.readFileSync(__dirname + '/templates.html'), 'utf8') works just fine

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does that work just fine on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does that work just fine on windows?

Yeah, Windows understands / and the fs methods will take care of the rest. The only real worry (that I know of) is if you want to specify an absolute path from the root, which is different cross platform, but we're using __dirname so it's not an issue here


class ReportGeneratorV2 {
/**
Expand Down Expand Up @@ -97,11 +98,14 @@ class ReportGeneratorV2 {
generateReportHtml(reportAsJson) {
const sanitizedJson = JSON.stringify(reportAsJson).replace(/</g, '\\u003c');
const sanitizedJavascript = REPORT_JAVASCRIPT.replace(/<\//g, '\\u003c/');
// Remove script in templates files just to be safe.
const sanitizedTemplates = REPORT_TEMPLATES.replace(/<script>([\s\S]*?)<\/script>/g, '');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how much we should emphasize this; it doesn't strip event handlers, for instance. It's all our own markup, so I vote for not worrying about sanitization of the templates (we don't sanitize report-template.html so why should we do that with these? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you. I recall that el.innerHTML = '<div>just a string</div>'; was deemed unsafe and that's our own markup :) The argument from someonnnnne was that we may add things later and not realize the mistakes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is different enough, though, because it's not markup assembled in (potentially) multiple places, it's all in one spot. So I think it's better on that front. I also don't like calling it sanitized unless it is actually sanitized :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really think we should remove this, or call it "templatesWithScriptTagsRemoves" or something. Current version is easily worked around with an attribute on the script tag...I don't think this is an arms race we want to even start.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


return ReportGeneratorV2.replaceStrings(REPORT_TEMPLATE, [
{search: '%%LIGHTHOUSE_JSON%%', replacement: sanitizedJson},
{search: '%%LIGHTHOUSE_JAVASCRIPT%%', replacement: sanitizedJavascript},
{search: '/*%%LIGHTHOUSE_CSS%%*/', replacement: REPORT_CSS},
{search: '%%LIGHTHOUSE_TEMPLATES%%', replacement: sanitizedTemplates},
]);
}
}
Expand Down
214 changes: 156 additions & 58 deletions lighthouse-core/report/v2/report-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,47 +15,54 @@
*/
'use strict';

/**
* @fileoverview The entry point for rendering the Lighthouse report based on the JSON output.
* This file is injected into the report HTML along with the JSON report.
*
* Dummy text for ensuring report robustness: </script> pre$`post %%LIGHTHOUSE_JSON%%
*/

/* eslint-env browser */
/* eslint indent: [2, 2, { "SwitchCase": 1, "outerIIFEBody": 0 }] */

(function() {
const RATINGS = {
GOOD: {label: 'good', minScore: 75},
PASS: {label: 'pass', minScore: 75},
AVERAGE: {label: 'average', minScore: 45},
POOR: {label: 'poor'}
FAIL: {label: 'fail'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does PASS/FAIL work here? Seems weird to have a three state pass/fail system :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really a pass/fail system. Just for labeling/coloring right now. We'll probably need to expand in the future if we support things like grades. Seems fine for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really a pass/fail system. Just for labeling/coloring right now.

yeah, I was mostly just commenting on the names since PASS/FAIL is usually a binary state. Fine for now

};

/**
* Convert a score to a rating label.
* @param {number} value
* @return {string}
* @param {!number} score
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

primitives (anything lowercase, basically) and record object literal types ({value: string}) don't need the !, FWIW

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk though @ebidel the great Object<string, string> debacle haunts us throughout our codebase now after listenin' to this guy 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to know. removed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, TECHNICALLY, Object<string> still works, Closure just won't type check it :)

* @param {!string} scoringMode One of Audit.SCORING_MODES.
* @return {!string}
*/
function calculateRating(value) {
let rating = RATINGS.POOR.label;
if (value >= RATINGS.GOOD.minScore) {
rating = RATINGS.GOOD.label;
} else if (value >= RATINGS.AVERAGE.minScore) {
rating = RATINGS.AVERAGE.label;
}
return rating;
function calculateRating(score, scoringMode) {
if (scoringMode === 'numeric') {
let rating = RATINGS.FAIL.label;
if (score >= RATINGS.PASS.minScore) {
rating = RATINGS.PASS.label;
} else if (score >= RATINGS.AVERAGE.minScore) {
rating = RATINGS.AVERAGE.label;
}
return rating;
}

// Treat as binary by default.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this unnecessary since score is now a number and for binary 0 will already be RATINGS.FAIL and 100 will be RATINGS.PASS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, good call. Left over from before. Removed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit pushed yet?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jk you haven't pushed yet :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed the first set of changes. Updating tests to come.

return score === 100 ? RATINGS.PASS.label : RATINGS.FAIL.label;
}

/**
* Format number.
* @param {number} number
* @return {string}
* @param {!number} number
* @return {!string}
*/
function formatNumber(number) {
return number.toLocaleString(undefined, {maximumFractionDigits: 1});
}

/**
* @fileoverview The entry point for rendering the Lighthouse report based on the JSON output.
* This file is injected into the report HTML along with the JSON report.
*
* Dummy text for ensuring report robustness: </script> pre$`post %%LIGHTHOUSE_JSON%%
*/
window.ReportRenderer = class ReportRenderer {
class DOM {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feels like something I know ahead of time you're going to disagree with me on :) but I don't personally see the need for a separate DOM class (yet, at least), especially because the split with ReportRenderer doesn't seem completely clear. Anything wrong with these living there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I knew you were going to bring this up! I wasn't convinced adding a bunch of generic methods to ReportRenderer made sense either. They could live as standalone helper functions, but devtools may need to extend them later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

/**
* @param {!Document} document
*/
Expand All @@ -64,15 +71,31 @@ window.ReportRenderer = class ReportRenderer {
}

/**
* @param {!ReportJSON} report
* @return {!Element}
* Sets the text content of an element, safely. If the element does not exist,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe drop the safely? At first I was wondering what additional protections these two methods give me then realized it just meant it checks to make sure the element exists first

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* results in a noop.
* @param {Element} element
* @param {!string} text
* @return {Element}
*/
renderReport(report) {
try {
return this._renderReport(report);
} catch (e) {
return this._renderException(e);
static setText(element, text) {
if (element) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we're worried about these, should they just throw? Seems like it's definitely an error since we're creating the templates and the selectors looking within them

Copy link
Contributor Author

@ebidel ebidel Apr 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulirish asked for them so that the report fails gracefully if a node cant be found. My pref would be to remove them altogether. Tests should really be catching missing nodes in templates.html (markup we control). It would also get rid of the DOM and simplify render methods a bit if we just set things directly (.classList, .textContent).

I'm also ok keeping.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulirish asked for them so that the report fails gracefully if a node cant be found.

I think @paulirish was asking before we had completely settled on the approach used here, so it may be less of an issue. Queries failing fast (as soon as we try to set textContent or add to the classList) is a good thing, I think. I also like the idea of moving some of the query verification to tests, but if setting element.textContent throws a TypeError, just testing e.g. _renderAuditScore will verify the relevant elements exist if it doesn't throw :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still think we should remove. Earlier (and non-silent) failures are better failures.

Copy link
Contributor Author

@ebidel ebidel Apr 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulirish says, "I am low stress on this. Do whateva :)" I'll remove the methods.

Note to future me: give PI 24hrs and he will be low stress.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

element.textContent = text;
}
return element;
}

/**
* Adds a class to an element, safely. If the element does not exist, results
* in a noop.
* @param {Element} element
* @param {...!string} classes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

classNames?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* @return {Element}
*/
static addClass(element, ...classes) {
if (element) {
element.classList.add(...classes);
}
return element;
}

/**
Expand All @@ -81,7 +104,7 @@ window.ReportRenderer = class ReportRenderer {
* @param {!Object<string, string>=} attrs Attribute key/val pairs.
* @return {!Element}
*/
_createElement(name, className, attrs = {}) {
createElement(name, className, attrs = {}) {
const element = this._document.createElement(name);
if (className) {
element.className = className;
Expand All @@ -93,24 +116,81 @@ window.ReportRenderer = class ReportRenderer {
}

/**
* @param {number} score
* @param {string} title
* @param {string} description
* @param {!string} selector
* @return {!DocumentFragment} A clone of the template content.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be !Node (though we're kind of assuming it's an !Element?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically a doc frag, which is not a full element. We use it like an !Element though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it isn't just grabbing the content, it also calls document.importNode which appears to return a Node.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, it's a terrible API and name. importNode() returns the node you import. Since we're importing a DocumentFragment, we get back a Node of that type.

screen shot 2017-04-06 at 12 35 37 pm

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

importNode() returns the node you import

ah, I see. Should _populateResult take a !DocumentFragment, then?

* @throws {Error}
*/
cloneTemplate(selector) {
const template = this._document.querySelector(selector);
if (!template) {
throw new Error(`Template not found: template${selector}`);
}
return this._document.importNode(template.content, true);
}
}

class ReportRenderer {
/**
* @param {!Document} document
*/
constructor(document) {
this._dom = new DOM(document);
}

/**
* @param {!ReportJSON} report
* @return {!Element}
*/
_renderScore(score, title, description) {
const element = this._createElement('div', 'lighthouse-score');
renderReport(report) {
try {
return this._renderReport(report);
} catch (e) {
return this._renderException(e);
}
}

const value = element.appendChild(this._createElement('div', 'lighthouse-score__value'));
value.textContent = formatNumber(score);
value.classList.add(`lighthouse-score__value--${calculateRating(score)}`);
/**
* @param {!Element} tmpl Parent node to populate with values.
* @param {!{value: string, scoringMode: string}} score
* @param {!string} title
* @param {!string} description
* @return {!Element}
*/
_populateScore(tmpl, score, title, description) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something besides tmpl? element or root or whatever

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this (and the render functions below) be called something like _populateResult instead of _populateScore since they include the score, title, and description of the audit/category?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

const rating = calculateRating(score.value, score.scoringMode);

const column = element.appendChild(this._createElement('div', 'lighthouse-score__text'));
column.appendChild(this._createElement('div', 'lighthouse-score__title')).textContent = title;
column.appendChild(
this._createElement('div', 'lighthouse-score__description')).textContent = description;
// Fill in the blanks.
const value = tmpl.querySelector('.lighthouse-score__value');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/value/elem/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when for valueEl so disambiguate.

DOM.setText(value, formatNumber(score.value));
DOM.addClass(value, `lighthouse-score__value--${rating}`,
`lighthouse-score__value--${score.scoringMode}`);

return element;
DOM.setText(tmpl.querySelector('.lighthouse-score__title'), title);
DOM.setText(tmpl.querySelector('.lighthouse-score__description'), description);

return tmpl;
}

/**
* @param {!{value: string, scoringMode: string}} score
* @param {!string} title
* @param {!string} description
* @return {!Element}
*/
_renderAuditScore(score, title, description) {
const tmpl = this._dom.cloneTemplate('#tmpl-lighthouse-audit-score');
return this._populateScore(tmpl, score, title, description);
}

/**
* @param {!{value: string, scoringMode: string}} score
* @param {!string} title
* @param {!string} description
* @return {!Element}
*/
_renderCategoryScore(score, title, description) {
const tmpl = this._dom.cloneTemplate('#tmpl-lighthouse-category-score');
return this._populateScore(tmpl, score, title, description);
}

/**
Expand All @@ -135,7 +215,7 @@ window.ReportRenderer = class ReportRenderer {
* @return {!Element}
*/
_renderText(text) {
const element = this._createElement('div', 'lighthouse-text');
const element = this._dom.createElement('div', 'lighthouse-text');
element.textContent = text.text;
return element;
}
Expand All @@ -145,7 +225,7 @@ window.ReportRenderer = class ReportRenderer {
* @return {!Element}
*/
_renderBlock(block) {
const element = this._createElement('div', 'lighthouse-block');
const element = this._dom.createElement('div', 'lighthouse-block');
for (const item of block.items) {
element.appendChild(this._renderDetails(item));
}
Expand All @@ -157,14 +237,14 @@ window.ReportRenderer = class ReportRenderer {
* @return {!Element}
*/
_renderList(list) {
const element = this._createElement('details', 'lighthouse-list');
const element = this._dom.createElement('details', 'lighthouse-list');
if (list.header) {
const summary = this._createElement('summary', 'lighthouse-list__header');
const summary = this._dom.createElement('summary', 'lighthouse-list__header');
summary.textContent = list.header.text;
element.appendChild(summary);
}

const items = this._createElement('div', 'lighthouse-list__items');
const items = this._dom.createElement('div', 'lighthouse-list__items');
for (const item of list.items) {
items.appendChild(this._renderDetails(item));
}
Expand All @@ -177,7 +257,7 @@ window.ReportRenderer = class ReportRenderer {
* @return {!Element}
*/
_renderException(e) {
const element = this._createElement('div', 'lighthouse-exception');
const element = this._dom.createElement('div', 'lighthouse-exception');
element.textContent = String(e.stack);
return element;
}
Expand All @@ -187,7 +267,7 @@ window.ReportRenderer = class ReportRenderer {
* @return {!Element}
*/
_renderReport(report) {
const element = this._createElement('div', 'lighthouse-report');
const element = this._dom.createElement('div', 'lighthouse-report');
for (const category of report.reportCategories) {
element.appendChild(this._renderCategory(category));
}
Expand All @@ -199,8 +279,10 @@ window.ReportRenderer = class ReportRenderer {
* @return {!Element}
*/
_renderCategory(category) {
const element = this._createElement('div', 'lighthouse-category');
element.appendChild(this._renderScore(category.score, category.name, category.description));
const element = this._dom.createElement('div', 'lighthouse-category');
const score = {value: Math.round(category.score), scoringMode: 'numeric'};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to have this as an object? Seems fine as separate arguments to renderCategoryScore/renderAuditScore (and you may not even need scoringMode in there, at least not yet)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just kidding, you do need scoringMode for the class name, but still seems like they should be separate parameters :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

element.appendChild(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_renderCategoryScore only takes three arguments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

this._renderAuditScore(score, category.name, category.description, category));
for (const audit of category.audits) {
element.appendChild(this._renderAudit(audit));
}
Expand All @@ -212,19 +294,35 @@ window.ReportRenderer = class ReportRenderer {
* @return {!Element}
*/
_renderAudit(audit) {
const element = this._createElement('div', 'lighthouse-audit');
const element = this._dom.createElement('div', 'lighthouse-audit');
let title = audit.result.description;
if (audit.result.displayValue) {
title += ': ' + audit.result.displayValue;
title += `: ${audit.result.displayValue}`;
}
if (audit.result.optimalValue) {
title += ` (target: ${audit.result.optimalValue})`;
}
element.appendChild(this._renderScore(audit.score, title, audit.result.helpText));
if (audit.result.details) {
element.appendChild(this._renderDetails(audit.result.details));

const score = {value: audit.score, scoringMode: audit.result.scoringMode};
element.appendChild(this._renderAuditScore(score, title, audit.result.helpText, audit));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also doesn't need the fourth argument

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


// Append audit details to to header section so the entire audit is within one <details>.
const header = element.querySelector('.lighthouse-score__header');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this depends on markup introduced by _renderAuditScore, move header/details stuff in there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if (header) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in what cases might there not be a header?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was meant to protect against null querySelector results like methods. But removed since we don't care about that anymore.

header.open = audit.score < 100; // expand failed audits
if (audit.result.details) {
header.appendChild(this._renderDetails(audit.result.details));
}
}

return element;
}
};
})();
}

// Exports
self.ReportRenderer = ReportRenderer;
self.DOM = DOM;
})(self);

/** @typedef {{type: string, text: string|undefined, header: DetailsJSON|undefined, items: Array<DetailsJSON>|undefined}} */
let DetailsJSON; // eslint-disable-line no-unused-vars
Expand Down
Loading