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: rule aria-hidden-focus #1166

Merged
merged 37 commits into from
Jan 9, 2019
Merged

feat: rule aria-hidden-focus #1166

merged 37 commits into from
Jan 9, 2019

Conversation

jeeyyy
Copy link
Contributor

@jeeyyy jeeyyy commented Sep 27, 2018

Rule: Aria Hidden Focus.
Spec for rule - Spec: auto-wcag.github.io/auto-wcag/rules/SC4-1-2-aria-hidden-focus.html

Closes issue:

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

@jeeyyy jeeyyy requested a review from a team as a code owner September 27, 2018 13:50
@jeeyyy jeeyyy changed the title [WIP] feat: new rule aria-hidden-focus [WIP] feat: rule aria-hidden-focus Sep 27, 2018
@jeeyyy jeeyyy closed this Oct 10, 2018
@jeeyyy jeeyyy added the rules Issue or false result from an axe-core rule label Oct 28, 2018
@jeeyyy
Copy link
Contributor Author

jeeyyy commented Oct 28, 2018

On hold, until issue #1208 is resolved.

@jeeyyy jeeyyy reopened this Oct 28, 2018
Copy link
Contributor

@marcysutton marcysutton left a comment

Choose a reason for hiding this comment

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

I'm so excited for this rule!!!

doc/rule-descriptions.md Outdated Show resolved Hide resolved
lib/checks/aria/aria-hidden-focus.js Outdated Show resolved Hide resolved
@jeeyyy jeeyyy changed the title [WIP] feat: rule aria-hidden-focus feat: rule aria-hidden-focus Nov 8, 2018
@jeeyyy jeeyyy dismissed marcysutton’s stale review November 8, 2018 12:09

Updated. Review again.

@jeeyyy jeeyyy requested review from a team and marcysutton November 8, 2018 12:09
lib/checks/aria/aria-hidden-focus.js Outdated Show resolved Hide resolved
lib/checks/aria/aria-hidden-focus.js Outdated Show resolved Hide resolved
lib/checks/aria/aria-hidden-focus.js Outdated Show resolved Hide resolved
lib/checks/aria/aria-hidden-focus.json Outdated Show resolved Hide resolved
lib/checks/aria/aria-hidden-focus.json Outdated Show resolved Hide resolved
test/checks/aria/aria-hidden-focus.js Outdated Show resolved Hide resolved
test/checks/aria/aria-hidden-focus.js Outdated Show resolved Hide resolved
test/checks/aria/aria-hidden-focus.js Outdated Show resolved Hide resolved
test/checks/aria/aria-hidden-focus.js Outdated Show resolved Hide resolved
test/checks/aria/aria-hidden-focus.js Outdated Show resolved Hide resolved
@jeeyyy jeeyyy dismissed WilcoFiers’s stale review November 14, 2018 13:30

Updated. Please review again.

@jeeyyy jeeyyy requested a review from a team November 14, 2018 16:28
@dequelabs dequelabs deleted a comment from jeeyyy Nov 15, 2018
@dequelabs dequelabs deleted a comment from jeeyyy Nov 15, 2018
lib/rules/aria-hidden-focus.json Outdated Show resolved Hide resolved
lib/checks/aria/aria-hidden-focus.js Outdated Show resolved Hide resolved
lib/checks/aria/aria-hidden-focus.js Outdated Show resolved Hide resolved
lib/checks/aria/aria-hidden-focus.js Outdated Show resolved Hide resolved
test/checks/aria/aria-hidden-focus.js Outdated Show resolved Hide resolved
test/checks/aria/aria-hidden-focus.js Outdated Show resolved Hide resolved
lib/checks/aria/aria-hidden-focus.json Outdated Show resolved Hide resolved
@WilcoFiers WilcoFiers changed the title feat: rule aria-hidden-focus [WIP] feat: rule aria-hidden-focus Dec 4, 2018
@jeeyyy
Copy link
Contributor Author

jeeyyy commented Dec 7, 2018

Performance analysis

Page Measure rule_aria-hidden-focus took
http://qateam.dequecloud.com/attest/p-as-heading.html (Aria Hidden on Body) 22.399999999947795ms
http://qateam.dequecloud.com/attest/skiplink_first.html (Aria Hidden on Body) 41.69999999999163ms
https://fontawesome.com/v4.7.0/cheatsheet/ 239.00000000003274ms
https://fontawesome.com/v4.7.0/cheatsheet/ (Aria Hidden on Body) 256.0000000000855ms
https://www.tensorflow.org/api_docs/python/tf/InteractiveSession 161.7999999998574ms
BIG PAGE https://www.tensorflow.org/api_docs/python/tf/InteractiveSession (Aria Hidden on Body) 1056.0999999997875ms

