Jdrush89/tree focus bug (#3300)

* Adding example of focus bug * Not using focusInStrategy when clicking * Clearing mousedown state correctly * Adding changeset * Removing unused import * Moving mouseup handler
primer · May 23, 2023 · 5d06738 · 5d06738
1 parent ac698b5
commit 5d06738
Show file tree

Hide file tree

Showing 5 changed files with 185 additions and 7 deletions.
diff --git a/.changeset/khaki-walls-applaud.md b/.changeset/khaki-walls-applaud.md
@@ -0,0 +1,5 @@
+---
+'@primer/react': patch
+---
+
+Fixing toggle bug on Treeview when it initially receives focus
diff --git a/src/TreeView/TreeView.features.stories.tsx b/src/TreeView/TreeView.features.stories.tsx
@@ -777,11 +777,118 @@ export const InitialFocus: Story = () => (
   <div>
     <Button>Focusable element before TreeView</Button>
     <TreeView aria-label="Test tree">
-      <TreeView.Item id="item-1">Item 1</TreeView.Item>
-      <TreeView.Item id="item-2" current>
-        Item 2
+      <TreeView.Item id="src" defaultExpanded>
+        <TreeView.LeadingVisual>
+          <TreeView.DirectoryIcon />
+        </TreeView.LeadingVisual>
+        src
+        <TreeView.SubTree>
+          <TreeView.Item id="src/Avatar.tsx">
+            <TreeView.LeadingVisual>
+              <FileIcon />
+            </TreeView.LeadingVisual>
+            Avatar.tsx
+          </TreeView.Item>
+          <TreeView.Item id="src/Button" defaultExpanded>
+            <TreeView.LeadingVisual>
+              <TreeView.DirectoryIcon />
+            </TreeView.LeadingVisual>
+            Button
+            <TreeView.SubTree>
+              <TreeView.Item id="src/Button/Button.tsx">
+                <TreeView.LeadingVisual>
+                  <FileIcon />
+                </TreeView.LeadingVisual>
+                Button.tsx
+              </TreeView.Item>
+              <TreeView.Item id="src/Button/Button.test.tsx">
+                <TreeView.LeadingVisual>
+                  <FileIcon />
+                </TreeView.LeadingVisual>
+                Button.test.tsx
+              </TreeView.Item>
+              <TreeView.Item id="src/Button/Button2.tsx">
+                <TreeView.LeadingVisual>
+                  <FileIcon />
+                </TreeView.LeadingVisual>
+                Button2.tsx
+              </TreeView.Item>
+              <TreeView.Item id="src/Button/Button2.test.tsx">
+                <TreeView.LeadingVisual>
+                  <FileIcon />
+                </TreeView.LeadingVisual>
+                Button2.test.tsx
+              </TreeView.Item>
+              <TreeView.Item id="src/Button/Button3.tsx">
+                <TreeView.LeadingVisual>
+                  <FileIcon />
+                </TreeView.LeadingVisual>
+                Button3.tsx
+              </TreeView.Item>
+              <TreeView.Item id="src/Button/Button3.test.tsx">
+                <TreeView.LeadingVisual>
+                  <FileIcon />
+                </TreeView.LeadingVisual>
+                Button3.test.tsx
+              </TreeView.Item>
+              <TreeView.Item id="src/Button/Button4.tsx">
+                <TreeView.LeadingVisual>
+                  <FileIcon />
+                </TreeView.LeadingVisual>
+                Button4.tsx
+              </TreeView.Item>
+              <TreeView.Item id="src/Button/Button4.test.tsx">
+                <TreeView.LeadingVisual>
+                  <FileIcon />
+                </TreeView.LeadingVisual>
+                Button4.test.tsx
+              </TreeView.Item>
+              <TreeView.Item id="src/Button/Button5.tsx">
+                <TreeView.LeadingVisual>
+                  <FileIcon />
+                </TreeView.LeadingVisual>
+                Button5.tsx
+              </TreeView.Item>
+              <TreeView.Item id="src/Button/Button5.test.tsx">
+                <TreeView.LeadingVisual>
+                  <FileIcon />
+                </TreeView.LeadingVisual>
+                Button5.test.tsx
+              </TreeView.Item>
+              <TreeView.Item id="src/Button/Button6.tsx">
+                <TreeView.LeadingVisual>
+                  <FileIcon />
+                </TreeView.LeadingVisual>
+                Button6.tsx
+              </TreeView.Item>
+              <TreeView.Item id="src/Button/Button6.test.tsx">
+                <TreeView.LeadingVisual>
+                  <FileIcon />
+                </TreeView.LeadingVisual>
+                Button6.test.tsx
+              </TreeView.Item>
+              <TreeView.Item id="src/Button/Button7.tsx">
+                <TreeView.LeadingVisual>
+                  <FileIcon />
+                </TreeView.LeadingVisual>
+                Button7.tsx
+              </TreeView.Item>
+              <TreeView.Item id="src/Button/Button7.test.tsx" current>
+                <TreeView.LeadingVisual>
+                  <FileIcon />
+                </TreeView.LeadingVisual>
+                Button7.test.tsx
+              </TreeView.Item>
+            </TreeView.SubTree>
+          </TreeView.Item>
+          <TreeView.Item id="src/ReallyLongFileNameThatShouldBeTruncated.tsx">
+            <TreeView.LeadingVisual>
+              <FileIcon />
+            </TreeView.LeadingVisual>
+            ReallyLongFileNameThatShouldBeTruncated.tsx
+          </TreeView.Item>
+        </TreeView.SubTree>
       </TreeView.Item>
-      <TreeView.Item id="item-3">Item 3</TreeView.Item>
     </TreeView>
     <Button>Focusable element after TreeView</Button>
   </div>

diff --git a/src/TreeView/TreeView.test.tsx b/src/TreeView/TreeView.test.tsx
@@ -247,6 +247,43 @@ describe('Markup', () => {
     const item2 = getByRole('treeitem', {name: /Item 2/})
     expect(item2).toHaveFocus()
   })
+
+  it('should toggle when receiving focus from chevron click', async () => {
+    const user = userEvent.setup({delay: null})
+    const {getByRole} = renderWithTheme(
+      <div>
+        <button>Focusable element</button>
+        <TreeView aria-label="Test tree">
+          <TreeView.Item id="item-1">
+            Item 1
+            <TreeView.SubTree>
+              <TreeView.Item id="subitem-1">SubItem 1</TreeView.Item>
+              <TreeView.Item id="subitem-2">SubItem 2</TreeView.Item>
+              <TreeView.Item id="subitem-3">SubItem 3</TreeView.Item>
+            </TreeView.SubTree>
+          </TreeView.Item>
+          <TreeView.Item id="item-2" current>
+            Item 2
+          </TreeView.Item>
+          <TreeView.Item id="item-3">Item 3</TreeView.Item>
+        </TreeView>
+      </div>,
+    )
+
+    // Focus button
+    const button = getByRole('button', {name: /Focusable element/})
+    await user.click(button)
+    expect(button).toHaveFocus()
+
+    // Move focus to tree
+    const item1 = getByRole('treeitem', {name: /Item 1/})
+    const toggle = item1.querySelector('.PRIVATE_TreeView-item-toggle')
+    await user.click(toggle!)
+
+    // Focus should be on current treeitem
+    const subItem1 = getByRole('treeitem', {name: /SubItem 1/})
+    expect(subItem1).toBeInTheDocument()
+  })
 })
 
 describe('Keyboard interactions', () => {

diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx
@@ -5,7 +5,7 @@ import {
   FileDirectoryOpenFillIcon,
 } from '@primer/octicons-react'
 import classnames from 'classnames'
-import React from 'react'
+import React, {useCallback, useEffect} from 'react'
 import styled, {keyframes} from 'styled-components'
 import {ConfirmationDialog} from '../Dialog/ConfirmationDialog'
 import Spinner from '../Spinner'
@@ -256,12 +256,27 @@ const Root: React.FC<TreeViewProps> = ({
   flat,
 }) => {
   const containerRef = React.useRef<HTMLUListElement>(null)
+  const mouseDownRef = React.useRef<boolean>(false)
   const [ariaLiveMessage, setAriaLiveMessage] = React.useState('')
   const announceUpdate = React.useCallback((message: string) => {
     setAriaLiveMessage(message)
   }, [])
 
-  useRovingTabIndex({containerRef})
+  const onMouseDown = useCallback(() => {
+    mouseDownRef.current = true
+  }, [])
+
+  useEffect(() => {
+    function onMouseUp() {
+      mouseDownRef.current = false
+    }
+    document.addEventListener('mouseup', onMouseUp)
+    return () => {
+      document.removeEventListener('mouseup', onMouseUp)
+    }
+  }, [])
+
+  useRovingTabIndex({containerRef, mouseDownRef})
   useTypeahead({
     containerRef,
     onFocusChange: element => {
@@ -294,6 +309,7 @@ const Root: React.FC<TreeViewProps> = ({
           aria-label={ariaLabel}
           aria-labelledby={ariaLabelledby}
           data-omit-spacer={flat}
+          onMouseDown={onMouseDown}
         >
           {children}
         </UlBox>

diff --git a/src/TreeView/useRovingTabIndex.ts b/src/TreeView/useRovingTabIndex.ts
@@ -2,7 +2,13 @@ import React from 'react'
 import {FocusKeys, useFocusZone} from '../hooks/useFocusZone'
 import {getScrollContainer} from '../utils/scroll'
 
-export function useRovingTabIndex({containerRef}: {containerRef: React.RefObject<HTMLElement>}) {
+export function useRovingTabIndex({
+  containerRef,
+  mouseDownRef,
+}: {
+  containerRef: React.RefObject<HTMLElement>
+  mouseDownRef: React.RefObject<boolean>
+}) {
   // TODO: Initialize focus to the aria-current item if it exists
   useFocusZone({
     containerRef,
@@ -19,6 +25,13 @@ export function useRovingTabIndex({containerRef}: {containerRef: React.RefObject
       return getNextFocusableElement(from, event) ?? from
     },
     focusInStrategy: () => {
+      // Don't try to execute the focusInStrategy if focus is coming from a click.
+      // The clicked row will receive focus correctly by default.
+      // If a chevron is clicked, setting the focus through the focuszone will prevent its toggle.
+      if (mouseDownRef.current) {
+        return undefined
+      }
+
       const currentItem = containerRef.current?.querySelector('[aria-current]')
       const firstItem = containerRef.current?.querySelector('[role="treeitem"]')