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

Controlled inputs do not synchronize value or checked attribute #10150

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@nhunzaker
Collaborator

nhunzaker commented Jul 11, 2017

This commit is a follow up to prior work to resolve issues with number inputs in React:

#7359 (comment)

More or less

A can of worms :). The discussion above concluded that React should not synchronize the value attribute/checked attribute for controlled inputs. This PR makes that change.

More info

Currently, inputs keep their value/checked attribute in sync with the value/checked property. This is a React behavior. Traditionally browser DOM manipulation does not rely on keeping the value/checked attribute in sync.

It's also very problematic for number inputs and (I believe) a few other obscure input bugs in IE/iOS Safari (this needs to be confirmed). After discussion, it was decided to make a breaking change to no longer sync up the value/checked attribute with it's associated property.

For this to work, I made the following changes:

  • The value, defaultValue, checked and defaultChecked properties are now maintained within the HTML property config. defaultValue maps to the value attribute, as does defaultChecked to checked
  • ☝️ required adding a filter to strip out the value property on selects and textareas
  • The logic to defer assignment of the value attribute has been removed form ChangeEventPlugin

How to test this change

I've pushed a build of this version of React for our DOM fixtures here:

http://nh-no-input-value-attribute.surge.sh

Controlled inputs do not synchronize value or checked attribute
This commit is a follow up to prior work to resolve issues with number
inputs in React. Inputs keep their value/checked attribute in sync with the
value/checked property. This is a React behavior. Traditionally
browser DOM manipulation does not rely on keeping the value/checked
attribute in sync.

It's also very problematic for number inputs. After discussion, it was
decided to make a breaking change to no longer sync up the
value/checked attribute with it's assoicated property.

For this to work, I made the following changes:

- The value, defaultValue, checked and defaultChecked properties are
  now maintained within the HTML property config.
- This required adding a filter to strip out the value property on
  selects and textareas
- The logic to defer assignment of the value attribute has been
  removed form ChangeEventPlugin
- defaultValue and defaultChecked are aliased to `value` and `checked`
  so that uncontrolled input attribute assignment works as intended.
@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Jul 11, 2017

Collaborator

Added a test plan.

Collaborator

nhunzaker commented Jul 11, 2017

Added a test plan.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 12, 2017

Member

Is this breaking? Should it go into 15.x or 16?

Member

gaearon commented Jul 12, 2017

Is this breaking? Should it go into 15.x or 16?

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Jul 12, 2017

Collaborator

Definitely breaking. This should go in 16.x

Collaborator

nhunzaker commented Jul 12, 2017

Definitely breaking. This should go in 16.x

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Jul 12, 2017

Collaborator

Are there consumers that depend on the attribute syncing? Why did we go that route originally, it was just for Form.reset? I'd think that if plugins/extensions relied on DOM syncing they would be wrong, right?

Collaborator

jquense commented Jul 12, 2017

Are there consumers that depend on the attribute syncing? Why did we go that route originally, it was just for Form.reset? I'd think that if plugins/extensions relied on DOM syncing they would be wrong, right?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 12, 2017

Member

Let's say we mention this as breaking change in the 16 blog post. What would the text be? (I'm trying to understand the full implications for consumers.)

Member

gaearon commented Jul 12, 2017

Let's say we mention this as breaking change in the 16 blog post. What would the text be? (I'm trying to understand the full implications for consumers.)

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Jul 12, 2017

Member

Are there consumers that depend on the attribute syncing? Why did we go that route originally, it was just for Form.reset? I'd think that if plugins/extensions relied on DOM syncing they would be wrong, right?

@jquense that's essentially the answer we got initially. It's not clear how many plugins/extensions depend on this behavior.

Let's say we mention this as breaking change in the 16 blog post. What would the text be? (I'm trying to understand the full implications for consumers.)

