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

downshift-js · Jan 17, 2021 · a868332 · a868332
1 parent 0d87ecb
commit a868332
Show file tree

Hide file tree

Showing 8 changed files with 115 additions and 35 deletions.
diff --git a/.size-snapshot.json b/.size-snapshot.json
@@ -1,23 +1,23 @@
 {
   "downshift.cjs.js": {
-    "bundled": 148071,
-    "minified": 68295,
-    "gzipped": 14405
+    "bundled": 148289,
+    "minified": 68373,
+    "gzipped": 14410
   },
   "downshift.umd.min.js": {
-    "bundled": 155837,
-    "minified": 51545,
-    "gzipped": 14048
+    "bundled": 156069,
+    "minified": 51613,
+    "gzipped": 14058
   },
   "downshift.umd.js": {
-    "bundled": 200449,
-    "minified": 69844,
-    "gzipped": 18274
+    "bundled": 200681,
+    "minified": 69912,
+    "gzipped": 18283
   },
   "downshift.esm.js": {
-    "bundled": 143341,
-    "minified": 64253,
-    "gzipped": 14163,
+    "bundled": 143545,
+    "minified": 64317,
+    "gzipped": 14173,
     "treeshaked": {
       "rollup": {
         "code": 2275,

diff --git a/src/hooks/testUtils.js b/src/hooks/testUtils.js
@@ -1,6 +1,7 @@
+import React from 'react'
 import {act} from '@testing-library/react'
 
-export const items = [
+const items = [
   'Neptunium',
   'Plutonium',
   'Americium',
@@ -29,14 +30,35 @@ export const items = [
   'Oganesson',
 ]
 
-export const defaultIds = {
+const defaultIds = {
   labelId: 'downshift-test-id-label',
   menuId: 'downshift-test-id-menu',
   getItemId: index => `downshift-test-id-item-${index}`,
   toggleButtonId: 'downshift-test-id-toggle-button',
   inputId: 'downshift-test-id-input',
 }
 
-export const waitForDebouncedA11yStatusUpdate = () => {
+const waitForDebouncedA11yStatusUpdate = () => {
   act(() => jest.advanceTimersByTime(200))
 }
+
+const MemoizedItem = React.memo(function Item({
+  index,
+  item,
+  getItemProps,
+  dataTestIds,
+  stringItem,
+  ...rest
+}) {
+  return (
+    <li
+      data-testid={dataTestIds.item(index)}
+      key={`${stringItem}${index}`}
+      {...getItemProps({item, index, ...rest})}
+    >
+      {stringItem}
+    </li>
+  )
+})
+
+export {items, defaultIds, waitForDebouncedA11yStatusUpdate, MemoizedItem}
diff --git a/src/hooks/useCombobox/__tests__/memo.test.js b/src/hooks/useCombobox/__tests__/memo.test.js
@@ -1,4 +1,6 @@
-import {renderUseCombobox} from '../testUtils'
+import React from 'react'
+import {renderUseCombobox, renderCombobox} from '../testUtils'
+import {items, defaultIds, MemoizedItem} from '../../testUtils'
 
 test('functions are memoized', () => {
   const {result, rerender} = renderUseCombobox()
@@ -7,3 +9,25 @@ test('functions are memoized', () => {
   const secondRenderResult = result.current
   expect(firstRenderResult).toEqual(secondRenderResult)
 })
+
+test('will skip disabled items after component rerenders and items are memoized', () => {
+  function renderItem(props) {
+    return (
+      <MemoizedItem disabled={props.index === items.length - 2} {...props} />
+    )
+  }
+
+  const {keyDownOnInput, input, rerender} = renderCombobox({
+    isOpen: true,
+    initialHighlightedIndex: items.length - 1,
+    renderItem,
+  })
+
+  rerender({renderItem})
+  keyDownOnInput('ArrowUp')
+
+  expect(input).toHaveAttribute(
+    'aria-activedescendant',
+    defaultIds.getItemId(items.length - 3),
+  )
+})
diff --git a/src/hooks/useCombobox/index.js b/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(

diff --git a/src/hooks/useCombobox/testUtils.js b/src/hooks/useCombobox/testUtils.js
@@ -94,15 +94,14 @@ const renderCombobox = (props, uiCallback) => {
   }
 }
 
-const DropdownCombobox = ({renderSpy, ...props}) => {
+const DropdownCombobox = ({renderSpy, renderItem, ...props}) => {
   const {
     isOpen,
     getToggleButtonProps,
     getLabelProps,
     getMenuProps,
     getInputProps,
     getComboboxProps,
-    highlightedIndex,
     getItemProps,
   } = useCombobox({items, ...props})
   const {itemToString} = props.itemToString ? props : defaultProps
@@ -126,12 +125,11 @@ const DropdownCombobox = ({renderSpy, ...props}) => {
           (props.items || items).map((item, index) => {
             const stringItem =
               item instanceof Object ? itemToString(item) : item
-            return (
+            return renderItem ? (
+              renderItem({index, item, getItemProps, dataTestIds, stringItem})
+            ) : (
               <li
                 data-testid={dataTestIds.item(index)}
-                style={
-                  highlightedIndex === index ? {backgroundColor: 'blue'} : {}
-                }
                 key={`${stringItem}${index}`}
                 {...getItemProps({item, index})}
               >

diff --git a/src/hooks/useSelect/__tests__/memo.test.js b/src/hooks/useSelect/__tests__/memo.test.js
@@ -1,4 +1,6 @@
-import {renderUseSelect} from '../testUtils'
+import React from 'react'
+import {renderUseSelect, renderSelect} from '../testUtils'
+import {items, defaultIds, MemoizedItem} from '../../testUtils'
 
 test('functions are memoized', () => {
   const {result, rerender} = renderUseSelect()
@@ -7,3 +9,25 @@ test('functions are memoized', () => {
   const secondRenderResult = result.current
   expect(firstRenderResult).toEqual(secondRenderResult)
 })
+
+test('will skip disabled items after component rerenders and items are memoized', () => {
+  function renderItem(props) {
+    return (
+      <MemoizedItem disabled={props.index === items.length - 2} {...props} />
+    )
+  }
+
+  const {keyDownOnMenu, menu, rerender} = renderSelect({
+    isOpen: true,
+    initialHighlightedIndex: items.length - 1,
+    renderItem,
+  })
+
+  rerender({renderItem})
+  keyDownOnMenu('ArrowUp')
+
+  expect(menu).toHaveAttribute(
+    'aria-activedescendant',
+    defaultIds.getItemId(items.length - 3),
+  )
+})
diff --git a/src/hooks/useSelect/index.js b/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(

diff --git a/src/hooks/useSelect/testUtils.js b/src/hooks/useSelect/testUtils.js
@@ -33,9 +33,11 @@ const renderUseSelect = props => {
 }
 
 const renderSelect = (props, uiCallback) => {
-  const ui = <DropdownSelect {...props} />
+  const renderSpy = jest.fn()
+  const ui = <DropdownSelect renderSpy={renderSpy} {...props} />
   const wrapper = render(uiCallback ? uiCallback(ui) : ui)
-  const rerender = p => wrapper.rerender(<DropdownSelect {...p} />)
+  const rerender = p =>
+    wrapper.rerender(<DropdownSelect renderSpy={renderSpy} {...p} />)
   const label = screen.getByText(/choose an element/i)
   const menu = screen.getByRole('listbox')
   const toggleButton = screen.getByTestId(dataTestIds.toggleButton)
@@ -87,18 +89,19 @@ const renderSelect = (props, uiCallback) => {
   }
 }
 
-const DropdownSelect = props => {
+const DropdownSelect = ({renderSpy, renderItem, ...props}) => {
   const {
     isOpen,
     selectedItem,
     getToggleButtonProps,
     getLabelProps,
     getMenuProps,
-    highlightedIndex,
     getItemProps,
   } = useSelect({items, ...props})
   const {itemToString} = props.itemToString ? props : defaultProps
 
+  renderSpy()
+
   return (
     <div>
       <label {...getLabelProps()}>Choose an element:</label>
@@ -115,12 +118,11 @@ const DropdownSelect = props => {
           (props.items || items).map((item, index) => {
             const stringItem =
               item instanceof Object ? itemToString(item) : item
-            return (
+            return renderItem ? (
+              renderItem({index, item, getItemProps, dataTestIds, stringItem})
+            ) : (
               <li
                 data-testid={dataTestIds.item(index)}
-                style={
-                  highlightedIndex === index ? {backgroundColor: 'blue'} : {}
-                }
                 key={`${stringItem}${index}`}
                 {...getItemProps({item, index})}
               >