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

Performance improvements for utils.isHidden #166

Closed
ckundo opened this issue Feb 14, 2016 · 6 comments · Fixed by #1503
Closed

Performance improvements for utils.isHidden #166

ckundo opened this issue Feb 14, 2016 · 6 comments · Fixed by #1503
Labels
core Issues in the core code (lib/core) feat New feature or enhancement help wanted We welcome PRs or discussions for issues marked as help wanted performance Performance related issues

Comments

@ckundo
Copy link
Contributor

ckundo commented Feb 14, 2016

I evaluated axe-core with Firefox performance tools, running it in http://www.html5accessibility.com/html5elements/ page. I saw that utils.isHidden costs about 20% of the total execution time of axe.a11yCheck call. On this particular page that equates to about 200ms. Thoughts on improving performance in that method? I'm happy to chip in or share ideas, but wanted to capture the issue first.

@WilcoFiers
Copy link
Contributor

Interesting. You confirmed something I've been wondering about myself. I'll have a go at it. Maybe memoization could help out here. We are touching that function an awful lot after all.

@jnurthen
Copy link

Have you considered checking the CSS offsetWidth , offsetHeight etc. (I use offsetTop and offsetLeft too - but I'm not sure that is necessary). If something is display:none these should be 0 (or null in some browsers). This avoids walking up the DOM tree to find out if an ancestor is hidden.

@dylanb
Copy link
Contributor

dylanb commented Feb 27, 2016

@jnurthen If you look at the code, you will see that it is a bit more complicated than just display:none https://github.com/dequelabs/axe-core/blob/master/lib/core/utils/is-hidden.js but we could possibly improve the performance somewhat by doing that test first and then the other tests. Anyone want to take a shot at doing that?

@dylanb dylanb added feat New feature or enhancement help wanted We welcome PRs or discussions for issues marked as help wanted core Issues in the core code (lib/core) labels Feb 27, 2016
@jnurthen
Copy link

I question a little your testing of aria-hidden here. I think (only a 5 minute look so apologies if incorrect) that your isHidden routine gets called on nodes when excludeHidden is not set to false in a rule.

I don't think it is correct to exclude nodes hidden using aria-hidden=true from a colour contrast check, only nodes hidden using techniques which hide from nodes visually should be excluded from contrast checks. I really think you need either 2 different isHidden checks or remove the aria-hidden check from this basic check which prevents the node even getting checked by the real check and check aria-hidden in the actual rule when appropriate.

Testcase for why this fails at http://output.jsbin.com/xulehoniyu.
Both lines should fail colour contrast, but only 1 gets picked up by aXe currently.

@dylanb
Copy link
Contributor

dylanb commented Mar 1, 2016

@jnurthen Can you open a separate issue for that specific rule?

@jnurthen
Copy link

jnurthen commented Mar 1, 2016

Had a feeling you would say that
#170

@WilcoFiers WilcoFiers added the performance Performance related issues label Feb 10, 2018
mrtnvh pushed a commit to mrtnvh/axe-core that referenced this issue Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Issues in the core code (lib/core) feat New feature or enhancement help wanted We welcome PRs or discussions for issues marked as help wanted performance Performance related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants