From d7d7430e49fafbb186b88bc02bd2c7f2bccf6773 Mon Sep 17 00:00:00 2001 From: Bernardo Sunderhus Date: Mon, 9 Sep 2024 21:29:22 +0200 Subject: [PATCH] bugfix(react-tree): recover from tabIndex=-1 when TreeItem is removed (#32442) --- ...-b6607987-a159-4abc-be0c-225294e9e8a7.json | 7 + .../react-tree/library/etc/react-tree.api.md | 1 + .../src/components/FlatTree/useFlatTree.ts | 6 +- .../FlatTree/useFlatTreeContextValues.ts | 2 + .../library/src/components/Tree/Tree.cy.tsx | 177 +++++++++++++----- .../library/src/components/Tree/useTree.ts | 5 +- .../components/Tree/useTreeContextValues.ts | 2 + .../src/components/TreeItem/useTreeItem.tsx | 18 +- .../library/src/contexts/treeContext.ts | 4 + .../src/hooks/useFlatTreeNavigation.ts | 8 +- .../library/src/hooks/useRootTree.ts | 3 + .../library/src/hooks/useRovingTabIndexes.ts | 19 +- .../library/src/hooks/useTreeNavigation.ts | 3 +- 13 files changed, 189 insertions(+), 66 deletions(-) create mode 100644 change/@fluentui-react-tree-b6607987-a159-4abc-be0c-225294e9e8a7.json diff --git a/change/@fluentui-react-tree-b6607987-a159-4abc-be0c-225294e9e8a7.json b/change/@fluentui-react-tree-b6607987-a159-4abc-be0c-225294e9e8a7.json new file mode 100644 index 0000000000000..cb3b9cf064f2d --- /dev/null +++ b/change/@fluentui-react-tree-b6607987-a159-4abc-be0c-225294e9e8a7.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "bugfix: recover from tabIndex=-1 when TreeItem is removed", + "packageName": "@fluentui/react-tree", + "email": "bernardo.sunderhus@gmail.com", + "dependentChangeType": "patch" +} diff --git a/packages/react-components/react-tree/library/etc/react-tree.api.md b/packages/react-components/react-tree/library/etc/react-tree.api.md index ebb82c1d301fd..1fbeac4b6638b 100644 --- a/packages/react-components/react-tree/library/etc/react-tree.api.md +++ b/packages/react-components/react-tree/library/etc/react-tree.api.md @@ -159,6 +159,7 @@ export type TreeContextValue = { openItems: ImmutableSet; checkedItems: ImmutableMap; requestTreeResponse(request: TreeItemRequest): void; + forceUpdateRovingTabIndex?(): void; }; // @public (undocumented) diff --git a/packages/react-components/react-tree/library/src/components/FlatTree/useFlatTree.ts b/packages/react-components/react-tree/library/src/components/FlatTree/useFlatTree.ts index e9b11f3544ff8..12d019abab874 100644 --- a/packages/react-components/react-tree/library/src/components/FlatTree/useFlatTree.ts +++ b/packages/react-components/react-tree/library/src/components/FlatTree/useFlatTree.ts @@ -37,7 +37,10 @@ function useRootFlatTree(props: FlatTreeProps, ref: React.Ref): Fla }, useMergedRefs(ref, navigation.rootRef), ), - { treeType: 'flat' } as const, + { + treeType: 'flat', + forceUpdateRovingTabIndex: navigation.forceUpdateRovingTabIndex, + } as const, ); } @@ -59,6 +62,7 @@ function useSubFlatTree(props: FlatTreeProps, ref: React.Ref): Flat openItems: ImmutableSet.empty, checkedItems: ImmutableMap.empty, requestTreeResponse: noop, + forceUpdateRovingTabIndex: noop, appearance: 'subtle', size: 'medium', // ------ defaultTreeContextValue diff --git a/packages/react-components/react-tree/library/src/components/FlatTree/useFlatTreeContextValues.ts b/packages/react-components/react-tree/library/src/components/FlatTree/useFlatTreeContextValues.ts index dec4263b4af54..a06d65e2ec568 100644 --- a/packages/react-components/react-tree/library/src/components/FlatTree/useFlatTreeContextValues.ts +++ b/packages/react-components/react-tree/library/src/components/FlatTree/useFlatTreeContextValues.ts @@ -12,6 +12,7 @@ export const useFlatTreeContextValues_unstable = (state: FlatTreeState): FlatTre appearance, size, requestTreeResponse, + forceUpdateRovingTabIndex, } = state; /** * This context is created with "@fluentui/react-context-selector", @@ -27,6 +28,7 @@ export const useFlatTreeContextValues_unstable = (state: FlatTreeState): FlatTre contextType, level, requestTreeResponse, + forceUpdateRovingTabIndex, }; return { tree }; diff --git a/packages/react-components/react-tree/library/src/components/Tree/Tree.cy.tsx b/packages/react-components/react-tree/library/src/components/Tree/Tree.cy.tsx index ded288f821c3e..0413eafba5eb9 100644 --- a/packages/react-components/react-tree/library/src/components/Tree/Tree.cy.tsx +++ b/packages/react-components/react-tree/library/src/components/Tree/Tree.cy.tsx @@ -409,59 +409,138 @@ describe('Tree', () => { }); }); - it('should ensure roving tab indexes when focusing programmatically', () => { - mount( - <> - - - - , - ); - cy.get('#btn-before-tree').focus().realPress('Tab'); - cy.get('[data-testid="item1"]').should('be.focused'); - cy.get('[data-testid="item2__item1"]').focus().realPress('Tab'); - cy.get('#btn-after-tree').should('be.focused').realPress(['Shift', 'Tab']); - cy.get('[data-testid="item2__item1"]').should('be.focused'); - }); - - it('should ensure roving tab indexes when children change', () => { - const RovingTreeTest = () => { - const [show, setShow] = React.useState(true); - return ( + describe('roving tab indexes', () => { + it('should ensure roving tab indexes when focusing programmatically', () => { + mount( <> - - - {show && ( - <> - - level 1, item 1 - + + + + , + ); + cy.get('#btn-before-tree').focus().realPress('Tab'); + cy.get('[data-testid="item1"]').should('be.focused'); + cy.get('[data-testid="item2__item1"]').focus().realPress('Tab'); + cy.get('#btn-after-tree').should('be.focused').realPress(['Shift', 'Tab']); + cy.get('[data-testid="item2__item1"]').should('be.focused'); + }); + + it('should ensure roving tab indexes when children change', () => { + const RovingTreeTest = () => { + const [show, setShow] = React.useState(true); + return ( + <> + + + {show && ( + <> + + level 1, item 1 + + + level 1, item 2 + + + )} + + level 1, item 3 + + + level 1, item 4 + + + + ); + }; + + mount(); + cy.get('[data-testid="item1"]').should('have.attr', 'tabindex', '0').focus().realPress('ArrowDown'); + cy.get('[data-testid="item2"]') + .should('be.focused') + .should('have.attr', 'tabindex', '0') + .get('#btn-before-tree') + .realClick(); + cy.get('[data-testid="item3"]').should('have.attr', 'tabindex', '0'); + }); + + it('should ensure a treeitem has tabIndex=0, when the current tabIndex=0 item is removed by collapsing its parent', () => { + const RovingTreeTest = () => { + const [openItems, setOpenItems] = React.useState(() => new Set()); + return ( + <> + + { + setOpenItems(data.openItems); + }} + > + + level 1, item 1 + + + level 2, item 1 + + + + + level 1, item 2 + + + level 1, item 3 + + + level 1, item 4 + + + + ); + }; + mount(); + cy.get('[data-testid="item1"]').should('have.attr', 'tabindex', '0').focus().realPress('Enter'); + cy.get('[data-testid="item1-1"]').should('exist').focus().should('have.attr', 'tabindex', '0'); + cy.get('#btn-before-tree').realClick(); + cy.get('[data-testid="item1-1"]').should('not.exist'); + cy.get('[data-testid="item1"]').should('have.attr', 'tabindex', '0'); + }); + + it('should ensure a treeitem has tabIndex=0, when the current tabIndex=0 item is removed without focus', () => { + const RovingTreeTest = () => { + const [show, setShow] = React.useState(true); + return ( + <> + + + + level 1, item 1 + + {show && ( level 1, item 2 - - )} - - level 1, item 3 - - - level 1, item 4 - - - - ); - }; - - mount(); - cy.get('[data-testid="item1"]').should('have.attr', 'tabindex', '0').focus().realPress('ArrowDown'); - cy.get('[data-testid="item2"]') - .should('be.focused') - .should('have.attr', 'tabindex', '0') - .get('#btn-before-tree') - .realClick(); - cy.get('[data-testid="item3"]').should('have.attr', 'tabindex', '0'); + )} + + level 1, item 3 + + + level 1, item 4 + + + + ); + }; + mount(); + cy.get('[data-testid="item1"]').should('have.attr', 'tabindex', '0').focus().realPress('ArrowDown'); + cy.get('[data-testid="item2"]').should('be.focused').should('have.attr', 'tabindex', '0'); + cy.get('#btn-before-tree').realClick(); + cy.get('[data-testid="item1"]').should('have.attr', 'tabindex', '0'); + }); }); }); diff --git a/packages/react-components/react-tree/library/src/components/Tree/useTree.ts b/packages/react-components/react-tree/library/src/components/Tree/useTree.ts index aaa4082cc39e1..233b7fdf32628 100644 --- a/packages/react-components/react-tree/library/src/components/Tree/useTree.ts +++ b/packages/react-components/react-tree/library/src/components/Tree/useTree.ts @@ -60,7 +60,10 @@ function useNestedRootTree(props: TreeProps, ref: React.Ref): TreeS }, useMergedRefs(ref, navigation.treeRef), ), - { treeType: 'nested' } as const, + { + treeType: 'nested', + forceUpdateRovingTabIndex: navigation.forceUpdateRovingTabIndex, + } as const, ); } diff --git a/packages/react-components/react-tree/library/src/components/Tree/useTreeContextValues.ts b/packages/react-components/react-tree/library/src/components/Tree/useTreeContextValues.ts index 7cc55b172b6fa..fdfa26728522c 100644 --- a/packages/react-components/react-tree/library/src/components/Tree/useTreeContextValues.ts +++ b/packages/react-components/react-tree/library/src/components/Tree/useTreeContextValues.ts @@ -16,6 +16,7 @@ export function useTreeContextValues_unstable(state: TreeState): TreeContextValu appearance, size, requestTreeResponse, + forceUpdateRovingTabIndex, } = state; /** * This context is created with "@fluentui/react-context-selector", @@ -31,6 +32,7 @@ export function useTreeContextValues_unstable(state: TreeState): TreeContextValu contextType, level, requestTreeResponse, + forceUpdateRovingTabIndex, }; return { tree }; diff --git a/packages/react-components/react-tree/library/src/components/TreeItem/useTreeItem.tsx b/packages/react-components/react-tree/library/src/components/TreeItem/useTreeItem.tsx index 2d7eaf68c5c51..fa5a72aaa25ab 100644 --- a/packages/react-components/react-tree/library/src/components/TreeItem/useTreeItem.tsx +++ b/packages/react-components/react-tree/library/src/components/TreeItem/useTreeItem.tsx @@ -38,6 +38,7 @@ export function useTreeItem_unstable(props: TreeItemProps, ref: React.Ref ctx.requestTreeResponse); + const forceUpdateRovingTabIndex = useTreeContext_unstable(ctx => ctx.forceUpdateRovingTabIndex); const { level: contextLevel } = useSubtreeContext_unstable(); const parentValue = useTreeItemContext_unstable(ctx => props.parentValue ?? ctx.value); @@ -79,13 +80,24 @@ export function useTreeItem_unstable(props: TreeItemProps, ref: React.Ref should be declared inside a component. - `); + @fluentui/react-tree [useTreeItem]: + should be declared inside a component. + `); } }, [hasTreeContext]); } + React.useEffect(() => { + const treeItem = treeItemRef.current; + return () => { + // When the tree item is unmounted, we need to update the roving tab index + // if the tree item is the current tab indexed item + if (treeItem && treeItem.tabIndex === 0) { + forceUpdateRovingTabIndex?.(); + } + }; + }, [forceUpdateRovingTabIndex]); + const open = useTreeContext_unstable(ctx => props.open ?? ctx.openItems.has(value)); const getNextOpen = () => (itemType === 'branch' ? !open : open); const selectionMode = useTreeContext_unstable(ctx => ctx.selectionMode); diff --git a/packages/react-components/react-tree/library/src/contexts/treeContext.ts b/packages/react-components/react-tree/library/src/contexts/treeContext.ts index 5eec0b9fe895b..721a33fb67b3d 100644 --- a/packages/react-components/react-tree/library/src/contexts/treeContext.ts +++ b/packages/react-components/react-tree/library/src/contexts/treeContext.ts @@ -18,6 +18,9 @@ export type TreeContextValue = { * requests root Tree component to respond to some tree item event, */ requestTreeResponse(request: TreeItemRequest): void; + // FIXME: this is only marked as optional to avoid breaking changes + // it should always be provided internally + forceUpdateRovingTabIndex?(): void; }; export type TreeItemRequest = { itemType: TreeItemType } & ( @@ -37,6 +40,7 @@ const defaultTreeContextValue: TreeContextValue = { openItems: ImmutableSet.empty, checkedItems: ImmutableMap.empty, requestTreeResponse: noop, + forceUpdateRovingTabIndex: noop, appearance: 'subtle', size: 'medium', }; diff --git a/packages/react-components/react-tree/library/src/hooks/useFlatTreeNavigation.ts b/packages/react-components/react-tree/library/src/hooks/useFlatTreeNavigation.ts index e3e8f1962ab8e..cf1cbaf262e09 100644 --- a/packages/react-components/react-tree/library/src/hooks/useFlatTreeNavigation.ts +++ b/packages/react-components/react-tree/library/src/hooks/useFlatTreeNavigation.ts @@ -13,7 +13,7 @@ export function useFlatTreeNavigation() { 'use no memo'; const { walkerRef, rootRef: walkerRootRef } = useHTMLElementWalkerRef(); - const { rove, initialize: initializeRovingTabIndex } = useRovingTabIndex(); + const { rove, forceUpdate: forceUpdateRovingTabIndex, initialize: initializeRovingTabIndex } = useRovingTabIndex(); const rootRefCallback: React.RefCallback = React.useCallback( root => { @@ -87,7 +87,11 @@ export function useFlatTreeNavigation() { rove(nextElement); } }); - return { navigate, rootRef: useMergedRefs(walkerRootRef, rootRefCallback) } as const; + return { + navigate, + rootRef: useMergedRefs(walkerRootRef, rootRefCallback), + forceUpdateRovingTabIndex, + } as const; } function firstChild(target: HTMLElement, treeWalker: HTMLElementWalker): HTMLElement | null { diff --git a/packages/react-components/react-tree/library/src/hooks/useRootTree.ts b/packages/react-components/react-tree/library/src/hooks/useRootTree.ts index f931b198fda6c..f127e698d88e2 100644 --- a/packages/react-components/react-tree/library/src/hooks/useRootTree.ts +++ b/packages/react-components/react-tree/library/src/hooks/useRootTree.ts @@ -95,6 +95,9 @@ export function useRootTree( openItems, checkedItems, requestTreeResponse, + forceUpdateRovingTabIndex: () => { + // noop + }, root: slot.always( getIntrinsicElementProps('div', { // FIXME: diff --git a/packages/react-components/react-tree/library/src/hooks/useRovingTabIndexes.ts b/packages/react-components/react-tree/library/src/hooks/useRovingTabIndexes.ts index a164e81793b3b..14d9bc448b178 100644 --- a/packages/react-components/react-tree/library/src/hooks/useRovingTabIndexes.ts +++ b/packages/react-components/react-tree/library/src/hooks/useRovingTabIndexes.ts @@ -13,15 +13,6 @@ export function useRovingTabIndex() { const walkerRef = React.useRef(null); const { targetDocument } = useFluent(); - React.useEffect(() => { - if ( - (currentElementRef.current === null || !targetDocument?.body.contains(currentElementRef.current)) && - walkerRef.current - ) { - initialize(walkerRef.current); - } - }); - useFocusedElementChange(element => { if ( element?.getAttribute('role') === 'treeitem' && @@ -60,8 +51,18 @@ export function useRovingTabIndex() { currentElementRef.current = nextElement; }, []); + const forceUpdate = React.useCallback(() => { + if ( + (currentElementRef.current === null || !targetDocument?.body.contains(currentElementRef.current)) && + walkerRef.current + ) { + initialize(walkerRef.current); + } + }, [targetDocument, initialize]); + return { rove, initialize, + forceUpdate, }; } diff --git a/packages/react-components/react-tree/library/src/hooks/useTreeNavigation.ts b/packages/react-components/react-tree/library/src/hooks/useTreeNavigation.ts index 843d7a8c2a4ca..93e85bfbef447 100644 --- a/packages/react-components/react-tree/library/src/hooks/useTreeNavigation.ts +++ b/packages/react-components/react-tree/library/src/hooks/useTreeNavigation.ts @@ -13,7 +13,7 @@ import { useMergedRefs } from '@fluentui/react-utilities'; export function useTreeNavigation() { 'use no memo'; - const { rove, initialize: initializeRovingTabIndex } = useRovingTabIndex(); + const { rove, initialize: initializeRovingTabIndex, forceUpdate: forceUpdateRovingTabIndex } = useRovingTabIndex(); const { walkerRef, rootRef: walkerRootRef } = useHTMLElementWalkerRef(); const rootRefCallback: React.RefCallback = React.useCallback( @@ -64,6 +64,7 @@ export function useTreeNavigation() { return { navigate, treeRef: useMergedRefs(walkerRootRef, rootRefCallback) as React.RefCallback, + forceUpdateRovingTabIndex, } as const; }