Fix incorrect active option in the Listbox/Combobox component (#1264)

* update tests to expose bug in React implementation

* fix incorrect `active` state on mouseLeave

The React code had a bug in the Listbox and Combobox components where it
incorrectly made the first selected value the active value.

The first selected option should be the active option when you open the
listbox. However when you already had the component in an `open` state,
hovered over a non-selected item and them left the option by moving it
to the body then the first selected option became the active one again.

This made sense because we used a `useEffect` in each option to make it
the active one if it was also selected. Since every component
re-renders, code got called and the bug arises.

Now, instead we moved the logic to make it the active option to the
reducer logic. We will check it when we register an option and doesn't
have an active option index yet or when we open the Listbox/Combobox.

This should also solve the strange scrolling behaviour where the options
scroll up if you have more options than you display.

* update changelog

CHANGELOG.md

@@ -30,6 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Improve Combobox Input value ([#1248](https://github.com/tailwindlabs/headlessui/pull/1248))
 - Fix Tree-shaking support ([#1247](https://github.com/tailwindlabs/headlessui/pull/1247))
 - Stop propagation on the Popover Button ([#1263](https://github.com/tailwindlabs/headlessui/pull/1263))
+- Fix incorrect `active` option in the Listbox/Combobox component ([#1264](https://github.com/tailwindlabs/headlessui/pull/1264))
 
 ### Added
 

packages/@headlessui-react/src/components/combobox/combobox.test.tsx

@@ -4057,7 +4057,7 @@ describe('Mouse interactions', () => {
     'should be possible to mouse leave an option and make it inactive',
     suppressConsoleLogs(async () => {
       render(
-        <Combobox value="test" onChange={console.log}>
+        <Combobox value="bob" onChange={console.log}>
           <Combobox.Input onChange={NOOP} />
           <Combobox.Button>Trigger</Combobox.Button>
           <Combobox.Options>

packages/@headlessui-react/src/components/combobox/combobox.tsx

@@ -66,6 +66,7 @@ interface StateDefinition {
 
   comboboxPropsRef: MutableRefObject<{
     value: unknown
+    mode: ValueMode
     onChange(value: unknown): void
     __demoMode: boolean
   }>
@@ -155,7 +156,25 @@ let reducers: {
   [ActionTypes.OpenCombobox](state) {
     if (state.disabled) return state
     if (state.comboboxState === ComboboxStates.Open) return state
-    return { ...state, comboboxState: ComboboxStates.Open }
+
+    // Check if we have a selected value that we can make active
+    let activeOptionIndex = state.activeOptionIndex
+    let { value, mode } = state.comboboxPropsRef.current
+    let optionIdx = state.options.findIndex((option) => {
+      let optionValue = option.dataRef.current.value
+      let selected = match(mode, {
+        [ValueMode.Multi]: () => (value as unknown[]).includes(optionValue),
+        [ValueMode.Single]: () => value === optionValue,
+      })
+
+      return selected
+    })
+
+    if (optionIdx !== -1) {
+      activeOptionIndex = optionIdx
+    }
+
+    return { ...state, comboboxState: ComboboxStates.Open, activeOptionIndex }
   },
   [ActionTypes.SetDisabled](state, action) {
     if (state.disabled === action.disabled) return state
@@ -187,9 +206,21 @@ let reducers: {
     }
   },
   [ActionTypes.RegisterOption]: (state, action) => {
-    let adjustedState = adjustOrderedState(state, (options) => {
-      return [...options, { id: action.id, dataRef: action.dataRef }]
-    })
+    let option = { id: action.id, dataRef: action.dataRef }
+    let adjustedState = adjustOrderedState(state, (options) => [...options, option])
+
+    // Check if we need to make the newly registered option active.
+    if (state.activeOptionIndex === null) {
+      let { value, mode } = state.comboboxPropsRef.current
+      let optionValue = action.dataRef.current.value
+      let selected = match(mode, {
+        [ValueMode.Multi]: () => (value as unknown[]).includes(optionValue),
+        [ValueMode.Single]: () => value === optionValue,
+      })
+      if (selected) {
+        adjustedState.activeOptionIndex = adjustedState.options.indexOf(option)
+      }
+    }
 
     let nextState = {
       ...state,
@@ -303,9 +334,14 @@ let ComboboxRoot = forwardRefWithAs(function Combobox<
 
   let comboboxPropsRef = useRef<StateDefinition['comboboxPropsRef']['current']>({
     value,
+    mode: Array.isArray(value) ? ValueMode.Multi : ValueMode.Single,
     onChange,
     __demoMode,
   })
+
+  comboboxPropsRef.current.value = value
+  comboboxPropsRef.current.mode = Array.isArray(value) ? ValueMode.Multi : ValueMode.Single
+
   let optionsPropsRef = useRef<StateDefinition['optionsPropsRef']['current']>({
     static: false,
     hold: false,
@@ -336,10 +372,6 @@ let ComboboxRoot = forwardRefWithAs(function Combobox<
     [value]
   )
 
-  useIsoMorphicEffect(() => {
-    comboboxPropsRef.current.value = value
-  }, [value])
-
   useIsoMorphicEffect(() => {
     comboboxPropsRef.current.onChange = (value: unknown) => {
       return match(dataBag.mode, {
@@ -965,18 +997,6 @@ let Option = forwardRefWithAs(function Option<
     [ValueMode.Multi]: () => (data.value as TType[]).includes(value),
     [ValueMode.Single]: () => data.value === value,
   })
-  let isFirstSelected = match(data.mode, {
-    [ValueMode.Multi]: () => {
-      let currentValues = data.value as TType[]
-
-      return (
-        state.options.find((option) =>
-          currentValues.includes(option.dataRef.current.value as TType)
-        )?.id === id
-      )
-    },
-    [ValueMode.Single]: () => selected,
-  })
   let internalOptionRef = useRef<HTMLLIElement | null>(null)
   let bag = useRef<ComboboxOptionDataRef['current']>({ disabled, value, domRef: internalOptionRef })
   let optionRef = useSyncRefs(ref, internalOptionRef)
@@ -995,21 +1015,6 @@ let Option = forwardRefWithAs(function Option<
 
   useIsoMorphicEffect(() => actions.registerOption(id, bag), [bag, id])
 
-  useIsoMorphicEffect(() => {
-    if (state.comboboxState !== ComboboxStates.Open) return
-    if (!selected) return
-    if (state.activeOptionIndex !== null) return
-
-    match(data.mode, {
-      [ValueMode.Multi]: () => {
-        if (isFirstSelected) actions.goToOption(Focus.Specific, id)
-      },
-      [ValueMode.Single]: () => {
-        actions.goToOption(Focus.Specific, id)
-      },
-    })
-  }, [state.comboboxState, state.activeOptionIndex, selected, isFirstSelected, id, actions, data])
-
   let enableScrollIntoView = useRef(state.comboboxPropsRef.current.__demoMode ? false : true)
   useIsoMorphicEffect(() => {
     if (!state.comboboxPropsRef.current.__demoMode) return

packages/@headlessui-react/src/components/listbox/listbox.test.tsx

@@ -3723,7 +3723,7 @@ describe('Mouse interactions', () => {
     'should be possible to mouse leave an option and make it inactive',
     suppressConsoleLogs(async () => {
       render(
-        <Listbox value={undefined} onChange={console.log}>
+        <Listbox value="bob" onChange={console.log}>
           <Listbox.Button>Trigger</Listbox.Button>
           <Listbox.Options>
             <Listbox.Option value="alice">alice</Listbox.Option>

packages/@headlessui-react/src/components/listbox/listbox.tsx

@@ -151,7 +151,25 @@ let reducers: {
   [ActionTypes.OpenListbox](state) {
     if (state.disabled) return state
     if (state.listboxState === ListboxStates.Open) return state
-    return { ...state, listboxState: ListboxStates.Open }
+
+    // Check if we have a selected value that we can make active
+    let activeOptionIndex = state.activeOptionIndex
+    let { value, mode } = state.propsRef.current
+    let optionIdx = state.options.findIndex((option) => {
+      let optionValue = option.dataRef.current.value
+      let selected = match(mode, {
+        [ValueMode.Multi]: () => (value as unknown[]).includes(optionValue),
+        [ValueMode.Single]: () => value === optionValue,
+      })
+
+      return selected
+    })
+
+    if (optionIdx !== -1) {
+      activeOptionIndex = optionIdx
+    }
+
+    return { ...state, listboxState: ListboxStates.Open, activeOptionIndex }
   },
   [ActionTypes.SetDisabled](state, action) {
     if (state.disabled === action.disabled) return state
@@ -220,10 +238,21 @@ let reducers: {
     return { ...state, searchQuery: '' }
   },
   [ActionTypes.RegisterOption]: (state, action) => {
-    let adjustedState = adjustOrderedState(state, (options) => [
-      ...options,
-      { id: action.id, dataRef: action.dataRef },
-    ])
+    let option = { id: action.id, dataRef: action.dataRef }
+    let adjustedState = adjustOrderedState(state, (options) => [...options, option])
+
+    // Check if we need to make the newly registered option active.
+    if (state.activeOptionIndex === null) {
+      let { value, mode } = state.propsRef.current
+      let optionValue = action.dataRef.current.value
+      let selected = match(mode, {
+        [ValueMode.Multi]: () => (value as unknown[]).includes(optionValue),
+        [ValueMode.Single]: () => value === optionValue,
+      })
+      if (selected) {
+        adjustedState.activeOptionIndex = adjustedState.options.indexOf(option)
+      }
+    }
 
     return { ...state, ...adjustedState }
   },
@@ -738,18 +767,6 @@ let Option = forwardRefWithAs(function Option<
     [ValueMode.Multi]: () => (state.propsRef.current.value as TType[]).includes(value),
     [ValueMode.Single]: () => state.propsRef.current.value === value,
   })
-  let isFirstSelected = match(state.propsRef.current.mode, {
-    [ValueMode.Multi]: () => {
-      let currentValues = state.propsRef.current.value as TType[]
-
-      return (
-        state.options.find((option) =>
-          currentValues.includes(option.dataRef.current.value as TType)
-        )?.id === id
-      )
-    },
-    [ValueMode.Single]: () => selected,
-  })
 
   let internalOptionRef = useRef<HTMLLIElement | null>(null)
   let optionRef = useSyncRefs(ref, internalOptionRef)
@@ -784,21 +801,6 @@ let Option = forwardRefWithAs(function Option<
     return () => dispatch({ type: ActionTypes.UnregisterOption, id })
   }, [bag, id])
 
-  useIsoMorphicEffect(() => {
-    if (state.listboxState !== ListboxStates.Open) return
-    if (!selected) return
-    if (state.activeOptionIndex !== null) return
-
-    match(state.propsRef.current.mode, {
-      [ValueMode.Multi]: () => {
-        if (isFirstSelected) dispatch({ type: ActionTypes.GoToOption, focus: Focus.Specific, id })
-      },
-      [ValueMode.Single]: () => {
-        dispatch({ type: ActionTypes.GoToOption, focus: Focus.Specific, id })
-      },
-    })
-  }, [state.listboxState, state.activeOptionIndex, selected, isFirstSelected, state.propsRef.current.mode, id])
-
   let handleClick = useCallback(
     (event: { preventDefault: Function }) => {
       if (disabled) return event.preventDefault()

packages/@headlessui-vue/src/components/combobox/combobox.test.ts

@@ -4302,7 +4302,7 @@ describe('Mouse interactions', () => {
             </ComboboxOptions>
           </Combobox>
         `,
-        setup: () => ({ value: ref(null) }),
+        setup: () => ({ value: ref('bob') }),
       })
 
       // Open combobox

packages/@headlessui-vue/src/components/listbox/listbox.test.tsx

@@ -3862,7 +3862,7 @@ describe('Mouse interactions', () => {
             </ListboxOptions>
           </Listbox>
         `,
-        setup: () => ({ value: ref(null) }),
+        setup: () => ({ value: ref('bob') }),
       })
 
       // Open listbox