Stop syncing value attribute for controlled inputs

[aweary]

Opening this as a follow up to some quick discussions in #11881. Syncing the value attribute has been a consistent source of bugs for us, and the benefits of doing so seem minimal. There's some previous discussion on the topic in #7359 and in other issues, I can't remember right now 😄

This would be a breaking change, so it would have to be done in a major release.

Reasons to keep syncing

• It prevents form.reset() from putting controlled form inputs into a weird state
• Some browser extensions (not sure which) read from the value attribute in some cases (not sure which)
• It can be useful for querying inputs with a specific value using an attribute selector

Reasons to stop syncing

• It will reduce the complexity of react-dom in a non-trivial way
• In turn, it will likely reduce bundle size as well
• We remove a whole class of bugs (fighting with browsers that want to be helpful about input values)
• Syncing the input value to the attribute potentially exposes sensitive data to third party tools (1)

---

What do we think? Are these reasons good enough to keep syncing the value attribute? Are there other more critical reasons we should keep doing so?

cc @nhunzaker @jquense @gaearon

[jquense]

I think we should really try and make this happen, many of these bugs are intractable. Do any other frameworks try and keep them in sync? It seems like something React does out of a sense that it wouldn't be too hard for some benefit (which isn't the case it's hard).

Some browser extensions (not sure which) read from the value attribute in some cases (not sure which)

I don't think we've been able to verify that this is the case? even so tho i'd say that's the extensions's problem (which they should fix). I'll admit tho that stance is only tenable if there isn't a big problem with extensions do this.

The form.reset case seems to be the most compelling for keeping the behavior, but as a strike against it, folks should probably not be using form.reset with controlled components, are their compelling reasons for that? (I've done a lot of work with forms in React and never used form reset with controlled components so IDK)

[Jessidhia]

If you are using controlled components, that means the state is kept somewhere, such as your form controller component. If you do want to be able to use form.reset(), then your component that owns the <form> should listen to onReset and reset its own state.

Stopping the syncing of value does not seem to do anything but enable anti-patterns; although I am curious about the "helpful browser" case.

EDIT: looks like I did confuse value attribute with property 😕

[gaearon]

I think the original justification for that was that a lot of people felt that mismatch between value attribute and property was counter-intuitive. By that time many people learned React before the DOM APIs and weren’t aware of the distinction. So it “felt wrong” to them that the attribute in the browser DevTools (looks like HTML!) doesn’t match the “attribute” they see in JSX (looks like HTML too!)

I agree this was a poor justification in hindsight. We probably should’ve solved this by educating users instead (e.g. through documentation and blog posts). Unfortunately now that we’ve “fixed” this it’s likely more people rely on this behavior now. But ultimately I agree we should give up on this and change it back.

[nhunzaker]

It prevents form.reset() from putting controlled form inputs into a weird state

I think we could do some neat stuff here. If the value attribute is not synchronized, we could allow controlled inputs to both control value and defaultValue. Then, on form reset, we could fire a synthetic change event on all of the associated inputs matching the defaultValue. Effectively controlling form.reset.

Some browser extensions (not sure which) read from the value attribute in some cases (not sure which)

It's possible I've forgotten already, but I feel like, every time this comes up, no-one is able to answer this question. I'd really love to nail this one down.

It can be useful for querying inputs with a specific value using an attribute selector

This is nice, but I think it is unique to React. I'd be okay with dropping this and conforming to the way standard HTML manages the value property/attribute natively. It's definitely simpler.

[gaearon]

If I get it right, #10150 is an implementation. I'll close it as stale but reference this issue.

[sebmarkbage]

Reasons to stop syncing: https://www.reddit.com/r/analytics/comments/7ukw4n/mixpanel_js_library_has_been_harvesting_passwords/

[nhunzaker]

Oof. Should we send out a minor release that removes syncing of password inputs?

[aweary]

This is something that's been brought up before as a concern, and it's a good argument in favor of removing attribute syncing (added to the list).

Oof. Should we send out a minor release that removes syncing of password inputs?

It's potentially dangerous behavior, but I believe it's still expected behavior regardless. I'm not sure it's worth releasing a breaking change in a minor release. If anything, it's a good incentive to upgrade to the next major release that removes attribute syncing, assuming we agree to do so.

[noinkling]

Relevant PoC exploit (relies on syncing) now making the rounds:

• https://news.ycombinator.com/item?id=16422696
• https://github.com/maxchehab/CSS-Keylogging

[sebmarkbage]

I wonder how much of this is over-focus on one particular vector when there are others similar ones such as :checked selectors, contentEditable content, just any HTML content in general visible on the page...

I don’t really have a good sense of how much worse it is with password fields.

However the use cases are also much more limited so it wouldn’t hurt that bad to do it for password fields in a minor if we’re going to do it for other things later.