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

Controlled input allows dot #9561

Closed
raulsebastianmihaila opened this issue Apr 29, 2017 · 10 comments
Closed

Controlled input allows dot #9561

raulsebastianmihaila opened this issue Apr 29, 2017 · 10 comments

Comments

@raulsebastianmihaila
Copy link

Do you want to request a feature or report a bug?
bug

What is the current behavior?
React allows a dot in a controlled input that doesn't allow changing the value, if the value set as a prop is a number. If it's a string the issue is not present.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar (template: https://jsfiddle.net/84v837e9/).
https://jsfiddle.net/84v837e9/23/

In this example the first input allows a dot, even though the code doesn't allow changes. The second input doesn't allow the dot.

What is the expected behavior?
The dot shouldn't be allowed.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
version 15.5.4, Chrome and Firefox (see the jsfiddle example)

@ndresx
Copy link
Contributor

ndresx commented Apr 29, 2017

This is interesting. The reason for this is because 2. or 2.0 (which is also allowed) equals to 2. Casting the value to a String first before doing the comparison could fix this problem, however it could be also intended behavior.

@raulsebastianmihaila
Copy link
Author

But note that the type of the input is "text" and such an input will always give you a string and "2." is by no means equal to either "2" or 2 (number). Also "2.000000000000" is very different from 2.

@raulsebastianmihaila
Copy link
Author

I'm not sure what you mean by casting the value to a String, since the input (with type "text") always gives you a string.

@ndresx
Copy link
Contributor

ndresx commented Apr 29, 2017

Think the actual problem is caused by this check value != node.value here https://github.com/facebook/react/blob/master/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js#L205, which gets skipped because of value 2 equals "2.". When it gets changed to !== it does the trick.

@Hypnosphi
Copy link
Contributor

Yes, value should be explicitly converted to string before comparing to node.value

@Hypnosphi
Copy link
Contributor

I'm not sure what you mean by casting the value to a String, since the input (with type "text") always gives you a string.

This is about the value that you give via props, not the actual input value

@aweary
Copy link
Contributor

aweary commented May 1, 2017

@nhunzaker what was the reason for using loose equality checks for value with text inputs?

@nhunzaker
Copy link
Contributor

I believe this was a relic of dealing with edge cases on number inputs. In short: if we touch the value property of number inputs whatsoever, Chrome/Safari will do unexpected things like drop trailing decimal places, clear out invalid input, etc.

So this needs to go. When I added the condition to specifically check for the number type, I didn't remove this check. I'm wrapping up some text fixtures for it, and I should have something out for review later today.

nhunzaker added a commit to nhunzaker/react that referenced this issue May 2, 2017
This commit removes a check I added when working on number input
issues where we perform a loose check on an input's value before we
assign it. This prevented controlled text inputs from disallowing
numeric text entry.

I also added a DOM fixture text case.

Related issues:

facebook#9561 (comment)
nhunzaker added a commit to nhunzaker/react that referenced this issue May 2, 2017
This commit removes a check I added when working on number input
issues where we perform a loose check on an input's value before we
assign it. This prevented controlled text inputs from disallowing
numeric text entry.

I also added a DOM fixture text case.

Related issues:

facebook#9561 (comment)
@nhunzaker
Copy link
Contributor

I sent out a PR here: #9584

Sorry for the trouble.

aweary pushed a commit that referenced this issue May 3, 2017
…issue. (#9584)

* Remove loose check when assigning non-number inputs

This commit removes a check I added when working on number input
issues where we perform a loose check on an input's value before we
assign it. This prevented controlled text inputs from disallowing
numeric text entry.

I also added a DOM fixture text case.

Related issues:

#9561 (comment)

* Use strict equality as a guard before assigning input.value

This commit adds back the guard around assigning the value property to
an input, however it does it using a strict equals. This prevents
validated inputs, like emails and urls from losing the cursor
position.

It also adds associated test fixtures.

* Add copy command after build for interup with surge.sh
flarnie pushed a commit that referenced this issue May 3, 2017
…issue. (#9584)

* Remove loose check when assigning non-number inputs

This commit removes a check I added when working on number input
issues where we perform a loose check on an input's value before we
assign it. This prevented controlled text inputs from disallowing
numeric text entry.

I also added a DOM fixture text case.

Related issues:

#9561 (comment)

* Use strict equality as a guard before assigning input.value

This commit adds back the guard around assigning the value property to
an input, however it does it using a strict equals. This prevents
validated inputs, like emails and urls from losing the cursor
position.

It also adds associated test fixtures.

* Add copy command after build for interup with surge.sh
@aweary
Copy link
Contributor

aweary commented Jun 5, 2017

#9584 is merged now, issue can be closed 👍

@aweary aweary closed this as completed Jun 5, 2017
flarnie pushed a commit to flarnie/react that referenced this issue Jun 7, 2017
…issue. (facebook#9584)

* Remove loose check when assigning non-number inputs

This commit removes a check I added when working on number input
issues where we perform a loose check on an input's value before we
assign it. This prevented controlled text inputs from disallowing
numeric text entry.

I also added a DOM fixture text case.

Related issues:

facebook#9561 (comment)

* Use strict equality as a guard before assigning input.value

This commit adds back the guard around assigning the value property to
an input, however it does it using a strict equals. This prevents
validated inputs, like emails and urls from losing the cursor
position.

It also adds associated test fixtures.

* Add copy command after build for interup with surge.sh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants