From 631d010246e8c9eae5d41ad86c96dbecf123fce2 Mon Sep 17 00:00:00 2001 From: vitaliirumiantsev-cengage Date: Wed, 25 Sep 2024 19:20:57 +0300 Subject: [PATCH] fix(treeview): fix switching to Focus Mode inside `TreeView` (#1453) --- .changeset/a11y-treeview-navigation.md | 5 ++ .../src/components/TreeView/TreeItem.tsx | 39 +++++---- .../src/components/TreeView/TreeView.test.js | 83 ++++++++++++------- .../src/components/TreeView/useTreeItem.ts | 31 +++++-- 4 files changed, 101 insertions(+), 57 deletions(-) create mode 100644 .changeset/a11y-treeview-navigation.md diff --git a/.changeset/a11y-treeview-navigation.md b/.changeset/a11y-treeview-navigation.md new file mode 100644 index 0000000000..c076b4868b --- /dev/null +++ b/.changeset/a11y-treeview-navigation.md @@ -0,0 +1,5 @@ +--- +'react-magma-dom': patch +--- + +fix(TreeView): Fix switching to Focus Mode inside `TreeView`. diff --git a/packages/react-magma-dom/src/components/TreeView/TreeItem.tsx b/packages/react-magma-dom/src/components/TreeView/TreeItem.tsx index 977c03231c..2aa2ea1520 100644 --- a/packages/react-magma-dom/src/components/TreeView/TreeItem.tsx +++ b/packages/react-magma-dom/src/components/TreeView/TreeItem.tsx @@ -64,6 +64,19 @@ const StyledTreeItem = styled.li<{ padding-inline-start: ${props => calculateOffset(props.nodeType, props.depth)}; + &:focus { + outline: none; + + & > *:first-child { + outline-offset: -2px; + outline: 2px solid + ${props => + props.isInverse + ? props.theme.colors.focusInverse + : props.theme.colors.focus}; +} + } + > div:first-of-type { background: ${props => props.selected && props.isInverse @@ -171,14 +184,6 @@ const StyledItemWrapper = styled.div<{ props.selectable, props.nodeType )}; - &:focus { - outline-offset: -2px; - outline: 2px solid - ${props => - props.isInverse - ? props.theme.colors.focusInverse - : props.theme.colors.focus}; - } `; export const TreeItem = React.forwardRef( @@ -310,6 +315,13 @@ export const TreeItem = React.forwardRef( selectableType={selectable} selected={selectedItem} theme={theme} + tabIndex={itemToFocus === itemId ? 0 : -1} + onKeyDown={handleKeyDown} + onClick={event => { + if (selectable===TreeViewSelectable.off && hasOwnTreeItems) { + onExpandedClicked(event); + } + }} > ( isDisabled={isDisabled} isInverse={isInverse} nodeType={nodeType} - onClick={event => { - if (selectable === TreeViewSelectable.off && hasOwnTreeItems) { - onExpandedClicked(event); - } - }} - onKeyDown={e => { - handleKeyDown(e); - }} - ref={ref} selectable={selectable} style={style} - tabIndex={itemToFocus === itemId ? 0 : -1} theme={theme} + ref={ref} > {hasOwnTreeItems && ( { }) ); - userEvent.click(getByTestId('item1-label')); + const item1 = getByTestId('item1'); + const item1Label = getByTestId('item1-label'); + + userEvent.click(item1Label); + expect(onSelectedItemChange).toHaveBeenCalledTimes(1) expect(onSelectedItemChange).toHaveBeenCalled(); + expect(item1).toHaveAttribute('aria-selected', 'true'); + + userEvent.click(item1Label); + expect(onSelectedItemChange).toHaveBeenCalledTimes(1); + expect(item1).toHaveAttribute('aria-selected', 'true'); }); it('function gets called when it has a preselected item', () => { @@ -1318,9 +1327,9 @@ describe('TreeView', () => { }) ); - const item1 = getByTestId('item1-itemwrapper'); - const item2 = getByTestId('item2-itemwrapper'); - const item3 = getByTestId('item3-itemwrapper'); + const item1 = getByTestId('item1'); + const item2 = getByTestId('item2'); + const item3 = getByTestId('item3'); userEvent.tab(); expect(item1).toHaveFocus(); @@ -1339,7 +1348,7 @@ describe('TreeView', () => { // expand item fireEvent.keyDown(item1, { key: 'ArrowRight' }); - const item1child = getByTestId('item-child1-itemwrapper'); + const item1child = getByTestId('item-child1'); expect(getByTestId('item1')).toHaveAttribute('aria-expanded', 'true'); expect(item1).toHaveFocus(); @@ -1361,8 +1370,8 @@ describe('TreeView', () => { ); - const item0 = getByTestId('item0-itemwrapper'); - const item1 = getByTestId('item1-itemwrapper'); + const item0 = getByTestId('item0'); + const item1 = getByTestId('item1'); userEvent.tab(); expect(item0).toHaveFocus(); @@ -1382,8 +1391,8 @@ describe('TreeView', () => { ); - const item0 = getByTestId('item0-itemwrapper'); - const item1 = getByTestId('item1-itemwrapper'); + const item0 = getByTestId('item0'); + const item1 = getByTestId('item1'); userEvent.tab(); expect(item0).toHaveFocus(); @@ -1398,8 +1407,8 @@ describe('TreeView', () => { it('should expand the focused branch item when pressing ArrowRight', () => { const { getByTestId } = render(getTreeItemsOneLevelSmall({})); - const item0Wrapper = getByTestId('item0-itemwrapper'); - const item1Wrapper = getByTestId('item1-itemwrapper'); + const item0Wrapper = getByTestId('item0'); + const item1Wrapper = getByTestId('item1'); const item1 = getByTestId('item1'); userEvent.tab(); @@ -1416,9 +1425,9 @@ describe('TreeView', () => { getTreeItemsOneLevelSmall({ initialExpandedItems: ['item1'] }) ); - const item0 = getByTestId('item0-itemwrapper'); - const item1 = getByTestId('item1-itemwrapper'); - const item1child = getByTestId('item-child1-itemwrapper'); + const item0 = getByTestId('item0'); + const item1 = getByTestId('item1'); + const item1child = getByTestId('item-child1'); userEvent.tab(); fireEvent.keyDown(item0, { key: 'ArrowDown' }); @@ -1430,7 +1439,7 @@ describe('TreeView', () => { it('should maintain focus when pressing ArrowRight on a leaf item', () => { const { getByTestId } = render(getTreeItemsOneLevelSmall({})); - const item0 = getByTestId('item0-itemwrapper'); + const item0 = getByTestId('item0'); userEvent.tab(); fireEvent.keyDown(item0, { key: 'ArrowRight' }); @@ -1443,8 +1452,8 @@ describe('TreeView', () => { getTreeItemsOneLevelSmall({ initialExpandedItems: ['item1'] }) ); - const item0 = getByTestId('item0-itemwrapper'); - const item1 = getByTestId('item1-itemwrapper'); + const item0 = getByTestId('item0'); + const item1 = getByTestId('item1'); userEvent.tab(); @@ -1458,7 +1467,7 @@ describe('TreeView', () => { it('should maintain focus when pressing ArrowLeft on a leaf item', () => { const { getByTestId } = render(getTreeItemsOneLevelSmall({})); - const item0 = getByTestId('item0-itemwrapper'); + const item0 = getByTestId('item0'); userEvent.tab(); fireEvent.keyDown(item0, { key: 'ArrowLeft' }); @@ -1469,8 +1478,8 @@ describe('TreeView', () => { it('should focus to the first item when pressing the Home key', () => { const { getByTestId } = render(getTreeItemsOneLevelSmall({})); - const item0 = getByTestId('item0-itemwrapper'); - const item1 = getByTestId('item1-itemwrapper'); + const item0 = getByTestId('item0'); + const item1 = getByTestId('item1'); userEvent.tab(); fireEvent.focus(item1); @@ -1484,8 +1493,8 @@ describe('TreeView', () => { getTreeItemsOneLevelSmall({ initialExpandedItems: ['item1'] }) ); - const item0 = getByTestId('item0-itemwrapper'); - const item1Child = getByTestId('item-child1-itemwrapper'); + const item0 = getByTestId('item0'); + const item1Child = getByTestId('item-child1'); userEvent.tab(); fireEvent.focus(item0); @@ -1499,9 +1508,9 @@ describe('TreeView', () => { getTreeItemsOneLevel({ initialExpandedItems: ['item-3'] }) ); - const item0 = getByTestId('item0-itemwrapper'); - const item2 = getByTestId('item2-itemwrapper'); - const item3 = getByTestId('item3-itemwrapper'); + const item0 = getByTestId('item0'); + const item2 = getByTestId('item2'); + const item3 = getByTestId('item3'); userEvent.tab(); fireEvent.keyDown(item0, { key: 'End' }); @@ -1543,7 +1552,7 @@ describe('TreeView', () => { ); - const item0 = getByTestId('item0-itemwrapper'); + const item0 = getByTestId('item0'); userEvent.tab(); expect(item0).toHaveFocus(); @@ -1553,7 +1562,7 @@ describe('TreeView', () => { const { getByTestId } = render( getTreeItemsOneLevelSmall({ selectable: TreeViewSelectable.off }) ); - const item1 = getByTestId('item1-itemwrapper'); + const item1 = getByTestId('item1'); userEvent.tab(); expect(item1).toHaveFocus(); @@ -1567,7 +1576,7 @@ describe('TreeView', () => { const { getByTestId } = render( getTreeItemsOneLevelSmall({ selectable: TreeViewSelectable.single }) ); - const item0 = getByTestId('item0-itemwrapper'); + const item0 = getByTestId('item0'); userEvent.tab(); expect(item0).toHaveFocus(); @@ -1586,7 +1595,7 @@ describe('TreeView', () => { initialExpandedItems: ['item1'], }) ); - const item1Child = getByTestId('item-child1-itemwrapper'); + const item1Child = getByTestId('item-child1'); userEvent.tab(); expect(item1Child).toHaveFocus(); @@ -1612,12 +1621,18 @@ describe('TreeView', () => { fireEvent.keyDown(item1wrapper, { key: 'Enter' }); expect(item1).toHaveAttribute('aria-selected', 'true'); + expect(onSelectedItemChange).toHaveBeenCalledTimes(1) expect(onSelectedItemChange).toHaveBeenCalledWith([ { itemId: 'item1', checkedStatus: IndeterminateCheckboxStatus.checked, }, ]); + + fireEvent.keyDown(item1wrapper, { key: 'Enter' }); + + expect(item1).toHaveAttribute('aria-selected', 'true'); + expect(onSelectedItemChange).toHaveBeenCalledTimes(1) }); it('should select the leaf item when pressing the Space key', () => { @@ -1638,12 +1653,18 @@ describe('TreeView', () => { fireEvent.keyDown(item0wrapper, { key: ' ' }); expect(item0).toHaveAttribute('aria-selected', 'true'); + expect(onSelectedItemChange).toHaveBeenCalledTimes(1) expect(onSelectedItemChange).toHaveBeenCalledWith([ { itemId: 'item0', checkedStatus: IndeterminateCheckboxStatus.checked, }, ]); + + fireEvent.keyDown(item0wrapper, { key: ' ' }); + + expect(item0).toHaveAttribute('aria-selected', 'true'); + expect(onSelectedItemChange).toHaveBeenCalledTimes(1) }); it('should toggle expand the branch item when pressing the Space key', () => { @@ -1686,7 +1707,7 @@ describe('TreeView', () => { const { getByTestId } = render( getTreeItemsOneLevelSmall({ selectable: TreeViewSelectable.multi }) ); - const item0 = getByTestId('item0-itemwrapper'); + const item0 = getByTestId('item0'); userEvent.tab(); expect(item0).toHaveFocus(); @@ -1708,7 +1729,7 @@ describe('TreeView', () => { ], }) ); - const item1 = getByTestId('item1-itemwrapper'); + const item1 = getByTestId('item1'); userEvent.tab(); expect(item1).toHaveFocus(); diff --git a/packages/react-magma-dom/src/components/TreeView/useTreeItem.ts b/packages/react-magma-dom/src/components/TreeView/useTreeItem.ts index 53e93ea7aa..361e90199d 100644 --- a/packages/react-magma-dom/src/components/TreeView/useTreeItem.ts +++ b/packages/react-magma-dom/src/components/TreeView/useTreeItem.ts @@ -147,8 +147,14 @@ export function useTreeItem(props: UseTreeItemProps, forwardedRef) { }; const handleClick = (event, itemId) => { + const isChecked = checkedStatus === IndeterminateCheckboxStatus.checked; + + if (selectable === TreeViewSelectable.single && isChecked) { + return; + } + if (selectable !== TreeViewSelectable.off) { - selectItem({ itemId, checkedStatus: checkedStatus === IndeterminateCheckboxStatus.checked ? IndeterminateCheckboxStatus.unchecked : IndeterminateCheckboxStatus.checked }) + selectItem({ itemId, checkedStatus: isChecked ? IndeterminateCheckboxStatus.unchecked : IndeterminateCheckboxStatus.checked }) onClick && typeof onClick === 'function' && onClick(); } }; @@ -179,7 +185,7 @@ export function useTreeItem(props: UseTreeItemProps, forwardedRef) { const filteredRefArray = filterNullEntries(treeItemRefArray); const curr = filteredRefArray['current']; - (curr?.[0].current as HTMLDivElement).focus(); + (curr?.[0].current as HTMLDivElement).closest('[role=treeitem]').focus(); }; const focusNext = () => { @@ -197,11 +203,11 @@ export function useTreeItem(props: UseTreeItemProps, forwardedRef) { } if (next) { - next.focus(); + next.closest('[role=treeitem]').focus(); } else { const nextNext = curr?.[focusIndex + 2]?.current as HTMLDivElement; if (nextNext) { - nextNext.focus(); + nextNext.closest('[role=treeitem]').focus(); } else { focusFirst(); } @@ -223,7 +229,7 @@ export function useTreeItem(props: UseTreeItemProps, forwardedRef) { } if (itemToFocus) { - itemToFocus.focus(); + itemToFocus.closest('[role=treeitem]').focus(); } }; @@ -233,7 +239,7 @@ export function useTreeItem(props: UseTreeItemProps, forwardedRef) { ( filteredRefArray['current']?.[arrLength - 1].current as HTMLDivElement - ).focus(); + ).closest('[role=treeitem]').focus(); }; const focusSelf = () => { @@ -241,7 +247,7 @@ export function useTreeItem(props: UseTreeItemProps, forwardedRef) { const curr = filteredRefArray['current']; focusIndex = getFocusIndex(curr); - (curr?.[focusIndex].current as HTMLDivElement).focus(); + (curr?.[focusIndex].current as HTMLDivElement).closest('[role=treeitem]').focus(); }; const expandFocusedNode = () => { @@ -273,10 +279,13 @@ export function useTreeItem(props: UseTreeItemProps, forwardedRef) { const curr = filteredRefArray['current']; const arrLength = curr.length; - if (['ArrowDown', 'ArrowUp', 'Home', 'End', ' '].includes(event.key)) { + if (['ArrowDown', 'ArrowUp', 'ArrowRight', 'ArrowLeft', 'Home', 'End', 'Enter', ' '].includes(event.key)) { event.preventDefault(); + event.stopPropagation(); } + const isChecked = checkedStatus === IndeterminateCheckboxStatus.checked; + switch (event.key) { case 'ArrowDown': { // Move to the next item, or wrap to first @@ -313,6 +322,9 @@ export function useTreeItem(props: UseTreeItemProps, forwardedRef) { if (selectable === TreeViewSelectable.off && hasOwnTreeItems) { setExpanded(!expanded); } else if (selectable === TreeViewSelectable.single) { + if (isChecked) { + return; + } // In single-select it selects the focused node. selectItem({ itemId, checkedStatus: IndeterminateCheckboxStatus.checked }) } else if (selectable === TreeViewSelectable.multi) { @@ -329,6 +341,9 @@ export function useTreeItem(props: UseTreeItemProps, forwardedRef) { if (hasOwnTreeItems) { setExpanded(!expanded); } else { + if (isChecked) { + return; + } selectItem({ itemId, checkedStatus: IndeterminateCheckboxStatus.checked }); } } else if (selectable === TreeViewSelectable.multi) {