Skip to content

fix: a submitter's empty formmethod/formenctype/formtarget wins, as native - #1352

Merged
vivek7405 merged 3 commits into
mainfrom
test/submit-bail-ladder
Aug 8, 2026
Merged

fix: a submitter's empty formmethod/formenctype/formtarget wins, as native#1352
vivek7405 merged 3 commits into
mainfrom
test/submit-bail-ladder

Conversation

@vivek7405

@vivek7405 vivek7405 commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

Closes #1322

Summary

The client router's onSubmit is a ladder of guards, each declining a submission and handing it back to the browser. Nine unit tests claimed to pin those bails and none of them could fail. In that harness there is no location global, so onSubmit throws a ReferenceError at new URL(action, location.href) and the bare catch swallows it; stub location and the next wall is new FormData(formElement), which throws under linkedom because the constructor's WebIDL brand check rejects a linkedom element. Either way preventDefault() was unreachable for every input, so an ordinary POST the router DOES intercept looked exactly like a bail. Demonstrated rather than argued: deleting the data-no-router rung outright left all 222 tests in that file green.

Every rung now lives in a real browser, in packages/core/test/routing/browser/submit-bail-ladder.test.js, one test per rung, each pairing a bail fixture with a near-miss control that differs by exactly the attribute that trips the rung. A submit probe on window bubble reads e.defaultPrevented, which is a direct read of the router's decision about that event, and probe.seen.length is asserted so "no submission happened at all" cannot pass. Rung 7 borrows Turbo's own assertion for the same rung: the <dialog> really closed.

Writing the ladder turned up a real bug, fixed here. A submitter's formmethod / formenctype / formtarget override the form on PRESENCE, never on the value being truthy. getSubmitAction already did that, which is why a present-but-empty formaction="" correctly means submit-to-self; the three siblings used a || chain, so an empty value was falsy and fell through to the form's. All three diverged from every engine, and all three are fixed. The three new tests are written so the engine's own IDL reflection (button.formMethod and friends) is asserted alongside the router's behaviour, so the native oracle is in the test rather than quoted from a measurement made elsewhere.

What changed

  • New packages/core/test/routing/browser/submit-bail-ladder.test.js: the floor, rungs 2 to 11, and the three empty-attribute precedence tests. Sixteen tests, green on Chromium, Firefox and WebKit.
  • Fixed packages/core/src/router-client.js: getSubmitMethod, getSubmitEnctype, and the target chain in onSubmit now resolve on hasAttribute, mirroring getSubmitAction.
  • Deleted the nine vacuous onSubmit tests in router-client.test.js, the fakeSubmitEvent helper nothing else used, and the note that described half the problem. The section header now explains where the ladder went and why it cannot live there. The resolver tests stay and gained two rows for the empty-attribute rule.
  • Moved the text/plain bail out of form-action-submit.test.js into the ladder, where it has a control. That file's ENCODING and dev-guard tests are untouched.
  • Synced the docs site and corrected the formtarget claim. formtarget is a plain string reflection with no enumerated states, so an empty value has no invalid-value default to fall to; it selects the current navigable. website/app/docs/client-router/page.ts listed the auto-skip as "target / formtarget not _self", which the presence fix makes false for formtarget="". packages/core/AGENTS.md now records the one nav-guard exception rung 7 takes, with both its conditions and the second-channel obligation it does not buy out.
  • Fixed a Firefox flake the move exposed. A routed submission's swap is async, so tearing the boundary comments out from under one still in flight makes it degrade after the nav guard has restored the real hard-navigate seam, which is a genuine page reload and aborts the whole web-test-runner session. The moved test's own tick had been acting as an accidental buffer between its two neighbours. Both files now settle in teardown before dismantling. Measured: form-action-submit.test.js on Firefox failed roughly 1 run in 3 without this and is 5 for 5 with it, while origin/main is 4 for 4 (so the flake was latent, not pre-existing in effect).

Per-rung counterfactual

Deleting each rung's return in router-client.js, one at a time, and running the ladder file:

