fix(content): submit set_field forms exactly once via requestSubmit - #2814
Conversation
|
@alectimison-maker is attempting to deploy a commit to the esokullu's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Reviewed this alongside #2808, #2809, #2813 and #2815. The double-submit this fixes is real, but the fix removes the synthetic Enter entirely for non-combobox fields inside a form, and that Enter was load-bearing for a whole class of pages. Before: the Enter trio always fired, and Where that breaks: Form with no submit handler. An SPA search or chat widget wraps its input in a bare Inputs that transform the value on Enter. Tag-chips and invite-by-email fields turn the typed text into a chip on Enter, then submit. Now the form posts the raw uncommitted value, or posts with zero recipients. Firefox contenteditable. Firefox handles contenteditable inline in Related, and independent of which branch runs: A narrower fix for the original bug: keep the Enter trio, and skip Two notes on the tests:
Minor: |
89de526 to
1b542e9
Compare
…age did not handle Enter Removing the synthetic Enter broke bare-form pages, value-transforming inputs (tag chips, invite-by-email), and Firefox contenteditable composers that commit only through their own keydown listener. Dispatch the Enter trio always; if dispatchEvent returns false the page already handled Enter, so skip requestSubmit to avoid a double send. requestSubmit now checks form validity first so an invalid form surfaces a clear failure with submitted:false instead of silently aborting, and the success result reports submitted:true when a native submit ran.
1b542e9 to
63c3a28
Compare
|
Reworked per review. The Enter trio was load-bearing, so the fix now keeps it:
Note the original |
webbrain-one
left a comment
There was a problem hiding this comment.
Blocking correctness issue remains in the single-submit contract. Please use one deliberate submission path, or observe an actual submit event before deciding whether a fallback is safe.
| // composers only commit through their own keydown listener. If | ||
| // the page cancelled the keydown it already handled Enter, so a | ||
| // second submit would double-send. | ||
| const enterHandled = !dispatchKey('keydown', 'Enter', 13); |
There was a problem hiding this comment.
[P1] Do not treat key cancellation as proof of submission
dispatchEvent() returning false only says that some listener called preventDefault(); it does not prove that the listener submitted. Conversely, a listener may submit on Enter without cancelling, in which case this code calls requestSubmit() and submits again. Cancellation therefore cannot guarantee exactly one action on checkout, message, or other consequential forms. Choose one submission path, or observe an actual submission event before falling back. The Firefox mirror has the same issue.
There was a problem hiding this comment.
Fixed in de929536. The Enter trio remains for page-owned transforms and Firefox contenteditable, but cancellation is no longer treated as proof. The handler now temporarily observes the form submit event: a page-side submit prevents the native fallback, a native requestSubmit() is used only when no submit was observed, and submitted: true is emitted only when an uncancelled submit event was observed. Cancelled/unobserved paths return outcomeUnknown: true; invalid forms still return the explicit invalid failure. Chrome and Firefox are mirrored. Regression coverage now includes page-submit-without-cancel (no double submit), cancelled keydown, cancelled submit, native submit, combobox, and invalid form. node test/run.js: 1772 passed.
There was a problem hiding this comment.
Updated through 3bb0fe13, on top of the maintainer-directed one-path fix 7976786f. Ordinary form controls now use only requestSubmit(); comboboxes, contenteditable, and form-less widgets use only the page-owned Enter path, so an unobservable form.submit() cannot trigger a second native fallback. The submit observer now stores the event and evaluates defaultPrevented after dispatch/requestSubmit, so bubble-phase cancellation is not reported as success. The validation gate respects form.noValidate, and tests cover native/page-owned paths, direct form.submit(), cancelled submit, contenteditable, invalid, and novalidate forms. node test/run.js: 1772 passed.
|
Current-head compatibility note after the exact-once rework: The current implementation deliberately chooses one path: plain form-backed inputs use The remaining tradeoff is compatibility. An otherwise ordinary form input whose page behavior lives only in its own Enter handler will no longer receive that key event. Custom SPA search/chat fields and tag/email-chip inputs wrapped in forms may therefore skip their page-owned transform or commit path. Restoring Enter followed by a conditional fallback would reintroduce the consequential double-submit ambiguity, so this is a rollout/canary caveat rather than a request for that design. Before broad rollout, browser-level coverage for a plain native form, an input-level Enter-only form, a tag/chip transform, and a contenteditable composer would make the boundary explicit. |
…o review-2814 # Conflicts: # src/chrome/src/content/content.js # src/firefox/src/content/content.js # test/run.js
|
Reviewed the current-head compatibility note. The one-path design remains intentional: plain form controls use native |
Summary
set_field({ submit: true })on a regular (non-combobox) field now submits through one trusted path:form.requestSubmit().Motivation
The old path dispatched synthetic
KeyboardEvents (neverisTrusted, so never producing native default actions) and then calledform.requestSubmit():requestSubmit()fires a second native submit. On POST forms (login, checkout, send) this duplicates posts/orders/messages.press_keys(content.js: "native browser default actions are only guaranteed for trusted events"), but this path still leaned on an untrusted Enter for submission semantics.Design
Introduced
_setFieldSubmitMode(isCombobox, form)returning the single commit path:combobox→synthetic-enter(page JS commits the picker; submitting the enclosing form while a popup is open is usually wrong — preserved rationale)requestSubmit→requestSubmitonly (trusted native submission fires the form's JS handlers and the browser's own submit exactly once)synthetic-enter(the only mechanism that reaches JS-listener-only submit handlers)Testing
node test/run.js— 1764 passed, 0 failed (1 new test, both builds)npm run test:security— 60/60 passednpm run test:toolbar-guard— 33 passedNew test covers the decision matrix (combobox / form / form-less) plus source-structure guards: the native branch contains no Enter dispatch, the fallback branch contains no
requestSubmit(), and ArrowDown stays gated onisCombobox.Compatibility and risks
Scope