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

Bug: Backspace in input type="number" behaves badly in Blink #7253

Closed
STRML opened this issue Jul 12, 2016 · 29 comments

Comments

Projects
None yet
8 participants
@STRML
Copy link
Contributor

commented Jul 12, 2016

This appears to have been introduced in a new Chrome version, but I can't find any reference to where.

Affected/Tested Browsers (OS X):

  • Chrome 51.0.2704.106 (64-bit)
  • Opera 39.0.2256.15

Unaffected Browsers:

  • Safari 9.1
  • Firefox 49

Backspacing in an input element with value or defaultValue set causes some very odd behavior. Once a decimal point is gone, it can't easily be re-added.

Example:

react-input-bug

In this example, I simply backspaced twice. On the second backspace, when I expect 3. to be showing, the input instead reads 3 and the cursor has moved to the beginning. The next two jumps are my attempts to add another decimal point.

Fiddle: https://jsfiddle.net/kmqz6kw8/

Tested with React 15.2.

Notes: This only occurs when value or defaultValue is set. If neither is set, the input behaves properly. We are currently working around this issue by (unfortunately) setting the input value on componentDidMount via a ref.

@zpao

This comment has been minimized.

Copy link
Member

commented Jul 12, 2016

😢 Thanks for filing. I confirmed 15.2.1 is still broken too. Most likely more fallout from #6406.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2016

Number inputs are broken in React, due to #6556, which was filed before #6406 was merged. You're going to see the same problem with email inputs (as per #6368, which is basically the same issue, again filed before #6406 was merged).

It is entirely possible that #6406 exposes that bug for defaultValue, but the bug was always there in the React core and was always visible for controlled inputs.

This has been broken in React for as long as I can remember:

var NumberInput = React.createClass({
  getInitialState: function() { return {value: 3.14}; },
  render: function() {
    return <input type="number" value={this.state.value} onChange={(e)=>{this.setState({value: e.target.value});}} />;
  }
});

ReactDOM.render(
  <NumberInput />,
  document.getElementById('container')
);

A workaround is to set type=text and do manual value filtering to ensure that only numbers are entered into the textbox. This has the downside that you don't get the numeric keyboard on mobile, but we'll need to figure out a solution to #6556 to handle that properly.

@STRML

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2016

@jimfb Understood, it's very frustrating that there is no way to get the raw value out of an <input type="number">. I think, though, that it deserves some investigation into why this bug does not manifest in Firefox, as I believe it still follows the spec on that one.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2016

@STRML That's because Firefox doesn't fire an onChange if the input's value is invalid. It's an interesting way of sidestepping the issue in the common case, since the onChange behavior is presumably undefined when the input is in an illegal state.

@stephenjwatkins

This comment has been minimized.

Copy link

commented Jul 21, 2016

This has been broken in React for as long as I can remember

Based on these two codepens, this bug was introduced in 15.2.0.

15.1.0: http://codepen.io/stephenjwatkins/pen/GqxGWJ [works]
15.2.0: http://codepen.io/stephenjwatkins/pen/yJpWQk [doesn't work]

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2016

Wow, did Chorme just release an update? I'm 98% positive that codepen would not have worked a week or two ago.

It looks like Chrome will now retain the "." when the user does a input.value=3

The fiddles in #6556 are now all behaving in a "nicer" way now. Looks like the ball is back in our court.

How much you want to bet that Chrome fixed this because of React?

@STRML

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2016

Still broken in 52.0.2743.82 (64-bit), are you seeing this working with 15.2.0?

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2016

What is still broken? I'm talking about the fiddles in #6556. Do those still look broken to you?

@STRML

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2016

Yes - on Chrome 52

screen shot 2016-07-21 at 4 19 39 pm

Edit: I see what you mean, the following sequence now behaves properly:

  • Type 3 - onInput calls back with '3'
  • Type . - onInput calls back with '3', previously ''
  • Type 1, onInput calls back with '3.1'
@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Jul 26, 2016

Confirmed for me on:
Version 52.0.2743.82

Specifically, it looks like input.value is lopping off the decimal point:

screen shot 2016-07-26 at 7 40 58 am

To reproduce:

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <title>Chrome Backspace Issue</title>
  </head>
  <body>
    <div id="container"></div>

    <script src="../../build/react.js"></script>
    <script src="../../build/react-dom.js"></script>
    <script src="https://cdnjs.cloudflare.com/ajax/libs/babel-core/5.8.24/browser.min.js"></script>

    <script type="text/babel">
      var NumberInput = React.createClass({
        render: function() {
          return <input type="number" defaultValue={ this.props.defaultValue } />;
        }
      });

      ReactDOM.render(
        <NumberInput defaultValue={3.14} />,
        document.getElementById('container')
      );
    </script>
  </body>
</html>

If you backspace to clear the 14 in 3.14, Chrome is reporting input.value to be 3, instead of 3..

Do I have this right? And should uncontrolled inputs even assign values to begin with?

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Jul 26, 2016

Would it be crazy to consider not emitting changes for invalid input? In the case I've detailed above, the stepMismatch validation returns:

screen shot 2016-07-26 at 7 59 51 am

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Jul 27, 2016

Ahhhh interesting. Maybe I was just in the dark, but assigning defaultValue causes side-effects on value. It seems to trigger validation on value.

Check out this Codepen:

http://codepen.io/nhunzaker/pen/zBaokp

wat

This covers uncontrolled inputs, but controlled inputs follow different rules and seem to rely on value, which always reports 3 instead of 3. for me.

I've sent out a PR here:

#7359

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Jul 27, 2016

@jimfb You mentioned:

