fix(plugin-forms): give each embedded form its own element ids

.changeset/hungry-pianos-sing.md

@@ -0,0 +1,5 @@
+---
+"@emdash-cms/plugin-forms": patch
+---
+
+Fixes duplicate element IDs when the same form is embedded more than once on a page. Field IDs were built from the form ID and the field name alone, so a form appearing in, say, a sidebar and a pop-up produced several elements sharing an ID: clicking a label focused the first copy rather than the one beside it, and anything resolving an ID — `aria-describedby`, a password manager, a test selector — reached the wrong instance. Each rendering now suffixes its IDs with a per-instance value, so labels, inputs and the honeypot stay paired within their own copy. Element IDs are not part of the plugin's API and nothing else references them; the client script scopes its lookups to the form element.

packages/plugins/forms/src/astro/FormEmbed.astro

@@ -34,9 +34,14 @@ const hasFiles = form.pages.some((p: FormPage) =>
 	p.fields.some((f: FormField) => f.type === "file")
 );
 
-/** Generate an element ID for a field */
+/** Suffix that distinguishes this rendering from any other on the same page. The same form is often embedded more
+ * than once - a sidebar, a footer and a pop-up - and without it every copy repeats the same element ids, so a
+ * label, an aria-describedby or a password manager resolves to the first copy rather than the one being used. */
+const instance = Math.random().toString(36).slice(2, 8);
+
+/** Generate an element ID for a field, unique to this rendering of the form */
 function fieldId(name: string): string {
-	return `${formId}-${name}`;
+	return `${formId}-${name}-${instance}`;
 }
 ---
 
@@ -205,10 +210,10 @@ function fieldId(name: string): string {
 				style="position:absolute;left:-9999px;"
 				aria-hidden="true"
 			>
-				<label for={`${formId}-_hp`}>Leave blank</label>
+				<label for={fieldId("_hp")}>Leave blank</label>
 				<input
 					type="text"
-					id={`${formId}-_hp`}
+					id={fieldId("_hp")}
 					name="_hp"
 					tabindex="-1"
 					autocomplete="off"

packages/plugins/forms/tests/form-embed-ids.render.test.ts

@@ -0,0 +1,80 @@
+/**
+ * Renders FormEmbed twice, the way a page embedding the same form in a rail and a pop-up does, and pins that the
+ * two renderings share no element ids.
+ */
+import { experimental_AstroContainer as AstroContainer } from "astro/container";
+import { beforeAll, describe, expect, test, vi } from "vitest";
+
+import type { PublicFormDefinition } from "../src/public-definition.js";
+
+const definition: PublicFormDefinition = {
+	name: "Newsletter",
+	slug: "newsletter",
+	pages: [
+		{
+			fields: [
+				{
+					id: "email",
+					type: "email",
+					label: "Email",
+					name: "email",
+					required: true,
+					width: "full",
+				},
+				{ id: "name", type: "text", label: "Name", name: "name", required: false, width: "full" },
+			],
+		},
+	],
+	settings: { spamProtection: "honeypot", submitLabel: "Subscribe" },
+	status: "active",
+	_turnstileSiteKey: null,
+};
+
+vi.mock("emdash/plugin-utils", () => ({ getPublicPluginApiRouteHandler: () => undefined }));
+vi.mock("../src/public-definition.js", () => ({
+	loadPublicFormDefinition: () => Promise.resolve(definition),
+}));
+
+const idsIn = (html: string): string[] => Array.from(html.matchAll(/\sid="([^"]+)"/g), (m) => m[1]);
+const labelTargetsIn = (html: string): string[] =>
+	Array.from(html.matchAll(/<label[^>]*\sfor="([^"]+)"/g), (m) => m[1]);
+
+describe("FormEmbed element ids", () => {
+	let first: string;
+	let second: string;
+
+	beforeAll(async () => {
+		const { default: FormEmbed } = await import("../src/astro/FormEmbed.astro");
+		const container = await AstroContainer.create();
+		const render = () =>
+			container.renderToString(FormEmbed, { props: { node: { formId: "newsletter" } } });
+		first = await render();
+		second = await render();
+	});
+
+	test("two renderings of the same form share no element ids", () => {
+		const a = idsIn(first);
+		const b = idsIn(second);
+		expect(a.length).toBeGreaterThan(0);
+		expect(a.filter((id) => b.includes(id))).toEqual([]);
+	});
+
+	test("ids are unique within a rendering", () => {
+		const a = idsIn(first);
+		expect(new Set(a).size).toBe(a.length);
+	});
+
+	test("every label points at an input in its own rendering", () => {
+		for (const [html, which] of [
+			[first, "first"],
+			[second, "second"],
+		] as const) {
+			const ids = new Set(idsIn(html));
+			const targets = labelTargetsIn(html);
+			expect(targets.length, `${which} rendering has labels`).toBeGreaterThan(0);
+			for (const target of targets) {
+				expect(ids.has(target), `${which}: label for="${target}" matches an id`).toBe(true);
+			}
+		}
+	});
+});

packages/plugins/forms/vitest.config.ts

@@ -1,6 +1,6 @@
-import { defineConfig } from "vitest/config";
+import { getViteConfig } from "astro/config";
 
-export default defineConfig({
+export default getViteConfig({
 	test: {
 		globals: true,
 		environment: "node",