@jeeyyy
Copy link
Contributor Author

jeeyyy commented Dec 7, 2018

@dylanb @WilcoFiers updated with perf measurements.

@jeeyyy jeeyyy changed the title [WIP] feat: rule aria-hidden-focus feat: rule aria-hidden-focus Dec 7, 2018
@WilcoFiers
Copy link
Contributor

See if you can get this to 500ms for the large page.

@dylanb
Copy link
Contributor

dylanb commented Dec 11, 2018

@JKODU I think the performance impact on large pages is significant enough that its worth refactoring to try to achieve performance improvements. I would like to see what the theoretical minimum would be (if we used all native code for selecting, did one pass only for the entire rule instead of two and did the filtering at the front instead of the back.

One more question, how did you measure those performance numbers?

@jeeyyy
Copy link
Contributor Author

jeeyyy commented Dec 12, 2018

One more question, how did you measure those performance numbers?

@dylanb

  • I took a snapshot of each of those pages (view source > save).
  • I added axe.js to those pages and executed axe.run with ruleOnly > aria-hidden-focus and setting the performanceTimer flag to true.
  • In some cases, I set aria-hidden=true on the body of the page for test reasons.

The numbers reported above are an average of 3 run's for each page, that was measured/ reported on the console for each of those pages.

@jeeyyy
Copy link
Contributor Author

jeeyyy commented Dec 12, 2018

@JKODU I think the performance impact on large pages is significant enough that its worth refactoring to try to achieve performance improvements. I would like to see what the theoretical minimum would be (if we used all native code for selecting, did one pass only for the entire rule instead of two and did the filtering at the front instead of the back.

@dylanb - Sure, I will attempt some refactoring as suggested.

@jeeyyy jeeyyy requested a review from dylanb December 12, 2018 22:45
@jeeyyy jeeyyy changed the title feat: rule aria-hidden-focus [WIP] feat: rule aria-hidden-focus Dec 17, 2018
@jeeyyy
Copy link
Contributor Author

jeeyyy commented Jan 2, 2019

For BIG PAGE https://www.tensorflow.org/api_docs/python/tf/InteractiveSession with aria-hidden=true, caching isFocusable on the virtualNode improved the run time of rule aria-hidden-focus to circa ~850ms - Measure runchecks_aria-hidden-focus took 848.2999999980093ms, and caching tabbableElements on the virtualNode improved the run time of rule aria-hidden-focus to circa ~550ms - Measure runchecks_aria-hidden-focus took 554.5000000020082ms.

I have cached isFocusable and tabbableElements on the virtualNode.

Also, had to update certain tests involving virtualNode as above functions were introduced in the object structure, and .deepEqual assertion fails when checking for referencing equality, see - chaijs/chai#896

Performance analysis (after caching results)

Page Measure rule_aria-hidden-focus took Previous Measure (non-caching)
http://qateam.dequecloud.com/attest/p-as-heading.html (Aria Hidden on Body) 4.300000000512227ms 22.399999999947795ms
http://qateam.dequecloud.com/attest/skiplink_first.html (Aria Hidden on Body) 3.6000000036437996ms 41.69999999999163ms
https://fontawesome.com/v4.7.0/cheatsheet/ 84.60000000079162ms 239.00000000003274ms
https://fontawesome.com/v4.7.0/cheatsheet/ (Aria Hidden on Body) 91.8999999994412ms 256.0000000000855ms
https://www.tensorflow.org/api_docs/python/tf/InteractiveSession 40.80000000249129ms 161.7999999998574ms
BIG PAGE https://www.tensorflow.org/api_docs/python/tf/InteractiveSession (Aria Hidden on Body) 554.5000000020082ms 1056.0999999997875ms

@jeeyyy jeeyyy changed the title [WIP] feat: rule aria-hidden-focus feat: rule aria-hidden-focus Jan 2, 2019
lib/rules/aria-hidden-focus-matches.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
lib/checks/shared/focusable-disabled.js Outdated Show resolved Hide resolved
@WilcoFiers WilcoFiers merged commit 4489965 into develop Jan 9, 2019
@WilcoFiers WilcoFiers deleted the new-rule-aria-hidden-focus branch January 9, 2019 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rules Issue or false result from an axe-core rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants