Adjust active {item,option} index (#1184)

* adjust active {item,option} index We had various ordering issues, and now we properly sort all the notes which is awesome. However, there is this case where we still use the `activeOptionIndex` / `activeItemIndex` from _before_ the sort happens. Now we will ensure that this is properly adjusted when performing the sort of the items. In addition, we will also properly adjust these values when `registering` and `unregistering` items, not only when performing actions. * update changelog
tailwindlabs · Mar 2, 2022 · 8208c07 · 8208c07
1 parent 8e7478d
commit 8208c07
Show file tree

Hide file tree

Showing 7 changed files with 299 additions and 174 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Ensure that `appear` works regardless of multiple rerenders ([#1179](https://github.com/tailwindlabs/headlessui/pull/1179))
 - Reset Combobox Input when the value gets reset ([#1181](https://github.com/tailwindlabs/headlessui/pull/1181))
 - Fix double `beforeEnter` due to SSR ([#1183](https://github.com/tailwindlabs/headlessui/pull/1183))
+- Adjust active {item,option} index ([#1184](https://github.com/tailwindlabs/headlessui/pull/1184))
 
 ## [Unreleased - @headlessui/vue]
 
@@ -32,6 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Guarantee DOM sort order when performing actions ([#1168](https://github.com/tailwindlabs/headlessui/pull/1168))
 - Improve outside click support ([#1175](https://github.com/tailwindlabs/headlessui/pull/1175))
 - Reset Combobox Input when the value gets reset ([#1181](https://github.com/tailwindlabs/headlessui/pull/1181))
+- Adjust active {item,option} index ([#1184](https://github.com/tailwindlabs/headlessui/pull/1184))
 
 ## [@headlessui/react@v1.5.0] - 2022-02-17
 

diff --git a/packages/@headlessui-react/src/components/combobox/combobox.tsx b/packages/@headlessui-react/src/components/combobox/combobox.tsx
@@ -92,6 +92,35 @@ enum ActionTypes {
   UnregisterOption,
 }
 
+function adjustOrderedState(
+  state: StateDefinition,
+  adjustment: (options: StateDefinition['options']) => StateDefinition['options'] = (i) => i
+) {
+  let currentActiveOption =
+    state.activeOptionIndex !== null ? state.options[state.activeOptionIndex] : null
+
+  let sortedOptions = sortByDomNode(
+    adjustment(state.options.slice()),
+    (option) => option.dataRef.current.domRef.current
+  )
+
+  // If we inserted an option before the current active option then the active option index
+  // would be wrong. To fix this, we will re-lookup the correct index.
+  let adjustedActiveOptionIndex = currentActiveOption
+    ? sortedOptions.indexOf(currentActiveOption)
+    : null
+
+  // Reset to `null` in case the currentActiveOption was removed.
+  if (adjustedActiveOptionIndex === -1) {
+    adjustedActiveOptionIndex = null
+  }
+
+  return {
+    options: sortedOptions,
+    activeOptionIndex: adjustedActiveOptionIndex,
+  }
+}
+
 type Actions =
   | { type: ActionTypes.CloseCombobox }
   | { type: ActionTypes.OpenCombobox }
@@ -135,39 +164,29 @@ let reducers: {
       return state
     }
 
-    let options = sortByDomNode(state.options, (option) => option.dataRef.current.domRef.current)
-
+    let adjustedState = adjustOrderedState(state)
     let activeOptionIndex = calculateActiveIndex(action, {
-      resolveItems: () => options,
-      resolveActiveIndex: () => state.activeOptionIndex,
+      resolveItems: () => adjustedState.options,
+      resolveActiveIndex: () => adjustedState.activeOptionIndex,
       resolveId: (item) => item.id,
       resolveDisabled: (item) => item.dataRef.current.disabled,
     })
 
-    if (state.activeOptionIndex === activeOptionIndex) return state
     return {
       ...state,
-      options, // Sorted options
+      ...adjustedState,
       activeOptionIndex,
       activationTrigger: action.trigger ?? ActivationTrigger.Other,
     }
   },
   [ActionTypes.RegisterOption]: (state, action) => {
-    let currentActiveOption =
-      state.activeOptionIndex !== null ? state.options[state.activeOptionIndex] : null
+    let adjustedState = adjustOrderedState(state, (options) => {
+      return [...options, { id: action.id, dataRef: action.dataRef }]
+    })
 
-    let options = [...state.options, { id: action.id, dataRef: action.dataRef }]
     let nextState = {
       ...state,
-      options,
-      activeOptionIndex: (() => {
-        if (currentActiveOption === null) return null
-
-        // If we inserted an option before the current active option then the
-        // active option index would be wrong. To fix this, we will re-lookup
-        // the correct index.
-        return options.indexOf(currentActiveOption)
-      })(),
+      ...adjustedState,
       activationTrigger: ActivationTrigger.Other,
     }
 
@@ -181,25 +200,15 @@ let reducers: {
     return nextState
   },
   [ActionTypes.UnregisterOption]: (state, action) => {
-    let nextOptions = state.options.slice()
-    let currentActiveOption =
-      state.activeOptionIndex !== null ? nextOptions[state.activeOptionIndex] : null
-
-    let idx = nextOptions.findIndex((a) => a.id === action.id)
-
-    if (idx !== -1) nextOptions.splice(idx, 1)
+    let adjustedState = adjustOrderedState(state, (options) => {
+      let idx = options.findIndex((a) => a.id === action.id)
+      if (idx !== -1) options.splice(idx, 1)
+      return options
+    })
 
     return {
       ...state,
-      options: nextOptions,
-      activeOptionIndex: (() => {
-        if (idx === state.activeOptionIndex) return null
-        if (currentActiveOption === null) return null
-
-        // If we removed the option before the actual active index, then it would be out of sync. To
-        // fix this, we will find the correct (new) index position.
-        return nextOptions.indexOf(currentActiveOption)
-      })(),
+      ...adjustedState,
       activationTrigger: ActivationTrigger.Other,
     }
   },

diff --git a/packages/@headlessui-react/src/components/listbox/listbox.tsx b/packages/@headlessui-react/src/components/listbox/listbox.tsx
@@ -83,6 +83,35 @@ enum ActionTypes {
   UnregisterOption,
 }
 
+function adjustOrderedState(
+  state: StateDefinition,
+  adjustment: (options: StateDefinition['options']) => StateDefinition['options'] = (i) => i
+) {
+  let currentActiveOption =
+    state.activeOptionIndex !== null ? state.options[state.activeOptionIndex] : null
+
+  let sortedOptions = sortByDomNode(
+    adjustment(state.options.slice()),
+    (option) => option.dataRef.current.domRef.current
+  )
+
+  // If we inserted an option before the current active option then the active option index
+  // would be wrong. To fix this, we will re-lookup the correct index.
+  let adjustedActiveOptionIndex = currentActiveOption
+    ? sortedOptions.indexOf(currentActiveOption)
+    : null
+
+  // Reset to `null` in case the currentActiveOption was removed.
+  if (adjustedActiveOptionIndex === -1) {
+    adjustedActiveOptionIndex = null
+  }
+
+  return {
+    options: sortedOptions,
+    activeOptionIndex: adjustedActiveOptionIndex,
+  }
+}
+
 type Actions =
   | { type: ActionTypes.CloseListbox }
   | { type: ActionTypes.OpenListbox }
@@ -127,19 +156,17 @@ let reducers: {
     if (state.disabled) return state
     if (state.listboxState === ListboxStates.Closed) return state
 
-    let options = sortByDomNode(state.options, (option) => option.dataRef.current.domRef.current)
-
+    let adjustedState = adjustOrderedState(state)
     let activeOptionIndex = calculateActiveIndex(action, {
-      resolveItems: () => options,
-      resolveActiveIndex: () => state.activeOptionIndex,
-      resolveId: (item) => item.id,
-      resolveDisabled: (item) => item.dataRef.current.disabled,
+      resolveItems: () => adjustedState.options,
+      resolveActiveIndex: () => adjustedState.activeOptionIndex,
+      resolveId: (option) => option.id,
+      resolveDisabled: (option) => option.dataRef.current.disabled,
     })
 
-    if (state.searchQuery === '' && state.activeOptionIndex === activeOptionIndex) return state
     return {
       ...state,
-      options, // Sorted options
+      ...adjustedState,
       searchQuery: '',
       activeOptionIndex,
       activationTrigger: action.trigger ?? ActivationTrigger.Other,
@@ -184,29 +211,23 @@ let reducers: {
     return { ...state, searchQuery: '' }
   },
   [ActionTypes.RegisterOption]: (state, action) => {
-    let options = [...state.options, { id: action.id, dataRef: action.dataRef }]
-    return { ...state, options }
+    let adjustedState = adjustOrderedState(state, (options) => [
+      ...options,
+      { id: action.id, dataRef: action.dataRef },
+    ])
+
+    return { ...state, ...adjustedState }
   },
   [ActionTypes.UnregisterOption]: (state, action) => {
-    let nextOptions = state.options.slice()
-    let currentActiveOption =
-      state.activeOptionIndex !== null ? nextOptions[state.activeOptionIndex] : null
-
-    let idx = nextOptions.findIndex((a) => a.id === action.id)
-
-    if (idx !== -1) nextOptions.splice(idx, 1)
+    let adjustedState = adjustOrderedState(state, (options) => {
+      let idx = options.findIndex((a) => a.id === action.id)
+      if (idx !== -1) options.splice(idx, 1)
+      return options
+    })
 
     return {
       ...state,
-      options: nextOptions,
-      activeOptionIndex: (() => {
-        if (idx === state.activeOptionIndex) return null
-        if (currentActiveOption === null) return null
-
-        // If we removed the option before the actual active index, then it would be out of sync. To
-        // fix this, we will find the correct (new) index position.
-        return nextOptions.indexOf(currentActiveOption)
-      })(),
+      ...adjustedState,
       activationTrigger: ActivationTrigger.Other,
     }
   },

diff --git a/packages/@headlessui-react/src/components/menu/menu.tsx b/packages/@headlessui-react/src/components/menu/menu.tsx
@@ -73,6 +73,32 @@ enum ActionTypes {
   UnregisterItem,
 }
 
+function adjustOrderedState(
+  state: StateDefinition,
+  adjustment: (items: StateDefinition['items']) => StateDefinition['items'] = (i) => i
+) {
+  let currentActiveItem = state.activeItemIndex !== null ? state.items[state.activeItemIndex] : null
+
+  let sortedItems = sortByDomNode(
+    adjustment(state.items.slice()),
+    (item) => item.dataRef.current.domRef.current
+  )
+
+  // If we inserted an item before the current active item then the active item index
+  // would be wrong. To fix this, we will re-lookup the correct index.
+  let adjustedActiveItemIndex = currentActiveItem ? sortedItems.indexOf(currentActiveItem) : null
+
+  // Reset to `null` in case the currentActiveItem was removed.
+  if (adjustedActiveItemIndex === -1) {
+    adjustedActiveItemIndex = null
+  }
+
+  return {
+    items: sortedItems,
+    activeItemIndex: adjustedActiveItemIndex,
+  }
+}
+
 type Actions =
   | { type: ActionTypes.CloseMenu }
   | { type: ActionTypes.OpenMenu }
@@ -102,19 +128,17 @@ let reducers: {
     return { ...state, menuState: MenuStates.Open }
   },
   [ActionTypes.GoToItem]: (state, action) => {
-    let items = sortByDomNode(state.items, (item) => item.dataRef.current.domRef.current)
-
+    let adjustedState = adjustOrderedState(state)
     let activeItemIndex = calculateActiveIndex(action, {
-      resolveItems: () => items,
-      resolveActiveIndex: () => state.activeItemIndex,
+      resolveItems: () => adjustedState.items,
+      resolveActiveIndex: () => adjustedState.activeItemIndex,
       resolveId: (item) => item.id,
       resolveDisabled: (item) => item.dataRef.current.disabled,
     })
 
-    if (state.searchQuery === '' && state.activeItemIndex === activeItemIndex) return state
     return {
       ...state,
-      items, // Sorted items
+      ...adjustedState,
       searchQuery: '',
       activeItemIndex,
       activationTrigger: action.trigger ?? ActivationTrigger.Other,
@@ -151,28 +175,23 @@ let reducers: {
     return { ...state, searchQuery: '', searchActiveItemIndex: null }
   },
   [ActionTypes.RegisterItem]: (state, action) => {
-    let items = [...state.items, { id: action.id, dataRef: action.dataRef }]
-    return { ...state, items }
+    let adjustedState = adjustOrderedState(state, (items) => [
+      ...items,
+      { id: action.id, dataRef: action.dataRef },
+    ])
+
+    return { ...state, ...adjustedState }
   },
   [ActionTypes.UnregisterItem]: (state, action) => {
-    let nextItems = state.items.slice()
-    let currentActiveItem = state.activeItemIndex !== null ? nextItems[state.activeItemIndex] : null
-
-    let idx = nextItems.findIndex((a) => a.id === action.id)
-
-    if (idx !== -1) nextItems.splice(idx, 1)
+    let adjustedState = adjustOrderedState(state, (items) => {
+      let idx = items.findIndex((a) => a.id === action.id)
+      if (idx !== -1) items.splice(idx, 1)
+      return items
+    })
 
     return {
       ...state,
-      items: nextItems,
-      activeItemIndex: (() => {
-        if (idx === state.activeItemIndex) return null
-        if (currentActiveItem === null) return null
-
-        // If we removed the item before the actual active index, then it would be out of sync. To
-        // fix this, we will find the correct (new) index position.
-        return nextItems.indexOf(currentActiveItem)
-      })(),
+      ...adjustedState,
       activationTrigger: ActivationTrigger.Other,
     }
   },