Rung Result
2 defaultPrevented 15 passed, 1 failed: rung 2 only
3 target not a FORM uncaught error, file-wide (see below)
4 form data-no-router 15 passed, 1 failed: rung 4 only
5 submitter data-no-router 15 passed, 1 failed: rung 5 only
6 target not _self 15 passed, 1 failed: rung 6 only
7 method dialog 15 passed, 1 failed: rung 7 only
8 unparseable action uncaught error, file-wide (see below)
9 cross-origin 15 passed, 1 failed: rung 9 only
10 non-HTML extension 15 passed, 1 failed: rung 10 only
11 text/plain 14 passed, 2 failed: the rung's two tests (11a and 11b), no others

Rungs 3 and 8 are the two a later line DEPENDS on rather than merely a decision the router makes, so deleting either turns onSubmit into a throw instead of a wrong verdict, and web-test-runner reports an uncaught page error across the file rather than against one test. For rung 3 that throw is TypeError: Failed to construct 'FormData': parameter 1 is not of type 'HTMLFormElement', which is worth knowing on its own: without that rung a stray dispatchEvent(new Event('submit')) in app code takes out the page. The wide blast radius is the honest signal there, and #1322 anticipated it for rung 8.

Run on Chromium; the rung logic reads attributes and urls with no engine-specific behaviour, and the file itself is green on all three.

Docs

  • AGENTS.md, the client-navigation paragraph: the enctype precedence claim now says it is decided on presence and names the invalid-value-default consequence.
  • .agents/skills/webjs/references/routing-and-pages.md: the same rule, stated for all three submitter overrides.

Test plan

  • npx wtr packages/core/test/routing/browser/submit-bail-ladder.test.js: 16/16 on Chromium, Firefox and WebKit
  • npx wtr packages/core/test/routing/browser/form-action-submit.test.js: 13/13, and 5 consecutive clean Firefox runs
  • npm run test:browser: 838 Chromium, 828 Firefox, 838 WebKit, 0 failed
  • npm test: 4106 of 4112 locally. The 5 failures are all test/bun/listener.test.mjs, which asserts the Bun listener's redirect path, fails identically on a clean main checkout on this machine, and is green in CI. Local Node version, not this diff.
  • npx webjs check: clean on examples/blog and on website
  • Per-rung counterfactual, all ten, table above
  • Vacuity counterfactual: deleting the data-no-router rung left all 222 unit tests green, which is the reason for the deletion

Bun parity: N/A. router-client.js runs only in a browser and nothing in the SSR, listener, action, or serializer path imports it, so Bun never executes it.
E2E: N/A. The ladder is a client-side decision observable entirely in the browser layer.

…ative

The form-submission algorithm resolves a submitter's override on whether the
attribute is PRESENT, never on its value being truthy. `getSubmitAction`
already did that, so a present-but-empty `formaction=""` correctly means
submit-to-self. The three siblings used a `||` chain instead, so an empty
value was falsy and silently fell through to the form's.

Measured against Chromium, Firefox and WebKit at the request level: a
`<button type="submit" formmethod="" formenctype="">` inside a
`<form method="post" enctype="multipart/form-data" action="/submit">`
submits natively as `GET /submit?a=1` with no body, while the router resolved
it as a multipart POST. Same template, two different requests with JS on and
with JS off, which is the class of divergence #1307 exists to eliminate.

Found while pinning the onSubmit bail ladder for #1322.
@vivek7405 vivek7405 self-assigned this Aug 8, 2026
A routed submission's swap is async, so tearing the boundary comments out
from under one still in flight makes it degrade after the nav guard has
already restored the real hard-navigate seam. That is a genuine page reload,
which aborts the whole web-test-runner session rather than failing one test.

It surfaced on Firefox in form-action-submit.test.js once the text/plain bail
moved out to the ladder file: that test's own tick had been acting as an
accidental buffer between its two neighbours. Both files now settle first.

