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

Respect forms with enctype set for view transitions #9466

Merged
merged 8 commits into from
Dec 20, 2023

Conversation

knpwrs
Copy link
Contributor

@knpwrs knpwrs commented Dec 19, 2023

Changes

This makes progressively-enhanced forms (via ViewTransitions) respect the enctype attribute.

Closes #9447.

Testing

I built the repo and added a form to the view transitions example and tried submitting the form with both enctype set and not set.

Docs

I will submit a PR to update the docs.

Copy link

changeset-bot bot commented Dec 19, 2023

🦋 Changeset detected

Latest commit: b3be36f

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Dec 19, 2023
Copy link
Member

@martrapp martrapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @knpwrs, thanks for your quick PR!

While it solves the original problem and maintains compatibility for the encoding behavior, it still leads to incompatibilities in the client-side router and transition events.

Could you please try to base your solution on the existing sourceElement property? I have left details in the comments in the code. That way you could access the form directly or via the event's handler from the defaultLoader(). If this works, no changes to the navigate() options or the events would be required.

packages/astro/src/transitions/types.ts Outdated Show resolved Hide resolved
packages/astro/src/transitions/router.ts Outdated Show resolved Hide resolved
@knpwrs
Copy link
Contributor Author

knpwrs commented Dec 19, 2023

@martrapp thank you for the feedback, I think these changes should address what you've said.

@martrapp
Copy link
Member

Great! Much more concise! Were you able to test it? Does it sill work?
I guess, sourceElement is not always the <form> element but might also be a "submitter" <button> or similar. In that case we first need to identify the form.

@knpwrs
Copy link
Contributor Author

knpwrs commented Dec 19, 2023

The latest commit is tested and working with both forms and submitters.

@martrapp
Copy link
Member

So far everything looks good and it was very pleasant and productive how you solved the problem! I'm very happy with where we are now. As you wrote, we can simply fall back to the standard of form.enctype for 5.0. I like to quote a dear colleague: definitely much "better than what we had before".

I don't want to overshoot the mark, but we need additional e2e tests now. Since we don't have a long history together yet, I'll ask very carefully: do you want to add them or are you satisfied with what we've achieved so far ;-)

@knpwrs
Copy link
Contributor Author

knpwrs commented Dec 19, 2023

I'm open to giving it a go tonight. It'll be a good opportunity to try out playwright test and I can see there's some prior-art for view transitions that I can model after.

@martrapp
Copy link
Member

😃 Thanks a lot! As you may have seen, all e2e tests for view transitions are located in packages/astro/e2e/view-transitions.test.js. The server-side for the current tests under packages/astro/e2e/fixtures/view-transitions/src/pages/form-* is defined in packages/astro/e2e/fixtures/view-transitions/src/pages/contact.ts. As you can see, we don't usually have full coverage, but enough to recognize when something isn't working. Maybe you can test that application/x-www-form-urlencoded makes it to the server-side for a form and submitter case, and test that multipart/form-data is used when enctype is not set. Get back to me if I can/should lend you a hand!

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me! Would be great to have tests, as @martrapp suggested!

.changeset/rude-geckos-rush.md Outdated Show resolved Hide resolved
@knpwrs
Copy link
Contributor Author

knpwrs commented Dec 20, 2023

Tests have been pushed!

@martrapp
Copy link
Member

Very good solution! Thank you again, it was a pleasure!

@martrapp martrapp merged commit 5062d27 into withastro:main Dec 20, 2023
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ViewTransitions form submission behavior does not match browser-native behavior
3 participants