fix: missing group aria label

- add aria-labelledby to each group nodes - removed error throwing when TreeNodeBase is not wrapped with TreeContext issue: https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-506156
momentum-design · Sep 27, 2024 · 37b9ca8 · 37b9ca8
1 parent 1973bef
commit 37b9ca8
Show file tree

Hide file tree

Showing 7 changed files with 85 additions and 20 deletions.
diff --git a/src/components/Tree/Tree.test.tsx b/src/components/Tree/Tree.test.tsx
@@ -7,8 +7,7 @@ import userEvent from '@testing-library/user-event';
 import Tree, { TREE_CONSTANTS, TreeProps } from './index';
 import { createTreeNode as tNode } from './test.utils';
 import TreeNodeBase from '../TreeNodeBase';
-import { convertNestedTree2MappedTree, getTreeRootId, mapTree } from './Tree.utils';
-import { act } from 'react-test-renderer';
+import { convertNestedTree2MappedTree, mapTree } from './Tree.utils';
 
 const getSampleTree = () => {
   // prettier-ignore
@@ -39,12 +38,6 @@ describe('<Tree />', () => {
     treeStructure: tNode('root', true, [tNode('1'), tNode('2')]),
   };
 
-  // Remove noisy console.warn
-  beforeAll(() => {
-    jest.spyOn(console, 'warn').mockImplementation(() => {
-      /**/
-    });
-  });
   afterAll(() => {
     jest.restoreAllMocks();
   });
@@ -946,6 +939,9 @@ describe('<Tree />', () => {
     });
 
     it('should update selected items based on the selectionMode', async () => {
+      jest.spyOn(console, 'warn').mockImplementation(() => {
+        /**/
+      });
       const tree = getSampleTree();
       const { rerender, getByTestId } = render(
         getTreeComponent(tree, { selectionMode: 'multiple', selectedItems: ['1', '2.2'] })
@@ -958,6 +954,8 @@ describe('<Tree />', () => {
 
       expect(getByTestId('1')).not.toHaveAttribute('aria-selected', 'true');
       expect(getByTestId('2.2')).not.toHaveAttribute('aria-selected', 'true');
+      // eslint-disable-next-line no-console
+      expect(console.warn).toHaveBeenCalled();
     });
 
     describe('controlled tree selection', () => {

diff --git a/src/components/Tree/Tree.tsx b/src/components/Tree/Tree.tsx
@@ -135,6 +135,7 @@ const Tree = forwardRef((props: Props, ref: ForwardedRef<TreeRefObject>) => {
       const groupProps = {
         role: 'group',
         'aria-owns': node.children.map(getNodeDOMId).join(' '),
+        'aria-labelledby': getNodeDOMId(node.id),
       };
 
       return {

diff --git a/src/components/Tree/Tree.utils.test.tsx b/src/components/Tree/Tree.utils.test.tsx
@@ -74,9 +74,17 @@ describe('Tree utils', () => {
       expect(result.current).toBe('treeContext');
     });
 
-    it('should throw an error when the tree context is not available', () => {
+    it('should return with null and log error when the tree context is not available', () => {
+      jest.spyOn(console, 'error').mockImplementation(() => {
+        /**/
+      });
       const { result } = renderHook(() => useTreeContext());
-      expect(result.error).toEqual(Error('useTreeContext hook used without TreeContext!'));
+      expect(result.current).toEqual(null);
+      // eslint-disable-next-line no-console
+      expect(console.error).toHaveBeenNthCalledWith(
+        1,
+        'useTreeContext hook used without TreeContext!'
+      );
     });
   });
 
@@ -126,10 +134,15 @@ describe('Tree utils', () => {
     });
 
     it('should returns with the same nodeId when it is not found in the tree', () => {
+      jest.spyOn(console, 'warn').mockImplementation(() => {
+        /**/
+      });
       const result = getNextActiveNode(tree, 'not-found', 'ArrowDown', true, toggleTreeNode);
 
       expect(toggleTreeNode).not.toHaveBeenCalled();
       expect(result).toEqual('not-found');
+      // eslint-disable-next-line no-console
+      expect(console.warn).toHaveBeenCalledTimes(1);
     });
 
     describe('when root excluded', () => {
@@ -258,6 +271,9 @@ describe('Tree utils', () => {
         });
 
         it('should return with the last node id before the loop restart when the tree has a loop', () => {
+          jest.spyOn(console, 'error').mockImplementation(() => {
+            /**/
+          });
           const loopTree = createTree();
           loopTree.get('2').parent = '2.2.3';
 
@@ -305,10 +321,15 @@ describe('Tree utils', () => {
       });
 
       it('should return with the last node id before the loop restart when the tree has a loop', () => {
+        jest.spyOn(console, 'error').mockImplementation(() => {
+          /**/
+        });
         const loopTree = createTree();
         loopTree.get('2.2').children.push('3');
 
         expect(getNextActiveNode(loopTree, '3', 'ArrowUp', true, toggleTreeNode)).toEqual('3');
+        // eslint-disable-next-line no-console
+        expect(console.error).toHaveBeenCalledTimes(1);
       });
     });
 
@@ -605,6 +626,9 @@ describe('Tree utils', () => {
     });
 
     it('should skip duplicated tree node id with the whole subtree', () => {
+      jest.spyOn(console, 'error').mockImplementation(() => {
+        /**/
+      });
       const tree = tNode('<root>', true, [tNode('0'), tNode('0', false, [tNode('1')])]);
 
       expect(convertNestedTree2MappedTree(tree)).toEqual(
@@ -650,6 +674,8 @@ describe('Tree utils', () => {
           ],
         ])
       );
+      // eslint-disable-next-line no-console
+      expect(console.error).toHaveBeenCalledTimes(1);
     });
   });
 
@@ -663,6 +689,9 @@ describe('Tree utils', () => {
     });
 
     it('should return with empty array when the there is no root node', () => {
+      jest.spyOn(console, 'error').mockImplementation(() => {
+        /**/
+      });
       // Tree with circular reference -> no node with parent === undefined
       const tree: TreeIdNodeMap = new Map([
         ['root', { id: 'root', parent: '1' } as TreeNodeRecord],
@@ -673,6 +702,8 @@ describe('Tree utils', () => {
       expect(mapTree(tree, callback, { rootNodeId: wrongId })).toEqual([]);
 
       expect(callback).not.toHaveBeenCalled();
+      // eslint-disable-next-line no-console
+      expect(console.error).toHaveBeenCalledTimes(1);
     });
 
     it('should not call the callback on the tree root by default', () => {

diff --git a/src/components/Tree/Tree.utils.ts b/src/components/Tree/Tree.utils.ts
@@ -21,7 +21,8 @@ export const TreeContext = React.createContext<TreeContextValue>(null);
 export const useTreeContext = (): TreeContextValue => {
   const value = useContext(TreeContext);
   if (!value) {
-    throw new Error('useTreeContext hook used without TreeContext!');
+    // eslint-disable-next-line no-console
+    console.error('useTreeContext hook used without TreeContext!');
   }
   return value;
 };

diff --git a/src/components/TreeNodeBase/TreeNodeBase.test.tsx b/src/components/TreeNodeBase/TreeNodeBase.test.tsx
@@ -73,6 +73,17 @@ describe('TreeNodeBase', () => {
       jest.restoreAllMocks();
     });
 
+    it('should match snapshot without tree context', () => {
+      useTreeContextSpy = useTreeContextSpy = jest
+        .spyOn(treeUtils, 'useTreeContext')
+        .mockImplementation(() => null);
+      expect.assertions(1);
+
+      container = mount(<TreeNodeBase nodeId="42">{() => 'Test'}</TreeNodeBase>);
+
+      expect(container).toMatchSnapshot();
+    });
+
     it('should match snapshot', () => {
       expect.assertions(1);
 
@@ -262,6 +273,7 @@ describe('TreeNodeBase', () => {
       expect(container.find('div[role="group"]').props()).toEqual({
         'aria-owns': 'md-tree-node-1 md-tree-node-2',
         className: 'md-tree-node-base-group',
+        'aria-labelledby': 'md-tree-node-root',
         role: 'group',
       });
     });
@@ -634,4 +646,17 @@ describe('TreeNodeBase', () => {
     rerender(<Wrapper value={getMockedTreeContext('42')} />);
     expect(getByTestId('tree-node-1')).not.toHaveFocus();
   });
+
+  it('should handle without error and do not render DOM element when the tree context is null', () => {
+    const Wrapper = () => (
+      <TreeNodeBase data-testid="tree-node-1" key="1" nodeId="42">
+        {() => <button data-testid="node-content">Content</button>}
+      </TreeNodeBase>
+    );
+
+    const { queryByTestId } = render(<Wrapper />);
+
+    expect(queryByTestId('tree-node-1')).not.toBeInTheDocument();
+    expect(queryByTestId('node-content')).not.toBeInTheDocument();
+  });
 });
diff --git a/src/components/TreeNodeBase/TreeNodeBase.test.tsx.snap b/src/components/TreeNodeBase/TreeNodeBase.test.tsx.snap
@@ -322,6 +322,12 @@ exports[`TreeNodeBase snapshot should match snapshot with style 1`] = `
 </TreeNodeBase>
 `;
 
+exports[`TreeNodeBase snapshot should match snapshot without tree context 1`] = `
+<TreeNodeBase
+  nodeId="42"
+/>
+`;
+
 exports[`TreeNodeBase snapshot should not render the content of the hidden nodes 1`] = `
 <Tree
   excludeTreeRoot={false}
@@ -397,6 +403,7 @@ exports[`TreeNodeBase snapshot should not render the content of the hidden nodes
               root
             </div>
             <div
+              aria-labelledby="md-tree-node-root"
               aria-owns="md-tree-node-1 md-tree-node-2"
               className="md-tree-node-base-group"
               role="group"
@@ -539,6 +546,7 @@ exports[`TreeNodeBase snapshot should render grouping element when the tree is f
           >
             root
             <div
+              aria-labelledby="md-tree-node-root"
               aria-owns="md-tree-node-1 md-tree-node-2"
               className="md-tree-node-base-group"
               role="group"

diff --git a/src/components/TreeNodeBase/TreeNodeBase.tsx b/src/components/TreeNodeBase/TreeNodeBase.tsx
@@ -51,7 +51,7 @@ const TreeNodeBase = (props: Props, providedRef: TreeNodeBaseRefOrCallbackRef):
   }
 
   const treeContext = useTreeContext();
-  const nodeDetails = treeContext.getNodeDetails(nodeId);
+  const nodeDetails = treeContext?.getNodeDetails(nodeId);
 
   const internalRef = useRef<HTMLDivElement>();
   const ref = providedRef && typeof providedRef !== 'function' ? providedRef : internalRef;
@@ -144,7 +144,7 @@ const TreeNodeBase = (props: Props, providedRef: TreeNodeBaseRefOrCallbackRef):
   /**
    * Focus management
    */
-  const tabIndex = nodeId === treeContext.activeNodeId ? 0 : -1;
+  const tabIndex = nodeId === treeContext?.activeNodeId ? 0 : -1;
 
   // makes sure that whenever an item is pressed, the tree focus state gets updated as well
   useEffect(() => {
@@ -166,10 +166,11 @@ const TreeNodeBase = (props: Props, providedRef: TreeNodeBaseRefOrCallbackRef):
   const lastActiveNode = usePrevious(treeContext?.activeNodeId);
   useDidUpdateEffect(() => {
     if (
+      treeContext &&
       ref.current &&
       lastActiveNode !== undefined &&
-      lastActiveNode !== treeContext?.activeNodeId &&
-      treeContext?.activeNodeId === nodeId &&
+      lastActiveNode !== treeContext.activeNodeId &&
+      treeContext.activeNodeId === nodeId &&
       treeContext.isFocusWithin
     ) {
       ref.current.focus();
@@ -189,10 +190,10 @@ const TreeNodeBase = (props: Props, providedRef: TreeNodeBaseRefOrCallbackRef):
     return null;
   }
 
-  const { nodeProps, groupProps } = treeContext.getNodeAriaProps(nodeId);
+  const { nodeProps, groupProps } = treeContext?.getNodeAriaProps(nodeId);
   const isSelected =
-    treeContext.itemSelection.selectionMode !== 'none'
-      ? treeContext.itemSelection.isSelected(nodeId)
+    treeContext?.itemSelection.selectionMode !== 'none'
+      ? treeContext?.itemSelection.isSelected(nodeId)
       : undefined;
 
   return (
@@ -208,7 +209,7 @@ const TreeNodeBase = (props: Props, providedRef: TreeNodeBaseRefOrCallbackRef):
         data-shape={shape}
         className={classnames(className, STYLE.wrapper, {
           selected: isPressed || isSelected,
-          'active-node': nodeId === treeContext.activeNodeId,
+          'active-node': nodeId === treeContext?.activeNodeId,
         })}
         lang={lang}
         {...{ [NODE_ID_ATTRIBUTE_NAME]: nodeId }}
@@ -217,7 +218,7 @@ const TreeNodeBase = (props: Props, providedRef: TreeNodeBaseRefOrCallbackRef):
         {...rest}
       >
         {content}
-        {treeContext.isRenderedFlat && !nodeDetails.isLeaf && (
+        {treeContext?.isRenderedFlat && !nodeDetails.isLeaf && (
           <div className={STYLE.group} {...groupProps} />
         )}
       </div>