Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional submitter argument to FormData constructor #366

Merged
merged 4 commits into from
Jan 27, 2023

Conversation

jenseng
Copy link
Member

@jenseng jenseng commented Jan 12, 2023

Closes #262, I went with the approach outlined here. Implementation/MDN issues have been filed, and some implementation-specific POCs have also been created (chromium, gecko, and jsdom).

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This will require some minor export tweaks to the HTML Standard. I'd be willing to help with that. Making this change still seems reasonable.

xhr.bs Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Jan 12, 2023

@mfreed7 @rniwa @smaug---- tagging you here for visibility and to ensure nothing much changed here since 2019...

jenseng and others added 2 commits January 12, 2023 08:54
@jenseng
Copy link
Member Author

jenseng commented Jan 23, 2023

@mfreed7 if it helps the discussion along, here's a first pass at a blink implementation (caveats: my first time working in Chromium, haven't written C/C++ in a decade, probably needs more testing, etc. 😅). From my quick manual test in the console it seems to be working: submitter-chromium

@jenseng
Copy link
Member Author

jenseng commented Jan 23, 2023

@smaug---- here's the equivalent POC for gecko/FF. Same caveats apply as in the comment above :)

submitter-firefox

@mfreed7
Copy link

mfreed7 commented Jan 23, 2023

@mfreed7 if it helps the discussion along, here's a first pass at a blink implementation (caveats: my first time working in Chromium, haven't written C/C++ in a decade, probably needs more testing, etc. 😅). From my quick manual test in the console it seems to be working:

Wow, this is the best ping I've received: "here's a prototype". Thanks!

I think we're generally still supportive of this proposal and this PR.

@jenseng
Copy link
Member Author

jenseng commented Jan 24, 2023

👋 @rniwa I haven't forgotten about WebKit, I have a comparable patch there too, though I got stuck in XCode/dlopen hell so I haven't been able to test it just yet. Happy to send it your way if it helps.

@cdumez
Copy link

cdumez commented Jan 25, 2023

👋 @rniwa I haven't forgotten about WebKit, I have a comparable patch there too, though I got stuck in XCode/dlopen hell so I haven't been able to test it just yet. Happy to send it your way if it helps.

Looks good to me from WebKit side. I don't mind implementing it and working on getting whatever patch you have landed. This seems like a minor change and I can see how this could be useful.

annevk pushed a commit to whatwg/html that referenced this pull request Jan 25, 2023
@annevk
Copy link
Member

annevk commented Jan 25, 2023

@jenseng I think we're all good here. Just need to wait for HTML to be indexed by Reffy. The only outstanding task is filing implementation bugs (and linking them to this PR and the tests). Would you like to do that as well?

@jenseng
Copy link
Member Author

jenseng commented Jan 25, 2023

@jenseng I think we're all good here. Just need to wait for HTML to be indexed by Reffy. The only outstanding task is filing implementation bugs (and linking them to this PR and the tests). Would you like to do that as well?

Sure thing, I'll do that in a bit here 👍

@@ -1609,7 +1609,7 @@ typedef (File or USVString) FormDataEntryValue;

[Exposed=(Window,Worker)]
interface FormData {
constructor(optional HTMLFormElement form);
constructor(optional HTMLFormElement form, optional HTMLElement? submitter = null);
Copy link
Member

Choose a reason for hiding this comment

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

Found a nit after all. Is there a good reason to allow null here? Or would requiring undefined by fine? If there's no use case for passing null we should not allow it and change the language below from "is non-null" to "is given".

(And then have an additional test that null throws.)

Copy link
Member Author

@jenseng jenseng Jan 25, 2023

Choose a reason for hiding this comment

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

I don't have a strong preference here, mainly I was making it consistent with HTMLFormElement.requestSubmit which allows a null submitter.

But I agree that allowing a null submitter but not a null form is inconsistent, so 🤷‍♂️😆

Copy link
Member

Choose a reason for hiding this comment

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

Okay, that's fairly compelling I suppose, although I don't remember the rationale for that one. 😅

Let's leave this as-is then unless @domenic cares more strongly than I do.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's possible it was an oversight, as you had a similar suggestion when requestSubmit was proposed and afaict it was never directly addressed

Copy link
Member Author

@jenseng jenseng Jan 25, 2023

Choose a reason for hiding this comment

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

Ah here's a good justification for requestSubmit accepting null: whatwg/html#3195 (comment)

Similarly this could be useful for the FormData constructor, e.g. SubmitEvent.submitter could be null, and it would be nice to avoid null checks:

myForm.addEventListener("submit", (e) => {
  e.preventDefault();
  const formData = new FormData(e.target, e.submitter);
  // ...
});

pmeenan pushed a commit to pmeenan/html that referenced this pull request Jan 25, 2023
@jenseng
Copy link
Member Author

jenseng commented Jan 25, 2023

There's one minor ambiguity around Image Button submitters that I'd like to clarify, see this comment on the WPT test PR

@jenseng
Copy link
Member Author

jenseng commented Jan 26, 2023

Seems like the build is failing due to some webdriver-bidi export ambiguity around events, though I'm not sure why it didn't fail before 🤔 ... looks similar to these recent issues (w3c/webdriver-bidi#348, w3c/webdriver-bidi#349). Should I just do some link-defaults stuff in the current PR, or would it be better to get webdriver-bidi fixed?

edited, aha, I see @annevk is already on it

@annevk
Copy link
Member

annevk commented Jan 26, 2023

@jenseng if you file the relevant bugs today I will merge this tomorrow (addressing the webdriver-bidi issue locally). If you don't get around to filing the bugs I might be able to take that on as well. Thanks for all your work on this and guiding me through the design decisions and bug in the HTML Standard. 😊

@jenseng
Copy link
Member Author

jenseng commented Jan 26, 2023

Ok issues have all been filed, thanks for your help on this @annevk!

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 27, 2023
@annevk annevk merged commit 1c2d7f2 into whatwg:main Jan 27, 2023
@jenseng jenseng deleted the form-data-submitter branch January 27, 2023 15:43
@jenseng
Copy link
Member Author

jenseng commented Jan 28, 2023

If anyone needs this in the meantime (or in older browsers), I made a polyfill that supports it with some form shenanigans 🙈

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jan 28, 2023
This patch adds support for the new submitter argument to the FormData constructor.

Spec: https://xhr.spec.whatwg.org/#interface-formdata
Spec PR (merged): whatwg/xhr#366
WPT PR: #37895

Bug: 1410542
Change-Id: If17f782f75ae40ae21241c169afc52761fc89544
jenseng added a commit to jenseng/content that referenced this pull request Jan 28, 2023
Also add a "Submit button" glossary page and link to it from various places
to help facilitate a consistent and correct understanding of what a submit
button is :)

Spec PR (merged): whatwg/xhr#366
Spec: https://xhr.spec.whatwg.org/#interface-formdata
webkit-early-warning-system pushed a commit to jenseng/WebKit that referenced this pull request Jan 30, 2023
https://bugs.webkit.org/show_bug.cgi?id=251220

Reviewed by Chris Dumez

Implement the new submitter parameter to the FormData constructor
Spec PR: whatwg/xhr#366
WPT PR: web-platform-tests/wpt#37895

Test: imported/w3c/web-platform-tests/xhr/formdata/constructor-submitter.html

* LayoutTests/imported/w3c/web-platform-tests/xhr/formdata/constructor-submitter-expected.txt: Added.
* LayoutTests/imported/w3c/web-platform-tests/xhr/formdata/constructor-submitter.html: Added.
* Source/WebCore/html/DOMFormData.cpp:
(WebCore::DOMFormData::create): add submitter support
* Source/WebCore/html/DOMFormData.h: update signature
* Source/WebCore/html/DOMFormData.idl: update interface
* Source/WebCore/html/HTMLFormElement.cpp:
(WebCore::HTMLFormElement::requestSubmit): improve error messages#

Canonical link: https://commits.webkit.org/259558@main
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 1, 2023
…, a=testonly

Automatic update from web-platform-tests
Add FormData constructor submitter tests

For whatwg/xhr#366.
--

wpt-commits: 0986c763928822dd6b62ee7223d8a54827f357c7
wpt-pr: 37895
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 2, 2023
…bidl,smaug

Also improve error messages for current submitter validations

Spec PR: whatwg/xhr#366
WPT PR: web-platform-tests/wpt#37895

Differential Revision: https://phabricator.services.mozilla.com/D167576
aarongable pushed a commit to chromium/chromium that referenced this pull request Feb 3, 2023
This patch adds support for the new submitter argument to the FormData constructor.

Spec: https://xhr.spec.whatwg.org/#interface-formdata
Spec PR (merged): whatwg/xhr#366
WPT PR: web-platform-tests/wpt#37895

Bug: 1410542
Change-Id: If17f782f75ae40ae21241c169afc52761fc89544
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4189297
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1100734}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 3, 2023
This patch adds support for the new submitter argument to the FormData constructor.

Spec: https://xhr.spec.whatwg.org/#interface-formdata
Spec PR (merged): whatwg/xhr#366
WPT PR: #37895

Bug: 1410542
Change-Id: If17f782f75ae40ae21241c169afc52761fc89544
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4189297
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1100734}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Feb 3, 2023
This patch adds support for the new submitter argument to the FormData constructor.

Spec: https://xhr.spec.whatwg.org/#interface-formdata
Spec PR (merged): whatwg/xhr#366
WPT PR: #37895

Bug: 1410542
Change-Id: If17f782f75ae40ae21241c169afc52761fc89544
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4189297
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1100734}
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Feb 3, 2023
…, a=testonly

Automatic update from web-platform-tests
Add FormData constructor submitter tests

For whatwg/xhr#366.
--

