Skip to content
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

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

Merged
merged 12 commits into from
Jul 6, 2021
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
195605f
Fallback to selector instead of tagName in nodeLabel generation
tannerdolby Jun 30, 2021
f0a9597
Refactor to avoid calling getNodeSelector twice per getNodeDetails call
tannerdolby Jun 30, 2021
676151a
Make tests pass for page-functions
tannerdolby Jun 30, 2021
a70e04e
Add test for getNodeDetails to cover nodeLabel fallback
tannerdolby Jul 1, 2021
a474e28
Move global.window to beforeAll/afterAll and improve test
tannerdolby Jul 1, 2021
ebce04c
That makes more sense, since document is being destructured from window
tannerdolby Jul 1, 2021
81c2c27
Make global.window undefined in afterAll()
tannerdolby Jul 1, 2021
8e5c132
Removes useless jsdom comment now that global.window is available
tannerdolby Jul 2, 2021
b38d83a
Reuse selector instead of recomputing
tannerdolby Jul 2, 2021
08cf6f5
Move getNodeDetails test to its own describe block
tannerdolby Jul 2, 2021
fc9e23a
Manually update a11y smoke test expectations
tannerdolby Jul 2, 2021
3cf7e72
Fix expectations in perf smoke test
tannerdolby Jul 3, 2021
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
9 changes: 5 additions & 4 deletions lighthouse-core/lib/page-functions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,9 +378,9 @@ function isPositionFixed(element) {
/**
* 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.
* Returns label string or null if no useful label is found.
* @param {Element} element
* @return {string}
* @return {string | null}
*/
function getNodeLabel(element) {
// Inline so that audits that import getNodeLabel don't
Expand Down Expand Up @@ -415,7 +415,7 @@ function getNodeLabel(element) {
}
}
}
return tagName;
return null;
}

/**
Expand Down Expand Up @@ -482,6 +482,7 @@ function getNodeDetails(element) {
}

element = element instanceof ShadowRoot ? element.host : element;
const selector = getNodeSelector(element);

// Create an id that will be unique across all execution contexts.
// The id could be any arbitrary string, the exact value is not important.
Expand All @@ -508,7 +509,7 @@ function getNodeDetails(element) {
selector: getNodeSelector(element),
boundingRect: getBoundingClientRect(element),
snippet: getOuterHTMLSnippet(element),
nodeLabel: getNodeLabel(element),
nodeLabel: getNodeLabel(element) || selector,
};

return details;
Expand Down
8 changes: 4 additions & 4 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 @@ -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', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@tannerdolby tannerdolby Jun 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@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()?

Copy link
Collaborator

@connorjclark connorjclark Jun 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tannerdolby tannerdolby Jul 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tannerdolby tannerdolby Jul 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tannerdolby tannerdolby Jul 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

@connorjclark connorjclark Jul 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@tannerdolby tannerdolby Jul 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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)

const el = dom.createElement('html');
assert.equal(pageFunctions.getNodeLabel(el), 'html');
assert.equal(pageFunctions.getNodeLabel(el), null);
});

it('Uses tag name if there is no better label', () => {
it('Returns null if there is no better label', () => {
const el = dom.createElement('div');
const childEl = dom.createElement('span');
el.appendChild(childEl);
assert.equal(pageFunctions.getNodeLabel(el), 'div');
assert.equal(pageFunctions.getNodeLabel(el), null);
});
});

Expand Down