From 0578b637c3f7d20f5dc780da09c71acbc3cca7a8 Mon Sep 17 00:00:00 2001 From: Laszlo Vadasz Date: Mon, 30 Sep 2024 21:58:06 +0100 Subject: [PATCH] fix: tree component focus loss issue In some case, for example on high CPU load the scroll to active node can not keep up with the user input and the active element removed because it is not in the view. - check and scroll to active element when it is not in the DOM after each arrow key event. - optimize MutationObserver creation --- src/components/Tree/Tree.constants.ts | 1 + src/components/Tree/Tree.hooks.tsx | 20 ++++++--- src/components/Tree/Tree.test.tsx | 28 +++++++++++- src/components/Tree/Tree.tsx | 59 +++++++++++++++---------- src/components/Tree/Tree.types.ts | 7 ++- src/components/Tree/Tree.utils.test.tsx | 1 - 6 files changed, 80 insertions(+), 36 deletions(-) diff --git a/src/components/Tree/Tree.constants.ts b/src/components/Tree/Tree.constants.ts index 7b1024c906..68dbb7550f 100644 --- a/src/components/Tree/Tree.constants.ts +++ b/src/components/Tree/Tree.constants.ts @@ -7,6 +7,7 @@ const DEFAULTS = { SELECTABLE_NODES: 'leafOnly' as const, IS_REQUIRED: false, NODE_ID_PREFIX: 'md-tree-node', + SHOULD_NODE_FOCUS_BE_INSET: true, }; const STYLE = { diff --git a/src/components/Tree/Tree.hooks.tsx b/src/components/Tree/Tree.hooks.tsx index c6bae687e0..41a5f484b5 100644 --- a/src/components/Tree/Tree.hooks.tsx +++ b/src/components/Tree/Tree.hooks.tsx @@ -9,13 +9,18 @@ import { NODE_ID_ATTRIBUTE_NAME, NODE_ID_DATA_NAME } from '../TreeNodeBase/TreeN /** * Handle DOM changes for virtual tree * + * This hook manage 2 use cases: + * 1) When Virtual Tree removes active node, the hook adds a cloned node to not lose focus. + * 2) When the active node added back to the DOM, the hook invoke focus on the element and trigger + * key event if it happened on the cloned element. + * * @param props * @internal */ export const useVirtualTreeNavigation = ({ virtualTreeConnector, treeRef, - activeNodeId, + activeNodeIdRef, }: UseVirtualTreeNavigationProps): void => { // Handle DOM changes for virtual tree useEffect(() => { @@ -36,12 +41,13 @@ export const useVirtualTreeNavigation = ({ ); // Filter element by active node id - const getByNodeId = (node: HTMLElement) => node.dataset[NODE_ID_DATA_NAME] === activeNodeId; + const getByNodeId = (node: HTMLElement) => + node.dataset[NODE_ID_DATA_NAME] === activeNodeIdRef.current; // Mutation observer to handle the focus change const mutationHandler: MutationCallback = (mutationList) => { for (const mutation of mutationList) { - // Handle removed active node + // Active node moved out of view and removed const removed = Array.from(mutation.removedNodes).find(getByNodeId) as HTMLElement; if (removed) { // Clone the active node and make some modifications @@ -58,11 +64,11 @@ export const useVirtualTreeNavigation = ({ // add focus and keydown event listeners clonedNode.addEventListener('focus', () => { - virtualTreeConnector.scrollToNode(activeNodeId); + virtualTreeConnector.scrollToNode(activeNodeIdRef.current); }); clonedNode.addEventListener('keydown', (evt) => { if (TREE_NAVIGATION_KEYS.includes(evt.key) || (evt.key === 'Tab' && !evt.shiftKey)) { - virtualTreeConnector.scrollToNode(activeNodeId); + virtualTreeConnector.scrollToNode(activeNodeIdRef.current); keyDownEvent = new KeyboardEvent('keydown', evt); evt.stopPropagation(); evt.preventDefault(); @@ -71,7 +77,7 @@ export const useVirtualTreeNavigation = ({ return; } - // Handle adding back the focused node + // Active node moved back into the view and it added back to the DOM const added = Array.from(mutation.addedNodes).find(getByNodeId) as HTMLElement; if (added) { cleanUp(); @@ -103,5 +109,5 @@ export const useVirtualTreeNavigation = ({ observer.disconnect(); cleanUp(); }; - }, [virtualTreeConnector, activeNodeId, treeRef.current]); + }, [virtualTreeConnector, treeRef.current]); }; diff --git a/src/components/Tree/Tree.test.tsx b/src/components/Tree/Tree.test.tsx index e7686b0073..0f546eb770 100644 --- a/src/components/Tree/Tree.test.tsx +++ b/src/components/Tree/Tree.test.tsx @@ -533,7 +533,7 @@ describe('', () => { expect(scrollToNode).toHaveBeenNthCalledWith(1, '1'); }); - it('should not call scrollToNode when cloned node exists and user press Shift+Tab', async () => { + it('should call scrollToNode when the focus moves back to the tree and the active node is not in the DOM', async () => { expect.assertions(2); const scrollToNode = jest.fn(); const { getByText } = await renderTreeAndRemoveNode({ @@ -545,9 +545,33 @@ describe('', () => { // Only adding back Node 1 will remove the cloned node expect(getByText('1')).not.toBeNull(); - expect(scrollToNode).toHaveBeenCalledTimes(0); + expect(scrollToNode).toHaveBeenCalledTimes(1); }); + it.each` + keyPressed + ${'ArrowUp'} + ${'ArrowDown'} + ${'ArrowLeft'} + ${'ArrowRight'} + `( + 'should call scrollToNode when active node is not in the DOM and $kepPressed key pressed', + async ({ keyPressed }) => { + expect.assertions(2); + const scrollToNode = jest.fn(); + const { getByText } = await renderTreeAndRemoveNode({ + scrollToNode, + setNodeOpen: jest.fn(), + }); + + await userEvent.keyboard(`{${keyPressed}}`); + + // Only adding back Node 1 will remove the cloned node + expect(getByText('1')).not.toBeNull(); + expect(scrollToNode).toHaveBeenCalledTimes(1); + } + ); + it('should remove the cloned node when the real node added back', async () => { expect.assertions(5); const scrollToNode = jest.fn(); diff --git a/src/components/Tree/Tree.tsx b/src/components/Tree/Tree.tsx index 8e1586e7c3..d506a653cd 100644 --- a/src/components/Tree/Tree.tsx +++ b/src/components/Tree/Tree.tsx @@ -14,11 +14,11 @@ import { TreeIdNodeMap, Props, TreeContextValue, TreeNodeId, TreeRefObject } fro import './Tree.style.scss'; import { convertNestedTree2MappedTree, + isActiveNodeInDOM, getFistActiveNode, getNextActiveNode, getNodeDOMId, getTreeRootId, - isActiveNodeInDOM, migrateTreeState, toggleTreeNodeRecord, TreeContext, @@ -35,7 +35,7 @@ const Tree = forwardRef((props: Props, ref: ForwardedRef) => { id, style, children, - shouldNodeFocusBeInset, + shouldNodeFocusBeInset = DEFAULTS.SHOULD_NODE_FOCUS_BE_INSET, treeStructure, isRenderedFlat = DEFAULTS.IS_RENDERED_FLAT, excludeTreeRoot = DEFAULTS.EXCLUDE_TREE_ROOT, @@ -60,10 +60,11 @@ const Tree = forwardRef((props: Props, ref: ForwardedRef) => { isRequired, }); const [tree, setTree] = useState(convertNestedTree2MappedTree(treeStructure)); - const [activeNodeId, setActiveNodeId] = useState( + const [activeNode, setActiveNode] = useState( getFistActiveNode(tree, excludeTreeRoot) ); const [isFocusWithin, setIsFocusWithin] = useState(false); + const activeNodeIdRef = useRef(activeNode); const previousTree = usePrevious(tree); @@ -72,7 +73,7 @@ const Tree = forwardRef((props: Props, ref: ForwardedRef) => { migrateTreeState(previousTree, newTree); setTree(newTree); // Find the closest node to the last active node in the new tree - let newActiveNodeId = activeNodeId; + let newActiveNodeId = activeNode; while (newActiveNodeId) { if (newTree.has(newActiveNodeId) && !newTree.get(newActiveNodeId).isHidden) break; newActiveNodeId = previousTree.get(newActiveNodeId)?.parent; @@ -86,17 +87,30 @@ const Tree = forwardRef((props: Props, ref: ForwardedRef) => { const isVirtualTree = virtualTreeConnector !== undefined; + const scrollToVTreeNode = useCallback(() => { + if (isVirtualTree && virtualTreeConnector && !isActiveNodeInDOM(treeRef, activeNode)) { + virtualTreeConnector.scrollToNode?.(activeNode); + } + }, [isVirtualTree, virtualTreeConnector, activeNode]); + // Handle DOM changes for virtual tree - useVirtualTreeNavigation({ virtualTreeConnector, treeRef: treeRef, activeNodeId }); + useVirtualTreeNavigation({ virtualTreeConnector, treeRef: treeRef, activeNodeIdRef }); + + const setActiveNodeId = useCallback( + (newNodeId) => { + activeNodeIdRef.current = newNodeId; + setActiveNode(newNodeId); + scrollToVTreeNode(); + }, + [isVirtualTree, setActiveNode] + ); const toggleTreeNode = useCallback( async (id: TreeNodeId, isOpen?: boolean): Promise => { const newOpenState = isOpen !== undefined ? isOpen : !tree.get(id).isOpen; if (isVirtualTree) { - if (!isActiveNodeInDOM(treeRef, activeNodeId)) { - virtualTreeConnector.scrollToNode?.(id); - } + scrollToVTreeNode(); await virtualTreeConnector.setNodeOpen?.(id, newOpenState); } @@ -115,10 +129,10 @@ const Tree = forwardRef((props: Props, ref: ForwardedRef) => { const parent = node.parent && tree.get(node.parent); const isRoot = parent === undefined; - // tabindex depends on the treeNodeBase params as well const nodeProps = { id: getNodeDOMId(node.id), + 'aria-setsize': isRoot ? 1 : parent.children.length, 'aria-level': node.level + (excludeTreeRoot ? 0 : 1), 'aria-posinset': node.index + 1, @@ -152,15 +166,15 @@ const Tree = forwardRef((props: Props, ref: ForwardedRef) => { getNodeDetails, isRenderedFlat, shouldNodeFocusBeInset, - activeNodeId, + activeNodeId: activeNode, setActiveNodeId, toggleTreeNode, selectableNodes, itemSelection, - isFocusWithin: isFocusWithin, + isFocusWithin, }), [ - activeNodeId, + activeNode, getNodeAriaProps, getNodeDetails, isRenderedFlat, @@ -186,7 +200,10 @@ const Tree = forwardRef((props: Props, ref: ForwardedRef) => { ); const { focusWithinProps } = useFocusWithin({ - onFocusWithinChange: setIsFocusWithin, + onFocusWithinChange: (val) => { + setIsFocusWithin(val); + scrollToVTreeNode(); + }, }); const { keyboardProps } = useKeyboard({ @@ -198,14 +215,8 @@ const Tree = forwardRef((props: Props, ref: ForwardedRef) => { case 'ArrowRight': case 'ArrowLeft': { evt.preventDefault(); - if (activeNodeId) { - const next = getNextActiveNode( - tree, - activeNodeId, - key, - excludeTreeRoot, - toggleTreeNode - ); + if (activeNode) { + const next = getNextActiveNode(tree, activeNode, key, excludeTreeRoot, toggleTreeNode); setActiveNodeId(next); } break; @@ -213,11 +224,11 @@ const Tree = forwardRef((props: Props, ref: ForwardedRef) => { case 'Space': // Space if ( selectionMode !== 'none' && - activeNodeId && - (selectableNodes === 'any' || tree.get(activeNodeId)?.isLeaf) + activeNode && + (selectableNodes === 'any' || tree.get(activeNode)?.isLeaf) ) { evt.preventDefault(); - itemSelection.toggle(activeNodeId); + itemSelection.toggle(activeNode); } break; diff --git a/src/components/Tree/Tree.types.ts b/src/components/Tree/Tree.types.ts index 4da6fd2dbf..2c0a4bc066 100644 --- a/src/components/Tree/Tree.types.ts +++ b/src/components/Tree/Tree.types.ts @@ -204,9 +204,12 @@ export interface Props */ export interface UseVirtualTreeNavigationProps extends Pick { /** - * The active node id in the tree. + * Reference to the active node id in the tree. + * + * @remarks This prevent to destroy and re-create MutationObserver every time when active node + * changes. Also, this solves the problem of missed mutations. */ - activeNodeId: TreeNodeId; + activeNodeIdRef: MutableRefObject; /** * The reference of the tree DOM element. */ diff --git a/src/components/Tree/Tree.utils.test.tsx b/src/components/Tree/Tree.utils.test.tsx index 24d7b894f2..2eb9c9f0d5 100644 --- a/src/components/Tree/Tree.utils.test.tsx +++ b/src/components/Tree/Tree.utils.test.tsx @@ -21,7 +21,6 @@ import { } from './Tree.types'; import { createTreeNode as tNode } from './test.utils'; import { renderHook } from '@testing-library/react-hooks'; -import tree from './Tree'; const createSingleLevelTree = () => // prettier-ignore