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

fix(color-contrast): greatly improve color-contrast-matches speed. add aria/get-accessible-ref #2635

Merged
merged 4 commits into from
Nov 17, 2020

Conversation

straker
Copy link
Contributor

@straker straker commented Nov 12, 2020

The color-contrast-matches function would try to query the entire DOM for every node with an id attribute, and all children of that node would also do the same check. This caused the matcher to take ~40s on a relatively small page with lots of IDs. Since we were already caching id references in aria/is-accessible-ref, I was able to leverage that same code to create a new function that returns an array of all id ref DOM nodes. That way we could just reference that cache in color-contrast-matches and reduce the time to about 1 second.

Here are the perf results in running only the color-contrast rule on https://www.chase.com/content/dam/chaseonline/en/demos/cbo/launch.html.

Time Before (ms) Time After (ms)
42947.73 1271.18

NOTE: We are in a code freeze so cannot merge this until after 4.1 is released on Monday.

Closes issue: #2479

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

@straker straker requested a review from a team as a code owner November 12, 2020 17:00
lib/commons/aria/is-accessible-ref.js Outdated Show resolved Hide resolved
lib/commons/aria/get-accessible-refs.js Outdated Show resolved Hide resolved
lib/commons/aria/get-accessible-refs.js Outdated Show resolved Hide resolved
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.

Looks good. I think there are a lot more places in axe-core that now we have a cache of ID references we could speed up. Probably best to open a tech debt issue for that one instead of pile it onto this PR. Here's a list though:

  • checks
    • aria-required-parent-evaluate
    • explicit-evaluate
    • hidden-explicit-label-evaluate
    • multiple-labels
    • duplicate-id-evaluate
  • matches
    • duplicate-id-active-matches
    • duplicate-id-misc-matches
    • scrollable-region-focusable-matches
  • commons
    • text.labelVirtual
    • dom.findElmsInContext
    • dom.isVisible (usemap lookup)

To make that easier, we can probably simplify by having something like a idrefsReverse function, that takes a node (with an ID) and an attribute and looks up any element references the ID with that attribute. I'll leave it up to you if you want to do that in this PR or another.

const virtualControls = idrefsReverse(ancestorNode, 'aria-labelledby')
	.map(control => getNodeFromTree(control));

@straker straker changed the title feat(color-contrast): greatly improve color-contrast-matches speed. add aria/get-accessible-ref fix(color-contrast): greatly improve color-contrast-matches speed. add aria/get-accessible-ref Nov 17, 2020
@straker straker merged commit ba174bd into develop Nov 17, 2020
@straker straker deleted the perf-contrast-matches branch November 17, 2020 17:17
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.

None yet

2 participants