fix(hooks): correctly compute item and index in getter function (#1473)

Author: silviuaavram

src/hooks/__tests__/utils.test.js

@@ -1,10 +1,10 @@
 import {renderHook} from '@testing-library/react-hooks'
 import {
-  getItemIndex,
   defaultProps,
   getInitialValue,
   getDefaultValue,
   useMouseAndTouchTracker,
+  getItemAndIndex,
 } from '../utils'
 
 describe('utils', () => {
@@ -15,21 +15,35 @@ describe('utils', () => {
     })
   })
 
-  describe('getItemIndex', () => {
-    test('returns -1 if no items', () => {
-      const index = getItemIndex(undefined, {}, [])
-      expect(index).toBe(-1)
+  describe('getItemAndIndex', () => {
+    test('returns arguments if passed as defined', () => {
+      expect(getItemAndIndex({}, 5, [])).toEqual([{}, 5])
     })
 
-    test('returns index if passed', () => {
-      const index = getItemIndex(5, {}, [])
-      expect(index).toBe(5)
+    test('throws an error when item and index are not passed', () => {
+      const errorMessage = 'Pass either item or index to the item getter prop!'
+
+      expect(() =>
+        getItemAndIndex(undefined, undefined, [1, 2, 3], errorMessage),
+      ).toThrowError(errorMessage)
+    })
+
+    test('returns index if item is passed', () => {
+      const item = {}
+
+      expect(getItemAndIndex(item, undefined, [{x: 1}, item, {x: 2}])).toEqual([
+        item,
+        1,
+      ])
     })
 
-    test('returns index of item', () => {
+    test('returns item if index is passed', () => {
+      const index = 2
       const item = {x: 2}
-      const index = getItemIndex(undefined, item, [{x: 1}, item, {x: 2}])
-      expect(index).toBe(1)
+      expect(getItemAndIndex(undefined, 2, [{x: 1}, {x: 3}, item])).toEqual([
+        item,
+        index,
+      ])
     })
   })
 

src/hooks/useCombobox/__tests__/getItemProps.test.js

@@ -17,7 +17,7 @@ describe('getItemProps', () => {
     const {result} = renderUseCombobox()
 
     expect(result.current.getItemProps).toThrowError(
-      'Pass either item or item index in getItemProps!',
+      'Pass either item or index to getItemProps!',
     )
   })
 

src/hooks/useCombobox/index.js

@@ -3,14 +3,14 @@ import {useRef, useEffect, useCallback, useMemo} from 'react'
 import {isPreact, isReactNative} from '../../is.macro'
 import {handleRefs, normalizeArrowKey, callAllEventHandlers} from '../../utils'
 import {
-  getItemIndex,
   useA11yMessageSetter,
   useMouseAndTouchTracker,
   useGetterPropsCalledChecker,
   useLatestRef,
   useScrollIntoView,
   useControlPropsValidator,
   useElementIds,
+  getItemAndIndex,
 } from '../utils'
 import {
   getInitialState,
@@ -284,8 +284,8 @@ function useCombobox(userProps = {}) {
 
   const getItemProps = useCallback(
     ({
-      item,
-      index,
+      item: itemProp,
+      index: indexProp,
       refKey = 'ref',
       ref,
       onMouseMove,
@@ -296,10 +296,12 @@ function useCombobox(userProps = {}) {
       ...rest
     } = {}) => {
       const {props: latestProps, state: latestState} = latest.current
-      const itemIndex = getItemIndex(index, item, latestProps.items)
-      if (itemIndex < 0) {
-        throw new Error('Pass either item or item index in getItemProps!')
-      }
+      const [, index] = getItemAndIndex(
+        itemProp,
+        indexProp,
+        latestProps.items,
+        'Pass either item or index to getItemProps!',
+      )
 
       const onSelectKey = isReactNative
         ? /* istanbul ignore next (react-native) */ 'onPress'
@@ -330,13 +332,13 @@ function useCombobox(userProps = {}) {
       return {
         [refKey]: handleRefs(ref, itemNode => {
           if (itemNode) {
-            itemRefs.current[elementIds.getItemId(itemIndex)] = itemNode
+            itemRefs.current[elementIds.getItemId(index)] = itemNode
           }
         }),
         disabled,
         role: 'option',
-        'aria-selected': `${itemIndex === latestState.highlightedIndex}`,
-        id: elementIds.getItemId(itemIndex),
+        'aria-selected': `${index === latestState.highlightedIndex}`,
+        id: elementIds.getItemId(index),
         ...(!disabled && {
           [onSelectKey]: callAllEventHandlers(
             customClickHandler,

src/hooks/useMultipleSelection/__tests__/getSelectedItemProps.test.js

@@ -18,7 +18,7 @@ describe('getSelectedItemProps', () => {
     const {result} = renderUseMultipleSelection()
 
     expect(result.current.getSelectedItemProps).toThrowError(
-      'Pass either selectedItem or index in getSelectedItemProps!',
+      'Pass either item or index to getSelectedItemProps!',
     )
   })
 

src/hooks/useMultipleSelection/index.js

@@ -4,10 +4,10 @@ import setStatus from '../../set-a11y-status'
 import {handleRefs, callAllEventHandlers, normalizeArrowKey} from '../../utils'
 import {
   useControlledReducer,
-  getItemIndex,
   useGetterPropsCalledChecker,
   useLatestRef,
   useControlPropsValidator,
+  getItemAndIndex,
 } from '../utils'
 import {
   getInitialState,
@@ -159,21 +159,18 @@ function useMultipleSelection(userProps = {}) {
       ref,
       onClick,
       onKeyDown,
-      selectedItem,
-      index,
+      selectedItem: selectedItemProp,
+      index: indexProp,
       ...rest
     } = {}) => {
       const {state: latestState} = latest.current
-      const itemIndex = getItemIndex(
-        index,
-        selectedItem,
+      const [, index] = getItemAndIndex(
+        selectedItemProp,
+        indexProp,
         latestState.selectedItems,
+        'Pass either item or index to getSelectedItemProps!',
       )
-      if (itemIndex < 0) {
-        throw new Error(
-          'Pass either selectedItem or index in getSelectedItemProps!',
-        )
-      }
+      const isFocusable = index > -1 && index === latestState.activeIndex
 
       const selectedItemHandleClick = () => {
         dispatch({
@@ -194,7 +191,7 @@ function useMultipleSelection(userProps = {}) {
             selectedItemRefs.current.push(selectedItemNode)
           }
         }),
-        tabIndex: index === latestState.activeIndex ? 0 : -1,
+        tabIndex: isFocusable ? 0 : -1,
         onClick: callAllEventHandlers(onClick, selectedItemHandleClick),
         onKeyDown: callAllEventHandlers(onKeyDown, selectedItemHandleKeyDown),
         ...rest,

src/hooks/useSelect/__tests__/getItemProps.test.js

@@ -17,7 +17,7 @@ describe('getItemProps', () => {
     const {result} = renderUseSelect()
 
     expect(result.current.getItemProps).toThrowError(
-      'Pass either item or item index in getItemProps!',
+      'Pass either item or index to getItemProps!',
     )
   })
 

src/hooks/useSelect/index.js

@@ -1,6 +1,6 @@
 import {useRef, useEffect, useCallback, useMemo} from 'react'
 import {
-  getItemIndex,
+  getItemAndIndex,
   isAcceptedCharacterKey,
   useControlledReducer,
   getInitialState,
@@ -456,8 +456,12 @@ function useSelect(userProps = {}) {
       ...rest
     } = {}) => {
       const {state: latestState, props: latestProps} = latest.current
-      const item = itemProp ?? items[indexProp]
-      const index = getItemIndex(indexProp, item, latestProps.items)
+      const [item, index] = getItemAndIndex(
+        itemProp,
+        indexProp,
+        latestProps.items,
+        'Pass either item or index to getItemProps!',
+      )
 
       const itemHandleMouseMove = () => {
         if (index === latestState.highlightedIndex) {
@@ -477,18 +481,14 @@ function useSelect(userProps = {}) {
         })
       }
 
-      const itemIndex = getItemIndex(index, item, latestProps.items)
-      if (itemIndex < 0) {
-        throw new Error('Pass either item or item index in getItemProps!')
-      }
       const itemProps = {
         disabled,
         role: 'option',
         'aria-selected': `${item === selectedItem}`,
-        id: elementIds.getItemId(itemIndex),
+        id: elementIds.getItemId(index),
         [refKey]: handleRefs(ref, itemNode => {
           if (itemNode) {
-            itemRefs.current[elementIds.getItemId(itemIndex)] = itemNode
+            itemRefs.current[elementIds.getItemId(index)] = itemNode
           }
         }),
         ...rest,
@@ -510,7 +510,7 @@ function useSelect(userProps = {}) {
 
       return itemProps
     },
-    [latest, items, selectedItem, elementIds, shouldScrollRef, dispatch],
+    [latest, selectedItem, elementIds, shouldScrollRef, dispatch],
   )
 
   return {

src/hooks/utils.js

@@ -112,14 +112,22 @@ function useElementIds({
   return elementIdsRef.current
 }
 
-function getItemIndex(index, item, items) {
-  if (index !== undefined) {
-    return index
-  }
-  if (items.length === 0) {
-    return -1
+function getItemAndIndex(itemProp, indexProp, items, errorMessage) {
+  let item, index
+
+  if (itemProp === undefined) {
+    if (indexProp === undefined) {
+      throw new Error(errorMessage)
+    }
+
+    item = items[indexProp]
+    index = indexProp
+  } else {
+    index = indexProp === undefined ? items.indexOf(itemProp) : indexProp
+    item = itemProp
   }
-  return items.indexOf(item)
+
+  return [item, index]
 }
 
 function itemToString(item) {
@@ -532,6 +540,6 @@ export {
   useLatestRef,
   capitalizeString,
   isAcceptedCharacterKey,
-  getItemIndex,
+  getItemAndIndex,
   useElementIds,
 }