From 5d067381243139b79ea65b47ce61782cd14a92a0 Mon Sep 17 00:00:00 2001 From: Joshua Rush Date: Tue, 23 May 2023 14:11:18 -0400 Subject: [PATCH] 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 --- .changeset/khaki-walls-applaud.md | 5 + src/TreeView/TreeView.features.stories.tsx | 115 ++++++++++++++++++++- src/TreeView/TreeView.test.tsx | 37 +++++++ src/TreeView/TreeView.tsx | 20 +++- src/TreeView/useRovingTabIndex.ts | 15 ++- 5 files changed, 185 insertions(+), 7 deletions(-) create mode 100644 .changeset/khaki-walls-applaud.md diff --git a/.changeset/khaki-walls-applaud.md b/.changeset/khaki-walls-applaud.md new file mode 100644 index 00000000000..71ef8ece8b7 --- /dev/null +++ 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 index aec221bd7ae..25316a10a6f 100644 --- a/src/TreeView/TreeView.features.stories.tsx +++ b/src/TreeView/TreeView.features.stories.tsx @@ -777,11 +777,118 @@ export const InitialFocus: Story = () => (
- Item 1 - - Item 2 + + + + + src + + + + + + Avatar.tsx + + + + + + Button + + + + + + Button.tsx + + + + + + Button.test.tsx + + + + + + Button2.tsx + + + + + + Button2.test.tsx + + + + + + Button3.tsx + + + + + + Button3.test.tsx + + + + + + Button4.tsx + + + + + + Button4.test.tsx + + + + + + Button5.tsx + + + + + + Button5.test.tsx + + + + + + Button6.tsx + + + + + + Button6.test.tsx + + + + + + Button7.tsx + + + + + + Button7.test.tsx + + + + + + + + ReallyLongFileNameThatShouldBeTruncated.tsx + + - Item 3
diff --git a/src/TreeView/TreeView.test.tsx b/src/TreeView/TreeView.test.tsx index 2a069a0e79a..a13c8c0277e 100644 --- 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( +
+ + + + Item 1 + + SubItem 1 + SubItem 2 + SubItem 3 + + + + Item 2 + + Item 3 + +
, + ) + + // 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 index 15c36120e31..0819c335c56 100644 --- 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 = ({ flat, }) => { const containerRef = React.useRef(null) + const mouseDownRef = React.useRef(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 = ({ aria-label={ariaLabel} aria-labelledby={ariaLabelledby} data-omit-spacer={flat} + onMouseDown={onMouseDown} > {children} diff --git a/src/TreeView/useRovingTabIndex.ts b/src/TreeView/useRovingTabIndex.ts index 6959e037b41..99cc4b9f319 100644 --- 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}) { +export function useRovingTabIndex({ + containerRef, + mouseDownRef, +}: { + containerRef: React.RefObject + mouseDownRef: React.RefObject +}) { // 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"]')