-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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.
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
Changes from 20 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,40 @@ function getNodeSelector(node) { | |
| return parts.join(' > '); | ||
| } | ||
|
|
||
| /** | ||
|
Contributor
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. |
||
| * @param {HTMLElement} node | ||
| * @return {string|null} | ||
| */ | ||
| /* istanbul ignore next */ | ||
| function getNodeLabel(node) { | ||
| const tagName = node.tagName.toLowerCase(); | ||
| if (tagName !== 'html' && tagName !== 'body') { | ||
|
Contributor
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 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 getNodeLabel(/** @type {HTMLElement} */ (nodeToUseForTitle)); | ||
| } | ||
| } | ||
| } | ||
mattzeunert marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return tagName; | ||
| } | ||
|
|
||
| /** | ||
| * @param {string} str | ||
| * @param {number} maxLength | ||
| * @return {string} | ||
| */ | ||
| /* istanbul ignore next */ | ||
| function truncate(str, maxLength) { | ||
| if (str.length <= maxLength) { | ||
| return str; | ||
| } | ||
| return str.slice(0, maxLength - 1) + '…'; | ||
mattzeunert marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| module.exports = { | ||
| wrapRuntimeEvalErrorInBrowserString: wrapRuntimeEvalErrorInBrowser.toString(), | ||
| registerPerformanceObserverInPageString: registerPerformanceObserverInPage.toString(), | ||
|
|
@@ -230,4 +264,7 @@ module.exports = { | |
| getNodePathString: getNodePath.toString(), | ||
| getNodeSelectorString: getNodeSelector.toString(), | ||
| getNodeSelector: getNodeSelector, | ||
| getNodeLabel: getNodeLabel, | ||
| getNodeLabelString: getNodeLabel.toString(), | ||
| truncateString: truncate.toString(), | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -352,15 +352,24 @@ class DetailsRenderer { | |
| */ | ||
| renderNode(item) { | ||
| const element = this._dom.createElement('span', 'lh-node'); | ||
| if (item.nodeLabel) { | ||
| const titleEl = this._dom.createElement('div'); | ||
|
||
| titleEl.textContent = item.nodeLabel; | ||
| element.appendChild(titleEl); | ||
| } | ||
| 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; | ||
| } | ||
|
|
||
|
|
||
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
a11yexpectations 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.itemsassertions.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! 😎