wpt-commits: 0986c763928822dd6b62ee7223d8a54827f357c7
wpt-pr: 37895
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Feb 3, 2023
…bidl,smaug

Also improve error messages for current submitter validations

Spec PR: whatwg/xhr#366
WPT PR: web-platform-tests/wpt#37895

Differential Revision: https://phabricator.services.mozilla.com/D167576
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Feb 5, 2023
…ata constructor, a=testonly

Automatic update from web-platform-tests
Add optional submitter argument to FormData constructor

This patch adds support for the new submitter argument to the FormData constructor.

Spec: https://xhr.spec.whatwg.org/#interface-formdata
Spec PR (merged): whatwg/xhr#366
WPT PR: web-platform-tests/wpt#37895

Bug: 1410542
Change-Id: If17f782f75ae40ae21241c169afc52761fc89544
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4189297
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1100734}

--

wpt-commits: 181fc870aeae110d95537bac969f76caa55d5112
wpt-pr: 38242
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Feb 7, 2023
…ata constructor, a=testonly

Automatic update from web-platform-tests
Add optional submitter argument to FormData constructor

This patch adds support for the new submitter argument to the FormData constructor.

Spec: https://xhr.spec.whatwg.org/#interface-formdata
Spec PR (merged): whatwg/xhr#366
WPT PR: web-platform-tests/wpt#37895

Bug: 1410542
Change-Id: If17f782f75ae40ae21241c169afc52761fc89544
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4189297
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1100734}

--

wpt-commits: 181fc870aeae110d95537bac969f76caa55d5112
wpt-pr: 38242
jenseng added a commit to jenseng/jsdom that referenced this pull request Feb 17, 2023
In order to fully support it, this requires more robust Image Button
support (i.e. track selected coordinate, use when constructing form
data set)

Spec: https://xhr.spec.whatwg.org/#interface-formdata
Spec PR: whatwg/xhr#366
jenseng added a commit to jenseng/content that referenced this pull request Mar 9, 2023
Also add a "Submit button" glossary page and link to it from various places
to help facilitate a consistent and correct understanding of what a submit
button is :)

Spec PR (merged): whatwg/xhr#366
Spec: https://xhr.spec.whatwg.org/#interface-formdata
jenseng added a commit to jenseng/content that referenced this pull request Mar 12, 2023
Also add a "Submit button" glossary page and link to it from various places
to help facilitate a consistent and correct understanding of what a submit
button is :)

Spec PR (merged): whatwg/xhr#366
Spec: https://xhr.spec.whatwg.org/#interface-formdata
Elchi3 pushed a commit to mdn/content that referenced this pull request Mar 14, 2023
* Document the FormData constructor submitter parameter

Also add a "Submit button" glossary page and link to it from various places
to help facilitate a consistent and correct understanding of what a submit
button is :)

Spec PR (merged): whatwg/xhr#366
Spec: https://xhr.spec.whatwg.org/#interface-formdata

* Fix some wording, add a link

* Tweak submitter wording/links, add text around form* attrs

* Add release note entry

* Remove tags
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 28, 2023
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 28, 2023
This patch adds support for the new submitter argument to the FormData constructor.

Spec: https://xhr.spec.whatwg.org/#interface-formdata
Spec PR (merged): whatwg/xhr#366
WPT PR: #37895

Bug: 1410542
Change-Id: If17f782f75ae40ae21241c169afc52761fc89544
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4189297
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Mason Freed <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1100734}
jenseng added a commit to jenseng/jsdom that referenced this pull request Apr 10, 2023
In order to fully support it, this requires more robust Image Button
support (i.e. track selected coordinate, use when constructing form
data set).

This feature is implemented in the latest versions of Chrome/Firefox/
Safari, where the newly enabled WPTs are also passing.

Spec: https://xhr.spec.whatwg.org/#interface-formdata
Spec PR: whatwg/xhr#366
domenic pushed a commit to jsdom/jsdom that referenced this pull request May 27, 2023
This was recently added to the standard in whatwg/xhr#366.
facebook-github-bot pushed a commit to facebook/flow that referenced this pull request May 10, 2024
Summary:
This has been widely available for a [while](https://caniuse.com/?search=FormData.submitter) now

References:
- https://xhr.spec.whatwg.org/#interface-formdata
- https://developer.mozilla.org/en-US/docs/Web/API/FormData/FormData
- whatwg/xhr#366 (review) (discussion on why null is allowed)

Pull Request resolved: #9145

Reviewed By: SamChou19815

Differential Revision: D57181660

fbshipit-source-id: 98502daf0a26d361820739a90607e7a2d1275d66
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this pull request Jun 1, 2024
…, a=testonly

Automatic update from web-platform-tests
Add FormData constructor submitter tests

For whatwg/xhr#366.
--

wpt-commits: 0986c763928822dd6b62ee7223d8a54827f357c7
wpt-pr: 37895
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

FormData: Add ability to specify submitter in addition to <form>
4 participants