-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
Changes from all commits
9a52573
7c5a98b
197d907
0af87da
42712bb
51cc42a
a97aba9
060d2e9
2e50172
0bec94e
8bba61f
fc4f272
37e3e11
eab63fc
6fdeac2
eb3406c
c9094c4
7880109
6993160
533ad17
1d11422
97fdad4
5fbdf5d
f893550
8e64be0
ea1621d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -187,7 +187,7 @@ function getNodePath(node) { | |
|
||
/** | ||
* @param {Element} node | ||
* @returns {string} | ||
* @return {string} | ||
*/ | ||
/* istanbul ignore next */ | ||
function getNodeSelector(node) { | ||
|
@@ -218,6 +218,47 @@ function getNodeSelector(node) { | |
return parts.join(' > '); | ||
} | ||
|
||
/** | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) + '…'; | ||
} | ||
mattzeunert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you thinking have it in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(), | ||
|
@@ -230,4 +271,6 @@ module.exports = { | |
getNodePathString: getNodePath.toString(), | ||
getNodeSelectorString: getNodeSelector.toString(), | ||
getNodeSelector: getNodeSelector, | ||
getNodeLabel: getNodeLabel, | ||
getNodeLabelString: getNodeLabel.toString(), | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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. */ | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -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 */ | ||
|
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
details.items
assertions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
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! 😎