naja-js / naja

Modern AJAX library for Nette Framework
https://naja.js.org
MIT License
109 stars 16 forks source link

buttons in forms prevents submit #401

Closed lubomirblazekcz closed 2 months ago

lubomirblazekcz commented 5 months ago

Bug Report

Before Naja 3.1.0 it was possible to send form via naja (button without type="submit") and process custom submit event listeners on form.

But now naja prevents all form submissions on every button type except type="button". Is this a good solution? Wouldn't be better if the button only sent new CustomEvent('submit', { cancelable: true }) onto form and let the form submission proceed naturally?

My usecase is that I have submit event listener on form that checks errors like this and prevents submission https://getbootstrap.com/docs/5.3/forms/validation/ or https://winduum.dev/docs/components/form.html

Or recaptcha submit event listener on form, that checks recaptcha score. https://cloud.google.com/recaptcha-enterprise/docs/instrument-web-pages#user-action

Naja prevents the form submission on buttons, so these listeners never execute. Current workaround is to use type="button" and add execute custom submit event on form so the events bubble naturally.

Is there any particular reason why naja binds event listeners on form buttons? There is already an event listener on form that listens for submit event?

And yes you could write an extension for validation or recaptcha, but I don't like that solution. You should be able to easy enhance current form functionality with naja without such workarounds.

filiphlm commented 4 months ago

Is there any particular reason why naja binds event listeners on form buttons? There is already an event listener on form that listens for submit event?

Well, this was/is pretty common practice because the submitter has to somehow get into the data submitted by the (ajx) form, but you're right, Naja does it in a way that makes the standard submit event almost irrelevant.

Although it has a fairly straightforward solution (SubmitEvent.submitter and the submitter parameter of the FormData constructor), it may not be to everyone's liking. It would introduce several BC breaks in the UIHandler), besides I am not able to figure it out without using event delegation – judge for yourself, no polyfills included.

There is quite decent cross-browser support for FormData(form, submitter), and it's really "guaranteed to work in the latest versions of Chromium (Chrome and Edge), Firefox, and WebKit (Safari)" :wink:, for the rest (Chrome/Edge 112<, Safari 16.4<, Firefox 111<) there is a polyfill. If you also need to support Safari 15.4<, there's another polyfill for SubmitEvent.submitter. (Or all together.)

@jiripudil : Should I prepare a PR request or is it a bit too much? I understand that this can be quite controversial, but personally I'd rather put up with a working polyfill of an established standard temporarily than a complex workaround(s) that might have unwanted side effects.

jiripudil commented 4 months ago

Hi, yep, this is mainly for historical reasons. Today, with SubmitEvent.submitter and FormData(data, submitter) (and form.requestSubmit()) widely available (and polyfill-able if needed), Naja could listen solely to form.submit, and if the form or the submitter matches the configured selector, process the form submission asynchronously.

I might need to think this through more thoroughly, but I think that if UIHandler uses the submitter for the element in the interaction event, this could even come without BC breaks :thinking:

filiphlm commented 4 months ago

You're right, maybe we can get by with a pretty minimal edit:

UIHandler.ts#L53-L58

if (element.tagName === 'FORM') {
    bindForm(element as HTMLFormElement);
} else {
    const forms = element.querySelectorAll('form');
    forms.forEach((form) => bindForm(form as HTMLFormElement));
}

(Well, it's not exactly ideal, but I can't see a better solution.)

UIHandler.ts#L75-L77

if (event.type === 'submit') {
    const {submitter} = (event as SubmitEvent);
    if ((element as HTMLFormElement).matches(this.selector) || submitter?.matches(this.selector)) {
        this.submitForm(element as HTMLFormElement, options, event).catch(ignoreErrors);
    }

UIHandler.ts#L116-L121

public async submitForm(form: HTMLFormElement, options: Options = {}, event?: SubmitEvent): Promise<Payload> {
    const submitter = event?.submitter;
    const method = (submitter?.getAttribute('formmethod') || form.getAttribute('method') || 'GET').toUpperCase();
    const url = submitter?.getAttribute('formaction') ?? form.getAttribute('action') ?? window.location.pathname + window.location.search;
    const data = new FormData(form, submitter);

    return this.processInteraction(submitter || form, method, url, data, options, event);
}
jiripudil commented 2 months ago

I've just released these changes in 3.2.0, thanks for your thoughts and @filiphlm for the PR!