Ensure correct order when conditionally rendering Menu.Item, `Listb…

…ox.Option` and `RadioGroup.Option` (#1045)

* ensure correct order in `Menu.Item`

* Update Vue version of menu component ordering issue

* ensure correct order of `Listbox.Option`s

* add test to verify that RadioGroup.Option order is correct

* ensure correct order of `ListboxOption`s

* cleanup

* add test to verify that `RadioGroupOption` order is correct

* update changelog

* use similar a,z signature compared to other places

Co-authored-by: Jordan Pittman <jordan@cryptica.me>

CHANGELOG.md

@@ -7,11 +7,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased - @headlessui/react]
 
-- Nothing yet!
+### Fixed
+
+- Ensure correct order when conditionally rendering `Menu.Item`, `Listbox.Option` and `RadioGroup.Option` ([#1045](https://github.com/tailwindlabs/headlessui/pull/1045))
 
 ## [Unreleased - @headlessui/vue]
 
-- Nothing yet!
+### Fixed
+
+- Ensure correct order when conditionally rendering `MenuItem`, `ListboxOption` and `RadioGroupOption` ([#1045](https://github.com/tailwindlabs/headlessui/pull/1045))
 
 ## [@headlessui/react@v1.4.3] - 2022-01-14
 

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

@@ -494,6 +494,49 @@ describe('Rendering', () => {
       })
     )
   })
+
+  it('should guarantee the order of DOM nodes when performing actions', async () => {
+    function Example({ hide = false }) {
+      return (
+        <>
+          <Listbox value={undefined} onChange={console.log}>
+            <Listbox.Button>Trigger</Listbox.Button>
+            <Listbox.Options>
+              <Listbox.Option value="a">Option 1</Listbox.Option>
+              {!hide && <Listbox.Option value="b">Option 2</Listbox.Option>}
+              <Listbox.Option value="c">Option 3</Listbox.Option>
+            </Listbox.Options>
+          </Listbox>
+        </>
+      )
+    }
+
+    let { rerender } = render(<Example />)
+
+    // Open the Listbox
+    await click(getByText('Trigger'))
+
+    rerender(<Example hide={true} />) // Remove Listbox.Option 2
+    rerender(<Example hide={false} />) // Re-add Listbox.Option 2
+
+    assertListbox({ state: ListboxState.Visible })
+
+    let options = getListboxOptions()
+
+    // Focus the first item
+    await press(Keys.ArrowDown)
+
+    // Verify that the first menu item is active
+    assertActiveListboxOption(options[0])
+
+    await press(Keys.ArrowDown)
+    // Verify that the second menu item is active
+    assertActiveListboxOption(options[1])
+
+    await press(Keys.ArrowDown)
+    // Verify that the third menu item is active
+    assertActiveListboxOption(options[2])
+  })
 })
 
 describe('Rendering composition', () => {

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

@@ -146,10 +146,20 @@ let reducers: {
     if (state.searchQuery === '') return state
     return { ...state, searchQuery: '' }
   },
-  [ActionTypes.RegisterOption]: (state, action) => ({
-    ...state,
-    options: [...state.options, { id: action.id, dataRef: action.dataRef }],
-  }),
+  [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]
+    )
+
+    return { ...state, options }
+  },
   [ActionTypes.UnregisterOption]: (state, action) => {
     let nextOptions = state.options.slice()
     let currentActiveOption =

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

@@ -350,6 +350,49 @@ describe('Rendering', () => {
       })
     )
   })
