fix(useListNavigation, Composite): correct index calculations for gri…

…d navigation with nullish and disabled items (#2912)
floating-ui · May 19, 2024 · d7f76f6 · d7f76f6
1 parent bac4282
commit d7f76f6
Show file tree

Hide file tree

Showing 6 changed files with 128 additions and 28 deletions.
diff --git a/.changeset/strong-frogs-ring.md b/.changeset/strong-frogs-ring.md
@@ -0,0 +1,5 @@
+---
+'@floating-ui/react': patch
+---
+
+fix(useListNavigation, Composite): correct index calculations for grid navigation with nullish and disabled items when `disabledIndices` is inferred
diff --git a/packages/react/src/components/Composite.tsx b/packages/react/src/components/Composite.tsx
@@ -15,6 +15,7 @@ import {
   getGridNavigatedIndex,
   getMaxIndex,
   getMinIndex,
+  isDisabled,
   isIndexOutOfBounds,
 } from '../utils/composite';
 import {enqueueFocus} from '../utils/enqueueFocus';
@@ -118,7 +119,7 @@ export const Composite = React.forwardRef<
     orientation = 'both',
     loop = true,
     cols = 1,
-    disabledIndices = [],
+    disabledIndices,
     activeIndex: externalActiveIndex,
     onNavigate: externalSetActiveIndex,
     itemSizes,
@@ -145,6 +146,8 @@ export const Composite = React.forwardRef<
     if (!allKeys.includes(event.key)) return;
 
     let nextIndex = activeIndex;
+    const minIndex = getMinIndex(elementsRef, disabledIndices);
+    const maxIndex = getMaxIndex(elementsRef, disabledIndices);
 
     if (isGrid) {
       const sizes =
@@ -157,12 +160,15 @@ export const Composite = React.forwardRef<
       // as if every item was 1x1, then convert back to real indices.
       const cellMap = buildCellMap(sizes, cols, dense);
       const minGridIndex = cellMap.findIndex(
-        (index) => index != null && !disabledIndices.includes(index),
+        (index) =>
+          index != null &&
+          !isDisabled(elementsRef.current, index, disabledIndices),
       );
       // last enabled index
       const maxGridIndex = cellMap.reduce(
         (foundIndex: number, index, cellIndex) =>
-          index != null && !disabledIndices?.includes(index)
+          index != null &&
+          !isDisabled(elementsRef.current, index, disabledIndices)
             ? cellIndex
             : foundIndex,
         -1,
@@ -183,13 +189,19 @@ export const Composite = React.forwardRef<
             // treat undefined (empty grid spaces) as disabled indices so we
             // don't end up in them
             disabledIndices: getCellIndices(
-              [...disabledIndices, undefined],
+              [
+                ...(disabledIndices ||
+                  elementsRef.current.map((_, index) =>
+                    isDisabled(elementsRef.current, index) ? index : undefined,
+                  )),
+                undefined,
+              ],
               cellMap,
             ),
             minIndex: minGridIndex,
             maxIndex: maxGridIndex,
             prevIndex: getCellIndexOfCorner(
-              activeIndex,
+              activeIndex > maxIndex ? minIndex : activeIndex,
               sizes,
               cellMap,
               cols,
@@ -207,9 +219,6 @@ export const Composite = React.forwardRef<
       ] as number; // navigated cell will never be nullish
     }
 
-    const minIndex = getMinIndex(elementsRef, disabledIndices);
-    const maxIndex = getMaxIndex(elementsRef, disabledIndices);
-
     const toEndKeys = {
       horizontal: [ARROW_RIGHT],
       vertical: [ARROW_DOWN],

diff --git a/packages/react/src/hooks/useListNavigation.ts b/packages/react/src/hooks/useListNavigation.ts
@@ -30,6 +30,7 @@ import {
   getGridNavigatedIndex,
   getMaxIndex,
   getMinIndex,
+  isDisabled,
   isIndexOutOfBounds,
 } from '../utils/composite';
 import {enqueueFocus} from '../utils/enqueueFocus';
@@ -635,17 +636,19 @@ export function useListNavigation(
         // as if every item was 1x1, then convert back to real indices.
         const cellMap = buildCellMap(sizes, cols, dense);
         const minGridIndex = cellMap.findIndex(
-          (index) => index != null && !disabledIndices?.includes(index),
+          (index) =>
+            index != null &&
+            !isDisabled(listRef.current, index, disabledIndices),
         );
         // last enabled index
         const maxGridIndex = cellMap.reduce(
           (foundIndex: number, index, cellIndex) =>
-            index != null && !disabledIndices?.includes(index)
+            index != null &&
+            !isDisabled(listRef.current, index, disabledIndices)
               ? cellIndex
               : foundIndex,
           -1,
         );
-
         indexRef.current = cellMap[
           getGridNavigatedIndex(
             {
@@ -661,13 +664,19 @@ export function useListNavigation(
               // treat undefined (empty grid spaces) as disabled indices so we
               // don't end up in them
               disabledIndices: getCellIndices(
-                [...(disabledIndices || []), undefined],
+                [
+                  ...(disabledIndices ||
+                    listRef.current.map((_, index) =>
+                      isDisabled(listRef.current, index) ? index : undefined,
+                    )),
+                  undefined,
+                ],
                 cellMap,
               ),
               minIndex: minGridIndex,
               maxIndex: maxGridIndex,
               prevIndex: getCellIndexOfCorner(
-                indexRef.current,
+                indexRef.current > maxIndex ? minIndex : indexRef.current,
                 sizes,
                 cellMap,
                 cols,

diff --git a/packages/react/src/utils/composite.ts b/packages/react/src/utils/composite.ts
@@ -53,21 +53,14 @@ export function findNonDisabledIndex(
 ): number {
   const list = listRef.current;
 
-  const isDisabledIndex = disabledIndices
-    ? (index: number) => disabledIndices.includes(index)
-    : (index: number) => {
-        const element = list[index];
-        return (
-          element == null ||
-          element.hasAttribute('disabled') ||
-          element.getAttribute('aria-disabled') === 'true'
-        );
-      };
-
   let index = startingIndex;
   do {
     index += decrement ? -amount : amount;
-  } while (index >= 0 && index <= list.length - 1 && isDisabledIndex(index));
+  } while (
+    index >= 0 &&
+    index <= list.length - 1 &&
+    isDisabled(list, index, disabledIndices)
+  );
 
   return index;
 }
@@ -192,8 +185,8 @@ export function getGridNavigatedIndex(
       if (prevIndex % cols !== 0) {
         nextIndex = findNonDisabledIndex(elementsRef, {
           startingIndex: prevIndex,
-          disabledIndices,
           decrement: true,
+          disabledIndices,
         });
 
         if (loop && isDifferentRow(nextIndex, cols, prevRow)) {
@@ -292,14 +285,21 @@ export function getCellIndexOfCorner(
   if (index === -1) return -1;
 
   const firstCellIndex = cellMap.indexOf(index);
+  const sizeItem = sizes[index];
 
   switch (corner) {
     case 'tl':
       return firstCellIndex;
     case 'tr':
-      return firstCellIndex + sizes[index].width - 1;
+      if (!sizeItem) {
+        return firstCellIndex;
+      }
+      return firstCellIndex + sizeItem.width - 1;
     case 'bl':
-      return firstCellIndex + (sizes[index].height - 1) * cols;
+      if (!sizeItem) {
+        return firstCellIndex;
+      }
+      return firstCellIndex + (sizeItem.height - 1) * cols;
     case 'br':
       return cellMap.lastIndexOf(index);
   }
@@ -314,3 +314,20 @@ export function getCellIndices(
     indices.includes(index) ? [cellIndex] : [],
   );
 }
+
+export function isDisabled(
+  list: Array<HTMLElement | null>,
+  index: number,
+  disabledIndices?: Array<number>,
+) {
+  if (disabledIndices) {
+    return disabledIndices.includes(index);
+  }
+
+  const element = list[index];
+  return (
+    element == null ||
+    element.hasAttribute('disabled') ||
+    element.getAttribute('aria-disabled') === 'true'
+  );
+}
diff --git a/packages/react/test/unit/useListNavigation.test.tsx b/packages/react/test/unit/useListNavigation.test.tsx
@@ -16,6 +16,7 @@ import {
 import type {UseListNavigationProps} from '../../src/hooks/useListNavigation';
 import {Main as ComplexGrid} from '../visual/components/ComplexGrid';
 import {Main as Grid} from '../visual/components/Grid';
+import {Main as EmojiPicker} from '../visual/components/EmojiPicker';
 
 function App(props: Omit<Partial<UseListNavigationProps>, 'listRef'>) {
   const [open, setOpen] = useState(false);
@@ -1040,3 +1041,59 @@ test('async selectedIndex', async () => {
   await userEvent.keyboard('{ArrowDown}');
   expect(screen.getAllByRole('option')[3]).toHaveFocus();
 });
+
+test('grid navigation with changing list items', async () => {
+  render(<EmojiPicker />);
+
+  fireEvent.click(screen.getByRole('button'));
+
+  await act(async () => {});
+
+  expect(screen.getByRole('textbox')).toHaveFocus();
+
+  await userEvent.keyboard('appl');
+  await userEvent.keyboard('{ArrowDown}');
+
+  expect(screen.getByLabelText('apple')).toHaveAttribute('data-active');
+
+  await userEvent.keyboard('{ArrowDown}');
+
+  expect(screen.getByLabelText('apple')).toHaveAttribute('data-active');
+});
+
+test('grid navigation with disabled list items', async () => {
+  const {unmount} = render(<EmojiPicker />);
+
+  fireEvent.click(screen.getByRole('button'));
+
+  await act(async () => {});
+
+  expect(screen.getByRole('textbox')).toHaveFocus();
+
+  await userEvent.keyboard('o');
+  await userEvent.keyboard('{ArrowDown}');
+
+  expect(screen.getByLabelText('orange')).not.toHaveAttribute('data-active');
+  expect(screen.getByLabelText('watermelon')).toHaveAttribute('data-active');
+
+  await userEvent.keyboard('{ArrowDown}');
+
+  expect(screen.getByLabelText('watermelon')).toHaveAttribute('data-active');
+
+  unmount();
+
+  render(<EmojiPicker />);
+
+  fireEvent.click(screen.getByRole('button'));
+
+  await act(async () => {});
+
+  expect(screen.getByRole('textbox')).toHaveFocus();
+
+  await userEvent.keyboard('{ArrowDown}');
+  await userEvent.keyboard('{ArrowDown}');
+  await userEvent.keyboard('{ArrowRight}');
+  await userEvent.keyboard('{ArrowUp}');
+
+  expect(screen.getByLabelText('cherry')).toHaveAttribute('data-active');
+});
diff --git a/packages/react/test/visual/components/EmojiPicker.tsx b/packages/react/test/visual/components/EmojiPicker.tsx
@@ -82,11 +82,14 @@ const Option = forwardRef<HTMLButtonElement, OptionProps>(function Option(
         {
           'bg-cyan-100': selected && !active,
           'bg-cyan-200': active,
+          'opacity-40': name === 'orange',
         },
       )}
       aria-selected={selected}
+      disabled={name === 'orange'}
       aria-label={name}
       tabIndex={-1}
+      data-active={active ? '' : undefined}
     >
       {children}
     </button>