diff --git a/src/lib/components/listbox/Listbox.svelte b/src/lib/components/listbox/Listbox.svelte index d316997..2d8aa2d 100644 --- a/src/lib/components/listbox/Listbox.svelte +++ b/src/lib/components/listbox/Listbox.svelte @@ -155,7 +155,31 @@ searchQuery = ""; }, registerOption(id: string, dataRef) { - options = [...options, { id, dataRef }]; + if (!$optionsRef) { + // We haven't mounted yet so just append + options = [...options, { id, dataRef }]; + return; + } + let currentActiveOption = + activeOptionIndex !== null ? options[activeOptionIndex] : null; + + let orderMap = Array.from( + $optionsRef.querySelectorAll('[id^="headlessui-listbox-option-"]')! + ).reduce( + (lookup, element, index) => + Object.assign(lookup, { [element.id]: index }), + {} + ) as Record; + + let nextOptions = [...options, { id, dataRef }]; + nextOptions.sort((a, z) => orderMap[a.id] - orderMap[z.id]); + options = nextOptions; + + // Maintain the correct item active + activeOptionIndex = (() => { + if (currentActiveOption === null) return null; + return options.indexOf(currentActiveOption); + })(); }, unregisterOption(id: string) { let nextOptions = options.slice(); diff --git a/src/lib/components/listbox/listbox.test.ts b/src/lib/components/listbox/listbox.test.ts index c5f495a..8f8f1d1 100644 --- a/src/lib/components/listbox/listbox.test.ts +++ b/src/lib/components/listbox/listbox.test.ts @@ -6,7 +6,7 @@ import { ListboxOptions, } from "."; import { suppressConsoleLogs } from "$lib/test-utils/suppress-console-logs"; -import { render } from "@testing-library/svelte"; +import { act, render } from "@testing-library/svelte"; import TestRenderer from "$lib/test-utils/TestRenderer.svelte"; import { assertActiveElement, @@ -48,6 +48,7 @@ import Button from "$lib/internal/elements/Button.svelte"; import Div from "$lib/internal/elements/Div.svelte"; import Span from "$lib/internal/elements/Span.svelte"; import svelte from "svelte-inline-compile"; +import { writable } from "svelte/store"; let mockId = 0; jest.mock('../../hooks/use-id', () => { @@ -192,7 +193,7 @@ describe('Rendering', () => { describe('ListboxLabel', () => { it( - 'should be possible to render a ListboxLabel using a render prop', + 'should be possible to render a ListboxLabel using slot props', suppressConsoleLogs(async () => { render(svelte` @@ -295,7 +296,7 @@ describe('Rendering', () => { ) it( - 'should be possible to render a ListboxButton using a render prop and an `as` prop', + 'should be possible to render a ListboxButton using slot props and an `as` prop', suppressConsoleLogs(async () => { render(svelte` @@ -510,6 +511,61 @@ describe('Rendering', () => { }) }) ) + + it('should guarantee the listbox option order after a few unmounts', async () => { + let showFirst = writable(false); + render(svelte` + + Trigger + + {#if $showFirst} + Option A + {/if} + Option B + Option C + + + `) + + assertListboxButton({ + state: ListboxState.InvisibleUnmounted, + attributes: { id: 'headlessui-listbox-button-1' }, + }) + assertListbox({ state: ListboxState.InvisibleUnmounted }) + + // Open Listbox + await click(getListboxButton()) + + let options = getListboxOptions() + expect(options).toHaveLength(2) + options.forEach(option => assertListboxOption(option)) + + // Make the first option active + await press(Keys.ArrowDown) + + // Verify that the first listbox option is active + assertActiveListboxOption(options[0]) + + // Now add a new option dynamically + await act(() => showFirst.set(true)); + + // New option should be treated correctly + options = getListboxOptions() + expect(options).toHaveLength(3) + options.forEach(option => assertListboxOption(option)) + + // Focused option should now be second + assertActiveListboxOption(options[1]) + + // We should be able to go to the first option + await press(Keys.Home) + assertActiveListboxOption(options[0]) + + // And the last one + await press(Keys.End) + assertActiveListboxOption(options[2]) + + }) }) }) diff --git a/src/lib/components/menu/Menu.svelte b/src/lib/components/menu/Menu.svelte index 6e89044..4ef10b7 100644 --- a/src/lib/components/menu/Menu.svelte +++ b/src/lib/components/menu/Menu.svelte @@ -111,7 +111,31 @@ searchQuery = ""; }, registerItem(id: string, data: MenuItemData) { - items.push({ id, data }); + if (!$itemsStore) { + // We haven't mounted yet so just append + items = [...items, { id, data }]; + return; + } + let currentActiveItem = + activeItemIndex !== null ? items[activeItemIndex] : null; + + let orderMap = Array.from( + $itemsStore.querySelectorAll('[id^="headlessui-menu-item-"]')! + ).reduce( + (lookup, element, index) => + Object.assign(lookup, { [element.id]: index }), + {} + ) as Record; + + let nextItems = [...items, { id, data }]; + nextItems.sort((a, z) => orderMap[a.id] - orderMap[z.id]); + items = nextItems; + + // Maintain the correct item active + activeItemIndex = (() => { + if (currentActiveItem === null) return null; + return items.indexOf(currentActiveItem); + })(); }, unregisterItem(id: string) { let nextItems = items.slice(); @@ -137,7 +161,7 @@ ...obj, menuState, buttonStore, - itemsStore: itemsStore, + itemsStore, items, searchQuery, activeItemIndex, diff --git a/src/lib/components/menu/menu.test.ts b/src/lib/components/menu/menu.test.ts index 8d76604..b18cb12 100644 --- a/src/lib/components/menu/menu.test.ts +++ b/src/lib/components/menu/menu.test.ts @@ -1,5 +1,5 @@ import { assertActiveElement, assertMenu, assertMenuButton, assertMenuButtonLinkedWithMenu, assertMenuItem, assertMenuLinkedWithMenuItem, assertNoActiveMenuItem, getByText, getMenu, getMenuButton, getMenuButtons, getMenuItems, getMenus, MenuState } from "$lib/test-utils/accessibility-assertions"; -import { render } from "@testing-library/svelte"; +import { act, render } from "@testing-library/svelte"; import { Menu, MenuButton, MenuItem, MenuItems } from "."; import { suppressConsoleLogs } from "$lib/test-utils/suppress-console-logs"; import TestRenderer from "$lib/test-utils/TestRenderer.svelte"; @@ -11,6 +11,7 @@ import Div from "$lib/internal/elements/Div.svelte"; import Form from "$lib/internal/elements/Form.svelte"; import Span from "$lib/internal/elements/Span.svelte"; import svelte from "svelte-inline-compile"; +import { writable } from "svelte/store"; let mockId = 0; jest.mock('../../hooks/use-id', () => { @@ -327,6 +328,61 @@ describe('Rendering', () => { }) }) ) + + it('should guarantee the menu item order after a few unmounts', async () => { + let showFirst = writable(false); + render(svelte` + + Trigger + + {#if $showFirst} + Item A + {/if} + Item B + Item C + + + `) + + assertMenuButton({ + state: MenuState.InvisibleUnmounted, + attributes: { id: 'headlessui-menu-button-1' }, + }) + assertMenu({ state: MenuState.InvisibleUnmounted }) + + // Open Listbox + await click(getMenuButton()) + + let items = getMenuItems() + expect(items).toHaveLength(2) + items.forEach(item => assertMenuItem(item)) + + // Make the first item active + await press(Keys.ArrowDown) + + // Verify that the first menu item is active + assertMenuLinkedWithMenuItem(items[0]) + + // Now add a new option dynamically + await act(() => showFirst.set(true)); + + // New option should be treated correctly + items = getMenuItems() + expect(items).toHaveLength(3) + items.forEach(item => assertMenuItem(item)) + + // Active item should now be second + assertMenuLinkedWithMenuItem(items[1]) + + // We should be able to go to the first option + await press(Keys.Home) + assertMenuLinkedWithMenuItem(items[0]) + + // And the last one + await press(Keys.End) + assertMenuLinkedWithMenuItem(items[2]) + + }) }) }) diff --git a/src/lib/components/radio-group/RadioGroup.svelte b/src/lib/components/radio-group/RadioGroup.svelte index 2ca21fa..2a491b1 100644 --- a/src/lib/components/radio-group/RadioGroup.svelte +++ b/src/lib/components/radio-group/RadioGroup.svelte @@ -86,7 +86,22 @@ return true; }, registerOption(action: Option) { - options = [...options, action]; + if (!radioGroupRef) { + // We haven't mounted yet so just append + options = [...options, action]; + return; + } + let orderMap = Array.from( + radioGroupRef.querySelectorAll('[id^="headlessui-radiogroup-option-"]')! + ).reduce( + (lookup, element, index) => + Object.assign(lookup, { [element.id]: index }), + {} + ) as Record; + + let newOptions = [...options, action]; + newOptions.sort((a, z) => orderMap[a.id] - orderMap[z.id]); + options = newOptions; }, unregisterOption(id: Option["id"]) { options = options.filter((radio) => radio.id !== id); diff --git a/src/lib/components/radio-group/radio-group.test.ts b/src/lib/components/radio-group/radio-group.test.ts index d63a596..a60a5ef 100644 --- a/src/lib/components/radio-group/radio-group.test.ts +++ b/src/lib/components/radio-group/radio-group.test.ts @@ -124,8 +124,7 @@ describe('Rendering', () => { assertNotFocusable(getByText('Dine in')) }) - // TODO: fix this test! - it.skip('should guarantee the radio option order after a few unmounts', async () => { + it('should guarantee the radio option order after a few unmounts', async () => { render(svelte`