core(report): show nodeLabel for DOM nodes in addition to snippet by mattzeunert · Pull Request #8961 · GoogleChrome/lighthouse

mattzeunert · 2019-05-15T16:17:24Z

Summary

Collect a human-readable title for each node and display it above the snippet.

Related Issues/PRs

brendankenny

nice! Definitely helps give context better than the snippet alone. I wonder if there's more we could do to tie the new text and the snippet together visually, though

brendankenny · 2019-05-15T19:29:11Z

@@ -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',


can you add to the a11y expectations as well?

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

Added details.items assertions.

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

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

mattzeunert · 2019-05-16T13:37:31Z

I wonder if there's more we could do to tie the new text and the snippet together visually, though

Removed some unnecessary margin between the two. Not sure what else to do exactly, @hwikyounglee any suggestions?

hwikyounglee · 2019-05-16T15:59:48Z

I wonder if there's more we could do to tie the new text and the snippet together visually, though

Removed some unnecessary margin between the two. Not sure what else to do exactly, @hwikyounglee any suggestions?

Thanks Matt. Only thing I would double check is the contrast of the green text on gray and on white. We need to aim for 4.5 at minimum.

@yuinchien for any further guidance

mattzeunert · 2019-05-16T16:13:07Z

Only thing I would double check is the contrast of the green text on gray and on white. We need to aim for 4.5 at minimum.

Good point. We weren't using a var for it before, using --color-green-700 for now:

brendankenny

this looks great. Last nits

brendankenny · 2019-05-17T23:16:13Z

@@ -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',


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

brendankenny · 2019-05-17T23:17:53Z

