bugfix(react-tree): recover from tabIndex=-1 when TreeItem is removed (…

…microsoft#32442)
igecloudsdev · Sep 9, 2024 · d7d7430 · d7d7430
1 parent 2a31760
commit d7d7430
Show file tree

Hide file tree

Showing 13 changed files with 189 additions and 66 deletions.
diff --git a/change/@fluentui-react-tree-b6607987-a159-4abc-be0c-225294e9e8a7.json 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": "[email protected]",
+  "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
@@ -159,6 +159,7 @@ export type TreeContextValue = {
     openItems: ImmutableSet<TreeItemValue>;
     checkedItems: ImmutableMap<TreeItemValue, 'mixed' | boolean>;
     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
@@ -37,7 +37,10 @@ function useRootFlatTree(props: FlatTreeProps, ref: React.Ref<HTMLElement>): 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<HTMLElement>): Flat
     openItems: ImmutableSet.empty,
     checkedItems: ImmutableMap.empty,
     requestTreeResponse: noop,
+    forceUpdateRovingTabIndex: noop,
     appearance: 'subtle',
     size: 'medium',
     // ------ defaultTreeContextValue

diff --git a/...s/react-components/react-tree/library/src/components/FlatTree/useFlatTreeContextValues.ts b/...s/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
@@ -409,59 +409,138 @@ describe('Tree', () => {
     });
   });
 
-  it('should ensure roving tab indexes when focusing programmatically', () => {
-    mount(
-      <>
-        <button id="btn-before-tree">before tree</button>
-        <TreeTest defaultOpenItems={['item1', 'item2', 'item2__item1']} />
-        <button id="btn-after-tree">after tree</button>
-      </>,
-    );
-    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(
         <>
-          <button onClick={() => setShow(s => !s)} id="btn-before-tree">
-            toggle tree
-          </button>
-          <TreeTest>
-            {show && (
-              <>
-                <TreeItem itemType="leaf" value="item1" data-testid="item1">
-                  <TreeItemLayout>level 1, item 1</TreeItemLayout>
-                </TreeItem>
+          <button id="btn-before-tree">before tree</button>
+          <TreeTest defaultOpenItems={['item1', 'item2', 'item2__item1']} />
+          <button id="btn-after-tree">after tree</button>
+        </>,
+      );
+      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 (
+          <>
+            <button onClick={() => setShow(s => !s)} id="btn-before-tree">
+              toggle tree
+            </button>
+            <TreeTest>
+              {show && (
+                <>
+                  <TreeItem itemType="leaf" value="item1" data-testid="item1">
+                    <TreeItemLayout>level 1, item 1</TreeItemLayout>
+                  </TreeItem>
+                  <TreeItem itemType="leaf" value="item2" data-testid="item2">
+                    <TreeItemLayout>level 1, item 2</TreeItemLayout>
+                  </TreeItem>
+                </>
+              )}
+              <TreeItem itemType="leaf" value="item3" data-testid="item3">
+                <TreeItemLayout>level 1, item 3</TreeItemLayout>
+              </TreeItem>
+              <TreeItem itemType="leaf" value="item4" data-testid="item4">
+                <TreeItemLayout>level 1, item 4</TreeItemLayout>
+              </TreeItem>
+            </TreeTest>
+          </>
+        );
+      };
+
+      mount(<RovingTreeTest />);
+      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<TreeItemValue>());
+        return (
+          <>
+            <button onClick={() => setOpenItems(new Set())} id="btn-before-tree">
+              close tree
+            </button>
+            <Tree
+              openItems={openItems}
+              onOpenChange={(_, data) => {
+                setOpenItems(data.openItems);
+              }}
+            >
+              <TreeItem itemType="branch" value="item1" data-testid="item1">
+                <TreeItemLayout>level 1, item 1</TreeItemLayout>
+                <Tree>
+                  <TreeItem itemType="leaf" value="item2" data-testid="item1-1">
+                    <TreeItemLayout>level 2, item 1</TreeItemLayout>
+                  </TreeItem>
+                </Tree>
+              </TreeItem>
+              <TreeItem itemType="leaf" value="item2" data-testid="item2">
+                <TreeItemLayout>level 1, item 2</TreeItemLayout>
+              </TreeItem>
+              <TreeItem itemType="leaf" value="item3" data-testid="item3">
+                <TreeItemLayout>level 1, item 3</TreeItemLayout>
+              </TreeItem>
+              <TreeItem itemType="leaf" value="item4" data-testid="item4">
+                <TreeItemLayout>level 1, item 4</TreeItemLayout>
+              </TreeItem>
+            </Tree>
+          </>
+        );
+      };
+      mount(<RovingTreeTest />);
+      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 (
+          <>
+            <button onClick={() => setShow(current => !current)} id="btn-before-tree">
+              toggle tree
+            </button>
+            <Tree>
+              <TreeItem itemType="leaf" value="item1" data-testid="item1">
+                <TreeItemLayout>level 1, item 1</TreeItemLayout>
+              </TreeItem>
+              {show && (
                 <TreeItem itemType="leaf" value="item2" data-testid="item2">
                   <TreeItemLayout>level 1, item 2</TreeItemLayout>
                 </TreeItem>
-              </>
-            )}
-            <TreeItem itemType="leaf" value="item3" data-testid="item3">
-              <TreeItemLayout>level 1, item 3</TreeItemLayout>
-            </TreeItem>
-            <TreeItem itemType="leaf" value="item4" data-testid="item4">
-              <TreeItemLayout>level 1, item 4</TreeItemLayout>
-            </TreeItem>
-          </TreeTest>
-        </>
-      );
-    };
-
-    mount(<RovingTreeTest />);
-    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');
+              )}
+              <TreeItem itemType="leaf" value="item3" data-testid="item3">
+                <TreeItemLayout>level 1, item 3</TreeItemLayout>
+              </TreeItem>
+              <TreeItem itemType="leaf" value="item4" data-testid="item4">
+                <TreeItemLayout>level 1, item 4</TreeItemLayout>
+              </TreeItem>
+            </Tree>
+          </>
+        );
+      };
+      mount(<RovingTreeTest />);
+      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
@@ -60,7 +60,10 @@ function useNestedRootTree(props: TreeProps, ref: React.Ref<HTMLElement>): 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
@@ -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
@@ -38,6 +38,7 @@ export function useTreeItem_unstable(props: TreeItemProps, ref: React.Ref<HTMLDi
     warnIfNoProperPropsFlatTreeItem(props);
   }
   const requestTreeResponse = useTreeContext_unstable(ctx => 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<HTMLDi
       if (treeItemRef.current?.querySelector(`.${treeClassNames.root}`)) {
         // eslint-disable-next-line no-console
         console.error(/** #__DE-INDENT__ */ `
-      @fluentui/react-tree [useTreeItem]:
-      <TreeItem> should be declared inside a <Tree> component.
-    `);
+          @fluentui/react-tree [useTreeItem]:
+          <TreeItem> should be declared inside a <Tree> 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
@@ -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
@@ -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<HTMLElement> = React.useCallback(
     root => {
@@ -87,7 +87,11 @@ export function useFlatTreeNavigation() {
       rove(nextElement);
     }
   });
-  return { navigate, rootRef: useMergedRefs<HTMLDivElement>(walkerRootRef, rootRefCallback) } as const;
+  return {
+    navigate,
+    rootRef: useMergedRefs<HTMLDivElement>(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
@@ -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
@@ -13,15 +13,6 @@ export function useRovingTabIndex() {
   const walkerRef = React.useRef<HTMLElementWalker | null>(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,
   };
 }