fix(hooks): prevent overwrite of itemRefs when hooks rerender

.size-snapshot.json

@@ -1,23 +1,23 @@
 {
   "downshift.cjs.js": {
-    "bundled": 146405,
-    "minified": 66980,
-    "gzipped": 14158
+    "bundled": 146623,
+    "minified": 67058,
+    "gzipped": 14165
   },
   "downshift.umd.min.js": {
-    "bundled": 155757,
-    "minified": 51452,
-    "gzipped": 13855
+    "bundled": 155989,
+    "minified": 51660,
+    "gzipped": 13966
   },
   "downshift.umd.js": {
-    "bundled": 200156,
-    "minified": 69500,
-    "gzipped": 18257
+    "bundled": 202673,
+    "minified": 70071,
+    "gzipped": 18485
   },
   "downshift.esm.js": {
-    "bundled": 141675,
-    "minified": 62938,
-    "gzipped": 13923,
+    "bundled": 141879,
+    "minified": 63002,
+    "gzipped": 13930,
     "treeshaked": {
       "rollup": {
         "code": 2275,

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

@@ -1,4 +1,5 @@
-import {renderUseCombobox} from '../testUtils'
+import {renderUseCombobox, renderMemoizedCombobox} from '../testUtils'
+import {items, defaultIds} from '../../testUtils'
 
 test('functions are memoized', () => {
   const {result, rerender} = renderUseCombobox()
@@ -7,3 +8,18 @@ test('functions are memoized', () => {
   const secondRenderResult = result.current
   expect(firstRenderResult).toEqual(secondRenderResult)
 })
+
+test('will skip disabled items after component rerenders and items are memoized', () => {
+  const {keyDownOnInput, input, rerender} = renderMemoizedCombobox({
+    isOpen: true,
+    initialHighlightedIndex: items.length - 1,
+  })
+
+  rerender();
+  keyDownOnInput('ArrowUp')
+
+  expect(input).toHaveAttribute(
+    'aria-activedescendant',
+    defaultIds.getItemId(items.length - 3),
+  )
+})

src/hooks/useCombobox/index.js

@@ -51,11 +51,10 @@ function useCombobox(userProps = {}) {
 
   // Element refs.
   const menuRef = useRef(null)
-  const itemRefs = useRef()
+  const itemRefs = useRef({})
   const inputRef = useRef(null)
   const toggleButtonRef = useRef(null)
   const comboboxRef = useRef(null)
-  itemRefs.current = {}
   const isInitialMountRef = useRef(true)
   // prevent id re-generation between renders.
   const elementIds = useElementIds(props)
@@ -143,6 +142,12 @@ function useCombobox(userProps = {}) {
   useEffect(() => {
     isInitialMountRef.current = false
   }, [])
+  // Reset itemRefs on close.
+  useEffect(() => {
+    if (!isOpen) {
+      itemRefs.current = {}
+    }
+  }, [isOpen])
 
   /* Event handler functions */
   const inputKeyDownHandlers = useMemo(

src/hooks/useCombobox/testUtils.js

@@ -144,8 +144,101 @@ const DropdownCombobox = ({renderSpy, ...props}) => {
   )
 }
 
+const renderMemoizedCombobox = props => {
+  const renderSpy = jest.fn()
+  const ui = <MemoizedDropdownCombobox renderSpy={renderSpy} {...props} />
+  const wrapper = render(ui)
+  const rerender = newProps =>
+    wrapper.rerender(<MemoizedDropdownCombobox renderSpy={renderSpy} {...newProps} />)
+  const input = screen.getByTestId(dataTestIds.input)
+  const keyDownOnInput = (key, options = {}) => {
+    fireEvent.keyDown(input, {key, ...options})
+  }
+
+  return {
+    ...wrapper,
+    renderSpy,
+    rerender,
+    input,
+    keyDownOnInput,
+  }
+}
+
+// Eslint incorrectly marks this as an error.
+// PR that should've fixed this: https://github.com/yannickcr/eslint-plugin-react/pull/2109
+// eslint-disable-next-line react/display-name
+const DropdownItem = React.memo(({
+  item,
+  index,
+  isDisabled,
+  isHighlighted,
+  getItemProps,
+  stringItem,
+  ...props
+}) => {
+  return (
+    <li
+      style={isHighlighted ? {backgroundColor: 'blue'} : {}}
+      {...getItemProps({
+        item,
+        index,
+        disabled: isDisabled,
+      })}
+      {...props}
+    >
+      {stringItem}
+    </li>
+  )
+})
+
+const MemoizedDropdownCombobox = ({renderSpy, ...props}) => {
+  const {
+    isOpen,
+    getToggleButtonProps,
+    getLabelProps,
+    getMenuProps,
+    getInputProps,
+    getComboboxProps,
+    highlightedIndex,
+    getItemProps,
+  } = useCombobox({items, ...props})
+  renderSpy()
+
+  return (
+    <div>
+      <label {...getLabelProps()}>Choose an element:</label>
+      <div data-testid={dataTestIds.combobox} {...getComboboxProps()}>
+        <input data-testid={dataTestIds.input} {...getInputProps()} />
+        <button
+          data-testid={dataTestIds.toggleButton}
+          {...getToggleButtonProps()}
+        >
+          Toggle
+        </button>
+      </div>
+      <ul data-testid={dataTestIds.menu} {...getMenuProps()}>
+        {isOpen &&
+          (props.items || items).map((item, index) => {
+            return (
+              <DropdownItem
+                data-testid={dataTestIds.item(index)}
+                getItemProps={getItemProps}
+                index={index}
+                isDisabled={items.length - 2 === index}
+                isHighlighted={highlightedIndex === index}
+                item={item}
+                key={`${item}${index}`}
+                stringItem={item}
+              />
+            )
+          })}
+      </ul>
+    </div>
+  )
+}
+
 const renderUseCombobox = props => {
   return renderHook(() => useCombobox({items, ...props}))
 }
 
-export {renderUseCombobox, dataTestIds, renderCombobox}
+export {renderUseCombobox, dataTestIds, renderCombobox, renderMemoizedCombobox}

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

@@ -1,4 +1,5 @@
-import {renderUseSelect} from '../testUtils'
+import {renderUseSelect, renderMemoizedSelect} from '../testUtils'
+import {items, defaultIds} from '../../testUtils'
 
 test('functions are memoized', () => {
   const {result, rerender} = renderUseSelect()
@@ -7,3 +8,19 @@ test('functions are memoized', () => {
   const secondRenderResult = result.current
   expect(firstRenderResult).toEqual(secondRenderResult)
 })
+
+
+test('will skip disabled items after component rerenders and items are memoized', () => {
+  const {keyDownOnMenu, menu, rerender} = renderMemoizedSelect({
+    isOpen: true,
+    initialHighlightedIndex: items.length - 1,
+  })
+
+  rerender();
+  keyDownOnMenu('ArrowUp')
+
+  expect(menu).toHaveAttribute(
+    'aria-activedescendant',
+    defaultIds.getItemId(items.length - 3),
+  )
+})

src/hooks/useSelect/index.js

@@ -54,8 +54,7 @@ function useSelect(userProps = {}) {
   // Element efs.
   const toggleButtonRef = useRef(null)
   const menuRef = useRef(null)
-  const itemRefs = useRef()
-  itemRefs.current = {}
+  const itemRefs = useRef({})
   // used not to trigger menu blur action in some scenarios.
   const shouldBlurRef = useRef(true)
   // used to keep the inputValue clearTimeout object between renders.
@@ -185,6 +184,12 @@ function useSelect(userProps = {}) {
   useEffect(() => {
     isInitialMountRef.current = false
   }, [])
+  // Reset itemRefs on close.
+  useEffect(() => {
+    if (!isOpen) {
+      itemRefs.current = {}
+    }
+  }, [isOpen])
 
   // Event handler functions.
   const toggleButtonKeyDownHandlers = useMemo(

src/hooks/useSelect/testUtils.js

@@ -133,4 +133,78 @@ const DropdownSelect = props => {
   )
 }
 
-export {items, renderUseSelect, renderSelect, DropdownSelect}
+const renderMemoizedSelect = props => {
+  const ui = <MemoizedDropdownSelect {...props} />
+  const wrapper = render(ui)
+  const rerender = p => wrapper.rerender(<MemoizedDropdownSelect {...p} />)
+  const menu = screen.getByRole('listbox')
+  const keyDownOnMenu = (key, options = {}) => {
+    fireEvent.keyDown(menu, {key, ...options})
+  }
+
+  return {
+    ...wrapper,
+    rerender,
+    menu,
+    keyDownOnMenu,
+  }
+}
+
+// Eslint incorrectly marks this as an error.
+// PR that should've fixed this: https://github.com/yannickcr/eslint-plugin-react/pull/2109
+// eslint-disable-next-line react/display-name
+const DropdownItem = React.memo(({
+  item,
+  index,
+  isDisabled,
+  isHighlighted,
+  getItemProps,
+  stringItem,
+  ...props
+}) => {
+  return (
+    <li
+      style={isHighlighted ? {backgroundColor: 'blue'} : {}}
+      {...getItemProps({
+        item,
+        index,
+        disabled: isDisabled,
+      })}
+      {...props}
+    >
+      {stringItem}
+    </li>
+  )
+})
+
+const MemoizedDropdownSelect = props => {
+  const {
+    isOpen,
+    getMenuProps,
+    highlightedIndex,
+    getItemProps,
+  } = useSelect({items, ...props})
+  return (
+    <div>
+      <ul data-testid={dataTestIds.menu} {...getMenuProps()}>
+        {isOpen &&
+          (props.items || items).map((item, index) => {
+            return (
+              <DropdownItem
+                data-testid={dataTestIds.item(index)}
+                getItemProps={getItemProps}
+                index={index}
+                isDisabled={items.length - 2 === index}
+                isHighlighted={highlightedIndex === index}
+                item={item}
+                key={`${item}${index}`}
+                stringItem={item}
+            />
+            )
+          })}
+      </ul>
+    </div>
+  )
+}
+
+export {items, renderUseSelect, renderSelect, DropdownSelect, renderMemoizedSelect}