+function getNodeLabel(node) {
+  const tagName = node.tagName.toLowerCase();
+  if (tagName !== 'html' && tagName !== 'body') {
+    const title = node.innerText || node.getAttribute('alt') || node.getAttribute('aria-label');


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

brendankenny · 2019-05-17T23:22:24Z

+function getNodeLabel(node) {
+  const tagName = node.tagName.toLowerCase();
+  if (tagName !== 'html' && tagName !== 'body') {
+    const title = node.innerText || node.getAttribute('alt') || node.getAttribute('aria-label');


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

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

brendankenny · 2019-05-17T23:22:48Z

  renderNode(item) {
    const element = this._dom.createElement('span', 'lh-node');
+    if (item.nodeLabel) {
+      const titleEl = this._dom.createElement('div');


labelEl, etc

brendankenny · 2019-05-17T23:25:47Z

  return parts.join(' > ');
 }

+/**


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.

brendankenny · 2019-05-17T23:26:23Z

+/* istanbul ignore next */
+function getNodeLabel(node) {
+  const tagName = node.tagName.toLowerCase();
+  if (tagName !== 'html' && tagName !== 'body') {


can you add back that comment about why we skip html and body

brendankenny · 2019-05-17T23:26:50Z

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


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

mattzeunert · 2019-05-18T11:33:59Z

  --color-blue-200: #90CAF9;
  --color-blue-900: #0D47A1;
+  --color-teal-300: #00d6c1;
+  --color-teal-700: #008577;


Re-introduced teal since it was a bit nicer than the green. Do the numbers at the end have a specific meaning, other than broadly identifying darkness?

It's the shade in the material design color palette https://material.io/design/color/#

is this the right number, then?

Changed it to use material design colors.

Also cut the snippet line height a bit.

brendankenny

LGTM!

@patrickhulce do you want to take another look?

brendankenny · 2019-05-20T18:49:06Z

  --color-blue-200: #90CAF9;
  --color-blue-900: #0D47A1;
+  --color-teal-300: #00d6c1;
+  --color-teal-700: #008577;


is this the right number, then?

patrickhulce

I don't love the truncate situation, but I won't block an otherwise approved merge because of it either.

I'll just pick "Comment" instead :)

LGTM otherwise!

patrickhulce · 2019-05-20T20:37:12Z

@@ -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',


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

patrickhulce · 2019-05-20T20:39:45Z

+  if (tagName !== 'html' && tagName !== 'body') {
+    const nodeLabel = node.innerText || node.getAttribute('alt') || node.getAttribute('aria-label');
+    if (nodeLabel) {
+      return truncate(nodeLabel, 80);


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?

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

Yeah that was my thought

patrickhulce · 2019-05-20T20:40:16Z

 .lh-vars.dark {
  --color-red-700: var(--color-red);
  --color-green-700: var(--color-green);
+  --color-teal-600: var(--color-cyan-500);


dark mode is weird o.O

brendankenny · 2019-05-20T20:47:12Z

I don't love the truncate situation, but I won't block an otherwise approved merge because of it either.

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.

with this, tap-targets, iframeelements, and...other stuff(?) making changes to page-functions in current PRs, my vote is definitely to make things simple rather than have to come back later and try to untangle (which will be really hard because debugging evaluateAsync functions in gatherers is really hard)

mattzeunert · 2019-05-20T20:57:00Z

Maybe the exported fnString shouldn't just be fn.toString() but also all its dependencies?

…d a new element at the top of the page

patrickhulce · 2019-05-20T21:00:22Z

Maybe the exported fnString shouldn't just be fn.toString() but also all its dependencies?

Yeah that's a good idea, though perhaps dupes might be an issue then? Since the function is so simple here I think inline is decent option, but we'll have to figure this out long-term for sure either.

mattzeunert · 2019-05-20T21:09:17Z

Maybe the exported fnString shouldn't just be fn.toString() but also all its dependencies?

Yeah that's a good idea, though perhaps dupes might be an issue then? Since the function is so simple here I think inline is decent option, but we'll have to figure this out long-term for sure either.

Is there an issue with dupes other than aesthetics? There'd be a perf impact but that seems very minor.

patrickhulce · 2019-05-20T21:16:52Z

Is there an issue with dupes other than aesthetics?

I was thinking of variable declarations not function declarations, but I suppose all our dupes would be of function declarations, so it's a non-issue. 👍

patrickhulce

LGTM! Thanks @mattzeunert for the fix! 🙏

)

mattzeunert added 3 commits May 15, 2019 16:56

Collect and show title for elements

9a52573

Update sample data

7c5a98b

Test cases

197d907

mattzeunert requested review from brendankenny, exterkamp, patrickhulce and paulirish as code owners May 15, 2019 16:17

mattzeunert mentioned this pull request May 15, 2019

WIP: Element screenshots #8797

Closed

6 tasks

mattzeunert added 2 commits May 15, 2019 18:15

Revert artifact changes

0af87da

Try change artifacts again

42712bb

vercel Bot deployed to staging May 15, 2019 17:22 View deployment

connorjclark reviewed May 15, 2019

View reviewed changes

Comment thread lighthouse-core/report/html/report-styles.css Outdated

connorjclark reviewed May 15, 2019

View reviewed changes

Comment thread lighthouse-core/lib/page-functions.js Outdated

brendankenny reviewed May 15, 2019

View reviewed changes

mattzeunert added 5 commits May 16, 2019 13:57

Fix text color in dark mode

51cc42a

Test case for truncation

a97aba9

@returns -> @return

060d2e9

Remove margin

2e50172

Fix TS types

0bec94e

vercel Bot deployed to staging May 16, 2019 13:29 View deployment

@return

8bba61f

vercel Bot deployed to staging May 16, 2019 13:31 View deployment

mattzeunert mentioned this pull request May 16, 2019

Improve how HTML elements are displayed in the results table #6683

Closed

Use var for snippet color

fc4f272

vercel Bot deployed to staging May 16, 2019 16:11 View deployment

mattzeunert added 2 commits May 16, 2019 17:16

Fall back to tagName if no other label

37e3e11

Update sample json

eab63fc

vercel Bot deployed to staging May 17, 2019 10:06 View deployment

mattzeunert changed the title ~~core(report): show title for DOM nodes in addition to snippet~~ core(report): show nodeLabel for DOM nodes in addition to snippet May 17, 2019

brendankenny reviewed May 17, 2019

View reviewed changes

brendankenny mentioned this pull request May 18, 2019

Improve node selectors shown in the details table #4015

Closed

More feedback

1d11422

vercel Bot deployed to staging May 18, 2019 11:08 View deployment

Teal

97fdad4

vercel Bot deployed to staging May 18, 2019 11:31 View deployment

mattzeunert commented May 18, 2019

View reviewed changes

brendankenny approved these changes May 20, 2019

View reviewed changes

Material colors and reduce snippet line height from 2em to 1.5em

5fbdf5d

vercel Bot deployed to staging May 20, 2019 19:32 View deployment

patrickhulce reviewed May 20, 2019

View reviewed changes

Remove path from a11y expectations because all paths change if you ad…

f893550

…d a new element at the top of the page

vercel Bot deployed to staging May 20, 2019 21:00 View deployment

Remove getNodeLabel dependency on truncate

8e64be0

vercel Bot deployed to staging May 20, 2019 21:06 View deployment

Fix

ea1621d

vercel Bot deployed to staging May 20, 2019 21:10 View deployment

patrickhulce approved these changes May 20, 2019

View reviewed changes

patrickhulce merged commit 6471b5f into master May 20, 2019

patrickhulce deleted the show-node-text branch May 20, 2019 22:15

paulirish pushed a commit that referenced this pull request May 28, 2019

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

129e367

)

brendankenny mentioned this pull request Jul 24, 2019

tests: relax contrast font size expectation #9441

Merged

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

Conversation

mattzeunert commented May 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattzeunert commented May 16, 2019

Uh oh!

hwikyounglee commented May 16, 2019

Uh oh!

mattzeunert commented May 16, 2019

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendankenny May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattzeunert commented May 15, 2019 •

edited

Loading

brendankenny May 17, 2019 •

edited

Loading

mattzeunert commented May 20, 2019 •

edited

Loading