whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
7.98k stars 2.61k forks source link

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

Closed bzbarsky closed 4 years ago

bzbarsky commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

unilynx commented 6 years ago

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 commented 6 years ago

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

paulirwin commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

/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 commented 5 years ago

@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). (https://github.com/whatwg/html/issues/4187#issuecomment-440846710)

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 https://github.com/w3ctag/design-principles/issues/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 commented 5 years ago

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 commented 5 years ago

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

muan commented 4 years ago

👋 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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

Specification PR: https://github.com/whatwg/html/pull/4984

annevk commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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:

annevk commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Ok, I'll update the PR for null.

muan commented 4 years ago

🎉 !!

I think adding submitter to submit event (and adding submitter argument to FormData constructor) is the straight-forward solution. (https://github.com/whatwg/html/issues/3195#issuecomment-539799163)

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 commented 4 years ago

I filed https://github.com/whatwg/xhr/issues/262 for the submitter on FormData.