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: make urls clickable #9224

Merged
merged 18 commits into from
Jun 29, 2019
Merged
31 changes: 18 additions & 13 deletions lighthouse-core/report/html/renderer/crc-details-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,10 @@ class CriticalRequestChainRenderer {
* @param {DOM} dom
* @param {DocumentFragment} tmpl
* @param {CRCSegment} segment
* @param {DetailsRenderer} detailsRenderer
* @return {Node}
*/
static createChainNode(dom, tmpl, segment) {
static createChainNode(dom, tmpl, segment, detailsRenderer) {
const chainsEl = dom.cloneTemplate('#tmpl-lh-crc__chains', tmpl);

// Hovering over request shows full URL.
Expand Down Expand Up @@ -120,10 +121,10 @@ class CriticalRequestChainRenderer {
}

// Fill in url, host, and request size information.
const {file, hostname} = Util.parseURL(segment.node.request.url);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tests are failing b/c of this change. DetailsRenderer.renderTextURL tosses the host part if parsed.file === '/'. Before, the chain renderer always used the results of Util.parseURL w/o post-processing. The chain renderer now looks like this when run on the root url of a site:

image

before:

image

Copy link
Member

Choose a reason for hiding this comment

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

The chain renderer now looks like this when run on the root url of a site:

this is an improvement, IMO.

const url = segment.node.request.url;
const linkEl = detailsRenderer.renderTextURL(url);
const treevalEl = dom.find('.crc-node__tree-value', chainsEl);
dom.find('.crc-node__tree-file', treevalEl).textContent = `${file}`;
dom.find('.crc-node__tree-hostname', treevalEl).textContent = hostname ? `(${hostname})` : '';
treevalEl.appendChild(linkEl);

if (!segment.hasChildren) {
const {startTime, endTime, transferSize} = segment.node.request;
Expand All @@ -146,14 +147,15 @@ class CriticalRequestChainRenderer {
* @param {CRCSegment} segment
* @param {Element} elem Parent element.
* @param {LH.Audit.Details.CriticalRequestChain} details
* @param {DetailsRenderer} detailsRenderer
*/
static buildTree(dom, tmpl, segment, elem, details) {
elem.appendChild(CriticalRequestChainRenderer.createChainNode(dom, tmpl, segment));
static buildTree(dom, tmpl, segment, elem, details, detailsRenderer) {
elem.appendChild(CRCRenderer.createChainNode(dom, tmpl, segment, detailsRenderer));
if (segment.node.children) {
for (const key of Object.keys(segment.node.children)) {
const childSegment = CriticalRequestChainRenderer.createSegment(segment.node.children, key,
const childSegment = CRCRenderer.createSegment(segment.node.children, key,
segment.startTime, segment.transferSize, segment.treeMarkers, segment.isLastChild);
CriticalRequestChainRenderer.buildTree(dom, tmpl, childSegment, elem, details);
CRCRenderer.buildTree(dom, tmpl, childSegment, elem, details, detailsRenderer);
}
}
}
Expand All @@ -162,9 +164,10 @@ class CriticalRequestChainRenderer {
* @param {DOM} dom
* @param {ParentNode} templateContext
* @param {LH.Audit.Details.CriticalRequestChain} details
* @param {DetailsRenderer} detailsRenderer
* @return {Element}
*/
static render(dom, templateContext, details) {
static render(dom, templateContext, details, detailsRenderer) {
const tmpl = dom.cloneTemplate('#tmpl-lh-crc', templateContext);
const containerEl = dom.find('.lh-crc', tmpl);

Expand All @@ -176,17 +179,19 @@ class CriticalRequestChainRenderer {
Util.formatMilliseconds(details.longestChain.duration);

// Construct visual tree.
const root = CriticalRequestChainRenderer.initTree(details.chains);
const root = CRCRenderer.initTree(details.chains);
for (const key of Object.keys(root.tree)) {
const segment = CriticalRequestChainRenderer.createSegment(root.tree, key,
root.startTime, root.transferSize);
CriticalRequestChainRenderer.buildTree(dom, tmpl, segment, containerEl, details);
const segment = CRCRenderer.createSegment(root.tree, key, root.startTime, root.transferSize);
CRCRenderer.buildTree(dom, tmpl, segment, containerEl, details, detailsRenderer);
}

return dom.find('.lh-crc-container', tmpl);
}
}

// Alias b/c the name is really long.
Copy link
Collaborator

Choose a reason for hiding this comment

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

😆 this is great

const CRCRenderer = CriticalRequestChainRenderer;

// Allow Node require()'ing.
if (typeof module !== 'undefined' && module.exports) {
module.exports = CriticalRequestChainRenderer;
Expand Down
12 changes: 6 additions & 6 deletions lighthouse-core/report/html/renderer/details-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class DetailsRenderer {
case 'table':
return this._renderTable(details);
case 'criticalrequestchain':
return CriticalRequestChainRenderer.render(this._dom, this._templateContext, details);
return CriticalRequestChainRenderer.render(this._dom, this._templateContext, details, this);
case 'opportunity':
return this._renderTable(details);

Expand Down Expand Up @@ -97,7 +97,7 @@ class DetailsRenderer {
* @param {string} text
* @return {HTMLElement}
*/
_renderTextURL(text) {
renderTextURL(text) {
const url = text;

let displayedPath;
Expand All @@ -113,7 +113,7 @@ class DetailsRenderer {
}

const element = this._dom.createElement('div', 'lh-text__url');
element.appendChild(this._renderText(displayedPath));
element.appendChild(this._renderLink({text: displayedPath, url}));

if (displayedHost) {
const hostElem = this._renderText(displayedHost);
Expand All @@ -130,7 +130,7 @@ class DetailsRenderer {
}

/**
* @param {LH.Audit.Details.LinkValue} details
* @param {{text: string, url: string}} details
* @return {Element}
*/
_renderLink(details) {
Expand Down Expand Up @@ -211,7 +211,7 @@ class DetailsRenderer {
return this.renderNode(value);
}
case 'url': {
return this._renderTextURL(value.value);
return this.renderTextURL(value.value);
}
default: {
throw new Error(`Unknown valueType: ${value.type}`);
Expand Down Expand Up @@ -256,7 +256,7 @@ class DetailsRenderer {
case 'url': {
const strValue = String(value);
if (URL_PREFIXES.some(prefix => strValue.startsWith(prefix))) {
return this._renderTextURL(strValue);
return this.renderTextURL(strValue);
} else {
// Fall back to <pre> rendering if not actually a URL.
return this._renderCode(strValue);
Expand Down
15 changes: 10 additions & 5 deletions lighthouse-core/report/html/report-styles.css
Original file line number Diff line number Diff line change
Expand Up @@ -1362,12 +1362,8 @@
width: 12%;
}

.lh-text__url:hover {
text-decoration: underline dotted #999;
text-decoration-skip-ink: auto;
}

.lh-text__url > .lh-text, .lh-text__url-host {
.lh-text__url-host {
display: inline;
Copy link
Member

Choose a reason for hiding this comment

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

@paulirish why is this a div in the first place then?

}

Expand All @@ -1383,6 +1379,15 @@
object-fit: contain;
}

.lh-text__url > a {
color: inherit;
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
text-decoration: none;
}

.lh-text__url > a:hover {
text-decoration: underline dotted #999;
}

/* Chevron
https://codepen.io/paulirish/pen/LmzEmK
*/
Expand Down
8 changes: 3 additions & 5 deletions lighthouse-core/report/html/templates.html
Original file line number Diff line number Diff line change
Expand Up @@ -741,12 +741,12 @@
.lh-crc .crc-node__tree-value {
margin-left: 10px;
}
.lh-crc .crc-node__tree-value div {
display: inline;
}
.lh-crc .crc-node__chain-duration {
font-weight: 700;
}
.lh-crc .crc-node__tree-hostname {
color: #595959;
}
.lh-crc .crc-initial-nav {
color: #595959;
font-style: italic;
Expand All @@ -769,8 +769,6 @@

</span>
<span class="crc-node__tree-value">
<span class="crc-node__tree-file"><!-- fill me: node.request.url.file --></span>
<span class="crc-node__tree-hostname">(<!-- fill me: node.request.url.host -->)</span>

</span>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const jsdom = require('jsdom');
const URL = require('../../../../lib/url-shim.js');
const Util = require('../../../../report/html/renderer/util.js');
const DOM = require('../../../../report/html/renderer/dom.js');
const DetailsRenderer = require('../../../../report/html/renderer/details-renderer.js');
const CriticalRequestChainRenderer =
require('../../../../report/html/renderer/crc-details-renderer.js');

Expand Down Expand Up @@ -75,12 +76,14 @@ const DETAILS = {

describe('DetailsRenderer', () => {
let dom;
let detailsRenderer;

beforeAll(() => {
global.URL = URL;
global.Util = Util;
const {document} = new jsdom.JSDOM(TEMPLATE_FILE).window;
dom = new DOM(document);
detailsRenderer = new DetailsRenderer(dom);
});

afterAll(() => {
Expand All @@ -89,18 +92,25 @@ describe('DetailsRenderer', () => {
});

it('renders tree structure', () => {
const el = CriticalRequestChainRenderer.render(dom, dom.document(), DETAILS);
const el = CriticalRequestChainRenderer.render(dom, dom.document(), DETAILS, detailsRenderer);
const chains = el.querySelectorAll('.crc-node');

// Main request
assert.equal(chains.length, 4, 'generates correct number of chain nodes');
assert.equal(chains[0].querySelector('.crc-node__tree-hostname').textContent, '(example.com)');
assert.ok(!chains[0].querySelector('.lh-text__url-host'), 'should be no origin for root url');
assert.equal(chains[0].querySelector('.lh-text__url a').textContent, 'https://example.com');
assert.equal(chains[0].querySelector('.lh-text__url a').href, 'https://example.com/');
assert.equal(chains[0].querySelector('.lh-text__url a').rel, 'noopener');
assert.equal(chains[0].querySelector('.lh-text__url a').target, '_blank');

// Children
assert.ok(chains[1].querySelector('.crc-node__tree-marker .vert-right'));
assert.equal(chains[1].querySelectorAll('.crc-node__tree-marker .right').length, 2);
assert.equal(chains[1].querySelector('.crc-node__tree-file').textContent, '/b.js');
assert.equal(chains[1].querySelector('.crc-node__tree-hostname').textContent, '(example.com)');
assert.equal(chains[1].querySelector('.lh-text__url a').textContent, '/b.js');
assert.equal(chains[1].querySelector('.lh-text__url a').href, 'https://example.com/b.js');
assert.equal(chains[1].querySelector('.lh-text__url a').rel, 'noopener');
assert.equal(chains[1].querySelector('.lh-text__url a').target, '_blank');
assert.equal(chains[1].querySelector('.lh-text__url-host').textContent, '(example.com)');
Copy link
Member

Choose a reason for hiding this comment

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

these tests should also be expanded

const durationNodes = chains[1].querySelectorAll('.crc-node__chain-duration');
assert.equal(durationNodes[0].textContent, ' - 5,000\xa0ms, ');
// Note: actual transferSize is 2000 bytes but formatter formats to KBs.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,10 @@ describe('DetailsRenderer', () => {
assert.equal(urlEl.localName, 'div');
assert.equal(urlEl.title, urlText);
assert.equal(urlEl.dataset.url, urlText);
assert.ok(urlEl.firstChild.classList.contains('lh-text'));
assert.equal(urlEl.firstChild.nodeName, 'A');
Copy link
Member

Choose a reason for hiding this comment

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

can you expand the tests of this anchor (here and below) now that it's more than just an .lh-text with the url? Can probably c/p from the renders link values test (though that doesn't appear to check the .lh-text__url-host stuff?)

assert.equal(urlEl.firstChild.href, urlText);
assert.equal(urlEl.firstChild.rel, 'noopener');
assert.equal(urlEl.firstChild.target, '_blank');
assert.equal(urlEl.textContent, displayUrlText);
});

Expand All @@ -410,7 +413,7 @@ describe('DetailsRenderer', () => {
assert.equal(urlEl.localName, 'div');
assert.equal(urlEl.title, urlText);
assert.equal(urlEl.dataset.url, urlText);
assert.ok(urlEl.firstChild.classList.contains('lh-text'));
assert.equal(urlEl.firstChild.nodeName, 'A');
assert.equal(urlEl.textContent, displayUrlText);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ describe('ReportUIFeatures', () => {

function getUrlsInTable() {
return dom
.findAll('#uses-webp-images .lh-details .lh-text__url .lh-text:first-child', container)
.findAll('#uses-webp-images .lh-details .lh-text__url a:first-child', container)
.map(el => el.textContent);
}

Expand Down