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(TreeView): add experimental TreeView component #6008

Merged
merged 105 commits into from
Aug 25, 2020

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented May 4, 2020

ref #5180

This PR adds the first iteration of the experimental (unstable) Carbon TreeView component. The initial feature list for this release includes:

  • Default and compact tree sizing
  • Support for node icons and rendering custom content for node labels
  • Node activation
  • Node expansion
  • Node selection (single and multiple node selection)
  • Keyboard controls for both all three actions
  • Enter will "activate" and select the currently focused node, but activation only occurs when the tree is a single selection tree, or if there is no other selected node in a multiselect tree
  • Space will make a node selection, and in a multiselect tree it will toggle selection
  • In a multiselect tree, holding Ctrl on Windows, Cmd on macOS, or Meta on Linux while making a selection will toggle the selection state of the currently focused node while preserving the currently selected range of nodes
  • up and down arrows will navigate through the tree
  • Right arrow:
    • When focus is on a closed node, opens the node; focus does not move.
    • When focus is on a open node, moves focus to the first child node.
    • When focus is on an end node, does nothing.
  • Left arrow:
    • When focus is on an open node, closes the node.
    • When focus is on a child node that is also either an end node or a closed node, moves focus to its parent node.
    • When focus is on a root node that is also either an end node or a closed node, does nothing.

@netlify
Copy link

netlify bot commented May 4, 2020

Deploy preview for carbon-elements ready!

Built with commit 74302e8

https://deploy-preview-6008--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented May 4, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 74302e8

https://deploy-preview-6008--carbon-components-react.netlify.app

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

The prototype looks great @emyarod 🎉

packages/react/src/components/TreeView/TreeView.js Outdated Show resolved Hide resolved
packages/react/src/components/TreeView/TreeNode.js Outdated Show resolved Hide resolved
packages/react/src/components/TreeView/TreeView.js Outdated Show resolved Hide resolved
packages/react/src/components/TreeView/TreeView.js Outdated Show resolved Hide resolved
packages/react/src/components/TreeView/TreeView.js Outdated Show resolved Hide resolved
packages/react/src/components/TreeView/TreeView.js Outdated Show resolved Hide resolved
@carmacleod
Copy link
Contributor

@emyarod

Review Notes:

Please give the tree a name. This tree could be called "Prototype" for now:

<div id="treelabel" class="bx--label">Prototype:</div>
<ul class="bx--tree" role="tree" aria-labelledby="treelabel">
  ...

Aside: Consider having a prop called "label" (required; throw an error if they don't give one), and another prop "isLabelVisible" (default true). For example, in the above code the label is visible. If isLabelVisible is set to false, then use aria-label on the tree instead of creating a div. (We can discuss this idea - I wish all Carbon components worked like this for labelling instead of providing an ariaLabel prop, which gets misused all the time. 😄 )

The toggle buttons (twistie triangles) shouldn't be focusable. They're just a visual affordance for pointing devices. These buttons need tabindex="-1" to take them out of the tab order, and aria-hidden="true" to hide them from screen readers. (Alternatively, don't use a button element at all - just the svg with a click handler will do). You can give them hover styling if you like, but please take away the focus styling. If a mouse user clicks on one, move focus to the corresponding branch treeitem (in addition to handling the expand/collapse).

When you focus() a treeitem, you need to move tabindex="0" to the focused treeitem so that exactly one treeitem is in the sequential tab navigation order, as described in APG section 6.6.1 Managing Focus Within Components Using a Roving tabindex.

When a tree branch is expanded, it needs to have aria-expanded="true". Conversely, when it is collapsed, it needs to have aria-expanded="false". Leaf nodes do not have aria-expanded.

When you add class "bx--tree-node--selected" to a treeitem, also set aria-selected="true" on the treeitem.

Please implement all of the keyboard behavior listed in the Keyboard Interaction section for trees. Note that the additional right/left arrow key behavior makes navigating in a tree with arrow keys quite nice. Try it out yourself using arrow keys in the APG's File Viewer tree example.

  • Right arrow:
    • When focus is on a closed node, opens the node; focus does not move.
    • When focus is on a open node, moves focus to the first child node.
    • When focus is on an end node, does nothing.
  • Left arrow:
    • When focus is on an open node, closes the node.
    • When focus is on a child node that is also either an end node or a closed node, moves focus to its parent node.
    • When focus is on a root node that is also either an end node or a closed node, does nothing.

Just curious - why is node 1 a button? Is it an example of something that Carbon users are expected to be putting in the tree? If so, please note that the only interactive elements allowed in an ARIA tree are treeitems, so if Carbon users are going to be adding buttons or anchors, they will have to be marked up as the treeitems. So:

- instead of this (li is a treeitem):
<li class="bx--tree-node bx--tree-leaf-node" role="treeitem" tabindex="-1">
  <div class="bx--tree-node__label">3-1</div>
</li>

- would need to do this (li's role is explicitly negated; child button is treeitem):
<li class="bx--tree-node bx--tree-leaf-node" role="none" tabindex="-1">
  <button class="bx--tree-node__label" role="treeitem" tabindex="-1">3-1</button>
</li>

- or this (li's role is explicitly negated; child anchor is treeitem):
<li class="bx--tree-node bx--tree-leaf-node" role="none" tabindex="-1">
  <a class="bx--tree-node__label" role="treeitem" href="#something" tabindex="-1">3-1</a>
</li>

Disabled tree branches would need aria-disabled="true".
Note that I have never seen disabled tree branches before, and I don't think screen reader users would be able to find the disabled branch easily. What is use case? Why not just remove the branch completely with display:none?

Screen reader testing:

None of the Windows screen readers work in this tree currently. I believe that what's messing them up is the missing dynamic tabindex="0" to indicate the currently-focused treeitem, the missing dynamic aria-expanded="true/false" on branch treeitems, and having twistie buttons in the tree. I can try again after these items have been addressed.

@emyarod emyarod force-pushed the 5180-treeview branch 2 times, most recently from 4299e9a to c7873ec Compare May 13, 2020 21:50
@emyarod emyarod force-pushed the 5180-treeview branch 5 times, most recently from 5fa3093 to 1607ae1 Compare May 29, 2020 19:45
@emyarod emyarod force-pushed the 5180-treeview branch 2 times, most recently from 6f00e74 to 5bfcc92 Compare June 4, 2020 21:14
@emyarod emyarod force-pushed the 5180-treeview branch 5 times, most recently from d5a7668 to f321692 Compare June 25, 2020 14:33
@emyarod emyarod force-pushed the 5180-treeview branch 5 times, most recently from 6db6aff to 379093f Compare July 2, 2020 14:24
@emyarod
Copy link
Member Author

emyarod commented Jul 3, 2020

after some testing with NVDA and JAWS earlier today @carmacleod identified an upstream issue in Firefox specifically where the screen reader would announce all items of a parent node when it is expanded

the associated bug report can be viewed here: https://bugzilla.mozilla.org/show_bug.cgi?id=1650462

@kodiakhq kodiakhq bot merged commit 1dc2ef5 into carbon-design-system:master Aug 25, 2020
@emyarod emyarod deleted the 5180-treeview branch August 25, 2020 17:04
@tw15egan tw15egan mentioned this pull request Sep 2, 2020
11 tasks
@samantha-kw-chan
Copy link

Hello, I am interested in using the tree view component. I am wondering what the plans are for making the tree view out of experimental, and for it to become a supported component?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants