vakata / jstree

jquery tree plugin
http://jstree.com
MIT License
5.15k stars 1.38k forks source link

Two big Accessibility fails, with a solution. #2498

Closed sdellis closed 1 year ago

sdellis commented 3 years ago

For accessibility purposes, this markup <i class='jstree-icon jstree-themeicon'></i> should be moved outside of the <a> tag. Since the contents are providing a value for aria-labeledby, it's throwing errors in accessibility auditors. Also, the <li> should be role="treeitem" (not "presentation").

Starting on line 650 of jstree.js, I re-labeled the LI to be role="treeitem" and shifted the order of the appends to make it accessible. Here's my fix:

        _create_prototype_node : function () {
            var _node = document.createElement('LI'), _temp1, _temp2;
            _node.setAttribute('role', 'treeitem');
            _temp1 = document.createElement('I');
            _temp1.className = 'jstree-icon jstree-ocl';
            _temp1.setAttribute('role', 'presentation');
            _node.appendChild(_temp1);
            _temp1 = document.createElement('A');
            _temp1.className = 'jstree-anchor';
            _temp1.setAttribute('href','#');
            _temp1.setAttribute('tabindex','-1');
            _temp1.setAttribute('role', 'treeitem');
            _node.appendChild(_temp1);
            _temp2 = document.createElement('I');
            _temp2.className = 'jstree-icon jstree-themeicon';
            _temp2.setAttribute('role', 'presentation');
            _node.appendChild(_temp2);
            _temp1 = _temp2 = null;

This is an easy fix with a big Accessibility payoff. I'm happy to submit a PR, if you'd like.

vakata commented 3 years ago

This has been changed multiple times over the last few months. The standards, the checkers and the real usage are completely different - recently I am focusing on real usage - have you tested with Jaws / NVDA / other screen readers in different browsers? Are there any problems? I am only asking because the previous set of changes and optimizations was performed using different reader / browser combinations, ensuring maximum compatibility for real world usage.

sdellis commented 3 years ago

I understand that there is not a lot of consistency among the tools and browsers. I tested with aXe (Deque) and Lighthouse (Google) automated checkers. Both of these threw errors before, but no longer do with my fix. As for screen readers, I can also verify that VoiceOver on Mac works in Safari, Chrome, and Firefox. I do not have access to JAWS and NVDA is Windows-only, so I'm sorry, but I cannot help test with those tools.

joelhsmith commented 2 years ago

I support @sdellis But I think a significant problem here is actually the _temp1.setAttribute('tabindex','-1');. It removes the ability of screen reader users to send :focus to the element. This is a problem with VoiceOver, JAWS, and NVDA. Is there a reason that the tabindex="-1" is on the element?

vakata commented 1 year ago

The WAI guidelines for tree navigation are to set tabindex to -1 for all focusable nodes and manage item focus using aria-selected. I am closing all v.3 accessibility tickets as I will not have time to invest in v.3 features - it is bug fixes only from now on - all effort on features is going to v.4. All those discussions will not be relevant for v.4 as it uses a totally different markup.