That's because Firefox doesn't fire an onChange if the input's value is invalid. It's an interesting way of sidestepping the issue in the common case, since the onChange behavior is presumably undefined when the input is in an illegal state.

This seems ideal to me: to trade off limiting change exposure for the sake of stable inputs. Is there any interest in moving towards this?

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2016

@nhunzaker Sorry, I'm on hackamonth, haven't really been following these threads super closely.

We can't mimic Firefox's behavior, because we can differentiate between the user clearing their text input and an invalid value, AFAIK.

However, I think Chrome fixed their inputs to work correctly, so this class of issues is now fixable. We would almost certainly be interested in a PR. I'll pass this off to @zpao @spicyj and @gaearon.

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Jul 27, 2016

No worries, sounds fun!

If there's a quick reference point, could you reveal more about the Chrome fix? What was actually broken?

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2016

@nhunzaker I don't know any quick reference points other than #6556, but the basic problem was that we couldn't get the actual input value (as entered by the user) when the input was in an invalid state (eg. as the user is typing "3.14", there is an illegal intermediate state "3.") and setting the last known legal value would remove the decimal point. Firefox's fix was to not fire onInput for illegal states, Chrome's fix was to not remove the decimal if the value is approximately the same ("3" instead of "3.").

You'll need to do a little digging to figure out the exact behaviors of the various browsers and figure out what a good fix would look like. I don't know the answer off the top of my head, but it looks like things are now sufficiently fixed on the browser's end, so this should be fixable now in React.

FWIW, it may be worth tracking keystrokes and cursor events and paste events, and maintain a perfect shadow state. Since we don't have to deal with non-numeric-ish characters, we don't need to worry about foreign languages (which is where all the input edge cases come from). I think this would allow us to simulate what the browser would do (allow us to know what value is actually in the text box) and sidestep all the weirdness of the type=number spec. Anyway, I'll let you guys figure that out.

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2016

Okay! So it looks like the issue here is that value is being assigned using the standard React method of updating attributes when the props get passed in here:

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/ReactDOMComponent.js#L926

There is some logic in ReactDOMInput to prevent duplicate values from being assigned here:

https://github.com/facebook/react/blob/master/src/renderers/dom/client/wrappers/ReactDOMInput.js#L201-L210

Unfortunately the new value has already been assigned.

So my first quick hack was to add an exception for value assignment so that it never sets the value attribute if it hasn't changed:

master...nhunzaker:nh-chrome-backspace

I don't really know why this works. I didn't think node.setAttribute('value', value) was meaningful. Still, it fixes all of the issues I've encountered.

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2016

Here's a quick GIF:

cases

Now it looks like I was dumb and pushed this update to the uncontrolled input fix I sent out here:

#7359

I'll work on getting those tests to pass until we can figure out what to do.

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2016

Okay. I've polished up #7359. It fixes all issues with uncontrolled and controlled inputs that I have encountered.

I'll put in some extra time to manually test this tomorrow. I'm still struggling to add automated test coverage for these quirks.

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2016

Here are the cases that I'm covering, using the following code as a test case:

https://gist.github.com/nhunzaker/923efbaddca16ae2384b547555157f61

Do not drop off the decimal place when backspacing

This happens because React sees "3." and tries to compare it against "3". Since it's a string comparison, these are always different.

before:

backspace-wrong

after:

backspace-good

Do not clear on bad input

This is sort of correct. This works because I'm doing parseInt(input.value) || 0. This makes NaN fallback to 0 if the number isn't recognized. This may require some documentation updating on best practices (or we should talk about it).

before:

bad-input-wrong

after:

bad-input

Do not "expand" values

This happens because parseFloat will parse "3e1" into 30. React did comparison based on string values, and "3e1" is always different than "30":

before:

expando-wrong

after:

expando-good


Pretty neat! Anything else I've missed?

@ccorcos

This comment has been minimized.

Copy link

commented Sep 20, 2016

I would suggest going one of two ways:

  1. Treat value and onChange just like with a text input. Leave it up to the developer to validate input, format, and prevent invalid numbers, etc. I've created all the validators I want and everything works great for a text input, but then I don't get the number pad on mobile...

  2. Prevent invalid input and let the developer handle the edge cases for "12e", "-", and ".". Meanwhile, do not allow multiple ".", "-", or "e". And enforce "-" must always be at the beginning. This is effectively what I've created with my logic for the text input.

@martin-svk

This comment has been minimized.

Copy link

commented Nov 18, 2016

What is the current state of this issue?

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Nov 19, 2016

I have an outstanding PR here:
#7359

Looks like it's now in conflict with master. I'll address those conflicts.

Update: Sorry. Looks like something on CI. Upstreamed anyway.

@STRML

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2016

Spam again... Seen a lot of this lately

On Nov 20, 2016 1:27 PM, "qagiux" notifications@github.com wrote:

Gonna do the first half of this in https://goo.gl/JBljdR


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7253 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABJFP7sCqRcqL6lHOT7LfnoSlIIRQrJYks5rAJ8sgaJpZM4JK6Nl
.

@nksfrank

This comment has been minimized.

Copy link

commented Mar 9, 2017

What's the state of this issue? We are still encountering this on v15.4.2

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Mar 9, 2017

@nksfrank I believe the plan is to ship this with 15.5:
#8854

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Mar 27, 2017

This should be addressed in 15.5, now that we've merged the number input PR:
#7359 (comment)

I'll leave this ticket open, I don't know what the protocol is on closing tickets before a release with a fix has been published.

@nhunzaker

This comment has been minimized.

Copy link
Collaborator

commented Mar 27, 2017

cc @aweary I just realized I can't close this :(

@STRML

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2017

Fixed! https://jsfiddle.net/09Lxxzn9/1/

Thanks for your hard work @nhunzaker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.