fix: Select/Combobox type="multiple" FormData (#961)

huntabyte · Nov 22, 2024 · c53d31a · c53d31a
1 parent 0d8c899
commit c53d31a
Show file tree

Hide file tree

Showing 7 changed files with 159 additions and 100 deletions.
diff --git a/.changeset/young-actors-wave.md b/.changeset/young-actors-wave.md
@@ -0,0 +1,5 @@
+---
+"bits-ui": patch
+---
+
+fix: only submit values with `FormData` on Combobox/Select `type="multiple"` if a value is selected to align with native `<select multiple>` behavior
diff --git a/packages/bits-ui/src/lib/bits/combobox/components/combobox.svelte b/packages/bits-ui/src/lib/bits/combobox/components/combobox.svelte
@@ -73,9 +73,7 @@
 </FloatingLayer>
 
 {#if Array.isArray(rootState.value.current)}
-	{#if rootState.value.current.length === 0}
-		<ListboxHiddenInput value="" />
-	{:else}
+	{#if rootState.value.current.length}
 		{#each rootState.value.current as item}
 			<ListboxHiddenInput value={item} />
 		{/each}

diff --git a/packages/bits-ui/src/lib/bits/select/components/select.svelte b/packages/bits-ui/src/lib/bits/select/components/select.svelte
@@ -73,9 +73,7 @@
 </FloatingLayer>
 
 {#if Array.isArray(rootState.value.current)}
-	{#if rootState.value.current.length === 0}
-		<SelectHiddenInput value="" />
-	{:else}
+	{#if rootState.value.current.length}
 		{#each rootState.value.current as item}
 			<SelectHiddenInput value={item} />
 		{/each}

diff --git a/packages/tests/src/tests/combobox/combobox-multi-test.svelte b/packages/tests/src/tests/combobox/combobox-multi-test.svelte
@@ -21,6 +21,7 @@
 		inputProps?: WithoutChildrenOrChild<Combobox.InputProps>;
 		items: Item[];
 		searchValue?: string;
+		onFormSubmit?: (fd: FormData) => void;
 	};
 </script>
 
@@ -34,6 +35,7 @@
 		searchValue = "",
 		inputProps,
 		onOpenChange,
+		onFormSubmit,
 		...restProps
 	}: ComboboxMultipleTestProps = $props();
 
@@ -45,58 +47,71 @@
 </script>
 
 <main data-testid="main">
-	<Combobox.Root
-		bind:value
-		bind:open
-		type="multiple"
-		{...restProps}
-		onOpenChange={(v) => {
-			onOpenChange?.(v);
-			if (!v) searchValue = "";
+	<form
+		data-testid="form"
+		method="POST"
+		onsubmit={(e) => {
+			e.preventDefault();
+			const formData = new FormData(e.target as HTMLFormElement);
+			onFormSubmit?.(formData);
 		}}
 	>
-		<Combobox.Trigger data-testid="trigger">Open combobox</Combobox.Trigger>
-		<Combobox.Input
-			data-testid="input"
-			aria-label="open combobox"
-			oninput={(e) => (searchValue = e.currentTarget.value)}
-			{...inputProps}
-		/>
-		<Combobox.Portal {...portalProps}>
-			<Combobox.Content data-testid="content" {...contentProps}>
-				<Combobox.Group data-testid="group">
-					<Combobox.GroupHeading data-testid="group-label">Options</Combobox.GroupHeading>
-					{#each filteredItems as { value, label, disabled }}
-						<Combobox.Item data-testid={value} {disabled} {value} {label}>
-							{#snippet children({ selected, highlighted: _highlighted })}
-								{#if selected}
-									<span data-testid="{value}-indicator">x</span>
-								{/if}
-								{label}
-							{/snippet}
-						</Combobox.Item>
-					{/each}
-				</Combobox.Group>
-			</Combobox.Content>
-		</Combobox.Portal>
-	</Combobox.Root>
-	<div data-testid="outside"></div>
-	<button data-testid="input-binding" onclick={() => (searchValue = "")}>
-		{#if searchValue === ""}
-			empty
-		{:else}
-			{searchValue}
-		{/if}
-	</button>
-	<button data-testid="open-binding" onclick={() => (open = !open)}>
-		{open}
-	</button>
-	<button data-testid="value-binding" onclick={() => (value = [])}>
-		{#if value.length === 0}
-			empty
-		{:else}
-			{value}
-		{/if}
-	</button>
+		<Combobox.Root
+			bind:value
+			bind:open
+			type="multiple"
+			{...restProps}
+			onOpenChange={(v) => {
+				onOpenChange?.(v);
+				if (!v) searchValue = "";
+			}}
+		>
+			<Combobox.Trigger data-testid="trigger">Open combobox</Combobox.Trigger>
+			<Combobox.Input
+				data-testid="input"
+				aria-label="open combobox"
+				oninput={(e) => (searchValue = e.currentTarget.value)}
+				{...inputProps}
+			/>
+			<Combobox.Portal {...portalProps}>
+				<Combobox.Content data-testid="content" {...contentProps}>
+					<Combobox.Group data-testid="group">
+						<Combobox.GroupHeading data-testid="group-label"
+							>Options</Combobox.GroupHeading
+						>
+						{#each filteredItems as { value, label, disabled }}
+							<Combobox.Item data-testid={value} {disabled} {value} {label}>
+								{#snippet children({ selected, highlighted: _highlighted })}
+									{#if selected}
+										<span data-testid="{value}-indicator">x</span>
+									{/if}
+									{label}
+								{/snippet}
+							</Combobox.Item>
+						{/each}
+					</Combobox.Group>
+				</Combobox.Content>
+			</Combobox.Portal>
+		</Combobox.Root>
+		<div data-testid="outside"></div>
+		<button type="button" data-testid="input-binding" onclick={() => (searchValue = "")}>
+			{#if searchValue === ""}
+				empty
+			{:else}
+				{searchValue}
+			{/if}
+		</button>
+		<button type="button" data-testid="open-binding" onclick={() => (open = !open)}>
+			{open}
+		</button>
+		<button type="button" data-testid="value-binding" onclick={() => (value = [])}>
+			{#if value.length === 0}
+				empty
+			{:else}
+				{value}
+			{/if}
+		</button>
+		<button data-testid="submit" type="submit"> Submit </button>
+	</form>
 </main>
 <div data-testid="portal-target" id="portal-target"></div>
diff --git a/packages/tests/src/tests/combobox/combobox.test.ts b/packages/tests/src/tests/combobox/combobox.test.ts
@@ -75,6 +75,7 @@ function setupMultiple(props: Partial<ComboboxMultipleTestProps> = {}, items: It
 	const openBinding = returned.getByTestId("open-binding");
 	const valueBinding = returned.getByTestId("value-binding");
 	const outside = returned.getByTestId("outside");
+	const submit = returned.getByTestId("submit");
 
 	function getHiddenInputs(name = "test") {
 		return returned.container.querySelectorAll<HTMLElement>(`input[name="${name}"]`);
@@ -91,6 +92,7 @@ function setupMultiple(props: Partial<ComboboxMultipleTestProps> = {}, items: It
 		openBinding,
 		valueBinding,
 		outside,
+		submit,
 		getHiddenInputs,
 		getContent,
 		...returned,
@@ -466,9 +468,9 @@ describe("combobox - multiple", () => {
 		expect(input).toHaveValue("B");
 	});
 
-	it("should render a hidden input if the `name` prop is passed", async () => {
+	it("should not render a hidden input if the `name` prop is passed and a value is not selected", async () => {
 		const { getHiddenInputs } = setupMultiple();
-		expect(getHiddenInputs()).toHaveLength(1);
+		expect(getHiddenInputs()).toHaveLength(0);
 	});
 
 	it("should render a hidden input for each value in the `value` array, each with the same `name` prop", async () => {
@@ -536,12 +538,12 @@ describe("combobox - multiple", () => {
 
 	it("should not portal if `disabled` is passed as portal prop", async () => {
 		const { content, getByTestId } = await openMultiple({ portalProps: { disabled: true } });
-		const main = getByTestId("main");
-		expect(content.parentElement?.parentElement).toBe(main);
+		const form = getByTestId("form");
+		expect(content.parentElement?.parentElement).toBe(form);
 	});
 
 	it("should respect binding the `open` prop", async () => {
-		const { getContent, getByTestId, user, openBinding } = await openMultiple();
+		const { getContent, user, openBinding } = await openMultiple();
 		expect(openBinding).toHaveTextContent("true");
 		await user.click(openBinding);
 		expect(openBinding).toHaveTextContent("false");
@@ -699,6 +701,19 @@ describe("combobox - multiple", () => {
 		const [_, item2] = getItems(getByTestId);
 		expectSelected(item2!);
 	});
+
+	it("should submit an empty array when the user submits the form without selecting any items", async () => {
+		let submittedValues: string[] | undefined;
+		const { submit, user } = setupMultiple({
+			onFormSubmit: (fd) => {
+				submittedValues = fd.getAll("themes") as string[];
+			},
+			name: "themes",
+		});
+
+		await user.click(submit);
+		expect(submittedValues).toHaveLength(0);
+	});
 });
 
 function getItems(getter: AnyFn, items = testItems) {

diff --git a/packages/tests/src/tests/select/select-multi-test.svelte b/packages/tests/src/tests/select/select-multi-test.svelte
@@ -18,6 +18,7 @@
 		items: Item[];
 		searchValue?: string;
 		onSelectedLabelChange?: (value: string) => void;
+		onFormSubmit?: (fd: FormData) => void;
 	};
 </script>
 
@@ -30,6 +31,7 @@
 		open = false,
 		searchValue = "",
 		onSelectedLabelChange,
+		onFormSubmit,
 		...restProps
 	}: SelectMultipleTestProps = $props();
 
@@ -53,39 +55,50 @@
 </script>
 
 <main data-testid="main">
-	<Select.Root bind:value bind:open {...restProps} type="multiple">
-		<Select.Trigger data-testid="trigger">
-			{selectedLabel}
-		</Select.Trigger>
+	<form
+		data-testid="form"
+		method="POST"
+		onsubmit={(e) => {
+			e.preventDefault();
+			const formData = new FormData(e.target as HTMLFormElement);
+			onFormSubmit?.(formData);
+		}}
+	>
+		<Select.Root bind:value bind:open {...restProps} type="multiple">
+			<Select.Trigger data-testid="trigger">
+				{selectedLabel}
+			</Select.Trigger>
 
-		<Select.Portal {...portalProps}>
-			<Select.Content data-testid="content" {...contentProps}>
-				<Select.Group data-testid="group">
-					<Select.GroupHeading data-testid="group-label">Options</Select.GroupHeading>
-					{#each filteredItems as { value, label, disabled }}
-						<Select.Item data-testid={value} {disabled} {value} {label}>
-							{#snippet children({ selected, highlighted: _highlighted })}
-								{#if selected}
-									<span data-testid="{value}-indicator">x</span>
-								{/if}
-								{label}
-							{/snippet}
-						</Select.Item>
-					{/each}
-				</Select.Group>
-			</Select.Content>
-		</Select.Portal>
-	</Select.Root>
-	<div data-testid="outside"></div>
-	<button data-testid="open-binding" onclick={() => (open = !open)}>
-		{open}
-	</button>
-	<button data-testid="value-binding" onclick={() => (value = [])}>
-		{#if value.length === 0}
-			empty
-		{:else}
-			{value}
-		{/if}
-	</button>
+			<Select.Portal {...portalProps}>
+				<Select.Content data-testid="content" {...contentProps}>
+					<Select.Group data-testid="group">
+						<Select.GroupHeading data-testid="group-label">Options</Select.GroupHeading>
+						{#each filteredItems as { value, label, disabled }}
+							<Select.Item data-testid={value} {disabled} {value} {label}>
+								{#snippet children({ selected, highlighted: _highlighted })}
+									{#if selected}
+										<span data-testid="{value}-indicator">x</span>
+									{/if}
+									{label}
+								{/snippet}
+							</Select.Item>
+						{/each}
+					</Select.Group>
+				</Select.Content>
+			</Select.Portal>
+		</Select.Root>
+		<div data-testid="outside"></div>
+		<button type="button" data-testid="open-binding" onclick={() => (open = !open)}>
+			{open}
+		</button>
+		<button type="button" data-testid="value-binding" onclick={() => (value = [])}>
+			{#if value.length === 0}
+				empty
+			{:else}
+				{value}
+			{/if}
+		</button>
+		<button data-testid="submit" type="submit"> Submit </button>
+	</form>
 </main>
 <div data-testid="portal-target" id="portal-target"></div>
diff --git a/packages/tests/src/tests/select/select.test.ts b/packages/tests/src/tests/select/select.test.ts
@@ -72,6 +72,7 @@ function setupMultiple(props: Partial<SelectMultipleTestProps> = {}, items: Item
 	const openBinding = returned.getByTestId("open-binding");
 	const valueBinding = returned.getByTestId("value-binding");
 	const outside = returned.getByTestId("outside");
+	const submit = returned.getByTestId("submit");
 
 	function getHiddenInputs(name = "test") {
 		return returned.container.querySelectorAll<HTMLElement>(`input[name="${name}"]`);
@@ -87,6 +88,7 @@ function setupMultiple(props: Partial<SelectMultipleTestProps> = {}, items: Item
 		openBinding,
 		valueBinding,
 		outside,
+		submit,
 		getHiddenInputs,
 		getContent,
 		...returned,
@@ -499,9 +501,9 @@ describe("select - multiple", () => {
 		expect(trigger).not.toHaveAttribute("data-placeholder");
 	});
 
-	it("should render a hidden input if the `name` prop is passed", async () => {
+	it("should not render a hidden input if the `name` prop is passed and a value is not selected", async () => {
 		const { getHiddenInputs } = setupMultiple();
-		expect(getHiddenInputs()).toHaveLength(1);
+		expect(getHiddenInputs()).toHaveLength(0);
 	});
 
 	it("should render a hidden input for each value in the `value` array, each with the same `name` prop", async () => {
@@ -569,12 +571,12 @@ describe("select - multiple", () => {
 
 	it("should not portal if `disabled` is passed as portal prop", async () => {
 		const { content, getByTestId } = await openMultiple({ portalProps: { disabled: true } });
-		const main = getByTestId("main");
-		expect(content.parentElement?.parentElement).toBe(main);
+		const form = getByTestId("form");
+		expect(content.parentElement?.parentElement).toBe(form);
 	});
 
 	it("should respect binding the `open` prop", async () => {
-		const { getContent, getByTestId, user, openBinding } = await openMultiple();
+		const { getContent, user, openBinding } = await openMultiple();
 		expect(openBinding).toHaveTextContent("true");
 		await user.click(openBinding);
 		expect(openBinding).toHaveTextContent("false");
@@ -727,6 +729,19 @@ describe("select - multiple", () => {
 		const [_, item2] = getItems(getByTestId);
 		expectSelected(item2!);
 	});
+
+	it("should submit an empty array when the user submits the form without selecting any items", async () => {
+		let submittedValues: string[] | undefined;
+		const { submit, user } = setupMultiple({
+			onFormSubmit: (fd) => {
+				submittedValues = fd.getAll("themes") as string[];
+			},
+			name: "themes",
+		});
+
+		await user.click(submit);
+		expect(submittedValues).toHaveLength(0);
+	});
 });
 
 function getItems(getter: AnyFn, items = testItems) {