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(utils): add support for complex CSS selectors #1494

Merged
merged 5 commits into from
Apr 12, 2019

Conversation

straker
Copy link
Contributor

@straker straker commented Apr 11, 2019

Updates the CSS Selector Parser to allow more complex selectors: :not, ^=, $=, and *=. Also update docs to describe what selectors we support.

Closes: #1493

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: @WilcoFiers

@straker straker requested a review from a team as a code owner April 11, 2019 18:28
@straker straker changed the title feat(utils): add support for complex CSS selectors [WIP] feat(utils): add support for complex CSS selectors Apr 11, 2019
doc/API.md Outdated Show resolved Hide resolved
@straker straker changed the title [WIP] feat(utils): add support for complex CSS selectors feat(utils): add support for complex CSS selectors Apr 11, 2019
doc/API.md Outdated
@@ -68,12 +68,12 @@ For a full listing of API offered by aXe, clone the repository and run `npm run

#### Supported CSS Selectors

aXe supports the following CSS selectors:
Axe supports the following CSS selectors:
Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

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

Change LGTM, but to be completely honest, I don't have enough exposure to this area of the codebase to be comfortable approving.

Let's wait for @WilcoFiers or @JKODU to OK this.

@straker
Copy link
Contributor Author

straker commented Apr 11, 2019

If anyone was concerned about performance, I ran these changes on a website with >11K dom nodes and >3k anchor tags. Specifically looking at the skip-link rule (which currently uses the selector a[href]), axe took:

Metric Time (ms)
rule_skip-link 292.04
rule_skip-link#gather 291.07
rule_skip-link#gather_axe.utils.isHidden 256.41
rule_skip-link#matches 0.31
rule_skip-link#runchecks 0.07

After just changing the selector to a[href^="#"], axe took:

Metric Time (ms)
rule_skip-link 36.45
rule_skip-link#gather 34.90
rule_skip-link#gather_axe.utils.isHidden 2.25
rule_skip-link#matches 0.13
rule_skip-link#runchecks 0.81

The difference is that before the rule had to look at >3k anchor elements to determine isHidden (a slow check), while the later had to only look at 40 anchor elements. If we look at the time difference between #gather and #gather_axe.utils.isHidden (where the qsa function is run), there's only about a 2ms difference between the simple and complex selector (in favor of the complex selector), but it's not a significant difference.

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Minor point about docs placement.

doc/API.md Outdated Show resolved Hide resolved
doc/API.md Outdated Show resolved Hide resolved
@WilcoFiers WilcoFiers merged commit a9f9ee5 into develop Apr 12, 2019
@WilcoFiers WilcoFiers deleted the updateCssSelectors branch April 12, 2019 15:04
stephenmathieson added a commit to mohanraj-r/axe-core that referenced this pull request May 10, 2019
* develop: (50 commits)
  docs: add jsdom example and tests (dequelabs#1530)
  fix(aria-valid-attr-value): allow aria-owns to pass when element is not in the DOM (dequelabs#1526)
  fix(isSkipLink): cache first page link (dequelabs#1525)
  chore: update Babel dependencies (dequelabs#1527)
  chore(package): Update karma to version 4.1.0 (dequelabs#1528)
  feat: Improve perf of axe.run [WWD-1821] (dequelabs#1503)
  fix(prettier): ignore generated api doc files (dequelabs#1522)
  fix(skip-link,region): Allow multiple skiplinks at page top (dequelabs#1496)
  fix(raw-reporter): do not output `DqElement`s (dequelabs#1513)
  fix: Scroll state had top and left properties flipped (dequelabs#1469)
  refactor: commons.color.getBackgroundColor method (dequelabs#1451)
  fix(aria-valid-attr-value): allow aria-controls to pass when element is not in the DOM
  chore: Update husky to the latest version 🚀 (dequelabs#1514)
  style: format HTML files with Prettier (dequelabs#1508)
  test: Fix invalid test html (dequelabs#1502)
  feat(rule): Inline text spacing must be adjustable with custom stylesheets (dequelabs#1446)
  chore: Remove version number from axe.d.ts (dequelabs#1499)
  chore: Update make-dir to the latest version 🚀 (dequelabs#1465)
  fix: Exclude  iframe for html-has-lang rule (Issue 1424) (dequelabs#1430)
  feat(utils): add support for complex CSS selectors (dequelabs#1494)
  ...
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.

Update cssParser to allow more complex selectors
3 participants