`ActionForm` does NOT respect `on:sumbit` event

[sabify]

Describe the bug
ActionForm's on:sumbit event will not be called prior to the ActionForm's inner on_sumbit closure. (broken from 0.7)

Leptos Dependencies

The latest commit (b45f982)

To Reproduce

1. Modify the ActionFrom in leptos/src/form.rs:

    let on_submit = {
        move |ev: SubmitEvent| {
            if ev.default_prevented() {
                crate::logging::log!("---1");
                return;
            }

            ev.prevent_default();

            crate::logging::log!("---2");

            match ServFn::from_event(&ev) {
                Ok(new_input) => {
                    action.dispatch(new_input);
                }
                Err(err) => {
                    crate::logging::error!(
                        "Error converting form field into server function \
                         arguments: {err:?}"
                    );
                    action.value().set(Some(Err(
                        ServerFnErrorErr::Serialization(err.to_string())
                            .into_app_error(),
                    )));
                    action.version().update(|n| *n += 1);
                }
            }
        }
    };

2. Modify the WithActionFrom in server_fn_axum example:

#[component]
pub fn WithActionForm() -> impl IntoView {
    let action = ServerAction::<AddRow>::new();
    let row_count =
        Resource::new(move || action.version().get(), |_| get_rows());

    view! {
        <h3>Using <code>"<ActionForm/>"</code></h3>
        <p>
            <code>"<ActionForm/>"</code>
            "lets you use an HTML "
            <code>"<form>"</code>
            "to call a server function in a way that gracefully degrades."
        </p>
        <ActionForm
            action
            on:sumbit=move |ev: SubmitEvent| {
                leptos::logging::log!("+++1");
                ev.prevent_default();
                leptos::logging::log!("+++2");
            }
        >
            <input
                // the `name` of the input corresponds to the argument name
                name="text"
                placeholder="Type something here."
            />
            <button>Submit</button>
        </ActionForm>
        <p>You submitted: {move || format!("{:?}", action.input().get())}</p>
        <p>The result was: {move || format!("{:?}", action.value().get())}</p>
        <Transition>
            archive underaligned: need alignment 4 but have alignment 1
            <p>Total rows: {row_count}</p>
        </Transition>
    }
}

3. Run the example and submit an input in Using <ActionFrom>.
4. See the log in the console, you will see that only ---2 is logged which is inside ActionFrom and the example's on:submit has been ignored.

Next Steps
[ ] I will make a PR
[ x ] I would like to make a PR, but need help getting started
[ x ] I want someone else to take the time to fix this
[ ] This is a low priority for me and is just shared for your information

[gbj]

Two notes:

1. There's a typo for submit in the repro step 2 (where it's spelled sumbit instead of submit), which is why the event handler is completely ignored. If it's spelled submit you'll see that the on:submit does fire. Nevertheless it fires after the ActionForm's internal submit handler, since the internal one is added first, so it still doesn't work as you'd like with the prevent_default()
2. However, this can already be fixed. See Disabling event delegation by default breaks client-side validation examples #3457, feat: add :capture flag for events to handle them during capture phase (closes #3457) #3575 — you can now use the :capture modifier to specify that an event handler should run during the capture phase, which means you can intercept the event on the way "down" the DOM tree before it bubbles "up" and is caught by the ActionForm. So using on:submit:capture here allows you successfully to call prevent_default()

I am happy to leave this open as a documentation issue, as I'm pretty sure this isn't documented anywhere other than release notes.

[sabify]

@gbj Thanks for the explanation.

From a developer experience perspective, shouldn't capture be the default behavior and then be disableable via nocapture, as we intentionally add the handler to run first most of the time? The current default behavior is a source of confusion.

