Use the same value synchronization function on number blur #11746
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I updated ReactDOMInput.synchronizeDefaultValue such that it assignes the defaultValue property instead of the value attribute. I never followed up on the ChangeEventPlugin's on blur behavior.
nhunzaker
added a commit
that referenced
this pull request
Dec 1, 2017
gaearon
reviewed
Dec 1, 2017
| @@ -235,10 +236,7 @@ function handleControlledInputBlur(inst, node) { | |||
| } | |||
|
|
|||
| // If controlled, assign the value attribute to the current value on blur | |||
There was a problem hiding this comment.
I don't think so. Though the controlled input guard triggers above this line. It is not apart of this specific block.
There was a problem hiding this comment.
But you asked! So we could probably improve it. Something like:
// Focused number inputs do not update their defaultValue on change. This
// avoids unexpected value changes in Chrome while the user is typing. On blur,
// we need to synchronize the defaultValue to match standard input behavior.
|
Is there any difference in observable behavior here? Does this need a fixture? |
I don't think so. We just need to test the number input fixtures. |
|
Tested in:
|
nhunzaker
added a commit
that referenced
this pull request
Dec 2, 2017
This was referenced Dec 4, 2017
gaearon
pushed a commit
that referenced
this pull request
Dec 4, 2017
…1741) * Ensure value and defaultValue do not assign functions and symbols * Eliminate assignProperty method from ReactDOMInput * Restore original placement of defaultValue reservedProp * Reduce branching. Make assignment more consistent * Control for warnings in symbol/function tests * Add boolean to readOnly assignments * Tweak the tests * Invalid value attributes should convert to an empty string * Revert ChangeEventPlugin update. See #11746 * Format * Replace shouldSetAttribute call with value specific type check DOMProperty.shouldSetAttribute runs a few other checks that aren't appropriate for determining if a value or defaultValue should be assigned on an input. This commit replaces that call with an input specific check. * Remove unused import * Eliminate unnecessary numeric equality checks (#11751) * Eliminate unnecessary numeric equality checks This commit changes the way numeric equality for number inputs works such that it compares against `input.valueAsNumber`. This eliminates quite a bit of branching around numeric equality. * There is no need to compare valueAsNumber * Add test cases for empty string to 0. * Avoid implicit boolean JSX props * Split up numeric equality test to isolate eslint disable command * Fix typo in ReactDOMInput test * Add todos * Update the attribute table
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I updated ReactDOMInput.synchronizeDefaultValue such that it assignes
the defaultValue property instead of the value attribute. I never
followed up on the ChangeEventPlugin's on blur behavior.
Follow-up from https://github.com/facebook/react/pull/11741/files
Fixes #8395