+
+  it('should guarantee the order of DOM nodes when performing actions', async () => {
+    function Example({ hide = false }) {
+      return (
+        <>
+          <Menu>
+            <Menu.Button>Trigger</Menu.Button>
+            <Menu.Items>
+              <Menu.Item as="button">Item 1</Menu.Item>
+              {!hide && <Menu.Item as="button">Item 2</Menu.Item>}
+              <Menu.Item as="button">Item 3</Menu.Item>
+            </Menu.Items>
+          </Menu>
+        </>
+      )
+    }
+
+    let { rerender } = render(<Example />)
+
+    // Open the Menu
+    await click(getByText('Trigger'))
+
+    rerender(<Example hide={true} />) // Remove Menu.Item 2
+    rerender(<Example hide={false} />) // Re-add Menu.Item 2
+
+    assertMenu({ state: MenuState.Visible })
+
+    let items = getMenuItems()
+
+    // Focus the first item
+    await press(Keys.ArrowDown)
+
+    // Verify that the first menu item is active
+    assertMenuLinkedWithMenuItem(items[0])
+
+    await press(Keys.ArrowDown)
+    // Verify that the second menu item is active
+    assertMenuLinkedWithMenuItem(items[1])
+
+    await press(Keys.ArrowDown)
+    // Verify that the third menu item is active
+    assertMenuLinkedWithMenuItem(items[2])
+  })
 })
 
 describe('Rendering composition', () => {

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

@@ -112,10 +112,20 @@ let reducers: {
     if (state.searchQuery === '') return state
     return { ...state, searchQuery: '' }
   },
-  [ActionTypes.RegisterItem]: (state, action) => ({
-    ...state,
-    items: [...state.items, { id: action.id, dataRef: action.dataRef }],
-  }),
+  [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]
+    )
+
+    return { ...state, items }
+  },
   [ActionTypes.UnregisterItem]: (state, action) => {
     let nextItems = state.items.slice()
     let currentActiveItem = state.activeItemIndex !== null ? nextItems[state.activeItemIndex] : null

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

@@ -268,6 +268,37 @@ describe('Rendering', () => {
     // Make sure that the onChange handler got called
     expect(changeFn).toHaveBeenCalledTimes(1)
   })
+
+  it('should guarantee the order of DOM nodes when performing actions', async () => {
+    function Example({ hide = false }) {
+      return (
+        <RadioGroup value={undefined} onChange={() => {}}>
+          <RadioGroup.Option value="a">Option 1</RadioGroup.Option>
+          {!hide && <RadioGroup.Option value="b">Option 2</RadioGroup.Option>}
+          <RadioGroup.Option value="c">Option 3</RadioGroup.Option>
+        </RadioGroup>
+      )
+    }
+
+    let { rerender } = render(<Example />)
+
+    // Focus the RadioGroup
+    await press(Keys.Tab)
+
+    rerender(<Example hide={true} />) // Remove RadioGroup.Option 2
+    rerender(<Example hide={false} />) // Re-add RadioGroup.Option 2
+
+    // Verify that the first radio group option is active
+    assertActiveElement(getByText('Option 1'))
+
+    await press(Keys.ArrowDown)
+    // Verify that the second radio group option is active
+    assertActiveElement(getByText('Option 2'))
+
+    await press(Keys.ArrowDown)
+    // Verify that the third radio group option is active
+    assertActiveElement(getByText('Option 3'))
+  })
 })
 
 describe('Keyboard interactions', () => {

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

@@ -104,8 +104,8 @@ export function focusElement(element: HTMLElement | null) {
 
 export function focusIn(container: HTMLElement | HTMLElement[], focus: Focus) {
   let elements = Array.isArray(container)
-    ? container.slice().sort((a, b) => {
-        let position = a.compareDocumentPosition(b)
+    ? 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

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

@@ -1,4 +1,4 @@
-import { defineComponent, nextTick, ref, watch, h } from 'vue'
+import { defineComponent, nextTick, ref, watch, h, reactive } from 'vue'
 import { render } from '../../test-utils/vue-testing-library'
 import { Listbox, ListboxLabel, ListboxButton, ListboxOptions, ListboxOption } from './listbox'
 import { suppressConsoleLogs } from '../../test-utils/suppress-console-logs'
@@ -21,6 +21,7 @@ import {
   getListboxOptions,
   getListboxLabel,
   ListboxState,
+  getByText,
 } from '../../test-utils/accessibility-assertions'
 import {
   click,
@@ -46,6 +47,16 @@ beforeAll(() => {
 
 afterAll(() => jest.restoreAllMocks())
 
+function nextFrame() {
+  return new Promise(resolve => {
+    requestAnimationFrame(() => {
+      requestAnimationFrame(() => {
+        resolve()
+      })
+    })
+  })
+}
+
 function renderTemplate(input: string | Partial<Parameters<typeof defineComponent>[0]>) {
   let defaultComponents = { Listbox, ListboxLabel, ListboxButton, ListboxOptions, ListboxOption }
 
@@ -571,6 +582,60 @@ describe('Rendering', () => {
       })
     )
   })
+
+  it('should guarantee the order of DOM nodes when performing actions', async () => {
+    let props = reactive({ hide: false })
+
+    renderTemplate({
+      template: html`
+        <Listbox v-model="value">
+          <ListboxButton>Trigger</ListboxButton>
+          <ListboxOptions>
+            <ListboxOption value="a">Option 1</ListboxOption>
+            <ListboxOption v-if="!hide" value="b">Option 2</ListboxOption>
+            <ListboxOption value="c">Option 3</ListboxOption>
+          </ListboxOptions>
+        </Listbox>
+      `,
+      setup() {
+        return {
+          value: ref(null),
+          get hide() {
+            return props.hide
+          },
+        }
+      },
+    })
+
+    // Open the Listbox
+    await click(getByText('Trigger'))
+
+    props.hide = true
+    await nextFrame()
+
+    props.hide = false
+    await nextFrame()
+
+    assertListbox({ state: ListboxState.Visible })
+
+    let options = getListboxOptions()
+
+    // Focus the first option
+    await press(Keys.ArrowDown)
+
+    // Verify that the first listbox option is active
+    assertActiveListboxOption(options[0])
+
+    await press(Keys.ArrowDown)
+
+    // Verify that the second listbox option is active
+    assertActiveListboxOption(options[1])
+
+    await press(Keys.ArrowDown)
+
+    // Verify that the third listbox option is active
+    assertActiveListboxOption(options[2])
+  })
 })
 
 describe('Rendering composition', () => {

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

@@ -161,8 +161,17 @@ export let Listbox = defineComponent({
         searchQuery.value = ''
       },
       registerOption(id: string, dataRef: ListboxOptionDataRef) {
+        let orderMap = Array.from(
+          optionsRef.value?.querySelectorAll('[id^="headlessui-listbox-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.push({ id, dataRef })
+        options.value = [...options.value, { id, dataRef }].sort(
+          (a, z) => orderMap[a.id] - orderMap[z.id]
+        )
       },
       unregisterOption(id: string) {
         let nextOptions = options.value.slice()