diff --git a/CHANGELOG.md b/CHANGELOG.md index fc230f0..8fff810 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/packages/@headlessui-react/src/components/listbox/listbox.test.tsx b/packages/@headlessui-react/src/components/listbox/listbox.test.tsx index 70b304c..f766f25 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.test.tsx +++ b/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 ( + <> + + Trigger + + Option 1 + {!hide && Option 2} + Option 3 + + + + ) + } + + let { rerender } = render() + + // Open the Listbox + await click(getByText('Trigger')) + + rerender() // Remove Listbox.Option 2 + rerender() // 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', () => { diff --git a/packages/@headlessui-react/src/components/listbox/listbox.tsx b/packages/@headlessui-react/src/components/listbox/listbox.tsx index 7fe3fb2..5fc4f05 100644 --- a/packages/@headlessui-react/src/components/listbox/listbox.tsx +++ b/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 + + 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 = diff --git a/packages/@headlessui-react/src/components/menu/menu.test.tsx b/packages/@headlessui-react/src/components/menu/menu.test.tsx index 4bf3399..0080e97 100644 --- a/packages/@headlessui-react/src/components/menu/menu.test.tsx +++ b/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 ( + <> + + Trigger + + Item 1 + {!hide && Item 2} + Item 3 + + + + ) + } + + let { rerender } = render() + + // Open the Menu + await click(getByText('Trigger')) + + rerender() // Remove Menu.Item 2 + rerender() // 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', () => { diff --git a/packages/@headlessui-react/src/components/menu/menu.tsx b/packages/@headlessui-react/src/components/menu/menu.tsx index 1d51ca2..c3299f3 100644 --- a/packages/@headlessui-react/src/components/menu/menu.tsx +++ b/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 + + 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 diff --git a/packages/@headlessui-react/src/components/radio-group/radio-group.test.tsx b/packages/@headlessui-react/src/components/radio-group/radio-group.test.tsx index 830abe8..f9f7076 100644 --- a/packages/@headlessui-react/src/components/radio-group/radio-group.test.tsx +++ b/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 ( + {}}> + Option 1 + {!hide && Option 2} + Option 3 + + ) + } + + let { rerender } = render() + + // Focus the RadioGroup + await press(Keys.Tab) + + rerender() // Remove RadioGroup.Option 2 + rerender() // 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', () => { diff --git a/packages/@headlessui-react/src/utils/focus-management.ts b/packages/@headlessui-react/src/utils/focus-management.ts index a51b1cd..f48158b 100644 --- a/packages/@headlessui-react/src/utils/focus-management.ts +++ b/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 diff --git a/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx b/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx index 1feee6b..73a6a55 100644 --- a/packages/@headlessui-vue/src/components/listbox/listbox.test.tsx +++ b/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[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` + + Trigger + + Option 1 + Option 2 + Option 3 + + + `, + 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', () => { diff --git a/packages/@headlessui-vue/src/components/listbox/listbox.ts b/packages/@headlessui-vue/src/components/listbox/listbox.ts index d431f1b..5ec48a4 100644 --- a/packages/@headlessui-vue/src/components/listbox/listbox.ts +++ b/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 + // @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() diff --git a/packages/@headlessui-vue/src/components/menu/menu.test.tsx b/packages/@headlessui-vue/src/components/menu/menu.test.tsx index 30bbf07..478cd7e 100644 --- a/packages/@headlessui-vue/src/components/menu/menu.test.tsx +++ b/packages/@headlessui-vue/src/components/menu/menu.test.tsx @@ -1,4 +1,4 @@ -import { defineComponent, h, nextTick, ref, watch } from 'vue' +import { defineComponent, h, nextTick, reactive, ref, watch } from 'vue' import { render } from '../../test-utils/vue-testing-library' import { Menu, MenuButton, MenuItems, MenuItem } from './menu' import { TransitionChild } from '../transitions/transition' @@ -12,6 +12,7 @@ import { assertMenuLinkedWithMenuItem, assertActiveElement, assertNoActiveMenuItem, + getByText, getMenuButton, getMenu, getMenuItems, @@ -719,6 +720,59 @@ describe('Rendering', () => { await click(getMenuButton()) }) }) + + it('should guarantee the order of DOM nodes when performing actions', async () => { + let props = reactive({ hide: false }) + + renderTemplate({ + template: jsx` + + Trigger + + Item 1 + Item 2 + Item 3 + + + `, + setup() { + return { + get hide() { + return props.hide + }, + } + }, + }) + + // Open the Menu + await click(getByText('Trigger')) + + props.hide = true + await nextFrame() + + props.hide = false + await nextFrame() + + 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', () => { diff --git a/packages/@headlessui-vue/src/components/menu/menu.ts b/packages/@headlessui-vue/src/components/menu/menu.ts index 3a252c6..42fc04a 100644 --- a/packages/@headlessui-vue/src/components/menu/menu.ts +++ b/packages/@headlessui-vue/src/components/menu/menu.ts @@ -120,8 +120,17 @@ export let Menu = defineComponent({ searchQuery.value = '' }, registerItem(id: string, dataRef: MenuItemDataRef) { + let orderMap = Array.from( + itemsRef.value?.querySelectorAll('[id^="headlessui-menu-item-"]') ?? [] + ).reduce( + (lookup, element, index) => Object.assign(lookup, { [element.id]: index }), + {} + ) as Record + // @ts-expect-error The expected type comes from property 'dataRef' which is declared here on type '{ id: string; dataRef: { textValue: string; disabled: boolean; }; }' - items.value.push({ id, dataRef }) + items.value = [...items.value, { id, dataRef }].sort( + (a, z) => orderMap[a.id] - orderMap[z.id] + ) }, unregisterItem(id: string) { let nextItems = items.value.slice() diff --git a/packages/@headlessui-vue/src/components/radio-group/radio-group.test.ts b/packages/@headlessui-vue/src/components/radio-group/radio-group.test.ts index f6aad37..1701836 100644 --- a/packages/@headlessui-vue/src/components/radio-group/radio-group.test.ts +++ b/packages/@headlessui-vue/src/components/radio-group/radio-group.test.ts @@ -1,4 +1,4 @@ -import { defineComponent, nextTick, ref, watch } from 'vue' +import { defineComponent, nextTick, ref, watch, reactive } from 'vue' import { render } from '../../test-utils/vue-testing-library' import { RadioGroup, RadioGroupOption, RadioGroupLabel, RadioGroupDescription } from './radio-group' @@ -24,6 +24,16 @@ beforeAll(() => { afterAll(() => jest.restoreAllMocks()) +function nextFrame() { + return new Promise(resolve => { + requestAnimationFrame(() => { + requestAnimationFrame(() => { + resolve() + }) + }) + }) +} + function renderTemplate(input: string | Partial[0]>) { let defaultComponents = { RadioGroup, RadioGroupOption, RadioGroupLabel, RadioGroupDescription } @@ -454,6 +464,48 @@ 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 () => { + let props = reactive({ hide: false }) + + renderTemplate({ + template: html` + + Option 1 + Option 2 + Option 3 + + `, + setup() { + return { + value: ref('a'), + get hide() { + return props.hide + }, + } + }, + }) + + // Focus the RadioGroup + await press(Keys.Tab) + + props.hide = true + await nextFrame() + + props.hide = false + await nextFrame() + + // 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', () => { diff --git a/packages/@headlessui-vue/src/utils/focus-management.ts b/packages/@headlessui-vue/src/utils/focus-management.ts index f446e84..c2dca21 100644 --- a/packages/@headlessui-vue/src/utils/focus-management.ts +++ b/packages/@headlessui-vue/src/utils/focus-management.ts @@ -97,8 +97,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