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.

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

Merged
merged 8 commits into from
Jul 20, 2020

Conversation

WilcoFiers
Copy link
Contributor

@WilcoFiers WilcoFiers commented Jul 16, 2020

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)

  • Follows the commit message policy, appropriate for next version
  • Code is reviewed for security

@WilcoFiers WilcoFiers requested a review from a team as a code owner July 16, 2020 16:29
@@ -0,0 +1,31 @@
import getShadowSelector from './get-shadow-selector';

function generateAncestry(node) {
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

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)

Copy link
Contributor Author

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)',
Copy link
Contributor

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?

straker
straker previously approved these changes Jul 20, 2020
@WilcoFiers WilcoFiers merged commit f2cccf5 into develop Jul 20, 2020
@WilcoFiers WilcoFiers deleted the ancestor-selector branch January 30, 2023 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants