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 break on presence of <input name="action"> #10849

Closed
1 task done
robertvanhoesel opened this issue Apr 22, 2024 · 2 comments · Fixed by #10856
Closed
1 task done

VIewTransitions break on presence of <input name="action"> #10849

robertvanhoesel opened this issue Apr 22, 2024 · 2 comments · Fixed by #10856
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: view transitions Related to the View Transitions feature (scope)

Comments

@robertvanhoesel
Copy link
Contributor

Astro Info

Astro                    v4.6.1
Node                     v18.20.2
System                   macOS (arm64)
Package Manager          pnpm
Output                   server
Adapter                  @astrojs/node
Integrations             @astrojs/solid-js
                         @astrojs/tailwind

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

No response

Describe the Bug

When using ViewTransitions and having semantically valid HTML Inputs with a name attribute that's either method or action, submitting a form results in a broken route: 404 /[object%20HTMLInputElement] not found

This is caused by a concept called DOM clobbering and by incorrectly (but totally intuitively and understandably) assuming form.action is always the same a <form action="">

Where usually you'd except form.action to be foo, the following DOM results in form.action to point to the HTMLInputElement instead.

<form action="foo">
 <input name="action" value="bar">
</form>

When trying to submit the form, it will read this element as the destination url instead.

What's the expected result?

The submit handler in View Transitions reading method and action should not assume reading form.action and form.method are safe to read in user land. Instead is should explicitly read the attribute. This follows the same pattern browser behavior without ViewTranstions enabled.

Suggested changes in workaround.

let action = submitter?.getAttribute('formaction') ?? form.action ?? location.pathname;
const method = submitter?.getAttribute('formmethod') ?? form.method;

const form = el as HTMLFormElement;
const submitter = ev.submitter;
const formData = new FormData(form, submitter);
// Use the form action, if defined, otherwise fallback to current path.
- 	let action = submitter?.getAttribute('formaction') ?? form.action ?? location.pathname;
+ let action = submitter?.getAttribute('formaction') ?? form.getAttribute('action') ?? location.pathname;
-  const method = submitter?.getAttribute('formmethod') ?? form.method;
+  const method = submitter?.getAttribute('formmethod') ?? form.getAttribute('method') ?? 'get'

Happy to create my first PR to Astro for this.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-3nkr3h?file=src%2Fpages%2Findex.astro

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 Apr 22, 2024
@matthewp
Copy link
Contributor

Wow, I didn't know about this feature. Yes, PR very much welcome, thank you!

@matthewp matthewp added - P3: minor bug An edge case that only affects very specific usage (priority) feat: view transitions Related to the View Transitions feature (scope) and removed needs triage Issue needs to be triaged labels Apr 22, 2024
@robertvanhoesel
Copy link
Contributor Author

@matthewp PR submitted at #10856

matthewp pushed a commit that referenced this issue Apr 23, 2024
…10856)

* Prevents inputs from changing ViewTransitions' form method or action. Fixes #10849

* Consistency 🧘‍♂️
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: view transitions Related to the View Transitions feature (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants