Skip to content

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

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.

Sign up for GitHub

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

Merged
patrickhulce merged 26 commits into from
May 20, 2019
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
Fall back to tagName if no other label
  • Loading branch information
@mattzeunert
mattzeunert committed May 16, 2019
commit 37e3e11d152edb038cedfc6a0fd08b85f3adb3b0
23 changes: 11 additions & 12 deletions lighthouse-core/lib/page-functions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,20 +224,19 @@ function getNodeSelector(node) {
*/
/* istanbul ignore next */
function getNodeTitle(node) {
if (node.tagName === 'HTML' || node.tagName === 'BODY') {
// too broad, contains all page content
return null;
}
const title = node.innerText || node.getAttribute('alt') || node.getAttribute('aria-label');
if (title) {
return truncate(title, 80);
} else {
const nodeToUseForTitle = node.querySelector('[alt], [aria-label]');
if (nodeToUseForTitle) {
return getNodeTitle(/** @type {HTMLElement} */ (nodeToUseForTitle));
const tagName = node.tagName.toLowerCase();
if (tagName !== 'html' && tagName !== 'body') {
Copy link
Contributor

@brendankenny brendankenny May 17, 2019

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 title = node.innerText || node.getAttribute('alt') || node.getAttribute('aria-label');
Copy link
Contributor

@brendankenny brendankenny May 17, 2019

Choose a reason for hiding this comment

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

not a big deal, but I guess s/title/label

Copy link
Contributor

@brendankenny brendankenny May 17, 2019

Choose a reason for hiding this comment

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

also should use the standard node.textContent (unless innerText was intentional?)

Copy link
Collaborator

@patrickhulce patrickhulce May 18, 2019

Choose a reason for hiding this comment

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

I specifically requested innerText because textContent gets us all that inline script instead of actual text

if (title) {
return truncate(title, 80);
} else {
const nodeToUseForTitle = node.querySelector('[alt], [aria-label]');
Copy link
Contributor

@brendankenny brendankenny May 17, 2019

Choose a reason for hiding this comment

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

maybe add comment about why it's useful to recursively search (what cases does this help with?)

if (nodeToUseForTitle) {
return getNodeTitle(/** @type {HTMLElement} */ (nodeToUseForTitle));
}
}
}
return null;
return tagName;
}

/**
Expand Down
8 changes: 8 additions & 0 deletions lighthouse-core/test/lib/page-functions-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,5 +77,13 @@ describe('Page Functions', () => {
el.setAttribute('alt', Array(100).fill('a').join(''));
assert.equal(pageFunctions.getNodeTitle(el).length, 80);
});
it('Uses tag name for html tags', () => {
const el = dom.createElement('html');
assert.equal(pageFunctions.getNodeTitle(el), 'html');
});
it('Uses tag name if there is no better label', () => {
const el = dom.createElement('div');
assert.equal(pageFunctions.getNodeTitle(el), 'div');
});
});
});