@gaearon I think it would need to touch on:

  • Attribute vs property, since a lot of people don't realize the difference
  • Why we initially implemented it this way (if we even really know)
  • What problems removing it solves, specifically:
    • avoids triggering browser input validation
    • simplifies the logic for detecting controlled input value changes
  • Potential breakages, which could be:
    • Extensions that rely on reading the value attribute for inputs
    • Relying on form.reset not resetting controlled input values
    • Test suites that read the value attribute instead of the value property
Member

aweary commented Jul 12, 2017

Are there consumers that depend on the attribute syncing? Why did we go that route originally, it was just for Form.reset? I'd think that if plugins/extensions relied on DOM syncing they would be wrong, right?

@jquense that's essentially the answer we got initially. It's not clear how many plugins/extensions depend on this behavior.

Let's say we mention this as breaking change in the 16 blog post. What would the text be? (I'm trying to understand the full implications for consumers.)

@gaearon I think it would need to touch on:

  • Attribute vs property, since a lot of people don't realize the difference
  • Why we initially implemented it this way (if we even really know)
  • What problems removing it solves, specifically:
    • avoids triggering browser input validation
    • simplifies the logic for detecting controlled input value changes
  • Potential breakages, which could be:
    • Extensions that rely on reading the value attribute for inputs
    • Relying on form.reset not resetting controlled input values
    • Test suites that read the value attribute instead of the value property
@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide Jul 12, 2017

Contributor

Are there consumers that depend on the attribute syncing? Why did we go that route originally, it was just for Form.reset?

Yes, part of the reasoning was just attribute syncing, but it also got conflated with the defaultValue problem IIRC. #5680 (comment)

I'd think that if plugins/extensions relied on DOM syncing they would be wrong, right?

Yes, very much so IMHO. This is not "native behavior" that you can normally rely on.

EDIT: AFAIK a lot of the background can be found in #5680 #6321 #6406, but they're long and hard to follow.

Contributor

syranide commented Jul 12, 2017

Are there consumers that depend on the attribute syncing? Why did we go that route originally, it was just for Form.reset?

Yes, part of the reasoning was just attribute syncing, but it also got conflated with the defaultValue problem IIRC. #5680 (comment)

I'd think that if plugins/extensions relied on DOM syncing they would be wrong, right?

Yes, very much so IMHO. This is not "native behavior" that you can normally rely on.

EDIT: AFAIK a lot of the background can be found in #5680 #6321 #6406, but they're long and hard to follow.

@gaearon gaearon referenced this pull request Aug 4, 2017

Closed

React 16 Umbrella ☂️ (and 15.5) #8854

117 of 120 tasks complete
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 4, 2017

Member

For posterity: we decided to postpone this until 17.

Member

gaearon commented Aug 4, 2017

For posterity: we decided to postpone this until 17.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Oct 14, 2017

Collaborator

Closing for now, but let's revisit it for 17 (?! :))

Collaborator

nhunzaker commented Oct 14, 2017

Closing for now, but let's revisit it for 17 (?! :))

@nhunzaker nhunzaker closed this Oct 14, 2017

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Oct 14, 2017

Member

Should we keep this open under a milestone if we think we want it? So we don't forget.

Member

sebmarkbage commented Oct 14, 2017

Should we keep this open under a milestone if we think we want it? So we don't forget.

@nhunzaker

This comment has been minimized.

Show comment
Hide comment
@nhunzaker

nhunzaker Oct 14, 2017

Collaborator

Sure. I definitely want this. I'll do just that. Were you thinking a React 17 milestone, or something more specific to DOM stuff?

Collaborator

nhunzaker commented Oct 14, 2017

Sure. I definitely want this. I'll do just that. Were you thinking a React 17 milestone, or something more specific to DOM stuff?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 5, 2018

Member

We're tracking it in #11896 so I think we can close this PR as stale. We can always revive it later.

Member

gaearon commented Jan 5, 2018

We're tracking it in #11896 so I think we can close this PR as stale. We can always revive it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment