From 0e9f8296fb9e24b44cff5f40e94675cfa4af4755 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Mon, 24 Oct 2022 12:12:06 -0500 Subject: [PATCH 1/7] feat(TreeView): add support for empty parent nodes --- src/TreeView/TreeView.stories.tsx | 36 +++++++++++++++++++++++++++ src/TreeView/TreeView.test.tsx | 2 +- src/TreeView/TreeView.tsx | 41 ++++++++++++++++++++++++++++--- 3 files changed, 74 insertions(+), 5 deletions(-) diff --git a/src/TreeView/TreeView.stories.tsx b/src/TreeView/TreeView.stories.tsx index e186675e1b0..bd9b1622687 100644 --- a/src/TreeView/TreeView.stories.tsx +++ b/src/TreeView/TreeView.stories.tsx @@ -442,6 +442,9 @@ export const AsyncWithCount: Story = args => { + + + src @@ -622,4 +625,37 @@ export const StressTest: Story = () => { ) } +export const EmptyDirectories: Story = () => { + const [state, setState] = React.useState('loading') + + return ( + <> + + { + if (expanded) { + setTimeout(() => { + setState('done') + }, 1000) + } + }} + > + + + + src + + + + + + + .github + + + + + ) +} + export default meta diff --git a/src/TreeView/TreeView.test.tsx b/src/TreeView/TreeView.test.tsx index c8b4c2d143b..348c649c35e 100644 --- a/src/TreeView/TreeView.test.tsx +++ b/src/TreeView/TreeView.test.tsx @@ -989,7 +989,7 @@ describe('Asyncronous loading', () => { }) }) - it.only('moves focus to parent item after closing error dialog', async () => { + it('moves focus to parent item after closing error dialog', async () => { function TestTree() { const [error, setError] = React.useState('Test error') diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index 3ea37f1bb4a..28643575c70 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -22,6 +22,7 @@ import VisuallyHidden from '../_VisuallyHidden' import {getAccessibleName} from './shared' import {getFirstChildElement, useRovingTabIndex} from './useRovingTabIndex' import {useTypeahead} from './useTypeahead' +import {set} from 'lodash' // ---------------------------------------------------------------------------- // Context @@ -35,6 +36,8 @@ const RootContext = React.createContext<{ const ItemContext = React.createContext<{ itemId: string level: number + isSubTreeEmpty: boolean + setIsSubTreeEmpty: React.Dispatch> isExpanded: boolean setIsExpanded: React.Dispatch> leadingVisualId: string @@ -42,6 +45,8 @@ const ItemContext = React.createContext<{ }>({ itemId: '', level: 1, + isSubTreeEmpty: false, + setIsSubTreeEmpty: () => {}, isExpanded: false, setIsExpanded: () => {}, leadingVisualId: '', @@ -135,6 +140,7 @@ const Item = React.forwardRef( }) const {level} = React.useContext(ItemContext) const {hasSubTree, subTree, childrenWithoutSubTree} = useSubTree(children) + const [isSubTreeEmpty, setIsSubTreeEmpty] = React.useState(!hasSubTree) // Expand or collapse the subtree const toggle = React.useCallback( @@ -189,6 +195,8 @@ const Item = React.forwardRef( value={{ itemId, level: level + 1, + isSubTreeEmpty, + setIsSubTreeEmpty, isExpanded, setIsExpanded, leadingVisualId, @@ -205,7 +213,7 @@ const Item = React.forwardRef( aria-labelledby={labelId} aria-describedby={`${leadingVisualId} ${trailingVisualId}`} aria-level={level} - aria-expanded={hasSubTree ? isExpanded : undefined} + aria-expanded={isSubTreeEmpty ? undefined : isExpanded} aria-current={isCurrentItem ? 'true' : undefined} onKeyDown={handleKeyDown} sx={{ @@ -328,7 +336,7 @@ const Item = React.forwardRef( - {isExpanded ? subTree : null} + {subTree} ) @@ -409,11 +417,22 @@ export type TreeViewSubTreeProps = { const SubTree: React.FC = ({count, state, children}) => { const {announceUpdate} = React.useContext(RootContext) - const {itemId, isExpanded} = React.useContext(ItemContext) + const {itemId, isExpanded, isSubTreeEmpty, setIsSubTreeEmpty} = React.useContext(ItemContext) const [isLoadingItemVisible, setIsLoadingItemVisible] = React.useState(false) const {safeSetTimeout, safeClearTimeout} = useSafeTimeout() const timeoutId = React.useRef(0) const loadingItemRef = React.useRef(null) + const ref = React.useRef(null) + + // If `state` is undefined, then we're in a synchronous context. Otherwise, + // check to see if `state` is `'done'` for the asynchronous use-case. + if (state === undefined || state === 'done') { + if (!isSubTreeEmpty && !children) { + setIsSubTreeEmpty(true) + } else if (isSubTreeEmpty && children) { + setIsSubTreeEmpty(false) + } + } // Announce when content has loaded React.useEffect(() => { @@ -422,8 +441,16 @@ const SubTree: React.FC = ({count, state, children}) => { if (!parentItem) return + const {current: node} = ref const parentName = getAccessibleName(parentItem) - announceUpdate(`${parentName} content loaded`) + + safeSetTimeout(() => { + if (node && node.childElementCount > 0) { + announceUpdate(`${parentName} content loaded`) + } else { + announceUpdate(`${parentName} is empty`) + } + }) } }, [state, itemId, announceUpdate]) @@ -462,6 +489,10 @@ const SubTree: React.FC = ({count, state, children}) => { } }, [state, safeSetTimeout, safeClearTimeout, isLoadingItemVisible, itemId]) + if (!isExpanded) { + return null + } + return ( = ({count, state, children}) => { padding: 0, margin: 0 }} + // @ts-ignore + ref={ref} > {isLoadingItemVisible ? : children} From 87c55d3ddb7aeacd239049c772c276862654869a Mon Sep 17 00:00:00 2001 From: Josh Black Date: Mon, 24 Oct 2022 12:22:47 -0500 Subject: [PATCH 2/7] test: update TreeView tests to use fake timers --- src/TreeView/TreeView.test.tsx | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/TreeView/TreeView.test.tsx b/src/TreeView/TreeView.test.tsx index 348c649c35e..010c285406b 100644 --- a/src/TreeView/TreeView.test.tsx +++ b/src/TreeView/TreeView.test.tsx @@ -1,8 +1,11 @@ import {fireEvent, render, waitFor} from '@testing-library/react' import React from 'react' +import {act} from 'react-dom/test-utils' import {ThemeProvider} from '../ThemeProvider' import {SubTreeState, TreeView} from './TreeView' +jest.useFakeTimers() + // TODO: Move this function into a shared location function renderWithTheme( ui: Parameters[0], @@ -940,6 +943,10 @@ describe('Asyncronous loading', () => { // Click done button to mimic the completion of async loading fireEvent.click(doneButton) + act(() => { + jest.runAllTimers() + }) + // Live region should be updated expect(liveRegion).toHaveTextContent('Parent content loaded') }) @@ -983,13 +990,15 @@ describe('Asyncronous loading', () => { // Wait for async loading to complete const firstChild = await findByRole('treeitem', {name: 'Child 1'}) - setTimeout(() => { - // First child should be focused - expect(firstChild).toHaveFocus() + act(() => { + jest.runAllTimers() }) + + // First child should be focused + expect(firstChild).toHaveFocus() }) - it('moves focus to parent item after closing error dialog', async () => { + it.skip('moves focus to parent item after closing error dialog', async () => { function TestTree() { const [error, setError] = React.useState('Test error') From c994f7b7489a713d17fef1bb951bc5b190fe8108 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Mon, 24 Oct 2022 12:31:02 -0500 Subject: [PATCH 3/7] chore: update story --- src/TreeView/TreeView.stories.tsx | 23 +++++++++++++++++------ src/TreeView/TreeView.tsx | 1 - 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/TreeView/TreeView.stories.tsx b/src/TreeView/TreeView.stories.tsx index bd9b1622687..ff4092d5af3 100644 --- a/src/TreeView/TreeView.stories.tsx +++ b/src/TreeView/TreeView.stories.tsx @@ -625,18 +625,29 @@ export const StressTest: Story = () => { ) } -export const EmptyDirectories: Story = () => { +export const EmptyDirectory: Story = () => { const [state, setState] = React.useState('loading') + const timeoutId = React.useRef | null>(null) + + React.useEffect(() => { + return () => { + if (timeoutId.current) { + clearTimeout(timeoutId.current) + timeoutId.current = null + } + } + }, []) return ( - <> + { if (expanded) { - setTimeout(() => { + timeoutId.current = setTimeout(() => { setState('done') - }, 1000) + timeoutId.current = null + }, 2000) } }} > @@ -644,7 +655,7 @@ export const EmptyDirectories: Story = () => { src - + @@ -654,7 +665,7 @@ export const EmptyDirectories: Story = () => { - + ) } diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index 28643575c70..79e1f4cc4b7 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -22,7 +22,6 @@ import VisuallyHidden from '../_VisuallyHidden' import {getAccessibleName} from './shared' import {getFirstChildElement, useRovingTabIndex} from './useRovingTabIndex' import {useTypeahead} from './useTypeahead' -import {set} from 'lodash' // ---------------------------------------------------------------------------- // Context From d00f4a9fc026deb1ead91f2a33600a35309b3c19 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Mon, 24 Oct 2022 12:34:52 -0500 Subject: [PATCH 4/7] chore: add changeset --- .changeset/eleven-countries-hang.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/eleven-countries-hang.md diff --git a/.changeset/eleven-countries-hang.md b/.changeset/eleven-countries-hang.md new file mode 100644 index 00000000000..fc25ba2c85a --- /dev/null +++ b/.changeset/eleven-countries-hang.md @@ -0,0 +1,5 @@ +--- +'@primer/react': patch +--- + +Add support for dynamic aria-expanded support on TreeView.Item From 88b5c8c2805d9e67c4477472bb72203f014d33cd Mon Sep 17 00:00:00 2001 From: Josh Black Date: Mon, 24 Oct 2022 14:46:40 -0500 Subject: [PATCH 5/7] chore: address eslint errors --- src/TreeView/TreeView.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index 79e1f4cc4b7..a915ea9122b 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -451,7 +451,7 @@ const SubTree: React.FC = ({count, state, children}) => { } }) } - }, [state, itemId, announceUpdate]) + }, [state, itemId, announceUpdate, safeSetTimeout]) // Show loading indicator after a short delay React.useEffect(() => { @@ -502,7 +502,7 @@ const SubTree: React.FC = ({count, state, children}) => { padding: 0, margin: 0 }} - // @ts-ignore + // @ts-ignore Box doesn't have type support for `ref` used in combination with `as` ref={ref} > {isLoadingItemVisible ? : children} From 5e8a6eac18c6ac320d19c946c9d6947c0bd8e784 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Mon, 24 Oct 2022 15:54:38 -0500 Subject: [PATCH 6/7] refactor(TreeView): move aria-expanded into useEffect, add tests --- src/TreeView/TreeView.test.tsx | 86 +++++++++++++++++++++++++++++++++- src/TreeView/TreeView.tsx | 20 ++++---- 2 files changed, 97 insertions(+), 9 deletions(-) diff --git a/src/TreeView/TreeView.test.tsx b/src/TreeView/TreeView.test.tsx index 010c285406b..1bf40195965 100644 --- a/src/TreeView/TreeView.test.tsx +++ b/src/TreeView/TreeView.test.tsx @@ -1,4 +1,5 @@ import {fireEvent, render, waitFor} from '@testing-library/react' +import userEvent from '@testing-library/user-event' import React from 'react' import {act} from 'react-dom/test-utils' import {ThemeProvider} from '../ThemeProvider' @@ -158,6 +159,40 @@ describe('Markup', () => { // space expect(noDescription).toHaveAccessibleDescription(' ') }) + + it('should include `aria-expanded` when a SubTree contains content', async () => { + const user = userEvent.setup({ + advanceTimers: jest.advanceTimersByTime + }) + const {getByLabelText, getByText} = renderWithTheme( + + + Item 1 + + Item 1.a + Item 1.b + Item 1.c + + + + Item 2 + + + + ) + + let treeitem = getByLabelText(/Item 1/) + expect(treeitem).toHaveAttribute('aria-expanded', 'false') + + await user.click(getByText(/Item 1/)) + expect(treeitem).toHaveAttribute('aria-expanded', 'true') + + treeitem = getByLabelText(/Item 2/) + expect(treeitem).not.toHaveAttribute('aria-expanded') + + await user.click(getByText(/Item 2/)) + expect(treeitem).not.toHaveAttribute('aria-expanded') + }) }) describe('Keyboard interactions', () => { @@ -998,7 +1033,7 @@ describe('Asyncronous loading', () => { expect(firstChild).toHaveFocus() }) - it.skip('moves focus to parent item after closing error dialog', async () => { + it('moves focus to parent item after closing error dialog', async () => { function TestTree() { const [error, setError] = React.useState('Test error') @@ -1073,4 +1108,53 @@ describe('Asyncronous loading', () => { // Parent item should still be expanded expect(parentItem).toHaveAttribute('aria-expanded', 'true') }) + + it('should remove `aria-expanded` if no content is loaded in', async () => { + function Example() { + const [state, setState] = React.useState('loading') + const timeoutId = React.useRef | null>(null) + + React.useEffect(() => { + return () => { + if (timeoutId.current) { + clearTimeout(timeoutId.current) + timeoutId.current = null + } + } + }, []) + + return ( + + { + if (expanded) { + timeoutId.current = setTimeout(() => { + setState('done') + }, 1000) + } + }} + > + Item 1 + + + + ) + } + const {getByLabelText, getByText} = renderWithTheme() + const user = userEvent.setup({ + advanceTimers: jest.advanceTimersByTime + }) + + let treeitem = getByLabelText(/Item 1/) + expect(treeitem).toHaveAttribute('aria-expanded', 'false') + + await user.click(getByText(/Item 1/)) + expect(treeitem).toHaveAttribute('aria-expanded', 'true') + + act(() => { + jest.runAllTimers() + }) + + expect(treeitem).not.toHaveAttribute('aria-expanded') + }) }) diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index a915ea9122b..25c4ff34aa1 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -423,15 +423,19 @@ const SubTree: React.FC = ({count, state, children}) => { const loadingItemRef = React.useRef(null) const ref = React.useRef(null) - // If `state` is undefined, then we're in a synchronous context. Otherwise, - // check to see if `state` is `'done'` for the asynchronous use-case. - if (state === undefined || state === 'done') { - if (!isSubTreeEmpty && !children) { - setIsSubTreeEmpty(true) - } else if (isSubTreeEmpty && children) { - setIsSubTreeEmpty(false) + React.useEffect(() => { + // If `state` is undefined, we're working in a synchronous context and need + // to detect if the sub-tree has content. If `state === 'done` then we're + // working in an asynchronous context and need to see if there is content + // that has been loaded in. + if (state === undefined || state === 'done') { + if (!isSubTreeEmpty && !children) { + setIsSubTreeEmpty(true) + } else if (isSubTreeEmpty && children) { + setIsSubTreeEmpty(false) + } } - } + }, [state, isSubTreeEmpty, setIsSubTreeEmpty, children]) // Announce when content has loaded React.useEffect(() => { From 19a9f0a846de7a9a71bc3eb994c79e3dde733d68 Mon Sep 17 00:00:00 2001 From: Josh Black Date: Mon, 24 Oct 2022 16:00:32 -0500 Subject: [PATCH 7/7] chore: address eslint violations --- src/TreeView/TreeView.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/TreeView/TreeView.test.tsx b/src/TreeView/TreeView.test.tsx index 1bf40195965..4072d8955a2 100644 --- a/src/TreeView/TreeView.test.tsx +++ b/src/TreeView/TreeView.test.tsx @@ -1145,7 +1145,7 @@ describe('Asyncronous loading', () => { advanceTimers: jest.advanceTimersByTime }) - let treeitem = getByLabelText(/Item 1/) + const treeitem = getByLabelText(/Item 1/) expect(treeitem).toHaveAttribute('aria-expanded', 'false') await user.click(getByText(/Item 1/))