core: fallback to selector, not tagName for nodeLabel #12727

tannerdolby · 2021-06-30T18:58:50Z

Summary

Bug fix.

Under SEO, Lighthouse suggests that "Image elements do not have [alt] attributes"
But, it doesn't point out which image element doesn't have the alt attribute. (original comment)

After running:

yarn and yarn build-all
node --inspect-brk lighthouse-cli https://ravikirans.com/az-204-azure-exam-study-guide/

and then clicking on the correct target after navigating to about:inspect in chrome, the audit report correctly utilizes the fallback selector and doesn't throw any failed audit warnings for "Images not having [alt] attribute". Can include generated audit report if needed.

status Auditing: Image elements have `[alt]` attributes +22ms

Related Issues/PRs

Fixes #12668
Related #10421

google-cla · 2021-06-30T18:58:55Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

tannerdolby · 2021-06-30T19:02:49Z

@googlebot I signed it!

tannerdolby · 2021-06-30T19:41:25Z

Oh I see why the "pr title lint" failed, there is an expected PR title format ${type}(${optional-scope}): ${subject}, just updated that.

connorjclark · 2021-06-30T19:54:19Z

lighthouse-core/lib/page-functions.js

@@ -400,6 +400,7 @@ function getNodeLabel(element) {
  }

  const tagName = element.tagName.toLowerCase();
+  const selector = getNodeSelector(element);


two options here to avoid calling getNodeSelector twice per call to getNodeDetails:

have this function return null if no useful label, and use result of getNodeSelector as fallback in getNodeDetails (getNodeLabel(...) || selector)

have getNodeSelector accept a default parameter and return that as default

Yeah that makes sense, thanks for the review! I like option 1 to return null if no useful label, will implement the changes you requested shortly.

also you'll likely need to fix the tests (run yarn jest page-functions)

Added the requested changes discussed in option 1, and yep two tests failed after running yarn jest page-functions

✕ Uses tag name for html tags (7 ms) assert.strictEqual(received, expected) Expected value to strictly be equal to: "html" Received: null ✕ Uses tag name if there is no better label (2 ms) ...

connorjclark · 2021-06-30T21:07:58Z

lighthouse-core/test/lib/page-functions-test.js

@@ -177,16 +177,16 @@ describe('Page Functions', () => {
      assert.equal(pageFunctions.getNodeLabel(el), Array(78).fill('a').join('') + '💡…');
    });