(I'm asking this as a point for reconsidering the default behavior, although it might be a little late...)

[gbj]

Just to clarify, to make sure we are on the same page: "bubble" and "capture" are DOM/browser terms, and the default when adding an event listener is for it to be added during the "bubble" phase.

The current system all follows very simple rules, because event listeners work exactly how they work when using ordinary browser APIs:

1. Event listeners are triggered during the bubble phase by default, but you can opt into the capture phase instead
2. Adding multiple event listeners to an element causes them to fire in the order that you added them
3. Adding an event listener to a component simply adds the event listener each of the top-level elements returned by that component
4. .prevent_default() or .stop_propagation() prevent further defaults or propagation, but (of course) don't retroactively prevent event listeners that have already fired

So, <ActionForm on:submit/> ends up returning a form with two submit handlers (the internal one and the user-added one) which run in order, during the bubble phase; <ActionForm on:submit:capture/> returns a form with two submit handlers, one of which runs during the capture phase (on the way "down" through the DOM), which allows the user to intercept the event.

The only reasonable place I see to change this would be to change 3) such that event listeners a user adds to a component are added before event listeners added by the component itself. Off the top of my head this seems like it would add a bunch of complexity to some of the relevant rendering traits, but maybe it's a good idea, I don't know.

[sabify]

Ah, I see. Thanks for the clarification. I believe we need to keep following browser APIs.

[paul-hansen]

This almost bit me when upgrading from 0.6. Forms started submitting before my confirmation dialogs are shown.

With normal html/js showing a confirmation dialog on a form is simply:
https://stackoverflow.com/a/6515632/5399098

<form onsubmit="return confirm('Do you really want to submit the form?');">

But the equivalent in Leptos, which used to work, now will show the dialog after the form is submitted instead of before:

let confirm = move |ev: SubmitEvent| {
    let success = window()
        .confirm_with_message("Do you really want to submit the form?")
        .unwrap_or_default();
    if !success {
        ev.prevent_default();
    }
};
view!{
    <ActionForm action=restore_action on:submit=confirm>
         /// form goes here
    </ActionForm>
}

Using on:submit:capture works but seems odd to default to behavior that is different than the browser. I'm thinking about creating a lint for my project to prevent any accidental on:submit that isn't on:submit:capture as it seems likely not what I would ever want.

Maybe just the default behavior of on:submit should be changed or something?

Just sharing my personal experience/expectations, welcome to ignore it if you still disagree.

[gbj]

Using on:submit:capture works but seems odd to default to behavior that is different than the browser.

I just want to clarify that the changed behavior comes from Leptos getting rid of a whole event delegation system that was different from how the browser works, and using the browser's default behavior instead. Specifically the fact that ActionForm already added a submit handler to the <form> it creates, and you've added a second one, and the browser runs them in order as they are both in the bubble phase (the default).

So the parallel example is not this

<form onsubmit="return confirm('Do you really want to submit the form?');">

it's this

const form = document.createElement("form");
form.addEventListener("submit", () => { /* do ActionForm things */ });
form.addEventListener("submit", (ev) => {
  // ...
  if(!success) {
    ev.preventDefault();
  }
});

Because the browser's default behavior is to run event listeners in order, the first one runs and submits the form, and then the second one runs and tries to call preventDefault() to stop the first one, but the first one has already run.

It's possible this pattern should really be re-worked and ActionForm should take a before_submit prop or something.

[paul-hansen]

Makes sense, essentially there's nothing special about ActionForm's on:{event} handlers, it just adds to the event stack but ActionForm has already added it's handler causing it to run first.

The result is that on:submit provided by users of ActionForm basically becomes a post submit event. Which creates two problems (imo):

1. Users might not realize this, causing unexpected bugs.
2. on:input becomes useless for most things people would have used <form onsubmit=submit/> for.

Adding a before_submit would solve problem 2 and would be a slight improvement for 1, it doesn't stop people from using on:input without realizing, but at least when they read the docs they would likely see it and know what to do then.

Could it make sense instead for ActionForm to add it's event handler last (maybe in an Effect if needed)? Since it is handling submitting the form which would be the last thing you want to happen and it is essentially replicating/replacing the default behavior of the form element's submit.

[gbj]

Could it make sense instead for ActionForm to add it's event handler last (maybe in an Effect if needed)? Since it is handling submitting the form which would be the last thing you want to happen and it is essentially replicating/replacing the default behavior of the form element's submit.

This is a reasonable idea, and in fact led me to what I think is a good solution in #4130: it simply delays running the body of the submit handler built in to ActionForm by a tick, before checking whether the event was default-prevented, which means that if a user on:submit handler ran and called ev.prevent_default() it will stop there.