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

feat: take form resets into account for two way bindings #10617

Merged
merged 10 commits into from
Mar 22, 2024

Conversation

dummdidumm
Copy link
Member

When resetting a form, the value of the inputs within it get out of sync with the bound value of those inputs. This PR introduces a reset listener on the parent form to reset the value in that case
closes #2659

Not ready yet because this needs more thought with respect to how the default value is determined, and how a user is able to set it. HTML is weird in that regard because

  • for textareas and inputs other than radio/checkbox the value attribute does set defaultValue at the same time, but that one's already occupied by bind:value. You can set defaultValue only through JavaScript. It might be best to special-case this attribute on inputs and make it set the default value (also needs to take SSR into account)
  • for radio/checkbox inputs the same applies, but the name is defaultChecked
  • for selects, the selected attribute on one of the options tells which one is the default value. But in SSR this also tells the current value, so we can't use that. One can also use the value attribute on a select for that, but that's occupied by the bind:value binding. So in this case it might make the most sense and follow React by allowing defaultValue on a select even if that property doesn't actually exist on that element

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

When resetting a form, the value of the inputs within it get out of sync with the bound value of those inputs. This PR introduces a reset listener on the parent form to reset the value in that case
closes #2659
Copy link

changeset-bot bot commented Feb 23, 2024

🦋 Changeset detected

Latest commit: 2ee1aff

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

This PR includes changesets to release 1 package
Name Type
svelte Patch

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

@brunnerh
Copy link
Member

brunnerh commented Mar 6, 2024

for selects, the selected attribute on one of the options ...

There can be many; <select> can have a multiple attribute (MDN example).

@ITenthusiasm
Copy link

ITenthusiasm commented Mar 14, 2024

I hope the cross-posting isn't bothersome. But please consider #9230 as well.

I really don't like React's solution to this problem. (Does it work with progressive enhancement? My understanding is that it does not. And for tools like SvelteKit, which argues for progressive enhancement in their docs, this is an important consideration.) So I don't think taking React's approach (e.g., with select elements) is the way to go. I think instead, having a way to tell Svelte to only use pure HTML/markup with no JS magic is the way to go.

Solid may give some insight on a potential solution. They basically have a directive syntax that says, "This is a raw HTML attribute, not a JS property". They also have something similar for using JS properties only. (Both arguably have use cases.)

The <textarea> element is unique in that its default value is its text content. In that scenario, if Svelte doesn't have an easy way to create an "HTML only escape hatch", I think React's approach would make sense. Basically, they allow <textarea> to behave as normal if people want. But if people decide to use both the value property and the <textarea> text content, then React logs an error because the JS conflicts with the raw HTML. They basically make you choose one or the other approach for <textarea>.


It's possible that there are other progressive-enhancement-related bugs that come from relying on JS properties instead of HTML attributes. Having a way to use pure HTML would automatically solve those problems (including the problem that this PR is trying to solve).

Any solution that solely relies on JS properties will always have deficiencies in progressive enhancement, because it inherently assumes that JavaScript is accessible to the end user. (Currently, Remix beats SvelteKit here because of how Svelte behaves with form controls.)

@dummdidumm
Copy link
Member Author

Marking this as ready for review - the default value discussion can happen separately (though I think we should talk about it sooner rather than later).

@trueadm trueadm merged commit 3eef1cb into main Mar 22, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resetting html form doesn't trigger input bindings
4 participants