-    it('Uses tag name for html tags', () => {
+    it('Returns null if nodeLabel is not usable for html tags', () => {


Can you add a test case to getNodeDetails that covers the fallack?

Yes! That's where my mind skipped, thank you, will commit change shortly :)

@connorjclark quick question, when I try to use:

const el = dom.createElement("html"); assert.equal(pageFunctions.getNodeDetails(el), { ... nodeLabel: 'html' });

its saying I should be using jsdom as window is not defined but that is already handled at the beginning of the file. Am I missing some small detail to write this test for getNodeDetails()?

you can add a global.window = {} / global.window = undefined to make that work–the first few lines of getNodeDetails (which i'm just realizing we have no unit tests for... only smoke tests) expected there to be a window variable in the global scope.

yeah that did the trick, I think I expected there to be a window variable in the global scope already too and was just stumped because there weren't any other unit tests that tested getNodeDetails and needed window.

(...which i'm just realizing we have no unit tests for... only smoke tests)

I'm happy to help write a few more unit tests for the other fields in details like devtoolsNodePath, selector, boundingRect etc similar to how were testing nodeLabel.

Here is a simple test to make sure the selector value is being used as the fallback for nodeLabel in details. We could pluck each property from details using destructuring assignment to write a few unit tests for the other properties besides nodeLabel, unless each test needed the entire details object.

Haven't looked into testing the other properties in details but I thought this was a start for potentially writing tests for the others down the line.

@connorjclark the above unit test should cover the fallback for nodeLabel when getNodeLabel() returns null, it could be a little more sturdy but let me know your thoughts.

The selector is the fallback now, not the tag name.

If you make a nested element, and give both of them a class and id, that should suffice for testing purposes.

(also, it's simpler to review if you just push your possible changes, as opposed to sharing snippets in comments)

The selector is the fallback now, not the tag name.

I think my tests didn't fully illustrate that, will update with the suggested improvements.

(ok thanks for the heads up, I will make sure to avoid sharing snippets in comments in the future)

connorjclark · 2021-07-01T19:50:20Z

lighthouse-core/test/lib/page-functions-test.js

+    it('Returns selector as fallback if nodeLabel equals html tag name', () => {
+      // define a global `window` object for the first
+      // few lines in `getNodeDetails`
+      global.window = {};


this (plus removing it) should be in the beforeAll/afterAll functions)

added it to beforeAll/afterAll, this will definitely help in writing other getNodeDetail() tests as now there is a window variable in the global scope, nice!

patrickhulce

thanks very much @tannerdolby for the contribution! this is great! 🎉

lighthouse-core/lib/page-functions.js

lighthouse-core/test/lib/page-functions-test.js

tannerdolby · 2021-07-02T18:18:00Z

your welcome @patrickhulce ! I'm glad I was able to contribute 🎉 I appreciate @connorjclark's and your help/guidance along the way!

patrickhulce

fantastic work :)

tannerdolby · 2021-07-02T20:35:21Z

thank you :) @patrickhulce ! it looks like some smoke tests are failing and expect a tagName instead of selector, let me know if there is anything else to finish up here.

and on a side note, do you think it would be worthwhile in a separate PR to write some more tests for getNodeDetails to make sure lhId, devToolsNodePath, and the other fields besides nodeLabel receive there expected values? Or since these helper functions used in details have their own describe blocks already, would it be redundant to add more tests that verify values already tested so to speak?

patrickhulce · 2021-07-02T21:34:38Z

it looks like some smoke tests are failing and expect a tagName instead of selector, let me know if there is anything else to finish up here.

Ah yeah, you're right those will need to be updated. It's fairly mechanical, just copy/paste the expected label into the expectations :)

patrickhulce · 2021-07-03T02:20:37Z

Thanks @tannerdolby! I think there are a few left

tannerdolby · 2021-07-03T02:54:39Z

Your welcome @patrickhulce! Yeah it seems the the perf smoketest still has a few cases that need to be changed. Should have everything fixed up shortly.

tannerdolby · 2021-07-06T19:15:30Z

dbw smoke test has one last expectation to fix

dbw smoketest complete.
  1 test(s) passing
  1 test(s) failing

patrickhulce · 2021-07-06T19:16:32Z

dbw smoke test has one last expectation to fix

That is actually a flaky test. We look good to go here, thank you @tannerdolby! :)

tannerdolby · 2021-07-06T19:20:33Z

Ok great! I'm happy to have been a part of this. Your welcome @patrickhulce! :)

Fallback to selector instead of tagName in nodeLabel generation

195605f

tannerdolby requested a review from a team as a code owner June 30, 2021 18:58

tannerdolby requested review from patrickhulce and removed request for a team June 30, 2021 18:58

vercel bot deployed to Preview June 30, 2021 18:58 View deployment

google-cla bot added the cla: no label Jun 30, 2021

devtools-bot assigned patrickhulce Jun 30, 2021

devtools-bot added the waiting4reviewer label Jun 30, 2021

google-cla bot added cla: yes and removed cla: no labels Jun 30, 2021

tannerdolby changed the title ~~Fallback to selector instead of tagName in nodeLabel generation~~ core: fallback to selector instead of tagName in nodeLabel generation Jun 30, 2021

connorjclark requested changes Jun 30, 2021

View reviewed changes

tannerdolby added 2 commits June 30, 2021 14:03

Refactor to avoid calling getNodeSelector twice per getNodeDetails call

f0a9597

Make tests pass for page-functions

676151a

vercel bot deployed to Preview June 30, 2021 21:05 View deployment

connorjclark reviewed Jun 30, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Jun 30, 2021

Add test for getNodeDetails to cover nodeLabel fallback

a70e04e

vercel bot deployed to Preview July 1, 2021 01:49 View deployment

tannerdolby requested a review from connorjclark July 1, 2021 01:54

connorjclark approved these changes Jul 1, 2021

View reviewed changes

Move global.window to beforeAll/afterAll and improve test

a474e28

vercel bot deployed to Preview July 1, 2021 20:13 View deployment

That makes more sense, since document is being destructured from window

ebce04c

vercel bot deployed to Preview July 1, 2021 20:23 View deployment

Make global.window undefined in afterAll()

81c2c27

vercel bot deployed to Preview July 1, 2021 21:04 View deployment

Removes useless jsdom comment now that global.window is available

8e5c132

vercel bot deployed to Preview July 2, 2021 03:15 View deployment

tannerdolby requested a review from connorjclark July 2, 2021 03:15

patrickhulce suggested changes Jul 2, 2021

View reviewed changes

lighthouse-core/lib/page-functions.js Outdated Show resolved Hide resolved

lighthouse-core/test/lib/page-functions-test.js Outdated Show resolved Hide resolved

tannerdolby added 2 commits July 2, 2021 11:14

Reuse selector instead of recomputing

b38d83a

Move getNodeDetails test to its own describe block

08cf6f5

vercel bot deployed to Preview July 2, 2021 18:16 View deployment

tannerdolby requested a review from patrickhulce July 2, 2021 18:18

patrickhulce approved these changes Jul 2, 2021

View reviewed changes

patrickhulce changed the title ~~core: fallback to selector instead of tagName in nodeLabel generation~~ core: fallback to selector, not tagName for nodeLabel Jul 2, 2021

Manually update a11y smoke test expectations

fc9e23a

vercel bot deployed to Preview July 2, 2021 22:09 View deployment

Fix expectations in perf smoke test

3cf7e72

vercel bot deployed to Preview July 3, 2021 03:16 View deployment

tannerdolby requested a review from patrickhulce July 6, 2021 17:16

patrickhulce merged commit 25cc9b6 into GoogleChrome:master Jul 6, 2021

tannerdolby deleted the 12668-fallback-to-selector-in-node-label-generation branch July 6, 2021 19:20

tannerdolby mentioned this pull request Jul 6, 2021

Better fallback for NodeLabel #10421

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: fallback to selector, not tagName for nodeLabel #12727

core: fallback to selector, not tagName for nodeLabel #12727

tannerdolby commented Jun 30, 2021

google-cla bot commented Jun 30, 2021

tannerdolby commented Jun 30, 2021

tannerdolby commented Jun 30, 2021

connorjclark Jun 30, 2021

tannerdolby Jun 30, 2021 •

edited

Loading

connorjclark Jun 30, 2021

tannerdolby Jun 30, 2021

connorjclark Jun 30, 2021

tannerdolby Jun 30, 2021

tannerdolby Jun 30, 2021 •

edited

Loading

connorjclark Jun 30, 2021 •

edited

Loading

tannerdolby Jul 1, 2021 •

edited

Loading

tannerdolby Jul 1, 2021 •

edited

Loading

tannerdolby Jul 1, 2021 •

edited

Loading

connorjclark Jul 1, 2021 •

edited

Loading

tannerdolby Jul 1, 2021 •

edited

Loading

connorjclark Jul 1, 2021

tannerdolby Jul 1, 2021 •

edited

Loading

patrickhulce left a comment

tannerdolby commented Jul 2, 2021 •

edited

Loading

patrickhulce left a comment

tannerdolby commented Jul 2, 2021 •

edited

Loading

patrickhulce commented Jul 2, 2021

patrickhulce commented Jul 3, 2021

tannerdolby commented Jul 3, 2021 •

edited

Loading

tannerdolby commented Jul 6, 2021

patrickhulce commented Jul 6, 2021

tannerdolby commented Jul 6, 2021

core: fallback to selector, not tagName for nodeLabel #12727

core: fallback to selector, not tagName for nodeLabel #12727

Conversation

tannerdolby commented Jun 30, 2021

google-cla bot commented Jun 30, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

tannerdolby commented Jun 30, 2021

tannerdolby commented Jun 30, 2021

connorjclark Jun 30, 2021

Choose a reason for hiding this comment

tannerdolby Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

connorjclark Jun 30, 2021

Choose a reason for hiding this comment

tannerdolby Jun 30, 2021

Choose a reason for hiding this comment

connorjclark Jun 30, 2021

Choose a reason for hiding this comment

tannerdolby Jun 30, 2021

Choose a reason for hiding this comment

tannerdolby Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

connorjclark Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

tannerdolby Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

tannerdolby Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

tannerdolby Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

connorjclark Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

tannerdolby Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

connorjclark Jul 1, 2021

Choose a reason for hiding this comment

tannerdolby Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

tannerdolby commented Jul 2, 2021 • edited Loading

patrickhulce left a comment

Choose a reason for hiding this comment

tannerdolby commented Jul 2, 2021 • edited Loading

patrickhulce commented Jul 2, 2021

patrickhulce commented Jul 3, 2021

tannerdolby commented Jul 3, 2021 • edited Loading

tannerdolby commented Jul 6, 2021

patrickhulce commented Jul 6, 2021

tannerdolby commented Jul 6, 2021

tannerdolby Jun 30, 2021 •

edited

Loading

tannerdolby Jun 30, 2021 •

edited

Loading

connorjclark Jun 30, 2021 •

edited

Loading

tannerdolby Jul 1, 2021 •

edited

Loading

tannerdolby Jul 1, 2021 •

edited

Loading

tannerdolby Jul 1, 2021 •

edited

Loading

connorjclark Jul 1, 2021 •

edited

Loading

tannerdolby Jul 1, 2021 •

edited

Loading

tannerdolby Jul 1, 2021 •

edited

Loading

tannerdolby commented Jul 2, 2021 •

edited

Loading

tannerdolby commented Jul 2, 2021 •

edited

Loading

tannerdolby commented Jul 3, 2021 •

edited

Loading