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

core(report): show nodeLabel for DOM nodes in addition to snippet #8961

Merged
merged 26 commits into from
May 20, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
9a52573
Collect and show title for elements
mattzeunert May 15, 2019
7c5a98b
Update sample data
mattzeunert May 15, 2019
197d907
Test cases
mattzeunert May 15, 2019
0af87da
Revert artifact changes
mattzeunert May 15, 2019
42712bb
Try change artifacts again
mattzeunert May 15, 2019
51cc42a
Fix text color in dark mode
mattzeunert May 16, 2019
a97aba9
Test case for truncation
mattzeunert May 16, 2019
060d2e9
@returns -> @return
mattzeunert May 16, 2019
2e50172
Remove margin
mattzeunert May 16, 2019
0bec94e
Fix TS types
mattzeunert May 16, 2019
8bba61f
@return
mattzeunert May 16, 2019
fc4f272
Use var for snippet color
mattzeunert May 16, 2019
37e3e11
Fall back to tagName if no other label
mattzeunert May 16, 2019
eab63fc
Update sample json
mattzeunert May 16, 2019
6fdeac2
Revert summary
mattzeunert May 16, 2019
eb3406c
title -> nodeLabel
mattzeunert May 17, 2019
c9094c4
Update sample json
mattzeunert May 17, 2019
7880109
More detailed a11y smoke test expectations
mattzeunert May 17, 2019
6993160
Remove unnecesary styles
mattzeunert May 17, 2019
533ad17
Fix tap targets smoke test
mattzeunert May 17, 2019
1d11422
More feedback
mattzeunert May 18, 2019
97fdad4
Teal
mattzeunert May 18, 2019
5fbdf5d
Material colors and reduce snippet line height from 2em to 1.5em
mattzeunert May 20, 2019
f893550
Remove path from a11y expectations because all paths change if you ad…
mattzeunert May 20, 2019
8e64be0
Remove getNodeLabel dependency on truncate
mattzeunert May 20, 2019
ea1621d
Fix
mattzeunert May 20, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
387 changes: 303 additions & 84 deletions lighthouse-cli/test/smokehouse/a11y/expectations.js

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions lighthouse-cli/test/smokehouse/seo/expectations.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ module.exports = [
'\n too small target\n </a>',
'path': '2,HTML,1,BODY,3,DIV,21,DIV,0,A',
'selector': 'body > div > div > a',
'nodeLabel': 'too small target',
},
'overlappingTarget': {
'type': 'node',
Expand All @@ -213,6 +214,7 @@ module.exports = [
'\n big enough target\n </a>',
'path': '2,HTML,1,BODY,3,DIV,21,DIV,1,A',
'selector': 'body > div > div > 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 add to the a11y expectations as well?

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 now all we assert there is { length: 1 }, but I'll change it when I do the final artifact update for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added details.items assertions.

Copy link
Member

Choose a reason for hiding this comment

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

Right now all we assert there is { length: 1 }, but I'll change it when I do the final artifact update for this PR.

haha, uh oh, I guess I didn't know how much I was asking for :) I guess we can just cross our fingers that these are stable and all worth testing. I do really like the extra coverage this gets us

Copy link
Collaborator

Choose a reason for hiding this comment

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

When @mattzeunert does something, he does it right, and you get the works! 😎

'nodeLabel': 'big enough target',
},
'size': '100x30',
'width': 100,
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/accessibility/axe-audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class AxeAudit extends Audit {
path: node.path,
snippet: node.html || node.snippet,
explanation: node.failureSummary,
nodeLabel: node.nodeLabel,
}),
}));
}
Expand Down
1 change: 1 addition & 0 deletions lighthouse-core/audits/seo/tap-targets.js
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ function targetToTableNode(target) {
snippet: target.snippet,
path: target.path,
selector: target.selector,
nodeLabel: target.nodeLabel,
};
}

Expand Down
5 changes: 4 additions & 1 deletion lighthouse-core/gather/gatherers/accessibility.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
'use strict';

/* global window, document, getOuterHTMLSnippet, getNodePath */
/* global window, document, getOuterHTMLSnippet, getNodePath, getNodeLabel */

