From 0c96df378cff87501723f92fd7617a152131ef11 Mon Sep 17 00:00:00 2001 From: Chancellor Clark Date: Wed, 4 Jan 2023 12:37:51 -0500 Subject: [PATCH 1/3] fix(component): only focus on TreeItem when focus within Tree component --- .../big-design/src/components/Tree/Tree.tsx | 7 ++++++- .../src/components/Tree/TreeNode/TreeNode.tsx | 17 +++++++++++++---- .../big-design/src/components/Tree/types.ts | 3 +++ 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/packages/big-design/src/components/Tree/Tree.tsx b/packages/big-design/src/components/Tree/Tree.tsx index 4ffd9fb64..c3deb8b97 100644 --- a/packages/big-design/src/components/Tree/Tree.tsx +++ b/packages/big-design/src/components/Tree/Tree.tsx @@ -1,4 +1,4 @@ -import React, { createContext, useMemo } from 'react'; +import React, { createContext, useMemo, useRef } from 'react'; import { StyledUl } from './styled'; import { TreeNode } from './TreeNode'; @@ -14,6 +14,7 @@ export const TreeContext = createContext>({ onFocus: () => null, }, onKeyDown: () => null, + treeRef: { current: null }, }); export const Tree = ({ @@ -27,6 +28,8 @@ export const Tree = ({ onNodeClick, selectable, }: TreeProps): React.ReactElement> => { + const treeRef = useRef(null); + const initialTreeContext: TreeContextState = { disabledNodes, expandable, @@ -35,6 +38,7 @@ export const Tree = ({ onKeyDown, onNodeClick, selectable, + treeRef, }; const renderedItems = useMemo( @@ -47,6 +51,7 @@ export const Tree = ({ diff --git a/packages/big-design/src/components/Tree/TreeNode/TreeNode.tsx b/packages/big-design/src/components/Tree/TreeNode/TreeNode.tsx index 787ffb6f4..71da1b52b 100644 --- a/packages/big-design/src/components/Tree/TreeNode/TreeNode.tsx +++ b/packages/big-design/src/components/Tree/TreeNode/TreeNode.tsx @@ -29,8 +29,16 @@ const InternalTreeNode = ({ value, id, }: TreeNodeProps): React.ReactElement> => { - const { disabledNodes, expandable, focusable, iconless, onKeyDown, onNodeClick, selectable } = - useContext(TreeContext); + const { + disabledNodes, + expandable, + focusable, + iconless, + onKeyDown, + onNodeClick, + selectable, + treeRef, + } = useContext(TreeContext); const nodeRef = useRef(null); const selectableRef = useRef(null); const isExpanded = expandable.expandedNodes.includes(id); @@ -46,11 +54,12 @@ const InternalTreeNode = ({ if ( focusable.focusedNode === id && nodeRef.current !== document.activeElement && - document.activeElement !== document.body + document.activeElement !== document.body && + treeRef.current?.contains(document.activeElement) ) { nodeRef.current?.focus(); } - }, [focusable, id]); + }, [focusable, id, treeRef]); // Could be multiple elements in which are clicked. // Typing to generic Element type since all other elements extend from it. diff --git a/packages/big-design/src/components/Tree/types.ts b/packages/big-design/src/components/Tree/types.ts index 75085bb69..f8e990df4 100644 --- a/packages/big-design/src/components/Tree/types.ts +++ b/packages/big-design/src/components/Tree/types.ts @@ -1,3 +1,5 @@ +import { RefObject } from 'react'; + export type TreeNodeId = string; export type TreeSelectableType = 'radio' | 'multi' | undefined; @@ -61,6 +63,7 @@ export interface TreeContextState { selectable?: TreeSelectable; onKeyDown: TreeOnKeyDown; onNodeClick?: TreeOnNodeClick; + treeRef: RefObject; } export interface MapValues { From 66e84eb3d08320a03fbd45783d935c9a2450bb6c Mon Sep 17 00:00:00 2001 From: Chancellor Clark Date: Wed, 4 Jan 2023 14:47:29 -0500 Subject: [PATCH 2/3] chore(component): apply better testing practices to Tree component --- .../big-design/src/components/Tree/spec.tsx | 153 +++++++++++------- 1 file changed, 91 insertions(+), 62 deletions(-) diff --git a/packages/big-design/src/components/Tree/spec.tsx b/packages/big-design/src/components/Tree/spec.tsx index 4250ab80b..b6b6bad92 100644 --- a/packages/big-design/src/components/Tree/spec.tsx +++ b/packages/big-design/src/components/Tree/spec.tsx @@ -1,7 +1,8 @@ import { theme } from '@bigcommerce/big-design-theme'; +import userEvent from '@testing-library/user-event'; import React from 'react'; -import { act, fireEvent, render, RenderResult } from '@test/utils'; +import { fireEvent, render, RenderResult, screen } from '@test/utils'; import { Tree } from './Tree'; import { TreeExpandable, TreeFocusable, TreeNodeProps, TreeOnKeyDown, TreeProps } from './types'; @@ -43,7 +44,7 @@ const nodes: Array> = [ let expandable: TreeExpandable; let focusable: TreeFocusable; -let onKeyDown: TreeOnKeyDown; +let onKeyDown: TreeOnKeyDown; beforeEach(() => { expandable = { expandedNodes: [], onToggle: jest.fn() }; @@ -52,11 +53,11 @@ beforeEach(() => { }); const renderDefaultTree = ( - additionalProps?: Partial>, + additionalProps?: Partial>, ): RenderResult & { expandable: TreeExpandable; focusable: TreeFocusable; - onKeyDown: TreeOnKeyDown; + onKeyDown: TreeOnKeyDown; } => { const rendered = render( { - const { container } = renderDefaultTree(); +test('renders simple tree', async () => { + renderDefaultTree(); - expect(container.firstChild).toMatchSnapshot(); + const tree = await screen.findByRole('tree'); + + expect(tree).toMatchSnapshot(); }); -test('does not forward styles', () => { +test('does not forward styles', async () => { // eslint-disable-next-line // @ts-ignore - const { container } = renderDefaultTree({ className: 'test', style: { background: 'red' } }); + renderDefaultTree({ className: 'test', style: { background: 'red' } }); + + const tree = await screen.findByRole('tree'); - expect(container.getElementsByClassName('test')).toHaveLength(0); - expect(container.firstChild).not.toHaveStyle('background: red'); + expect(tree).not.toHaveClass('test'); + expect(tree.getElementsByClassName('test')).toHaveLength(0); + expect(tree).not.toHaveStyle('background: red'); }); -test('expands node on click', () => { - const { getByText, expandable } = renderDefaultTree(); - const firstTreeNode = getByText('Test Node 0'); +test('expands node on click', async () => { + const { expandable } = renderDefaultTree(); + const firstTreeNode = await screen.findByText('Test Node 0'); - act(() => { - fireEvent.click(firstTreeNode); - }); + await userEvent.click(firstTreeNode); expect(expandable.onToggle).toHaveBeenCalledWith('0', false); }); -test('calles onKeyDown event', () => { - const { getByText, onKeyDown } = renderDefaultTree(); - const firstTreeNode = getByText('Test Node 0').parentElement?.parentElement; +test('calles onKeyDown event', async () => { + const { onKeyDown } = renderDefaultTree(); + const firstTreeNode = await screen.findByRole('treeitem', { name: 'Test Node 0' }); - if (firstTreeNode) { - act(() => { - fireEvent.keyDown(firstTreeNode); - }); + await userEvent.type(firstTreeNode, '{Enter}'); - expect(onKeyDown).toHaveBeenCalledTimes(1); - } + expect(onKeyDown).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + id: '0', + }), + ); }); -test('renders with no icons', () => { - const { container } = renderDefaultTree({ +test('renders with no icons', async () => { + renderDefaultTree({ nodes: [{ id: '0', label: 'Test Node 0' }], iconless: true, }); - expect(container.querySelector('svg')).not.toBeInTheDocument(); + const tree = await screen.findByRole('tree'); + + expect(tree.querySelector('svg')).not.toBeInTheDocument(); }); -test('renders node with custom icon', () => { - const { getByTestId } = renderDefaultTree({ +test('renders node with custom icon', async () => { + renderDefaultTree({ nodes: [{ id: '0', icon: , label: 'Test Node 0' }], }); - expect(getByTestId('test-id')).toBeInTheDocument(); + const icon = await screen.findByTestId('test-id'); + + expect(icon).toBeInTheDocument(); }); test('iconless renders without custom icons', () => { - const { queryByTestId } = renderDefaultTree({ + renderDefaultTree({ nodes: [{ id: '0', icon: , label: 'Test Node 0' }], iconless: true, }); - expect(queryByTestId('test-id')).not.toBeInTheDocument(); + const icon = screen.queryByTestId('test-id'); + + expect(icon).not.toBeInTheDocument(); }); -test('renders radio select', () => { - const { container } = renderDefaultTree({ +test('renders radio select', async () => { + renderDefaultTree({ nodes: [{ id: '0', value: 0, label: 'Test Node 0' }], selectable: { type: 'radio' }, }); - const radio = container.querySelector('label'); + const tree = await screen.findByRole('tree'); + const radio = tree.querySelector('label'); - expect(radio).toBeInTheDocument(); + expect(tree).toHaveAttribute('aria-multiselectable', 'false'); expect(radio).toHaveStyle(`border-radius: ${theme.borderRadius.circle}`); }); -test('renders multiselect buttons', () => { - const { container } = renderDefaultTree({ +test('renders multiselect buttons', async () => { + renderDefaultTree({ nodes: [{ id: '0', value: 0, label: 'Test Node 0' }], selectable: { type: 'multi' }, }); - const multi = container.querySelector('label'); + const tree = await screen.findByRole('tree'); + const multi = tree.querySelector('label'); - expect(multi).toBeInTheDocument(); + expect(tree).toHaveAttribute('aria-multiselectable', 'true'); expect(multi).toHaveStyle(`border-radius: ${theme.borderRadius.normal}`); }); @@ -182,24 +195,30 @@ test('trigger onSelect', () => { }); test('renders expanded nodes', () => { - const { getByText } = renderDefaultTree({ + renderDefaultTree({ expandable: { expandedNodes: ['0', '5'], onExpand: jest.fn() }, }); - expect(getByText('Test Node 5')).toBeVisible(); - expect(getByText('Test Node 6')).not.toBeVisible(); - expect(getByText('Test Node 9')).toBeVisible(); + const node5 = screen.getByText('Test Node 5'); + const node6 = screen.getByText('Test Node 6'); + const node9 = screen.getByText('Test Node 9'); + + expect(node5).toBeVisible(); + expect(node6).not.toBeVisible(); + expect(node9).toBeVisible(); }); -test("disabled nodes don't trigger onSelect", () => { +test("disabled nodes don't trigger onSelect", async () => { const onSelect = jest.fn(); - const { container } = renderDefaultTree({ + + renderDefaultTree({ nodes: [{ id: '0', value: 0, label: 'Test Node 0' }], selectable: { type: 'multi', onSelect }, disabledNodes: ['0'], }); - const multi = container.querySelector('label'); + const tree = await screen.findByRole('tree'); + const multi = tree.querySelector('label'); if (multi) { fireEvent.click(multi); @@ -208,52 +227,62 @@ test("disabled nodes don't trigger onSelect", () => { } }); -test('triggers onNodeClick', () => { +test('triggers onNodeClick', async () => { const onNodeClick = jest.fn(); - const { getByText } = renderDefaultTree({ + + renderDefaultTree({ nodes: [{ id: '0', label: 'Test Node 0' }], onNodeClick, }); - fireEvent.click(getByText('Test Node 0')); + const node = await screen.findByText('Test Node 0'); + + await userEvent.click(node); expect(onNodeClick).toHaveBeenCalledTimes(1); }); -test('clicking node triggers onFocus', () => { +test('clicking node triggers onFocus', async () => { const onFocus = jest.fn(); - const { getByText } = renderDefaultTree({ + + renderDefaultTree({ nodes: [{ id: '0', label: 'Test Node 0' }], focusable: { focusedNode: '', onFocus }, }); - fireEvent.click(getByText('Test Node 0')); + const node = await screen.findByText('Test Node 0'); + + await userEvent.click(node); expect(onFocus).toHaveBeenCalledTimes(1); }); -test('collapsing node triggers onCollapse', () => { +test('collapsing node triggers onCollapse', async () => { const onCollapse = jest.fn(); - const { getByText } = renderDefaultTree({ + + renderDefaultTree({ nodes: [{ id: '0', label: 'Test Node 0', children: [{ id: '1', label: 'Test Node 1' }] }], expandable: { expandedNodes: ['0'], onCollapse }, }); - fireEvent.click(getByText('Test Node 0')); + const node = await screen.findByText('Test Node 0'); + + await userEvent.click(node); expect(onCollapse).toHaveBeenCalledWith('0'); }); test('expanding node triggers onExpand', async () => { const onExpand = jest.fn(); - const { getByText } = renderDefaultTree({ + + renderDefaultTree({ nodes: [{ id: '0', label: 'Test Node 0', children: [{ id: '1', label: 'Test Node 1' }] }], expandable: { expandedNodes: [], onExpand }, }); - await act(async () => { - await fireEvent.click(getByText('Test Node 0')); - }); + const node = await screen.findByText('Test Node 0'); + + await userEvent.click(node); expect(onExpand).toHaveBeenCalledWith('0'); }); From 9b2957a81fdf923333cb0b39f3c2e6bafb64736a Mon Sep 17 00:00:00 2001 From: Chancellor Clark Date: Wed, 4 Jan 2023 15:27:00 -0500 Subject: [PATCH 3/3] test(component): add tests to capture change --- .../src/components/StatefulTree/spec.tsx | 20 ++++++++++++++++++- .../big-design/src/components/Tree/spec.tsx | 15 ++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/packages/big-design/src/components/StatefulTree/spec.tsx b/packages/big-design/src/components/StatefulTree/spec.tsx index 7adbcd983..d6b0b96b5 100644 --- a/packages/big-design/src/components/StatefulTree/spec.tsx +++ b/packages/big-design/src/components/StatefulTree/spec.tsx @@ -1,7 +1,8 @@ import { theme } from '@bigcommerce/big-design-theme'; +import userEvent from '@testing-library/user-event'; import React from 'react'; -import { fireEvent, render } from '@test/utils'; +import { fireEvent, render, screen } from '@test/utils'; import { StatefulTree, StatefulTreeProps, TreeNodeProps } from '.'; @@ -182,3 +183,20 @@ describe('selectable = radio', () => { } }); }); + +test('should focus on TreeItem on arrow down', async () => { + render(getSimpleTree()); + + const node0 = await screen.findByRole('treeitem', { name: 'Test Node 0' }); + const node1 = await screen.findByRole('treeitem', { name: 'Test Node 1' }); + + await userEvent.tab(); + + expect(node0).toHaveFocus(); + expect(node1).not.toHaveFocus(); + + await userEvent.keyboard('{ArrowDown}'); + + expect(node0).not.toHaveFocus(); + expect(node1).toHaveFocus(); +}); diff --git a/packages/big-design/src/components/Tree/spec.tsx b/packages/big-design/src/components/Tree/spec.tsx index b6b6bad92..83cb5e131 100644 --- a/packages/big-design/src/components/Tree/spec.tsx +++ b/packages/big-design/src/components/Tree/spec.tsx @@ -286,3 +286,18 @@ test('expanding node triggers onExpand', async () => { expect(onExpand).toHaveBeenCalledWith('0'); }); + +test('should focus when tabbed on', async () => { + renderDefaultTree({ + nodes: [{ id: '0', label: 'Test Node 0' }], + focusable: { focusedNode: '0', onFocus: jest.fn() }, + }); + + const node = await screen.findByRole('treeitem'); + + expect(node).not.toHaveFocus(); + + await userEvent.tab(); + + expect(node).toHaveFocus(); +});