downshift icon indicating copy to clipboard operation
downshift copied to clipboard

[useMultipleSelection] Backspace/Delete on selected items does not progress focus on tags correctly

Open juzerzarif opened this issue 1 year ago • 2 comments

  • downshift version: 9.0.6
  • node version: 18.20.3
  • npm (or yarn) version: 10.2.3

Reproduction repository:

Example: This is basically just the multiple selection with useSelect example from the docs (sans styling - sorry!) https://stackblitz.com/edit/vitejs-vite-wimxfe?file=src%2FSelect.jsx

Problem description:

When deleting selected item tags using Backspace/Delete, if the selected item you're deleting is not the last item in the selectedItems array the focus does not move to the next selected item after as expected. This happens because focus is only moved to a selectedItem when the activeIndex changes here https://github.com/downshift-js/downshift/blob/ee2a828ac70035c1e6156523b72c11abae4c07e4/src/hooks/useMultipleSelection/index.js#L62-L74 But when you delete an item that isn't the last selected item the activeIndex stays the same: https://github.com/downshift-js/downshift/blob/ee2a828ac70035c1e6156523b72c11abae4c07e4/src/hooks/useMultipleSelection/reducer.js#L30-L52

The reason it works in the example in the docs is because that example derives the key for the selected item tags using the index of the item in the selectedItems array so the element that the Delete/Backspace event was dispatched on isn't removed from the DOM (but just re-renders with the next selectedItem's data) and so it continues to keep focus. If you use say the item ID for the React component key though, then the focus does not move correctly to the next element because the aforementioned effect never fires.

Suggested solution:

I think if we set some sort of internal boolean state to indicate that we need to set focus on the next render when a Delete/Backspace event happens where the activeIndex is gonna stay the same, that should resolve this. Though obviously that will not work if the consumer has not updated the selectedItems list by that next render - I don't know if that's actually something this library is (or even should be) concerned with. I'd be willing to submit a PR for this if I can get some folks' opinions on the suggested fix.

juzerzarif avatar Jul 14 '24 21:07 juzerzarif

Just adding selectedItems.length to the useEffect deps should fix this no? I've implemented done this in a useEffect in the consumer to fix this bug.

baseten avatar Nov 15 '24 16:11 baseten

I've just bumped into the issue in our application. IMHO it's a bug and the deps array of the "Sets focus on active item" hook in useMultipleSelection should be updated to include selectedItems.length to address the case when a selected item is removed but the activeIndex state remains unchanged. The effect will fire more often but I don't think it's a big deal.

Steps to reproduce

  1. Update the source code of the example at https://www.downshift-js.com/use-multiple-selection/ by replacing key={`selected-item-${index}`} with key={selectedItemForRender.id} at the location where the selectedItems array is being rendered selectedItems.map(...)
  2. Highlight the first selected book in the running example and hit Delete or Backspace

Expected result The second selected book should receive the focus.

Actual result The second selected book doesn't receive the focus and as a result the entire component loses the focus (a browser moves the focus to the document's body).

sergey-be avatar Jun 20 '25 16:06 sergey-be