const Gatherer = require('./gatherer');
const fs = require('fs');
Expand Down Expand Up @@ -57,6 +57,8 @@ function runA11yChecks() {
node.path = getNodePath(node.element);
// @ts-ignore - getOuterHTMLSnippet put into scope via stringification
node.snippet = getOuterHTMLSnippet(node.element);
// @ts-ignore - getNodeLabel put into scope via stringification
node.nodeLabel = getNodeLabel(node.element);
// avoid circular JSON concerns
node.element = node.any = node.all = node.none = undefined;
}));
Expand All @@ -77,6 +79,7 @@ class Accessibility extends Gatherer {
const expression = `(function () {
${pageFunctions.getOuterHTMLSnippetString};
${pageFunctions.getNodePathString};
${pageFunctions.getNodeLabelString};
${axeLibSource};
return (${runA11yChecks.toString()}());
})()`;
Expand Down
7 changes: 5 additions & 2 deletions lighthouse-core/gather/gatherers/seo/tap-targets.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
'use strict';

/* global getComputedStyle, getElementsInDocument, Node, getNodePath, getNodeSelector */
/* global getComputedStyle, getElementsInDocument, Node, getNodePath, getNodeSelector, getNodeLabel */

const Gatherer = require('../gatherer');
const pageFunctions = require('../../../lib/page-functions.js');
Expand Down Expand Up @@ -239,7 +239,7 @@ function elementIsPositionFixedStickyOrAbsolute(element) {
/**
* @param {string} str
* @param {number} maxLength
* @returns {string}
* @return {string}
*/
/* istanbul ignore next */
function truncate(str, maxLength) {
Expand Down Expand Up @@ -294,6 +294,8 @@ function gatherTapTargets() {
path: getNodePath(tapTargetElement),
// @ts-ignore - getNodeSelector put into scope via stringification
selector: getNodeSelector(tapTargetElement),
// @ts-ignore - getNodeLabel put into scope via stringification
nodeLabel: getNodeLabel(tapTargetElement),
href: /** @type {HTMLAnchorElement} */(tapTargetElement)['href'] || '',
});
});
Expand Down Expand Up @@ -323,6 +325,7 @@ class TapTargets extends Gatherer {
${rectContainsString};
${pageFunctions.getNodePathString};
${pageFunctions.getNodeSelectorString};
${pageFunctions.getNodeLabelString};
${gatherTapTargets.toString()};

return gatherTapTargets();
Expand Down
45 changes: 44 additions & 1 deletion lighthouse-core/lib/page-functions.js
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ function getNodePath(node) {

/**
* @param {Element} node
* @returns {string}
* @return {string}
*/
/* istanbul ignore next */
function getNodeSelector(node) {
Expand Down Expand Up @@ -218,6 +218,47 @@ function getNodeSelector(node) {
return parts.join(' > ');
}

/**
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 add a comment for this function? Basically trying to provide a human recognizable label for the given element. Falls back to the tagName if no useful label is found.

* Generate a human-readable label for the given element, based on end-user facing
* strings like the innerText or alt attribute.
* Falls back to the tagName if no useful label is found.
* @param {HTMLElement} node
* @return {string|null}
*/
/* istanbul ignore next */
function getNodeLabel(node) {
// Inline so that audits that import getNodeLabel don't
// also need to import truncate
/**
* @param {string} str
* @param {number} maxLength
* @return {string}
*/
function truncate(str, maxLength) {
if (str.length <= maxLength) {
return str;
}
return str.slice(0, maxLength - 1) + '…';
}

const tagName = node.tagName.toLowerCase();
// html and body content is too broad to be useful, since they contain all page content
if (tagName !== 'html' && tagName !== 'body') {
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 add back that comment about why we skip html and body

const nodeLabel = node.innerText || node.getAttribute('alt') || node.getAttribute('aria-label');
if (nodeLabel) {
return truncate(nodeLabel, 80);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was making this comment over in #8979 , but it's unfortunate when page-functions in a lib have dependencies on each other that you have to know about ahead of time.

since it's basically a single ternary line WDYT about copying the function definition in the two files instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you thinking have it in tap-targets.js and also inlined inside getNodeLabel?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah that was my thought

} else {
// If no useful label was found then try to get one from a child.
// E.g. if an a tag contains an image but no text we want the image alt/aria-label attribute.
const nodeToUseForLabel = node.querySelector('[alt], [aria-label]');
if (nodeToUseForLabel) {
return getNodeLabel(/** @type {HTMLElement} */ (nodeToUseForLabel));
}
}
}
return tagName;
}

module.exports = {
wrapRuntimeEvalErrorInBrowserString: wrapRuntimeEvalErrorInBrowser.toString(),
registerPerformanceObserverInPageString: registerPerformanceObserverInPage.toString(),
Expand All @@ -230,4 +271,6 @@ module.exports = {
getNodePathString: getNodePath.toString(),
getNodeSelectorString: getNodeSelector.toString(),
getNodeSelector: getNodeSelector,
getNodeLabel: getNodeLabel,
getNodeLabelString: getNodeLabel.toString(),
};
11 changes: 10 additions & 1 deletion lighthouse-core/report/html/renderer/details-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -352,15 +352,24 @@ class DetailsRenderer {
*/
renderNode(item) {
const element = this._dom.createElement('span', 'lh-node');
if (item.nodeLabel) {
const nodeLabelEl = this._dom.createElement('div');
nodeLabelEl.textContent = item.nodeLabel;
element.appendChild(nodeLabelEl);
}
if (item.snippet) {
element.textContent = item.snippet;
const snippetEl = this._dom.createElement('div');
snippetEl.classList.add('lh-node__snippet');
snippetEl.textContent = item.snippet;
element.appendChild(snippetEl);
}
if (item.selector) {
element.title = item.selector;
}
if (item.path) element.setAttribute('data-path', item.path);
if (item.selector) element.setAttribute('data-selector', item.selector);
if (item.snippet) element.setAttribute('data-snippet', item.snippet);

return element;
}

Expand Down
13 changes: 9 additions & 4 deletions lighthouse-core/report/html/report-styles.css
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@
--chevron-size: 12px;
--inner-audit-left-padding: calc(var(--score-shape-size) + var(--score-shape-margin-left) + var(--score-shape-margin-right));

/* Palette. */
/* Palette using Material Design Colors
* https://www.materialui.co/colors */
--color-black-100: #F5F5F5;
--color-black-200: #E0E0E0;
--color-black-400: #BDBDBD;
Expand All @@ -100,6 +101,8 @@
--color-white: #FFFFFF;
--color-blue-200: #90CAF9;
--color-blue-900: #0D47A1;
--color-cyan-500: #00BCD4;
--color-teal-600: #00897B;


/* TODO(cjamcl) clean up unused variables. */
Expand Down Expand Up @@ -175,6 +178,7 @@
.lh-vars.dark {
--color-red-700: var(--color-red);
--color-green-700: var(--color-green);
--color-teal-600: var(--color-cyan-500);
Copy link
Collaborator

Choose a reason for hiding this comment

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

dark mode is weird o.O

--color-orange-700: var(--color-orange);

--color-black-200: var(--color-black-800);
Expand Down Expand Up @@ -388,10 +392,11 @@
}

/* Node */
.lh-node {
display: block;
.lh-node__snippet {
font-family: var(--monospace-font-family);
color: hsl(174, 100%, 27%);
color: var(--color-teal-600);
font-size: 12px;
line-height: 1.5em;
}

/* Score */
Expand Down
33 changes: 33 additions & 0 deletions lighthouse-core/test/lib/page-functions-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,37 @@ describe('Page Functions', () => {
assert.equal(pageFunctions.getNodeSelector(childEl), 'div#wrapper > div.child');
});
});

describe('getNodeLabel', () => {
it('Returns innerText if element has visible text', () => {
const el = dom.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'});
el.appendChild(childEl);
assert.equal(pageFunctions.getNodeLabel(el), 'Something');
});

it('Truncates long text', () => {
const el = dom.createElement('div');
el.setAttribute('alt', Array(100).fill('a').join(''));
assert.equal(pageFunctions.getNodeLabel(el).length, 80);
});

it('Uses tag name for html tags', () => {
const el = dom.createElement('html');
assert.equal(pageFunctions.getNodeLabel(el), 'html');
});

it('Uses tag name if there is no better label', () => {
const el = dom.createElement('div');
const childEl = dom.createElement('span');
el.appendChild(childEl);
assert.equal(pageFunctions.getNodeLabel(el), 'div');
});
});
});
Loading