I find it a little strange that we'd be looking for a form inside maybe_redirect.

I imagine it'd be more duplication to do this in the places where we're submitting forms, but I wonder if we could find a better design for this?

I'd image there are places where we're dealing with a form (probably through a phx-change) where we already have the form data structure. Maybe we could check for a Form.phx_trigger_action? there? (similar to how we do it with other attributes like that)

Actually, I think the phx-trigger-action is a bit like a client side redirect.

Two things to consider:

• phx-trigger-action can be set on any re-render, not just when clicking a submit button for a form
• phx-trigger-action can be set on any form, not necessarily the one the user is interacting with

I agree that the common use case is:

1. User submits form
2. handle_event("save" assigns trigger_submit: true
3. Browser performs HTTP POST

But I think we should limit the implementation to that common use case.

I think its best to mimmick LiveView JS to support all kinds of uses cases:
Whenever something changed, check the HTML for forms with phx-trigger-action.
If present, then submit that form.

That's what I tried to to in this PR.

We could separate maybe_redirect and maybe_trigger_action.
Do you think that would help?

Commit e2dd5b4

I separated them into two separate post processing steps.
I tried to keep both functions completely separate by introducing a token flow.

Feels over-engineered. Maybe just renaming maybe_redirect to maybe_trigger_form_action_or_redirect would be good enough?

Feels over-engineered. Maybe just renaming maybe_redirect to maybe_trigger_form_action_or_redirect would be good enough?

Yeah, I agree. I appreciate you taking the time to try a different approach to see if we landed at something better. But it does seem to introduce more confusion. I think your original maybe_redirect implementation was best -- especially since you explained how trigger_action works. I thought it was more limited.

I think its best to mimmick LiveView JS to support all kinds of uses cases:

I think that's a good north star for sure. As we evaluate what to include and not include, I think there are two levels that I like to keep in mind:

1. PhoenixTest not lie to users -- so, tests shouldn't pass if production is going to break.
2. It's less bad for tests to break when production would pass (e.g. maybe we don't have a complete implementation). Ideally, we can match production, but I think this should always be secondary to (1).

All that being said, any chance to revert the last commit, and I think we might be good to go? Do you think it'd be helpful to write test cases for the scenarios you mentioned? (when a different event happens and the trigger-action still happens?)

Reverted and added tests.

Also stumbled over #143.

I have a question here. Do you know what happens in a normal LiveView if a form has something like a phx-change that redirects you somewhere and it has a phx-trigger-action? Which redirect "wins out"?

In this case, we're giving "priority" to the phx-trigger-action, which I imagine is the right call since most times I'd expect phx-change not to be doing the redirecting. But I'm curious if we know what happens in those scenarios (and if we should even concern ourselves with them, or if it's just an edge case that we shouldn't cover).

I think you maybe have misread the code. Or I'm wrong. Either way, I'm adding a test to clarify.

My understanding:
This implementation already prioritizes redirects or patches.
phx-trigger-action is only followed if no patch or redirect happened.
I'm pretty sure this is also what LiveView does.

These two clauses match first:

defp maybe_redirect({:error, {:redirect, %{to: path}}}, session) do
defp maybe_redirect({:error, {:live_redirect, %{to: path}}} = result, session) do

So, given

<form phx-trigger-action={@trigger_submit} phx-change="redirect">

handle_event("redirect", _, socket) do
  socket
  |> push_navigate(to: "/elsewhere")
  |> assign(:trigger_submit: true)
end

a change to the form will cause a redirect to /elsewhere. And ignore the phx-trigger-action.

Double checked the actual behaviour:

• redirect: wins over phx-trigger-action (✅ this PR is ok)
• navigate: wins over phx-trigger-action (✅ this PR is ok)
• patch: is executed before phx-trigger-action, both are applied (✅ this PR is ok)

So I think we're fine.

You're right in that phx-trigger-action is given priority over maybe_put_patch_path.
But if there's a phx-trigger-action, there's no point in updating the session.current_path, because we're about to POST to another path anyway.

Once again, running the same test against an actual browser as well as the Live driver would give peace of mind here :).

I would imagine our implementation here is good. But I haven't done stuff with phx-trigger-action, so want to ask. Do you know if we should be using LiveViewTest's follow_trigger_action function to get the method and data to send?

Looking at the follow_trigger_action, we should be fine.
It does the same things we're doing (ensure phx-trigger-action attr present, then use method and action to HTTP submit the form).

Should we use follow_trigger_action anyway?

Maybe.

In future the implementations might drift apart (theoretically, not saying it's likely).
Using follow_trigger_action under the hood would prevent that.

No, rather be consistent with how we submit static forms from LiveViews

We have a precedent in PhoenixTest: static form submits from a LiveView (without phx-trigger-action).

For that we could use LiveViewTest.submit_form. Which uses the same helper function as LiveViewTest.follow_trigger_action.

For the sake of simplicity (my interpretation) we don't do that and rather just call the Static driver directly.

🎉

👍