-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Conversation
d7ca7f6
to
574328a
Compare
e788142
to
4ba86d1
Compare
@jimfb updated the pull request. |
3714855
to
4ba86d1
Compare
@jimfb updated the pull request. |
bec734c
to
9aabd4c
Compare
@jimfb updated the pull request. |
d049ebf
to
1e8a3e8
Compare
@jimfb updated the pull request. |
Ok, I think this is ready to go. EDIT: Actually, never mind, the bug still exists, fixing :/ . |
This is not for 15.0, right? |
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 |
ff1e9d8
to
3186f4c
Compare
@jimfb updated the pull request. |
3186f4c
to
fef7028
Compare
@jimfb updated the pull request. |
fef7028
to
8c06195
Compare
Ok, I think this is good to go now. Here are some fiddles that demonstrate everything working:
|
8c06195
to
9eeca54
Compare
var node = ReactDOMComponentTree.getNodeFromInstance(inst); | ||
if (node.defaultValue !== props.defaultValue) { | ||
var tmp = node.value; | ||
node.defaultValue = ''+props.defaultValue; |
There was a problem hiding this comment.
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 +
355bf75
to
f6c393c
Compare
Can you refresh me and explain why we need to change |
Are we targeting 15 or 16 with this? |
I don't have a super strong preference. Should be safe for 15, but whatever works. |
textareaPostMount, | ||
this | ||
); | ||
break; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…6406) * Have `defaultValue` reach DOM node for html input box for facebook#4618 * Cleanup and bug fixes for merge. (cherry picked from commit 4338c8d)
Hello |
If you have an issue with the latest version please file a new issue. Thanks! |
// 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation
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 fordefaultValue
.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.