-
Notifications
You must be signed in to change notification settings - Fork 776
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
feat(options): add ancestry CSS selector to nodes #2389
Conversation
@@ -0,0 +1,31 @@ | |||
import getShadowSelector from './get-shadow-selector'; | |||
|
|||
function generateAncestry(node) { |
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.
I think we should cache these results on the virtual node as we process them so we aren't recalculating the entire ancestry tree of every descendant of a node.
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.
I don't see why that would make much of a difference. The way this is written is very fast. I doubt doing a lookup in a large set is going to be noticeably faster than walking a an additional two or three ancestors.
if ( | ||
nodeName !== 'head' && | ||
nodeName !== 'body' && | ||
parent.children.length > 1 |
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.
Would it be better if this was only checking if the parent had multiple of the nodeName? I know getSelector
only adds nth-child stuff if it needs to.
Right now if the parent has two children but they are both different nodeNames, you still get the nth-child selector which doesn't do much but add noise:
<html>
<body>
<div>
<a href="/foo">Foo</a>
<span>Hello</span>
</div>
</body>
</html>
Results in html > body > div:nth-child(1) > a:nth-child(1)
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.
What would the benefit of that be? This seems faster.
@@ -69,6 +73,7 @@ describe('helpers.processAggregate', function() { | |||
selector: '#dopel', | |||
source: '<input id="dopel"/>', | |||
xpath: '/main/input[@id="dopel"]', | |||
ancestry: 'html > body > main > input:nth-child(0)', |
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.
how do we get an nth-child of 0?
Adds an option for axe.run() to return an "ancestry" css selector. Similar to "selector" but instead including just the node names and child index up to the root node of the tree. Handles iframe and shadow DOM nesting in the same way .selector does.
Closes issue: #2209
Reviewer checks
Required fields, to be filled out by PR reviewer(s)