search

vakata / jstree

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

Accessibility Fixes for Checkbox and JsTree #2490

Closed dgorti closed 1 year ago

dgorti commented 3 years ago

Checkbox Plugin The built in checbox plugin tries to add aria-selected attribute to the LI nodes with role "none". This is not valid because the aria-selected attribute belongs on the treenodes, which are the anchor elements. The main div element also gets aria-selected=false when you check and uncheck child nodes. The fix is to update only anchor nodes with aria-selected = true/false when checking / unchecking checkboxes and cascading up/down.

Main JSTree The UL element that is a direct child of the div element causes some issues with accessibility checkers. Please see this stackoverflow discussion https://stackoverflow.com/questions/66072903/is-a-role-group-valid-under-a-role-tree. Essentially the only allowed nodes under an element of "tree" are "treenodes" (as far as the accessibility tree is concerned).

Without impacting the functionality, we can change the UL role to "presentation".

Checking with NVDA Screen Reader I checked with NVDA screen reader before and after the changes. There seems to be no functionality difference without these changes, but still, the accessibility checkers will complain without these changes.

Please take a look. Thanks for considering

Before fixes Capture

After Fixes Capture

Karkhutvy commented 3 years ago

Hello @dgorti , using "group" with nested "treeitems" is allowed for role="tree": According to 'tree' role description https://www.w3.org/TR/wai-aria-1.1/#tree the next required children described: group → treeitem treeitem image

I suppose that accessibility checkers report about "Required ARIA child role not present: treeitem" is false positive. I already created ticket about this issue for AXE : https://github.com/dequelabs/axe-core/issues/2805 and this situation should be fixed in axe-core version 4.2. If Accessibility Insights also reports this issue - perhaps it makes sence to create ticket with this issue for them instead of deleting role "group" from jsTree.

dgorti commented 3 years ago

Hello @Karkhutvy , I can understand your take on this. I too thought a tree item can have a group. In my extended discussion on stackoverflow, it seems that it is not a good idea to gave group as the only child of a tree node. In any case, the specs may or many not be clear and the rule checkers may or may not be following the specs. It will be good to get clarification from A11Y team on what exactly they mean. The fix I did in my PR is not against the specs, and it makes the rule checkers happy. So that what we did for our implementation.