Also records what the per-rung counterfactual actually looks like for rungs 3
and 8, which are load-bearing for a later line rather than merely a decision,
so deleting either turns onSubmit into a throw.
@vivek7405

Copy link
Copy Markdown
Collaborator Author

Design rationale: why the probe reads window bubble, and the one rung it cannot carry

The whole file hinges on where the probe listens. Window bubble is the last step of the propagation path, so it runs after the router's own document-bubble listener no matter which of the two was registered first, and e.defaultPrevented read there is a direct read of what the router decided about that event. That is what makes a bail assertion positive: false means the browser is about to perform the submission natively, which is exactly what every rung claims. A listener on the container would have run BEFORE the router and set defaultPrevented itself, so the router would return at its first line and the test would pass without the router ever making the decision it claims to measure. The probe also has to be installed BEFORE the nav guard, since both sit on window bubble and the guard's own preventDefault() would otherwise be what the probe reads.

Rung 2 is the one rung the probe cannot carry, and it is worth stating why rather than leaving the odd-looking assertion block to be discovered. That rung's whole subject is an event the USER already prevented, so defaultPrevented is true at the probe for both halves of the pair and says nothing about who did it. What separates them there is the fetch: the bail form's handler runs and the router stays out, the control has no handler and is routed. Deleting the rung makes the bail half issue a fetch, so it is still non-vacuous, just measured on a different channel.

On the issue's rejected option 1, making buildSubmitFormData total: leaving it alone turned out to be right for a reason beyond the one recorded on #1322. The new FormData(x) throw is not only a linkedom artifact, it is also what stops a stray dispatchEvent(new Event('submit')) from reaching a FormData constructor with a <div>. Deleting rung 3 and running the ladder shows it directly, as TypeError: Failed to construct 'FormData': parameter 1 is not of type 'HTMLFormElement'. A total fallback would have swallowed that into a silent wrong submission instead.

formtarget is a plain string reflection with no enumerated states, so an
empty value has no invalid-value default to fall to; it selects the current
navigable under the rules for choosing a navigable. The root AGENTS.md
parenthetical lumped it in with formmethod and formenctype and so contradicted
the code comment it was describing.

The docs site listed the auto-skip as "target / formtarget not _self", which
after the presence fix is wrong for formtarget="", and stated submitter
precedence without the presence qualifier the fix made load-bearing.

Also records the one sanctioned nav-guard exception in packages/core/AGENTS.md,
with both conditions and the second-channel obligation it does not buy out, and
drops the _onSubmit test binding the deleted ladder tests left behind.

@vivek7405 vivek7405 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the whole diff against the ladder it replaces. The source fix is right: all three resolvers now match the form-submission algorithm's presence test and line up with getSubmitAction and with the renderer's own ABSENT sentinel, and the new tests assert the engine's IDL reflection alongside the router's behaviour, so the native oracle is in the test rather than quoted from somewhere else. The ladder's probe ordering is the load-bearing part of the whole file and it is correct.

What I would keep an eye on is the docs half, which is where everything I found sits. The presence rule is a fiddly one to state, and the root file stated it wrong for formtarget, the one of the three that is not an enumerated attribute. The docs site was not touched at all and carried a line this change makes false. There was also a leftover binding from the deletion, and rung 7's nav-guard exception is real but was unrecorded in the rule it deviates from.

All four are fixed in 459c0af0; the per-line threads say which is which.

Comment thread AGENTS.md
Comment thread website/app/docs/client-router/page.ts
Comment thread website/app/docs/client-router/page.ts
Comment thread website/app/docs/progressive-enhancement/page.ts
Comment thread packages/core/test/routing/router-client.test.js
Comment thread packages/core/AGENTS.md
@vivek7405
vivek7405 marked this pull request as ready for review August 8, 2026 17:51
@vivek7405
vivek7405 merged commit 0206464 into main Aug 8, 2026
10 checks passed
@vivek7405
vivek7405 deleted the test/submit-bail-ladder branch August 8, 2026 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: pin each onSubmit bail to its own rung, not just "not routed"

1 participant