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

Properly set value and defaultValue for input and textarea #6406

Merged
merged 2 commits into from
May 25, 2016

Conversation

jimfb
Copy link
Contributor

@jimfb jimfb commented Apr 4, 2016

Spiritual successor to #5680. This is a fix for #6119 and #6219, both of which are blocking v15. As an added bonus, it happens to also fix #4618 :P. It works by setting/removing both the attribute and property for value on inputs, and setting the property for defaultValue.

I fixed the tests to be more reasonable (inputs retain their initial value, rather than having the value potentially change when defaultValue changes). I also fixed the browser inconsistencies.

@facebook-github-bot
Copy link

@jimfb updated the pull request.

@facebook-github-bot
Copy link

@jimfb updated the pull request.

@jimfb jimfb force-pushed the attribute-for-value branch 2 times, most recently from bec734c to 9aabd4c Compare April 4, 2016 22:21
@facebook-github-bot
Copy link

@jimfb updated the pull request.

@jimfb jimfb force-pushed the attribute-for-value branch 4 times, most recently from d049ebf to 1e8a3e8 Compare April 4, 2016 23:38
@facebook-github-bot
Copy link

@jimfb updated the pull request.

@jimfb
Copy link
Contributor Author

jimfb commented Apr 4, 2016

Ok, I think this is ready to go.

EDIT: Actually, never mind, the bug still exists, fixing :/ .

@sophiebits
Copy link
Collaborator

This is not for 15.0, right?

@jimfb
Copy link
Contributor Author

jimfb commented Apr 5, 2016

Paul Sebastian and I were at the PR review, and we weren't 100% decided, but Paul said to fix up #5680 to potentially land for v15.0

@jimfb jimfb force-pushed the attribute-for-value branch 2 times, most recently from ff1e9d8 to 3186f4c Compare April 5, 2016 02:30
@facebook-github-bot
Copy link

@jimfb updated the pull request.

@facebook-github-bot
Copy link

@jimfb updated the pull request.

@jimfb
Copy link
Contributor Author

jimfb commented Apr 5, 2016

Ok, I think this is good to go now. Here are some fiddles that demonstrate everything working:

http://jsfiddle.net/3r0krefu/ (fixes Ben's concerns from #5680)
http://jsfiddle.net/ub6ec5rt/ (fixes Ben's concerns from #5680)
http://jsfiddle.net/v6fz2ct1/ (fixes #6119)
http://jsfiddle.net/wavkdt6b/ (fixes #6219)
http://jsfiddle.net/c0u6v6fr/ (fixes #4618)

var node = ReactDOMComponentTree.getNodeFromInstance(inst);
if (node.defaultValue !== props.defaultValue) {
var tmp = node.value;
node.defaultValue = ''+props.defaultValue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: missing spaces before and after +

@jimfb jimfb force-pushed the attribute-for-value branch 2 times, most recently from 355bf75 to f6c393c Compare April 5, 2016 12:39
@zpao
Copy link
Member

zpao commented Apr 5, 2016

Can you refresh me and explain why we need to change defaultValue behavior here?

@zpao
Copy link
Member

zpao commented May 25, 2016

Are we targeting 15 or 16 with this?

@jimfb
Copy link
Contributor Author

jimfb commented Jun 1, 2016

I don't have a super strong preference. Should be safe for 15, but whatever works.

@zpao zpao added this to the 15-next milestone Jun 1, 2016
textareaPostMount,
this
);
break;
Copy link
Member

Choose a reason for hiding this comment

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

So I just came back here due to a merge conflict and perhaps I'm missing something but doesn't this change mean we'll stop autofocusing inputs and textareas?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's what it looks like to me. @jimfb?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

zpao pushed a commit to zpao/react that referenced this pull request Jun 8, 2016
…6406)

* Have `defaultValue` reach DOM node for html input box for facebook#4618

* Cleanup and bug fixes for merge.

(cherry picked from commit 4338c8d)
zpao pushed a commit that referenced this pull request Jun 14, 2016
* Have `defaultValue` reach DOM node for html input box for #4618

* Cleanup and bug fixes for merge.

(cherry picked from commit 4338c8d)
@zpao zpao modified the milestones: 15-next, 15.2.0 Jun 14, 2016
@TheLudd
Copy link

TheLudd commented Jun 8, 2017

Hello
I noticed that in react 15.2.0 (but not in 15.1.0) there is a change in how checkboxes are rendered. And I suspect it might have something to do with this change.
In version 15.2.0 rendered checkbox inputs will always have the property value="on" unless value is specified to be something else.
I wonder if this is intentional and/or wanted. It causes errors in my application/lib where I make assumptions based on if a checkbox has a value set or not. Now they always have values...

@gaearon
Copy link
Collaborator

gaearon commented Jun 8, 2017

If you have an issue with the latest version please file a new issue. Thanks!
Maybe @aweary or @nhunzaker could help with older versions.

// This is in postMount because we need access to the DOM node, which is not
// available until after the component has mounted.
var node = ReactDOMComponentTree.getNodeFromInstance(inst);
node.value = node.value; // Detach value from defaultValue
Copy link

Choose a reason for hiding this comment

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

I'm trying to understand the logic here. Could somebody tell what "detach" means here?

Copy link

Choose a reason for hiding this comment

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

If I recall from way back when, once you call the setter for value, it then ignores changes to the defaultValue. In this case, we set the value to the same value, which has this effect.

Copy link

Choose a reason for hiding this comment

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

Thanks for the explanation

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

Successfully merging this pull request may close these issues.

Can't update defaultChecked/defaultValue.
9 participants