TreeView: Implement arrow key navigation

[colebemis]

Summary

Adds support for arrow key navigation of a TreeView using aria-activedescendant (based on the TreeView API spec).

👉 Try it out

CleanShot.2022-09-15.at.15.39.38.mp4

Impact

This PR will enable keyboard and screen reader users to navigate a TreeView.

VoiceOver demo

Screen.Recording.2022-09-15.at.3.51.06.PM.mov

Test cases

Here are the test cases I wrote for the TreeView keyboard interactions (based on the ARIA spec):

Keyboard interactions
    ArrowDown
      ✓ moves aria-activedescendant to the next visible treeitem (118 ms)
    ArrowUp
      ✓ moves aria-activedescendant to the previous visible treeitem (127 ms)
    ArrowLeft
      ✓ collapses an expanded item (47 ms)
      ✓ does nothing on a root-level collapsed item (27 ms)
      ✓ does nothing on a root-level end item (19 ms)
      ✓ moves aria-activedescendant to parent of end item (54 ms)
      ✓ moves aria-activedescendant to parent of collapsed item (77 ms)
    ArrowRight
      ✓ expands a collapsed item (42 ms)
      ✓ moves aria-activedescendant to first child of an expanded item (45 ms)
      ✓ does nothing on an end item (46 ms)
    Home
      ✓ moves aria-activedescendant to first visible item (100 ms)
    End
      ✓ moves aria-activedescendant to last visible item (141 ms)
    Enter
      ✓ calls onSelect function if provided (18 ms)
      ✓ toggles expanded state if no onSelect function is provided (72 ms)
      ✓ navigates to href if provided (72 ms)

Reviewers

I'm open to feedback about any part of the implementation but specifically, I'd love confirmation that the keyboard and screen reader behavior work as expected.

Merge checklist

• Added/updated tests
• Added/updated documentation
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

[changeset-bot]

🦋 Changeset detected

Latest commit: 574d2c8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	76.39 KB (0%)
dist/browser.umd.js	77.05 KB (0%)

[mperrotti]

This is very exciting! Everything is looking good, but we don't want Space to call onSelect or expand a parent node. We're reserving that for when/if we have nodes that can be checked or unchecked.

For future reference, check Primer's documentation on tree view accessibility, before the W3C's tree view APG. I worked with @jscholes and the a11y team to suss out what the APG gets "wrong" and define improved patterns.

---

I skimmed the implementation and mostly focused on testing the UI. I can look closer at the code once @ericwbailey gives his ✅ on the keyboard navigation.

[mperrotti]

Is it expected that none of the "links" in File Tree With Directory Links do anything when they're clicked or you press "Enter" when focused on them?

[jdrush89]

@colebemis @mperrotti should loading items in the tree be focusable? Or should aria-activedescendant skip over them?

[ericwbailey]

@jdrush89 We'll want only one focusable node for loading node(s), with an announcement that indicates how many loading nodes are present (if we know). More granular behavior to follow in the second round of TreeView API work.

[mperrotti]

Approval assumes @ericwbailey also approves.

[mperrotti]

Non-blocking nitpicks:

1. Would it make more sense to move these to a utilities file instead of keeping them in TreeView.tsx?
2. Should these functions have their own unit tests? Or do we think the TreeView tests are sufficient?

[colebemis]

1. Would it make more sense to move these to a utilities file instead of keeping them in TreeView.tsx?

Yeah, it might make sense to pull this out into a hook later. I'm going to keep it all in the same file for now so I can iterate more quickly

2. Should these functions have their own unit tests? Or do we think the TreeView tests are sufficient?

I'd like to avoid testing implementation details like this because we'd need to update lots of tests if we decide to change the underlying implementation (even if we don't change behavior). I think the "Keyboard interactions" unit tests give us solid coverage.

[ericwbailey]

I've just tested this in VoiceOver and NVDA and it's looking/sounding great! Amazing stuff 🎉