Fix uncontrolled input decimal point "chopping" on number inputs, and validation warnings on email inputs #7750

Merged
merged 2 commits into from Oct 13, 2016

Projects

None yet

7 participants

@nhunzaker
Contributor

This fix is stuck inside of #7359 until I can sort out the outstanding concerns around controlled inputs. Hopefully this is easier to merge and people can start using uncontrolled number inputs again!

Basically avoid assigning defaultValue unless you have to. This prevents Chrome's number inputs from dropping decimal places or clearing the input when typing 3e. As documented here: #7253 (comment)

@nhunzaker nhunzaker changed the title from Fix uncontrolled input decimal point "chopping" on number inputs, and vlaidation warnings on email inputs to Fix uncontrolled input decimal point "chopping" on number inputs, and validation warnings on email inputs Sep 16, 2016
@yaycmyk
Contributor
yaycmyk commented Sep 16, 2016

Honestly, I'm a bit confused why React changes defaultValue and defaultChecked at all after initial mount. Seems contradictory.

@nhunzaker
Contributor

@yaycmyk I can confirm later today, but I believe it is to support an input changing from controlled to uncontrolled. I think this technically should raise a console warning from React, but there's definitely a test for it.

I can dig in and explain more later today.

@nhunzaker
Contributor

Beyond that, unsure to to ping, but I'd love to get this through if possible. Anything I can do to help move this along?

@yaycmyk
Contributor
yaycmyk commented Sep 19, 2016

Yeah I was more commenting that the behavior and accompanying test may not actually make sense. Would love to get a collaborator's opinion like @zpao

@aweary
Collaborator
aweary commented Sep 19, 2016

I've reviewed this (when it was part of #7359) and it looks decent to me, but I would like someone from the core team to give the OK here.

@nhunzaker
Contributor

@Aweary Awesome. I feel like I've taken you down a rocky road with all of this, thank you for your help!

@ghost ghost added the CLA Signed label Sep 19, 2016
@nhunzaker
Contributor

Upstreamed with master.

@jimfb jimfb was assigned by aweary Sep 29, 2016
@martin-svk

Hello, is there a timeline for this PR to be merged and released?

@nhunzaker
Contributor

Any testing materials I can put together to help verify this fix? I'd really like this to ship too.

@gaearon gaearon assigned gaearon and unassigned jimfb Oct 10, 2016
@gaearon
Member
gaearon commented Oct 10, 2016 edited

Can you please provide a table of the browsers you tested, test cases, and the behavior before and after this change?

@nhunzaker nhunzaker Only assign defaultValue if it has changed.
2df2023
@nhunzaker
Contributor
nhunzaker commented Oct 11, 2016 edited

To reproduce the issue, visit here:
http://natehunzaker.com/react-issue-reproducer/cases/input/

When, in Chrome, backspacing to 3 in the uncontrolled number input, you'll see the decimal place gets "lost":

issue

This fix is hosted here:
http://natehunzaker.com/react-issue-reproducer/edge/cases/input

And here's the list of browsers (though Chrome is the only place this matters):

Browser Before After
Chrome 53.0.2785.143 Decimal place is incorrectly "lost" No removal
Safari 10 No removal No removal
Safari iOS 9 No removal No removal
Firefox Dev Edition 51.0a2 No removal No removal
Firefox 49 No removal No removal
IE Edge (14) No removal No removal
IE 11 No removal No removal
IE 10 No removal No removal

This table is a bit excessive, but at least I got an excuse to use the markdown table generator for Emacs 😀 🔌.

Just to reiterate, this appears to be specific to Chrome. It happens because Chrome updates the value of a number input whenever the value property, defaultValue property, or value attribute is changed. This PR fixes the issue by avoiding assignment unless the value has actually changed.

@aweary
Collaborator
aweary commented Oct 12, 2016

This fix is hosted here:
http://natehunzaker.com/react-issue-reproducer/edge/cases/input

@nhunzaker I'm still seeing the issue on this page?

out

@gaearon
Member
gaearon commented Oct 12, 2016

@Aweary

This PR only fixes it for uncontrolled inputs.

@@ -213,7 +213,11 @@ var ReactDOMInput = {
}
} else {
if (props.value == null && props.defaultValue != null) {
- node.defaultValue = '' + props.defaultValue;
+ // Assigning defaultValue causes side-effects on value, which can cause
@gaearon
gaearon Oct 12, 2016 Member

Can you please rephrase this comment? It's not clear what you mean exactly to a person new to this.
A link to the issue would also be most helpful.

@nhunzaker
Contributor

@Aweary Sorry :(. That was confusing. @gaearon how does this sound:

// In Chrome, assigning defaultValue to certain input types triggers input validation.
// For number inputs, the display value loses trailing decimal points. For email inputs,
// Chrome raises "The specified value <x> is not a valid email address".
//
// Here we check to see if the defaultValue has actually changed, avoiding these problems
// when the user is inputting text
//
// https://github.com/facebook/react/issues/7253

Too verbose?

@aweary
Collaborator
aweary commented Oct 12, 2016

Ah, I misread 😄 I think this change is reasonable. It makes sense regardless not to set an attribute unless it's required.

@gaearon
Member
gaearon commented Oct 12, 2016

@nhunzaker Sounds great, there's never a verbose Chrome bug description.

@nhunzaker nhunzaker Improve comment about reason for defaultValue conditional assignment
8f8a833
@nhunzaker
Contributor

Done! Now if I can only figure out how to solve #7359 ....

@aweary
Collaborator
aweary commented Oct 13, 2016

@gaearon look good to you? I think this is safe to merge 👍

@gaearon
Member
gaearon commented Oct 13, 2016

Do it

@nhunzaker
Contributor

yay

@aweary aweary merged commit 0d20dcf into facebook:master Oct 13, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 87.872%
Details
@gaearon gaearon added this to the 15-next milestone Oct 13, 2016
@gaearon
Member
gaearon commented Oct 13, 2016

@Aweary Please don't forget to tag the 15-next milestone on merge.

@gaearon gaearon modified the milestone: 15-hipri, 15-lopri, 15.4.2 Jan 6, 2017
@gaearon gaearon added a commit that referenced this pull request Jan 6, 2017
@nhunzaker @gaearon nhunzaker + gaearon Fix uncontrolled input decimal point "chopping" on number inputs, and…
… validation warnings on email inputs (#7750)

* Only assign defaultValue if it has changed.

* Improve comment about reason for defaultValue conditional assignment

(cherry picked from commit 0d20dcf)
ed760d1
@gaearon gaearon added a commit that referenced this pull request Jan 9, 2017
@gaearon gaearon Add missing entry for #7750 to 15.4.2 changelog 6b1c860
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment