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

Fix Chrome number input backspace and invalid input issue #7359

Merged
merged 31 commits into from
Mar 27, 2017

Conversation

nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Jul 27, 2016

In Google Chrome, backspacing an controlled and uncontrolled number inputs causes an unexpected behavior:

backspace

More cases (and what fixes look like) here: #7253 (comment)

Issues with uncontrolled inputs

This is because assigning defaultValue causes side-effects on value. It seems to trigger validation on value as if it were directly assigned itself.

This Codepen provides a step by step reproduction case:

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

wat

This PR wraps the defaultValue assignment in a conditional so that it will only ever be assigned if the defaultValue has changed.

Issues with controlled inputs

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.

This PR adds a check for the value prop, and only assigns it if is different. It never assigns the value attribute if it hasn't changed:

I don't really know why this works. I didn't think node.setAttribute('value', value) was meaningful. What do you think?


Related to #7253

} else if (attributeName === 'value') {
if (node.value != value) {
node.value = '' + value;
}
Copy link
Member

Choose a reason for hiding this comment

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

Ironically our code used to look a lot like this… https://github.com/facebook/react/blob/0.14-stable/src/renderers/dom/shared/DOMPropertyOperations.js#L187-L197

@jimfb - I think this is more fallout from your changes. Can you take a look and figure out a way forward?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack! I crossed the streams!

This PR was supposed to be solely for fixing defaultValue, but it looks I got my branches mixed up and pushed the more general fix for both inputs. I actually just replied here:

#7253 (comment)

Again, pushing to this branch was a total accident. A bunch of tests fail now. I'll work on getting those to pass in the mean time, but could definitely use some advice.

@ghost ghost added the CLA Signed label Jul 28, 2016
@nhunzaker
Copy link
Contributor Author

@zpao Thanks for the historical insight. All tests pass. I believe I've caught every edge case. I'll enumerate them in #7253. I'm happy to add back the HAS_SIDE_EFFECTS constant, however this seems to be truly specific to the value attribute. What do you think?

@nhunzaker nhunzaker changed the title Fix Chrome uncontrolled number input backspace issue. Only re-assign defaultValue if it is different Fix Chrome number input backspace and invalid input issue Jul 29, 2016
@ghost ghost added the CLA Signed label Jul 29, 2016
@nhunzaker
Copy link
Contributor Author

Cases enumerated here:

#7253 (comment)

@nhunzaker
Copy link
Contributor Author

nhunzaker commented Jul 29, 2016

Ah and it looks like lint is going to fail because my solution relies on loose type coercion. Hmm. Do I have permission to ignore these rules?

@@ -158,6 +158,10 @@ var DOMPropertyOperations = {
} else if (propertyInfo.hasBooleanValue ||
(propertyInfo.hasOverloadedBooleanValue && value === true)) {
node.setAttribute(attributeName, '');
} else if (attributeName === 'value' && node.hasAttribute('value')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type coercion solved my problems, however it makes the initial case hard. Empty inputs are always equal to 0.

I think this could be better. Any suggestions?

@nhunzaker nhunzaker force-pushed the nh-chrome-backspace branch 2 times, most recently from 1e2a9ab to dcd31d3 Compare August 1, 2016 12:20
@nhunzaker
Copy link
Contributor Author

Added ignore statements for ESLint rules and a test for setAttributeForProperty.

// Use loose coercion to prevent replacement on comparisons like
// '3e1' == 30 in Chrome (~52).
if (node.value != value) { // eslint-disable-line
node.setAttribute(attributeName, '' + value);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing I'm not clear on yet: Why assign the value attribute at all?.

ReactDOMInput will assign the value property in updateWrapper. We could do that in mountWrapper as well. Is it important that the attribute be assigned?

Copy link
Contributor

Choose a reason for hiding this comment

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

So, there's a difference between setting node.value and calling node.setAttribute('value'). node.value tracks the value of the node in JS, and setAttribute will actually set the attribute on the element in the DOM.

This change causes an issue where the DOM attribute is never updated, because node.value is already updated prior to this check, so it never actually sets the attribute.

Copy link
Contributor Author

@nhunzaker nhunzaker Sep 4, 2016

Choose a reason for hiding this comment

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

Hmm. So actually: should the value attribute update for anything other than checkbox and radio?

Following browser behavior, editing the text inside of a text input doesn't update the value attribute.

Possible my current code doesn't do this, but to me that's the correct functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

React does deviate from browser behavior when it comes to controlled components (e.g., change events on keydown instead of on blur). Currently the value attribute does update on controlled inputs, and it looks like we're explicitly setting it, so there may be a good reason I'm not aware of.

cc @spicyj @zpao

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an extra complication here... Assigning the value attribute forces validation for number inputs. Check this out:

control

Notice how the decimal gets stripped? My fix focuses on avoiding touching the input as much as possible to avoid unexpected behavior like this. Fortunately, I believe this also conforms to the standard way inputs work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the current behavior of setting the attribute is intentional though I could be misremembering. I know we definitely need to set value on the <progress> element. You can look at the history of ReactDOMInput to see when jimfb last made changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like this is taken care of, because node.value on progress is never assigned, so it's out of sync with props.value, and it reassigns the attribute.

progress

@STRML
Copy link
Contributor

STRML commented Aug 24, 2016

Anything this is waiting on? This is a pretty obnoxious bug. Thanks so much to @nhunzaker for putting this PR together.

@ghost ghost added the CLA Signed label Aug 24, 2016
@aweary
Copy link
Contributor

aweary commented Aug 24, 2016

I'll try and get this reviewed when I get back from vacation next week, if no one else gets a chance to do it in the meantime. Overall it looks good to me though!

@nhunzaker
Copy link
Contributor Author

nhunzaker commented Aug 25, 2016

@aweary No worries -- I'm on vacation too! Looking forward to getting this one fix in...

@nhunzaker
Copy link
Contributor Author

Rebased with master. I also added @iangilman's case for trailing dashes, and a test case for negative numbers:

dashes

@jquense
Copy link
Contributor

jquense commented Mar 25, 2017

I'm interested in why 3- needs to be supported (if only to make sure react-widgets does it)

@nhunzaker
Copy link
Contributor Author

@jquense You know what, I think I have this wrong. The test case is when you select the entire number value and - to start typing a new number. Stable react sees - as an invalid number and wipes away the value.

I'll update the test case. I also need to figure out what to do about prettier formatting. prettier wants to put my ESLint ignore comments on a new line, which causes linting to fail.

Away I go.

@jquense
Copy link
Contributor

jquense commented Mar 25, 2017

ah gotcha. If it helps btw please feel free to look at https://github.com/jquense/react-widgets/blob/v4/packages/react-widgets/src/NumberInput.js to see how i fumbled through on this logic

@nhunzaker
Copy link
Contributor Author

Cool. I just had to update the test case, everything works fine. If the number isn't valid, it parses to a blank value, which is also what the number input reports, so no change occurs.

I've also run prettier on this branch and dealt with the associated eslint issue.

@nhunzaker
Copy link
Contributor Author

@jquense This NumberInput is cool. How crazy would it be to bake something like this into React?

Later... later.. I want to get this PR merged so badly, haha.

Copy link
Contributor

@aweary aweary left a comment

Choose a reason for hiding this comment

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

Later... later.. I want to get this PR merged so badly, haha.

Oh yeah? well, how about we go ahead and do that then 😄 I was able to test this out today using the DOM fixtures. I verified everything seems to be working correctly in:

  • IE9
  • IE10
  • IE11
  • Edge 14
  • Chrome 57 (Windows)
  • Chrome 57 (macOS)
  • Chrome (Android, Galaxy S5)
  • Firefox 52 (Windows)
  • Firefox 52 (macOS)
  • Opera 44 (Windows)
  • Safari 10 (macOS)
  • Safari (iOS 9)
  • Safari (iOS 8)

Everything appears to be working great, and I see nothing immediately wrong with the changes made. So, LGTM!

@aweary aweary merged commit 29d9710 into facebook:master Mar 27, 2017
@aweary aweary added this to the 15-hipri milestone Mar 27, 2017
bvaughn pushed a commit that referenced this pull request Mar 29, 2017
* Only re-assign defaultValue if it is different

* Do not set value if it is the same

* Properly cover defaultValue

* Use coercion to be smart about value assignment

* Add explanation of loose type checks in value assignment.

* Add test coverage for setAttribute update.

* Only apply loose value check to text inputs

* Fix case where empty switches to zero

* Handle zero case in controlled input

* Correct mistake with default value assignment after rebase

* Do not assign bad input to number input

* Only trigger number input value attribute updates on blur

* Remove reference to LinkedValueUtils

* Record new fiber tests

* Add tests for blurred number input behavior

* Replace onBlur wrapper with rule in ChangeEventPlugin

* Sift down to only number inputs

* Re-record fiber tests

* Add test case for updating attribute on uncontrolled inputs. Make related correction

* Handle uncontrolled inputs, integrate fiber

* Reorder boolean to mitigate DOM checks

* Only assign value if it is different

* Add number input browser test fixtures

During the course of the number input fix, we uncovered many edge
cases. This commit adds browser test fixtures for each of those instances.

* Address edge case preventing number precision lower than 1 place

0.0 coerces to 0, however they are not the same value when doing
string comparision. This prevented controlled number inputs from
inputing the characters `0.00`.

Also adds test cases.

* Accommodate lack of IE9 number input support

IE9 does not support number inputs. Number inputs in IE9 fallback to
traditional text inputs. This means that accessing `input.value` will
report the raw text, rather than parsing a numeric value.

This commit makes the ReactDOMInput wrapper check to see if the `type`
prop has been configured to `"number"`. In those cases, it will
perform a comparison based upon `parseFloat` instead of the raw input
value.

* Remove footnotes about IE exponent issues

With the recent IE9 fix, IE properly inserts `e` when it produces an
invalid number.

* Address exception in IE9/10 ChangeEventPlugin blur event

On blur, inputs have their values assigned. This is so that number
inputs do not conduct unexpected behavior in
Chrome/Safari. Unfortunately, there are cases where the target
instance might be undefined in IE9/10, raising an exception.

* Migrate over ReactDOMInput.js number input fixes to Fiber

Also re-record tests

* Update number fixtures to use latest components

* Add number input test case for dashes and negative numbers

* Replace trailing dash test case with replace with dash

Also run prettier
// If controlled, assign the value attribute to the current value on blur
let value = '' + node.value;
if (node.getAttribute('value') !== value) {
node.setAttribute('value', value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I'm not caught up on the entire history here.

Why is this special case needed? extractEvents should not have side-effects in it.

Can this be handled by the normal controlledness flow in updateWrapper?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be enough to flag this with enqueueStateRestore so that the normal updateWrapper kicks in at the end of the blur event?

Copy link
Contributor Author

@nhunzaker nhunzaker Mar 30, 2017

Choose a reason for hiding this comment

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

I can verify. As for history:

React controlled inputs sync up the value attribute with the value property. This prevents state mismatches with form.reset() and browser plugins (though i never got a reply on which ones). Unfortunately, on number inputs in Chrome and Safari, this forces a check on the input, removing trailing decimal places, invalid characters, etc.

The only way i could get around this (i tried for months, off and on) was to defer setting the value attribute on blur.

I haven't done much with enqueueStateRestore. I'll give it a go.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO we should just not set the value attribute. People writing manual DOM operations don't do that and it works fine for them, so not sure why we do.

Would just avoiding setting the attribute all together fix the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be lovely. The only edge case is form.reset(). Honestly, I'd be in favor of just adding a warning about this, or enqueing updateWrapper on the reset form event if it can be done.

Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to be clear that form.reset is totally fine when the form contains only uncontrolled inputs. If we did go the route of warning it should only warn if the form contains one or more controlled inputs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think just .reset() and CSS input[value=foo] selectors. TBH the latter was a pretty common feature request so I'd be sad to see it go.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are other attributes that matter. Such as all of those that don't have corresponding properties (generally things outside the HTML spec). There are edge cases that need removing attributes (like progress). However, for value (or checked) I don't believe there were any cases that needed it outside of intuition. E.g. the attribute shows up in DOM dev tools in Chrome etc.

However, there is nothing that relies on it being there because all those thousands of websites that do manual DOM management. Nobody manually calls setAttribute in those cases. However, as we've seen, there are cases where setting it all the time is unusual such as in this case. So it leads to undiscovered browser deviations.

That's why I think our design should be to model what a user would write manually - even if that seems unintuitive of how people think React works.

Copy link
Contributor Author

@nhunzaker nhunzaker Mar 31, 2017

Choose a reason for hiding this comment

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

In most cases I can think of with manual manipulation it's actually undesirable to set the value attribute because you want to rely on form.reset. at least, this has always been the case for me. Being able to style text inputs based on a dynamic value attribute is a privilege react provides, one I would be comfortable with no longer having.

React doing Dom manipulation, the correct way, is the abstraction I've always had. I think this is a very strong metaphor that makes it easier to reason about what react is doing. We deviate from that with controlled inputs. I'm in favor of this change, which moves closer towards that idea.

So I agree, armchair theory aside 😂. I can start scaffolding out what that would look like, but I won't be able to pick it up until Monday, if someone else wants to give it a shot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oof. I finally did it. Please see:

#10150

@btoueg
Copy link

btoueg commented Apr 12, 2017

I've bumped my project to 15.5.4 and the issue is still there.

@gaearon
Copy link
Collaborator

gaearon commented Apr 12, 2017

@btoueg Provide a reproducing case please?

@btoueg
Copy link

btoueg commented Jul 12, 2017

Sorry for the delay. My report was a false negative. Works fine now, congrats on fixing the issue.

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.