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

ViewTransitions form submission behavior does not match browser-native behavior #9447

Closed
1 task done
knpwrs opened this issue Dec 16, 2023 · 10 comments · Fixed by #9466
Closed
1 task done

ViewTransitions form submission behavior does not match browser-native behavior #9447

knpwrs opened this issue Dec 16, 2023 · 10 comments · Fixed by #9466
Labels
- P2: has workaround Bug, but has workaround (priority) feat: view transitions Related to the View Transitions feature (scope)

Comments

@knpwrs
Copy link
Contributor

knpwrs commented Dec 16, 2023

Astro Info

Astro                    v4.0.6
Node                     v18.18.0
System                   Linux (x64)
Package Manager          npm
Output                   static
Adapter                  none
Integrations             none

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

When submitting a form with method="POST" with the <ViewTransitions /> component, Astro submits the form as Content-Type: multipart/form-data, whereas the browser default behavior without JavaScript is to submit as Content-Type: application/x-www-form-urlencoded.

In the linked repo, I have the server log out await Astro.request.text() on the server side. It looks like this:

image

When you disable JavaScript, or just do this same thing on a non-astro project, the log looks like this:

image

See also the request header with JavaScript enabled:

image

Vs JavaScript disabled:

image

What's the expected result?

The ViewTransitions progressive enhancement should submit forms as Content-Type: application/x-www-form-urlencoded.

Here is where Remix handles this in their codebase: https://github.com/remix-run/remix/blob/e26ddc724e78b9cf48110cd4f854516a7f1a87a2/packages/remix-react/data.ts#L95

Note that it might be hard to disable JavaScript while using StackBlitz. The easiest way to reproduce this bug is to clone the repo locally: https://github.com/knpwrs/astro-view-transition-post-bug

Link to Minimal Reproducible Example

https://stackblitz.com/~/github.com/knpwrs/astro-view-transition-post-bug

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Dec 16, 2023
@knpwrs
Copy link
Contributor Author

knpwrs commented Dec 16, 2023

Aside from compatibility, the default browser behavior allows for some cool stuff with advanced forms. See this from the Remix docs: https://remix.run/docs/en/main/guides/faq#how-can-i-have-structured-data-in-a-form

@neo7-studio-web
Copy link

neo7-studio-web commented Dec 17, 2023

Just to say BTW, all my Vercel serverless functions that were handling forms crashed suddenly. Vercel parses request body depending on content-type and uses the default one (application/x-www-form-urlencoded) : https://vercel.com/docs/functions/serverless-functions/runtimes/node-js#request-body
With ViewTransitions activated, forms submit with multipart/form-data and Vercel return request.body as undefined.
I leave this comment here in case someone has the same problem.

Solution : add an attribute data-astro-reload to the form tag as mentioned here :
https://docs.astro.build/en/guides/view-transitions/#preventing-client-side-navigation

@matthewp matthewp added - P2: has workaround Bug, but has workaround (priority) feat: view transitions Related to the View Transitions feature (scope) and removed needs triage Issue needs to be triaged labels Dec 18, 2023
@matthewp
Copy link
Contributor

Thanks for raising this. When implementing this I went back and forth on whether to do formdata or urlencoded and went with formdata because:

  1. Astro supports formdata and for API endpoints there's no real consumption difference, you use request.formData() to get the data in either case.
  2. To align with the navigate() API which can take a formData object and (I think) always posts it as formdata. I don't think there's any way to change the encoding for this method.
  3. Form submission does use the navigate() API under the hood so it was just a cleaner implementation to not make it special.

That being said, I'm open to being wrong here. Unfortunately we can't change the default until 5.0 which is going to be a while. But I would think we should, at the least, honor the enctype attribute so you could special urlencoded that way.

is the primary reason for wanting urlencoded so you can do the [] array type tricks?

@knpwrs
Copy link
Contributor Author

knpwrs commented Dec 18, 2023

is the primary reason for wanting urlencoded so you can do the [] array type tricks?

I wouldn't call that the primary reason, my primary concern was platform compatibility. Being able to do the query-string package trick is certainly a nice side effect, and being able to take some code out of a remix/solid-start/etc project and use it as an Astro island with few to no changes is certainly appealing.

  1. you use request.formData() to get the data in either case.

I was unaware of this. It seems like @neo7-studio-web has a similar concern though? @neo7-studio-web does that relate to astro itself being deployed on vercel or just some other functions being deployed there?

All of that said I think honoring enctype is probably the easiest path forward for the short term.

@neo7-studio-web
Copy link

It seems like @neo7-studio-web has a similar concern though? @neo7-studio-web does that relate to astro itself being deployed on vercel or just some other functions being deployed there?

Yes it does relate to Astro. The moment I implemented ViewTransitions with Astro, all my Vercel serverless functions crashed because of the change of enctype of the forms. For me, explicitly removing ViewTransition for each form worked. But I struggled to understand what happened until I found your post here (thanks BTW).

All of that said I think honoring enctype is probably the easiest path forward for the short term.

I agree. And I think this case should be documented in the ViewTransition chapter as a warning, because anyone with forms could encounter a variety of problems because of the enctype.

@martrapp
Copy link
Member

And I think this case should be documented in the ViewTransition chapter as a warning

yes and in https://docs.astro.build/en/recipes/build-forms/ as already suggested in #9368.

@martrapp
Copy link
Member

All of that said I think honoring enctype is probably the easiest path forward for the short term.

@knpwrs do you want to provide a PR for this issue?

@knpwrs
Copy link
Contributor Author

knpwrs commented Dec 18, 2023

I'll be able to look into that tonight 👍🏻

@martrapp
Copy link
Member

Great! Thank you very much!

@knpwrs
Copy link
Contributor Author

knpwrs commented Sep 19, 2024

I wanted to follow up on this. With Astro 5 on the horizon is the Astro team going to bring the default options to be more in alignment with the browser-standard behavior?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P2: has workaround Bug, but has workaround (priority) feat: view transitions Related to the View Transitions feature (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants