fix(plugin-forms): give each embedded form its own element ids - #3031
fix(plugin-forms): give each embedded form its own element ids#3031eisenbruch wants to merge 3 commits into
Conversation
Field ids were built from the form id and the field name only, so embedding the same form twice on a page produced duplicate ids: a label focused the first copy rather than the one beside it, and anything resolving an id reached the wrong instance. Each rendering now carries a per-instance suffix. The honeypot built its id inline in two places and had the same problem, so it goes through fieldId() now as well.
🦋 Changeset detectedLatest commit: 18d5ffa The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the right fix for the reported bug. Generating a per-render instance suffix for element IDs correctly prevents duplicate IDs when the same form is embedded multiple times on a page, and routing the honeypot through fieldId() fixes the last hard-coded ID pair. The name attributes (which the server cares about) are unchanged, and the client script scopes its error lookups to [data-error-for="name"] within the form, so submissions and progressive enhancement keep working.
I checked FormEmbed.astro, Form.astro, the client script, the public definition/schema flow, and the submit handler. All label/input pairs, the honeypot, and hidden/file/select/textarea/checkbox inputs now go through fieldId(); radio and checkbox-group options correctly wrap their labels so they don’t need IDs. The changeset is clear, present-tense, and names the affected package and user-visible symptom.
The only gap is the missing regression test. The PR description already offers to add an experimental_AstroContainer test that renders the same form twice and asserts the resulting element IDs are distinct; I think that small test is worth including so this bug can’t quietly regress. No other blockers.
| /** 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. */ |
There was a problem hiding this comment.
[suggestion] A regression test would guard this fix. The PR description mentions rendering the component twice with experimental_AstroContainer and asserting the IDs are distinct — that test is worth adding, since the current suite has no Astro-component coverage and the fix is otherwise only a string suffix that a future refactor could easily drop.
github-actions
Bot
added
review/awaiting-author
Renders FormEmbed twice through AstroContainer and asserts the id sets are disjoint, that ids are unique within a rendering, and that every label points at an input in its own copy. Fails on the previous fieldId() with "expected [ 'newsletter-email', ...(2) ] to deeply equal []". The package's vitest config goes through astro's getViteConfig so a .astro component can be imported, matching packages/core's repro render tests.
|
Added the regression test you asked for.
It fails on the previous One thing worth flagging for review: the package's |
github-actions
Bot
added
size/M
review/needs-rereview
1 participant
What does this PR do?
Fixes duplicate element IDs when the same form is embedded more than once on a page.
fieldId()built IDs from the form ID and the field name alone:Every rendering of a given form therefore produced the same IDs. Each rendering now carries a short per-instance suffix, so labels, inputs and the honeypot stay paired within their own copy.
The honeypot built its ID inline in two places (
`${formId}-_hp`) and had the same problem, so it goes throughfieldId()now too.Closes #2909
Verified on a live site
A site embedding its newsletter form three times per page — a rail panel, an inline module and a pop-up — serves two IDs three times each on every page:
Clicking the second or third label focuses the first input, which is the reported symptom.
Why this is safe to change
Element IDs are not part of the plugin's public API, and nothing outside the component reads them:
fieldId()is used only insideFormEmbed.astro, and the client script scopes its lookups to the form element (form.querySelector(...),[data-error-for="name"]). The one document-wide lookup, inhandlePopState, matches on[data-ec-form][data-form-id=...]rather than an element ID, so multi-page navigation is unaffected. No tests or e2e selectors reference the ID shape.data-form-iddeliberately still repeats across copies of the same form — that is the form's identity, not an element identity, and changing it is out of scope here.Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.Honest notes on the ticks:
packages/plugins/forms/testscovers schemas, the public definition and the client submit response; there is no harness that rendersFormEmbed.astro, and adding one for this felt like more surface than the fix warrants. If you would like the component rendered underexperimental_AstroContainer— asserting that two renderings of the same form share no element IDs — say so and I will add it.pnpm --filter @emdash-cms/plugin-forms testis 19/19,pnpm lint:jsonreports nothing for the touched file, andpnpm formatis clean.messages.pochanges included.AI-generated code disclosure
Screenshots / test output
No screenshot. The change is invisible — identical markup with different
idattribute values — and the only site I can reproduce it on is a private pre-launch client site. The observable difference is in the HTML: three copies of a form previously emittedid="newsletter-email"three times, and now emit three distinct IDs, with each<label for>matching the input beside it.I have not rendered the patched component. The package ships the
.astrofile as source with no build step, and I could not run it against a realistic page locally. The change is a string suffix and the existing suite passes, but I would rather say that than imply I watched it render.