Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Makes tree accessible #1984

Merged

Conversation

cmcculloh-kr
Copy link

fixes #1964

@@ -3,6 +3,10 @@ module.exports = function test (grunt) {
grunt.registerTask('test', 'run jshint, qunit source w/ coverage, and validate HTML',
['jshint', 'connect:testServer', 'qunit:noMoment', 'qunit:globals', 'test-dist', 'htmllint']);

grunt.registerTask('unittest', 'run jshint, qunit source w/ coverage, and validate HTML',
Copy link
Contributor

Choose a reason for hiding this comment

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

why i don't see unittest being used anywhere

js/tree.js Outdated
@@ -32,46 +34,57 @@
// TREE CONSTRUCTOR AND PROTOTYPE

var Tree = function Tree(element, options) {
this.$element = $(element);
var $element = $(element);
Copy link
Contributor

Choose a reason for hiding this comment

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

I only see $element used once at https://github.com/ExactTarget/fuelux/pull/1984/files?#diff-e5768895bd9d9f525527077130cf278bR70

It could be replaced with

this.$element.on('focus', function setFocusOnTab () {
	var $tree = $(this);
	focusIn($tree, $tree);
});

making this assignment unnecessary.

@swilliamset
Copy link
Contributor

372b0eb
Thanks for cleaning up the eslint but the commit message doesn't reflect the scope of the work done. Please include some note stating where the eslint clean up focused.

fixes most eslint warnings and all errors in tree tests

@interactivellama
Copy link
Contributor

interactivellama commented Jun 7, 2017

  • Needs visual focus indicator SLDS currently has:
.slds-tree__item a:focus {
  outline: 0;
  text-decoration: underline; }
  • Shift+tab isn't working (might need less of a keyboard trap). This should just work based on tabindex.
  • ul role="tree" node needs either aria-label or aria-labelledby
  • selected items need aria-selected="true"
  • tree needs aria-multiselectable unless it is single selection. multi should be default.
  • if tree is single select, aria-multiselectable needs to be false. We've handled this in the past through the dataSource I think. I'm not sure how to expose with the API. a dataSource option might be good. I'm not sure the best way to do the labels and the selection ability on the top level tree node.

@cmcculloh-kr
Copy link
Author

cmcculloh-kr commented Jun 9, 2017

Working through @interactivellama's feedback now.

  • Needs visual focus indicator SLDS currently has
  • ul role="tree" node needs either aria-label or aria-labelledby
  • selected items need aria-selected="true"
  • non-selected selectable items need aria-selected="false"
  • tree needs aria-multiselectable unless it is single selection. multi should be default. (Fuel UX default is single select. Implementors are free to set multi-select to true...)
  • if tree is single select, aria-multiselectable needs to be false
  • Shift+tab to leave the tree (and tab to previous item) isn't working. "Shift" is probably being swallowed up by the keyboard event listener.

@cmcculloh-kr cmcculloh-kr force-pushed the GH1964---tree-should-be-accessible branch 2 times, most recently from e50acc2 to ab2241f Compare June 14, 2017 00:49
@cmcculloh-kr
Copy link
Author

Note that the tree markup changes slightly with this PR adding aria-* attributes and tabindex to many of the nodes in tree. It's a non-breaking change.

@interactivellama interactivellama temporarily deployed to fuelux-edge-pr-1984 June 14, 2017 13:56 Inactive
@cmcculloh-kr cmcculloh-kr requested a review from futuremint June 14, 2017 14:07
Christopher McCulloh and others added 25 commits June 14, 2017 13:46
…ey pressed over 'load more'. Fixes bug where hidden elements block keyboard navigation
…ed for selecting/deselection according to spec. Adam might know a bit more about the nuance here and recommend making spacebar only do a subset of what enter does...
Updates comment for `unittest` task for accuracy
@futuremint futuremint merged commit 2c7c6e6 into ExactTarget:master Jun 15, 2017
@cmcculloh-kr cmcculloh-kr deleted the GH1964---tree-should-be-accessible branch June 16, 2017 18:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tree should be accessible
4 participants