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

Would be nice if submit events exposed the "submitter" element #3195

Closed
bzbarsky opened this issue Nov 3, 2017 · 36 comments · Fixed by #4984
Closed

Would be nice if submit events exposed the "submitter" element #3195

bzbarsky opened this issue Nov 3, 2017 · 36 comments · Fixed by #4984
Assignees
Labels
addition/proposal New features or enhancements needs concrete proposal Moving the issue forward requires someone to figure out a detailed plan topic: forms

Comments

@bzbarsky
Copy link
Contributor

bzbarsky commented Nov 3, 2017

See thread starting at https://lists.w3.org/Archives/Public/public-whatwg-archive/2017Oct/0013.html

Properly building the form submission requires knowing the submitter element. See https://html.spec.whatwg.org/#constructing-the-form-data-set and the "submitter" argument it takes.

If someone is trying to do the equivalent from a submit event, they can create a FormData from the form, but there's no way to figure out the submitter element at that point.

Ideally the submit event would expose the submitter element, if any, and the FormData constructor would take it as a second optional argument.

@domenic domenic added addition/proposal New features or enhancements topic: forms needs implementer interest Moving the issue forward requires implementers to express interest labels Nov 3, 2017
@domenic
Copy link
Member

domenic commented Nov 3, 2017

Thanks. This seems pretty reasonable.

I take it you'd be interested in implementing this in Firefox? @tkent-google, our usual Chrome forms person, is out this quarter, so I'm not sure we can give much help from Chrome's side on getting a second implementer. Perhaps @rniwa or @cdumez have thoughts from Safari, or @travisleithead from Edge.

@bzbarsky
Copy link
Contributor Author

bzbarsky commented Nov 3, 2017

I suspect we'd want to at least look at implementing this, yes, if people like the idea.

@unilynx
Copy link

unilynx commented May 30, 2018

This would be great. The current workaround is probably either looking for the focused element (https://stackoverflow.com/questions/2066162/how-can-i-get-the-button-that-caused-the-submit-from-the-form-submit-event) or recording the target of click events until the next tick, just in case a submit event immediately follows the click event.

@tkent-google
Copy link
Contributor

I think this is reasonable. Chrome will implement this if the specification has it.

@annevk annevk added needs concrete proposal Moving the issue forward requires someone to figure out a detailed plan and removed needs implementer interest Moving the issue forward requires implementers to express interest labels May 31, 2018
@paulirwin
Copy link

paulirwin commented Jun 12, 2019

Came here to post this issue if it wasn't already present. Firefox exposes the explicitOriginalTarget property but it is Mozilla-specific. I'd be happy seeing that added to the spec. When you click on a submit button, this property has the reference to the button, whereas originalTarget has a reference to the form. From this you can get the value of the submitted button.

However, it doesn't entirely solve the problem. If we have the following form:

<form>
    <input type="text" name="title" />
    <button type="submit" name="mode" value="publish">Publish</button>
    <button type="submit" name="mode" value="draft">Save as Draft</button>
</form>

And the user hits enter on the text box, it will submit the form with mode=publish as it is the first submit button. But there's no way to get that via JavaScript, because explicitOriginalTarget is the input element in this case.

Edit: I suppose it's possible to check explicitOriginalTarget to see if it's a button, and if not, grab the first button[type=submit] in the form to mimic that behavior.

Another way to solve this problem, which is not mutually exclusive to the above, is to have a toFormData() method on the submit event. Calling the FormData constructor and passing in the form will not have any value for mode in the example above. But if the event had such a toFormData() method, that would return a new FormData with mode set to publish, as it would be submitted "normally" to the server. For example:

// event listener for form submit
function handleSubmit(e) {
    e.preventDefault();
    const formData = e.toFormData();
    if (formData.get("mode") === "draft") {
        ...
    } else {
        ...
    }
}

@annevk
Copy link
Member

annevk commented Jun 14, 2019

@paulirwin you can get that information from the formdata event that was recently added.

Given that this would impact "construct the entry list" should we add submitter to FormDataEvent instead?

The main thing blocking this at this point (aside from deciding where to add it) is a volunteer to drive the work.

@paulirwin
Copy link

Thanks @annevk, did not know about that one. (MDN hasn't even been updated yet, and didn't find a relevant issue here before I posted initially.)

In the docs for "Constructing the entry list", it does not specify if the formdata event is cancelable or not. My understanding (which may be wrong) is that if it's not explicitly cancelable, preventDefault does nothing.

Is the formdata event supposed to be cancelable? If not, then I can't use this event to solve the problem, as typically you would need to prevent submission of the form (i.e. to post the data via Fetch instead). If it is cancelable, then the spec probably needs to be updated.

@annevk
Copy link
Member

annevk commented Jun 14, 2019

Interesting, it's not cancelable by design (the algorithm is also used by the FormData constructor and it failing would not make much sense) and also fires after submit.

I'm not sure why we decided to fire it after submit though, that might have mostly been an artifact from the structure of the algorithm before we made the order observable. @tkent-google what do you think, should we change that?

@tkent-google
Copy link
Contributor

formdata event is dispatched at the end of 'constructing the entry list' so that event handlers can edit entries generated from control elements.
We can't change the order of submit event and 'constructing the entry list'. I have seen real sites that add <input type=hidden> in submit event handlers.

I think it's possible to make formdata event cancelable.

@domenic
Copy link
Member

domenic commented Jun 18, 2019

/cc @muan who if I recall initially wanted formdata to be cancelable, but then found a more elegant way of coding the scenario in question. I am curious if @paulirwin's issue could be similar to what she encountered?

@muan
Copy link
Member

muan commented Jun 18, 2019

@domenic Yes, but we talked about custom elements, not submitters (ref). This is what happens with our remote submit handler:

  1. Catch submit
  2. Prevent default submit (this means the default formdata doesn't happen)
  3. Construct request data with new FormData(form)
    • formdata event is triggered 👈 this was what we realized resolved our issue
  4. Submit with fetch

So come to think of it, this would be an issue for our remote submit handler as well. I guess we won't be able to drop our submit helper yet for form.requestSubmit(submitter). (#4187 (comment))

In "constructing the entry list" it says:

The algorithm to construct the entry list given a form, an optional submitter, and an optional encoding, is as follows. If not specified otherwise, submitter is null.

event.formData from the default formdata event fired from within the submit steps does include the submitter data because of the optional submitter, but if we trigger one via new FormData() it doesn't.

So I guess there are two possibilities:

  1. We get submitter from submit event, and new FormData() takes a submitter.
form.addEventListener('submit', event => {
  event.preventDefault()
  const formData = new FormData(form, event.howeverWeGetSubmitter)
  fetch(form.action, {method: 'post', body: formData})
})
  1. formdata event is cancelable.
form.addEventListener('formdata', event => {
  event.preventDefault()
  fetch(form.action, {method: 'post', body: event.formData})
})

I don't know what implication 2. might have, but I think 1. makes more sense intention-wise and code changes would be more straightforward as we have a lot of other app code listening the submit event.

cc @josh

@muan
Copy link
Member

muan commented Jun 18, 2019

Apologies. My head is clearer now and I see that my example of catching formdata event doesn't make sense for this use case, since it is not an intent to submit.

So I think 1. is probably the way I'd see this work. Thoughts?

@paulirwin
Copy link

Two other options, that aren't necessarily mutually exclusive to those proposed above:

  1. Have a toFormData/getFormData method or formData property getter on the submit event as I proposed earlier (haven't seen a response to that yet). I would say this would be ideal, as then the FormData returned would be exactly as it would be submitted to the server, without having to handle the submitter manually. It could even fire the formdata event when requested. Also this would probably remove the need for adding the second parameter to the FormData constructor, which as you know should have a lot more consideration given to it than adding a method/property, as you can't go back from that easily later.

  2. Add a submitter property to the form itself with a reference to the submitting element, which is cleared after submit is complete. The form already tracks its state of firing events and such, so this seems reasonable.

@annevk
Copy link
Member

annevk commented Jun 20, 2019

@paulirwin 1 does not work as per @tkent-google above as we don't want to collect the form data until after this event, as this event can modify what is being submitted. 2 is rather interesting, but I worry a bit about clearing it appropriately across the various exit points of form submission.

Now, a problem with adding a second parameter to the FormData constructor is that then if we ever added other ways of initialization similar to URLSearchParams and Headers, we'd end up with a second parameter that's only meaningful if the first parameter is a <form>, which is a type of overloading I think we should avoid introducing more of (see w3ctag/design-principles#131). A way around this would be accepting a dictionary, but that could not be distinguished from a record.

So adding state to <form> might be the most reasonable option? Hmm.

@domenic
Copy link
Member

domenic commented Jun 20, 2019

It feels like the last few posts have been based on a false premise. Namely, the idea that the submitter element on submit events would be at all related to the proprietary Firefox-only explicitOriginalTarget, and thus could end up as an input element or similar.

I don't think that's the proposal. The proposal is instead to expose the submitter element, which in the implicit submission case, would be the first submit button (i.e. the one with mode=publish in the given example).

@annevk
Copy link
Member

annevk commented Jun 20, 2019

I agree that explicitOriginalTarget is unrelated, FWIW. The problem is how to expose the submitter element as per @muan's comments above.

@muan
Copy link
Member

muan commented Sep 18, 2019

👋 We recently attempted to add requestSubmit polyfill but had to back out of the change because of this issue.

So adding state to <form> might be the most reasonable option? Hmm. – annevk

This sounds like a good solution to us too. With this approach I think we won't have to change our remote submit handler at all and can just delete our workarounds! 🎩 🐰

We recently open sourced our remote submit handler: https://github.com/github/remote-form. And here's a demonstration for the exact issue we are blocked on (test in Chrome for form.requestSubmit): https://html-is.glitch.me/demo/request-remote-form.html

@domenic
Copy link
Member

domenic commented Sep 23, 2019

@tkent-google, would you have the bandwidth for a spec pull request and tests for this feature? It seems to have multi-implementer interest, as well as web developer interest, and you've been doing great work on forms features recently :).

@muan
Copy link
Member

muan commented Sep 23, 2019

Somewhat relevant: I noticed a Safari specific behavior that seems to resemble what we're talking about here. The behavior is: "FormData constructed in the submit event handler includes the submitter value". Perhaps Safari devs have thoughts on or more details about this?

Please see the demo here: https://html-is.glitch.me/demo/formdata-entries.html

Safari:

GIF showing submitter value is reflected after clicking on submit button

Firefox/Chrome/Edge:

GIF showing submitter value is not reflected after clicking on submit button

@tkent-google
Copy link
Contributor

@tkent-google, would you have the bandwidth for a spec pull request and tests for this feature?

I think I can start to work on this in two weeks.

tkent-google added a commit to tkent-google/html that referenced this issue Oct 8, 2019
It has 'submitter' attribute.
This fixes whatwg#3195
@tkent-google
Copy link
Contributor

Specification PR: #4984

@annevk
Copy link
Member

annevk commented Oct 8, 2019

@tkent-google rereading the discussion here it's not immediately apparent why adding this to the submit event is the way to go.

And also, it would be good to get some insight into the behavior @muan mentions above in Safari. @cdumez perhaps?

@tkent-google
Copy link
Contributor

tkent-google commented Oct 9, 2019

I think adding submitter to submit event (and adding submitter argument to FormData constructor) is the straight-forward solution. It exposes building blocks of form submission algorithm. Making formdata event cancelable looks a tricky workaround to me.

As for the Safari behavior, I think it's just a bug. Chrome inherited it from WebKit, and I fixed it three years ago for a user-reported issue. Adding another state to <form> is not great.

@annevk
Copy link
Member

annevk commented Oct 9, 2019

Thank you, that helps.

The one wrinkle with the mentioned change to FormData's constructor is that if we also wanted to support a union similar to https://url.spec.whatwg.org/#interface-urlsearchparams (as has been requested at times) we'd end up with overloading. I.e., the constructor would support two arguments if the first argument is HTMLFormElement, and one argument otherwise. Generally we try to stay clear of overloading these days as it's not very idiomatic. (FormData does have some already, with its append() and set() methods.)

And the obvious alternative, letting the first argument be a dictionary to specify both HTMLFormElement and HTMLElement, clashes with record<>.

Maybe this is something that only concerns me however.

@tkent-google
Copy link
Contributor

tkent-google commented Oct 11, 2019

Hmm, I don't have an idea of a perfect solution. There are some options.

A) constructor(optional (HTMLFormElement or record<USVString, FormDataEntryValue>) formOrMap, optional HTMLElement submitter)
Idiomatic.

B) constructor(optional (HTMLElement or record<...>) formOrSubmitterOrMap)
HTMLElement represents a form or a submitter.

C) constructor(optional (HTMLFormElement or FormDataInit) formOrDict)

dictionary FormDataInit {
 HTMLFormElement form;
 HTMLElement submitter;
 record<...> map;
}

The content of the dictionary is idiomatic.
Need to wrap a record with a dictionary.
Extensible. It's easy to add new members to the dictionary in the future.

D) constructor(optional (HTMLFormElement or FormDataInit or URLSearchParams) init)

dictionary FormDataInit {
  required HTMLFormElement form;
  HTMLElement submitter;
}

FormData doesn't support record<> directly. Developers have to write new FormData(new URLSearchParams(map)).

E) constructor(optional HTMLFormElement form, optional HTMLElement submitter)
FormData append(record<USVString, FormDataEntryValue> map);
Developers have to write new FormData().append(map).

@annevk
Copy link
Member

annevk commented Oct 11, 2019

A could work, but in the non-<form> case submitter is effectively a noop which is weird, unless we no longer largely delegate to "constructing the entry list".

And E seems somewhat reasonable.

Curious to hear what @muan and @domenic think.

@muan
Copy link
Member

muan commented Oct 11, 2019

@annevk I agree with your assessments.

It's a shame that the Safari behavior is a bug, but I understand that keeping this magic (submitter as a state) in browser is less ideal, and that exposing this would be better for developers.

@tkent-google
Copy link
Contributor

F) (no changes on the constructor)

FormData append(HTMLFormElement form, optional HTMLElement submitter);
FormData append(record<USVString, FormDataEntryValue> map);

Developers have to write let fd = new FormData().append(form, submitter); if they want to collect entries including an entry for the submitter.

@domenic
Copy link
Member

domenic commented Oct 15, 2019

I like A, or overloading. Overloading seems equivalent for authors, and in this particular case, better for spec people/implementers. See https://heycam.github.io/webidl/#idl-overloading-vs-union ; this is one of the few places where overloading is pretty applicable.

Perhaps if we were designing things greenfield, there would be a FormData.fromForm() separate from the constructor. That seems like a nicer separate design than the current one. But we're not in that universe so overloading seems reasonable.

@tkent-google
Copy link
Contributor

Specification PR: #4984

During the review, the following question came up:

If a <form> is submitted without a submit button and dispatches a 'submit' event, should the submitter property of SubmitEvent be null, or the <form>? It happens in the following cases:

  • Implicit form submission with no submit buttons.
  • form.requestSubmit() without an argument.

@annevk
Copy link
Member

annevk commented Oct 29, 2019

Since they are functionally identical null seems simpler to me and closer to the requestSubmit() API we already have. Would developers write code where they pass event.submitter to form.requestSubmit()? Currently null is not an allowed value there (and neither is <form>, to be clear) so maybe we should make that allowed there.

@muan thoughts?

@muan
Copy link
Member

muan commented Oct 29, 2019

I agree that null makes more sense, per the definition of a submitter being not a <form>. And for convenience, accepting form.requestSubmit(null) seems reasonable to me too, but I don't think the alternative (null checking before calling requestSubmit) is too bad.

@domenic
Copy link
Member

domenic commented Oct 29, 2019

I'm happy to do null. But I would like us to change requestSubmit() to accept null too then, so you can do requestSubmit(e.submitter) without awkard null-checking.

@tkent-google
Copy link
Contributor

Ok, I'll update the PR for null.

domenic pushed a commit that referenced this issue Oct 31, 2019
It has a submitter attribute, so this fixes #3195.

As part of this, update requestSubmit() to accept null, so that
requestSubmit(e.submitter) always works. (e.submitter is null for
implicit submission, per the discussion in
#3195 (comment).)

Tests: web-platform-tests/wpt#19562
@muan
Copy link
Member

muan commented Oct 31, 2019

🎉 !!

I think adding submitter to submit event (and adding submitter argument to FormData constructor) is the straight-forward solution. (#3195 (comment))

Looks like #4984 does resolve this current issue. That leaves us still the FormData part of the puzzle. Is that being tracked or worked on somewhere?

@tkent-google
Copy link
Contributor

I filed whatwg/xhr#262 for the submitter on FormData.

zcorpan pushed a commit that referenced this issue Nov 6, 2019
It has a submitter attribute, so this fixes #3195.

As part of this, update requestSubmit() to accept null, so that
requestSubmit(e.submitter) always works. (e.submitter is null for
implicit submission, per the discussion in
#3195 (comment).)

Tests: web-platform-tests/wpt#19562
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs concrete proposal Moving the issue forward requires someone to figure out a detailed plan topic: forms
Development

Successfully merging a pull request may close this issue.

7 participants