Guarantee DOM sort order when performing actions (#1168)

* ensure proper sort order

We already fixed a bug in the past where the order of DOM nodes wasn't
stored in the correct order when performing operations (e.g.: using your
keyboard to go to the next option).

We fixed this by ensuring that when we register/unregister an
option/item, that we sorted the list properly. This worked fine, until
we introduced the Combobox components. This is because items in a
Combobox are continuously filtered and because of that moved around.

Moving a DOM node to a new position _doesn't_ require a full
unmount/remount. This means that the sort gets messed up and the order
is wrong when moving around again.

To fix this, we will always perform a sort when performing actions. This
could have performance drawbacks, but the alternative is to re-sort when
the component gets updated. The bad part is that you can update a
component via many ways (like changes on the parent), in those
scenario's you probably don't care to properly re-order the internal
list. Instead we do it while performing an action (`goToOption` / `goToItem`).

To make things a bit more efficient, instead of querying the DOM all the
time using `document.querySelectorAll`, we will keep track of the
underlying DOM node instead. This does increase memory usage a bit but I
think that this is a fine trade-off.

Performance wise this could also be a bottleneck to perform the sorting
if you have a lot of data. But this problem already exists today,
therefore I consider this a complete new problem instead to solve. Maybe
we don't solve it in Headless UI itself, but figure out a way to make it
composable with existing virtualization libraries.

* update changelog

Author: RobinMalfait

CHANGELOG.md

@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Ensure links are triggered inside `Popover Panel` components ([#1153](https://github.com/tailwindlabs/headlessui/pull/1153))
 - Improve SSR for `Tab` component ([#1155](https://github.com/tailwindlabs/headlessui/pull/1155))
 - Fix `hover` scroll ([#1161](https://github.com/tailwindlabs/headlessui/pull/1161))
+- Guarantee DOM sort order when performing actions ([#1168](https://github.com/tailwindlabs/headlessui/pull/1168))
 
 ## [Unreleased - @headlessui/vue]
 
@@ -23,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fix Dialog usage in Tabs ([#1149](https://github.com/tailwindlabs/headlessui/pull/1149))
 - Ensure links are triggered inside `Popover Panel` components ([#1153](https://github.com/tailwindlabs/headlessui/pull/1153))
 - Fix `hover` scroll ([#1161](https://github.com/tailwindlabs/headlessui/pull/1161))
+- Guarantee DOM sort order when performing actions ([#1168](https://github.com/tailwindlabs/headlessui/pull/1168))
 
 ## [@headlessui/react@v1.5.0] - 2022-02-17
 

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

@@ -35,6 +35,7 @@ import { useOpenClosed, State, OpenClosedProvider } from '../../internal/open-cl
 import { useResolveButtonType } from '../../hooks/use-resolve-button-type'
 import { useLatestValue } from '../../hooks/use-latest-value'
 import { useTreeWalker } from '../../hooks/use-tree-walker'
+import { sortByDomNode } from '../../utils/focus-management'
 
 enum ComboboxStates {
   Open,
@@ -50,6 +51,7 @@ type ComboboxOptionDataRef = MutableRefObject<{
   textValue?: string
   disabled: boolean
   value: unknown
+  domRef: MutableRefObject<HTMLElement | null>
 }>
 
 interface StateDefinition {
@@ -129,11 +131,14 @@ let reducers: {
       state.optionsRef.current &&
       !state.optionsPropsRef.current.static &&
       state.comboboxState === ComboboxStates.Closed
-    )
+    ) {
       return state
+    }
+
+    let options = sortByDomNode(state.options, (option) => option.dataRef.current.domRef.current)
 
     let activeOptionIndex = calculateActiveIndex(action, {
-      resolveItems: () => state.options,
+      resolveItems: () => options,
       resolveActiveIndex: () => state.activeOptionIndex,
       resolveId: (item) => item.id,
       resolveDisabled: (item) => item.dataRef.current.disabled,
@@ -142,6 +147,7 @@ let reducers: {
     if (state.activeOptionIndex === activeOptionIndex) return state
     return {
       ...state,
+      options, // Sorted options
       activeOptionIndex,
       activationTrigger: action.trigger ?? ActivationTrigger.Other,
     }
@@ -150,17 +156,7 @@ let reducers: {
     let currentActiveOption =
       state.activeOptionIndex !== null ? state.options[state.activeOptionIndex] : null
 
-    let orderMap = Array.from(
-      state.optionsRef.current?.querySelectorAll('[id^="headlessui-combobox-option-"]')!
-    ).reduce(
-      (lookup, element, index) => Object.assign(lookup, { [element.id]: index }),
-      {}
-    ) as Record<string, number>
-
-    let options = [...state.options, { id: action.id, dataRef: action.dataRef }].sort(
-      (a, z) => orderMap[a.id] - orderMap[z.id]
-    )
-
+    let options = [...state.options, { id: action.id, dataRef: action.dataRef }]
     let nextState = {
       ...state,
       options,
@@ -859,8 +855,9 @@ let Option = forwardRefWithAs(function Option<
   let active =
     state.activeOptionIndex !== null ? state.options[state.activeOptionIndex].id === id : false
   let selected = state.comboboxPropsRef.current.value === value
-  let bag = useRef<ComboboxOptionDataRef['current']>({ disabled, value })
-  let optionRef = useSyncRefs(ref)
+  let internalOptionRef = useRef<HTMLLIElement | null>(null)
+  let bag = useRef<ComboboxOptionDataRef['current']>({ disabled, value, domRef: internalOptionRef })
+  let optionRef = useSyncRefs(ref, internalOptionRef)
 
   useIsoMorphicEffect(() => {
     bag.current.disabled = disabled

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

@@ -29,7 +29,7 @@ import { disposables } from '../../utils/disposables'
 import { Keys } from '../keyboard'
 import { Focus, calculateActiveIndex } from '../../utils/calculate-active-index'
 import { isDisabledReactIssue7711 } from '../../utils/bugs'
-import { isFocusableElement, FocusableMode } from '../../utils/focus-management'
+import { isFocusableElement, FocusableMode, sortByDomNode } from '../../utils/focus-management'
 import { useWindowEvent } from '../../hooks/use-window-event'
 import { useOpenClosed, State, OpenClosedProvider } from '../../internal/open-closed'
 import { useResolveButtonType } from '../../hooks/use-resolve-button-type'
@@ -48,6 +48,7 @@ type ListboxOptionDataRef = MutableRefObject<{
   textValue?: string
   disabled: boolean
   value: unknown
+  domRef: MutableRefObject<HTMLElement | null>
 }>
 
 interface StateDefinition {
@@ -126,8 +127,10 @@ 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 activeOptionIndex = calculateActiveIndex(action, {
-      resolveItems: () => state.options,
+      resolveItems: () => options,
       resolveActiveIndex: () => state.activeOptionIndex,
       resolveId: (item) => item.id,
       resolveDisabled: (item) => item.dataRef.current.disabled,
@@ -136,6 +139,7 @@ let reducers: {
     if (state.searchQuery === '' && state.activeOptionIndex === activeOptionIndex) return state
     return {
       ...state,
+      options, // Sorted options
       searchQuery: '',
       activeOptionIndex,
       activationTrigger: action.trigger ?? ActivationTrigger.Other,
@@ -180,17 +184,7 @@ let reducers: {
     return { ...state, searchQuery: '' }
   },
   [ActionTypes.RegisterOption]: (state, action) => {
-    let orderMap = Array.from(
-      state.optionsRef.current?.querySelectorAll('[id^="headlessui-listbox-option-"]')!
-    ).reduce(
-      (lookup, element, index) => Object.assign(lookup, { [element.id]: index }),
-      {}
-    ) as Record<string, number>
-
-    let options = [...state.options, { id: action.id, dataRef: action.dataRef }].sort(
-      (a, z) => orderMap[a.id] - orderMap[z.id]
-    )
-
+    let options = [...state.options, { id: action.id, dataRef: action.dataRef }]
     return { ...state, options }
   },
   [ActionTypes.UnregisterOption]: (state, action) => {
@@ -665,9 +659,10 @@ let Option = forwardRefWithAs(function Option<
   let active =
     state.activeOptionIndex !== null ? state.options[state.activeOptionIndex].id === id : false
   let selected = state.propsRef.current.value === value
-  let optionRef = useSyncRefs(ref)
+  let internalOptionRef = useRef<HTMLElement | null>(null)
+  let optionRef = useSyncRefs(ref, internalOptionRef)
 
-  let bag = useRef<ListboxOptionDataRef['current']>({ disabled, value })
+  let bag = useRef<ListboxOptionDataRef['current']>({ disabled, value, domRef: internalOptionRef })
 
   useIsoMorphicEffect(() => {
     bag.current.disabled = disabled

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

@@ -30,7 +30,7 @@ import { useId } from '../../hooks/use-id'
 import { Keys } from '../keyboard'
 import { Focus, calculateActiveIndex } from '../../utils/calculate-active-index'
 import { isDisabledReactIssue7711 } from '../../utils/bugs'
-import { isFocusableElement, FocusableMode } from '../../utils/focus-management'
+import { isFocusableElement, FocusableMode, sortByDomNode } from '../../utils/focus-management'
 import { useWindowEvent } from '../../hooks/use-window-event'
 import { useTreeWalker } from '../../hooks/use-tree-walker'
 import { useOpenClosed, State, OpenClosedProvider } from '../../internal/open-closed'
@@ -46,7 +46,11 @@ enum ActivationTrigger {
   Other,
 }
 
-type MenuItemDataRef = MutableRefObject<{ textValue?: string; disabled: boolean }>
+type MenuItemDataRef = MutableRefObject<{
+  textValue?: string
+  disabled: boolean
+  domRef: MutableRefObject<HTMLElement | null>
+}>
 
 interface StateDefinition {
   menuState: MenuStates
@@ -98,8 +102,10 @@ let reducers: {
     return { ...state, menuState: MenuStates.Open }
   },
   [ActionTypes.GoToItem]: (state, action) => {
+    let items = sortByDomNode(state.items, (item) => item.dataRef.current.domRef.current)
+
     let activeItemIndex = calculateActiveIndex(action, {
-      resolveItems: () => state.items,
+      resolveItems: () => items,
       resolveActiveIndex: () => state.activeItemIndex,
       resolveId: (item) => item.id,
       resolveDisabled: (item) => item.dataRef.current.disabled,
@@ -108,6 +114,7 @@ let reducers: {
     if (state.searchQuery === '' && state.activeItemIndex === activeItemIndex) return state
     return {
       ...state,
+      items, // Sorted items
       searchQuery: '',
       activeItemIndex,
       activationTrigger: action.trigger ?? ActivationTrigger.Other,
@@ -144,17 +151,7 @@ let reducers: {
     return { ...state, searchQuery: '', searchActiveItemIndex: null }
   },
   [ActionTypes.RegisterItem]: (state, action) => {
-    let orderMap = Array.from(
-      state.itemsRef.current?.querySelectorAll('[id^="headlessui-menu-item-"]')!
-    ).reduce(
-      (lookup, element, index) => Object.assign(lookup, { [element.id]: index }),
-      {}
-    ) as Record<string, number>
-
-    let items = [...state.items, { id: action.id, dataRef: action.dataRef }].sort(
-      (a, z) => orderMap[a.id] - orderMap[z.id]
-    )
-
+    let items = [...state.items, { id: action.id, dataRef: action.dataRef }]
     return { ...state, items }
   },
   [ActionTypes.UnregisterItem]: (state, action) => {
@@ -560,7 +557,8 @@ let Item = forwardRefWithAs(function Item<TTag extends ElementType = typeof DEFA
   let [state, dispatch] = useMenuContext('Menu.Item')
   let id = `headlessui-menu-item-${useId()}`
   let active = state.activeItemIndex !== null ? state.items[state.activeItemIndex].id === id : false
-  let itemRef = useSyncRefs(ref)
+  let internalItemRef = useRef<HTMLElement | null>(null)
+  let itemRef = useSyncRefs(ref, internalItemRef)
 
   useIsoMorphicEffect(() => {
     if (state.menuState !== MenuStates.Open) return
@@ -573,7 +571,7 @@ let Item = forwardRefWithAs(function Item<TTag extends ElementType = typeof DEFA
     return d.dispose
   }, [id, active, state.menuState, state.activationTrigger, /* We also want to trigger this when the position of the active item changes so that we can re-trigger the scrollIntoView */ state.activeItemIndex])
 
-  let bag = useRef<MenuItemDataRef['current']>({ disabled })
+  let bag = useRef<MenuItemDataRef['current']>({ disabled, domRef: internalItemRef })
 
   useIsoMorphicEffect(() => {
     bag.current.disabled = disabled

packages/@headlessui-react/src/components/radio-group/radio-group.tsx

@@ -20,7 +20,7 @@ import { useId } from '../../hooks/use-id'
 import { match } from '../../utils/match'
 import { useIsoMorphicEffect } from '../../hooks/use-iso-morphic-effect'
 import { Keys } from '../../components/keyboard'
-import { focusIn, Focus, FocusResult } from '../../utils/focus-management'
+import { focusIn, Focus, FocusResult, sortByDomNode } from '../../utils/focus-management'
 import { useFlags } from '../../hooks/use-flags'
 import { Label, useLabels } from '../../components/label/label'
 import { Description, useDescriptions } from '../../components/description/description'
@@ -53,12 +53,14 @@ let reducers: {
   ) => StateDefinition
 } = {
   [ActionTypes.RegisterOption](state, action) {
+    let nextOptions = [
+      ...state.options,
+      { id: action.id, element: action.element, propsRef: action.propsRef },
+    ]
+
     return {
       ...state,
-      options: [
-        ...state.options,
-        { id: action.id, element: action.element, propsRef: action.propsRef },
-      ],
+      options: sortByDomNode(nextOptions, (option) => option.element.current),
     }
   },
   [ActionTypes.UnregisterOption](state, action) {

packages/@headlessui-react/src/utils/focus-management.ts

@@ -102,15 +102,27 @@ export function focusElement(element: HTMLElement | null) {
   element?.focus({ preventScroll: true })
 }
 
+export function sortByDomNode<T>(
+  nodes: T[],
+  resolveKey: (item: T) => HTMLElement | null = (i) => i as unknown as HTMLElement | null
+): T[] {
+  return nodes.slice().sort((aItem, zItem) => {
+    let a = resolveKey(aItem)
+    let z = resolveKey(zItem)
+
+    if (a === null || z === null) return 0
+
+    let position = a.compareDocumentPosition(z)
+
+    if (position & Node.DOCUMENT_POSITION_FOLLOWING) return -1
+    if (position & Node.DOCUMENT_POSITION_PRECEDING) return 1
+    return 0
+  })
+}
+
 export function focusIn(container: HTMLElement | HTMLElement[], focus: Focus) {
   let elements = Array.isArray(container)
-    ? container.slice().sort((a, z) => {
-        let position = a.compareDocumentPosition(z)
-
-        if (position & Node.DOCUMENT_POSITION_FOLLOWING) return -1
-        if (position & Node.DOCUMENT_POSITION_PRECEDING) return 1
-        return 0
-      })
+    ? sortByDomNode(container)
     : getFocusableElements(container)
   let active = document.activeElement as HTMLElement
 

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

@@ -26,6 +26,7 @@ import { useOpenClosed, State, useOpenClosedProvider } from '../../internal/open
 import { match } from '../../utils/match'
 import { useResolveButtonType } from '../../hooks/use-resolve-button-type'
 import { useTreeWalker } from '../../hooks/use-tree-walker'
+import { sortByDomNode } from '../../utils/focus-management'
 
 enum ComboboxStates {
   Open,
@@ -37,7 +38,11 @@ enum ActivationTrigger {
   Other,
 }
 
-type ComboboxOptionDataRef = Ref<{ disabled: boolean; value: unknown }>
+type ComboboxOptionDataRef = Ref<{
+  disabled: boolean
+  value: unknown
+  domRef: Ref<HTMLElement | null>
+}>
 type StateDefinition = {
   // State
   comboboxState: Ref<ComboboxStates>
@@ -140,24 +145,27 @@ export let Combobox = defineComponent({
           optionsRef.value &&
           !optionsPropsRef.value.static &&
           comboboxState.value === ComboboxStates.Closed
-        )
+        ) {
           return
+        }
+
+        let orderedOptions = sortByDomNode(options.value, (option) => option.dataRef.domRef.value)
 
         let nextActiveOptionIndex = calculateActiveIndex(
           focus === Focus.Specific
             ? { focus: Focus.Specific, id: id! }
             : { focus: focus as Exclude<Focus, Focus.Specific> },
           {
-            resolveItems: () => options.value,
+            resolveItems: () => orderedOptions,
             resolveActiveIndex: () => activeOptionIndex.value,
             resolveId: (option) => option.id,
             resolveDisabled: (option) => option.dataRef.disabled,
           }
         )
 
-        if (activeOptionIndex.value === nextActiveOptionIndex) return
         activeOptionIndex.value = nextActiveOptionIndex
         activationTrigger.value = trigger ?? ActivationTrigger.Other
+        options.value = orderedOptions
       },
       syncInputValue() {
         let value = api.value.value
@@ -189,17 +197,9 @@ export let Combobox = defineComponent({
       registerOption(id: string, dataRef: ComboboxOptionDataRef) {
         let currentActiveOption =
           activeOptionIndex.value !== null ? options.value[activeOptionIndex.value] : null
-        let orderMap = Array.from(
-          optionsRef.value?.querySelectorAll('[id^="headlessui-combobox-option-"]') ?? []
-        ).reduce(
-          (lookup, element, index) => Object.assign(lookup, { [element.id]: index }),
-          {}
-        ) as Record<string, number>
 
         // @ts-expect-error The expected type comes from property 'dataRef' which is declared here on type '{ id: string; dataRef: { textValue: string; disabled: boolean; }; }'
-        options.value = [...options.value, { id, dataRef }].sort(
-          (a, z) => orderMap[a.id] - orderMap[z.id]
-        )
+        options.value = [...options.value, { id, dataRef }]
 
         // 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
@@ -630,6 +630,7 @@ export let ComboboxOption = defineComponent({
   setup(props, { slots, attrs }) {
     let api = useComboboxContext('ComboboxOption')
     let id = `headlessui-combobox-option-${useId()}`
+    let internalOptionRef = ref<HTMLElement | null>(null)
 
     let active = computed(() => {
       return api.activeOptionIndex.value !== null
@@ -642,6 +643,7 @@ export let ComboboxOption = defineComponent({
     let dataRef = computed<ComboboxOptionDataRef['value']>(() => ({
       disabled: props.disabled,
       value: props.value,
+      domRef: internalOptionRef,
     }))
 
     onMounted(() => api.registerOption(id, dataRef))
@@ -696,6 +698,7 @@ export let ComboboxOption = defineComponent({
       let slot = { active: active.value, selected: selected.value, disabled }
       let propsWeControl = {
         id,
+        ref: internalOptionRef,
         role: 'option',
         tabIndex: disabled === true ? undefined : -1,
         'aria-disabled': disabled === true ? true : undefined,