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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
0c8f935
Only re-assign defaultValue if it is different
nhunzaker Jul 27, 2016
8654886
Do not set value if it is the same
nhunzaker Jul 28, 2016
0addf2b
Properly cover defaultValue
nhunzaker Jul 29, 2016
1845151
Use coercion to be smart about value assignment
nhunzaker Jul 29, 2016
1e78559
Add explanation of loose type checks in value assignment.
nhunzaker Aug 1, 2016
8ceb1f4
Add test coverage for setAttribute update.
nhunzaker Aug 1, 2016
c56f548
Only apply loose value check to text inputs
nhunzaker Sep 5, 2016
2af7e15
Fix case where empty switches to zero
nhunzaker Sep 5, 2016
a00150e
Handle zero case in controlled input
nhunzaker Sep 5, 2016
6e8c818
Correct mistake with default value assignment after rebase
nhunzaker Oct 18, 2016
60eef17
Do not assign bad input to number input
nhunzaker Oct 20, 2016
7bd5551
Only trigger number input value attribute updates on blur
nhunzaker Oct 20, 2016
36459b9
Remove reference to LinkedValueUtils
nhunzaker Nov 19, 2016
b642d92
Record new fiber tests
nhunzaker Nov 20, 2016
99e87b8
Add tests for blurred number input behavior
nhunzaker Dec 14, 2016
b2085f4
Replace onBlur wrapper with rule in ChangeEventPlugin
nhunzaker Dec 15, 2016
b0bc9d4
Sift down to only number inputs
nhunzaker Dec 15, 2016
4fc8c2e
Re-record fiber tests
nhunzaker Dec 15, 2016
d410290
Add test case for updating attribute on uncontrolled inputs. Make rel…
nhunzaker Dec 15, 2016
1e69612
Handle uncontrolled inputs, integrate fiber
nhunzaker Dec 15, 2016
d181202
Reorder boolean to mitigate DOM checks
nhunzaker Dec 15, 2016
9beadf0
Only assign value if it is different
nhunzaker Dec 22, 2016
aa30507
Add number input browser test fixtures
nhunzaker Jan 21, 2017
c44a87a
Address edge case preventing number precision lower than 1 place
nhunzaker Jan 21, 2017
d0441cc
Accommodate lack of IE9 number input support
nhunzaker Jan 21, 2017
38a583f
Remove footnotes about IE exponent issues
nhunzaker Jan 21, 2017
71d7f45
Address exception in IE9/10 ChangeEventPlugin blur event
nhunzaker Jan 21, 2017
6fe3372
Migrate over ReactDOMInput.js number input fixes to Fiber
nhunzaker Jan 21, 2017
d7f9b4e
Update number fixtures to use latest components
nhunzaker Mar 3, 2017
4a3eeca
Add number input test case for dashes and negative numbers
nhunzaker Mar 25, 2017
7cb979f
Replace trailing dash test case with replace with dash
nhunzaker Mar 25, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions fixtures/dom/src/components/Header.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ const Header = React.createClass({
<option value="/">Select a Fixture</option>
<option value="/range-inputs">Range Inputs</option>
<option value="/text-inputs">Text Inputs</option>
<option value="/number-inputs">Number Input</option>
<option value="/selects">Selects</option>
<option value="/textareas">Textareas</option>
<option value="/input-change-events">Input change events</option>
Expand Down
3 changes: 3 additions & 0 deletions fixtures/dom/src/components/fixtures/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import TextInputFixtures from './text-inputs';
import SelectFixtures from './selects';
import TextAreaFixtures from './textareas';
import InputChangeEvents from './input-change-events';
import NumberInputFixtures from './number-inputs/';

/**
* A simple routing component that renders the appropriate
Expand All @@ -22,6 +23,8 @@ const FixturesPage = React.createClass({
return <TextAreaFixtures />;
case '/input-change-events':
return <InputChangeEvents />;
case '/number-inputs':
return <NumberInputFixtures />;
default:
return <p>Please select a test fixture.</p>;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
const React = window.React;

import Fixture from '../../Fixture';

const NumberTestCase = React.createClass({
getInitialState() {
return { value: '' };
},
onChange(event) {
const parsed = parseFloat(event.target.value, 10)
const value = isNaN(parsed) ? '' : parsed

this.setState({ value })
},
render() {
return (
<Fixture>
<div>{this.props.children}</div>

<div className="control-box">
<fieldset>
<legend>Controlled</legend>
<input type="number" value={this.state.value} onChange={this.onChange} />
<span className="hint"> Value: {JSON.stringify(this.state.value)}</span>
</fieldset>

<fieldset>
<legend>Uncontrolled</legend>
<input type="number" defaultValue={0.5} />
</fieldset>
</div>
</Fixture>
);
},
});

export default NumberTestCase;
167 changes: 167 additions & 0 deletions fixtures/dom/src/components/fixtures/number-inputs/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
const React = window.React;

import FixtureSet from '../../FixtureSet';
import TestCase from '../../TestCase';
import NumberTestCase from './NumberTestCase';

const NumberInputs = React.createClass({
render() {
return (
<FixtureSet
title="Number inputs"
description="Number inputs inconsistently assign and report the value
property depending on the browser."
>
<TestCase
title="Backspacing"
description="The decimal place should not be lost"
>
<TestCase.Steps>
<li>Type "3.1"</li>
<li>Press backspace, eliminating the "1"</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
The field should read "3.", preserving the decimal place
</TestCase.ExpectedResult>

<NumberTestCase />

<p className="footnote">
<b>Notes:</b> Chrome and Safari clear trailing
decimals on blur. React makes this concession so that the
value attribute remains in sync with the value property.
</p>
</TestCase>

<TestCase
title="Decimal precision"
description="Supports decimal precision greater than 2 places"
>
<TestCase.Steps>
<li>Type "0.01"</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
The field should read "0.01"
</TestCase.ExpectedResult>

<NumberTestCase />
</TestCase>

<TestCase
title="Exponent form"
description="Supports exponent form ('2e4')"
>
<TestCase.Steps>
<li>Type "2e"</li>
<li>Type 4, to read "2e4"</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
The field should read "2e4". The parsed value should read "20000"
</TestCase.ExpectedResult>

<NumberTestCase />
</TestCase>

<TestCase
title="Exponent Form"
description="Pressing 'e' at the end"
>
<TestCase.Steps>
<li>Type "3.14"</li>
<li>Press "e", so that the input reads "3.14e"</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
The field should read "3.14e", the parsed value should be empty
</TestCase.ExpectedResult>

<NumberTestCase />
</TestCase>

<TestCase
title="Exponent Form"
description="Supports pressing 'ee' in the middle of a number"
>
<TestCase.Steps>
<li>Type "3.14"</li>
<li>Move the text cursor to after the decimal place</li>
<li>Press "e" twice, so that the value reads "3.ee14"</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
The field should read "3.ee14"
</TestCase.ExpectedResult>

<NumberTestCase />
</TestCase>

<TestCase
title="Trailing Zeroes"
description="Typing '3.0' preserves the trailing zero"
>
<TestCase.Steps>
<li>Type "3.0"</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
The field should read "3.0"
</TestCase.ExpectedResult>

<NumberTestCase />
</TestCase>

<TestCase
title="Inserting decimals precision"
description="Inserting '.' in to '300' maintains the trailing zeroes"
>
<TestCase.Steps>
<li>Type "300"</li>
<li>Move the cursor to after the "3"</li>
<li>Type "."</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
The field should read "3.00", not "3"
</TestCase.ExpectedResult>
<NumberTestCase />
</TestCase>

<TestCase
title="Replacing numbers with -"
description="Replacing a number with the '-' sign should not clear the value"
>
<TestCase.Steps>
<li>Type "3"</li>
<li>Select the entire value"</li>
<li>Type '-' to replace '3' with '-'</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
The field should read "-", not be blank.
</TestCase.ExpectedResult>
<NumberTestCase />
</TestCase>

<TestCase
title="Negative numbers"
description="Typing minus when inserting a negative number should work"
>
<TestCase.Steps>
<li>Type "-"</li>
<li>Type '3'</li>
</TestCase.Steps>

<TestCase.ExpectedResult>
The field should read "-3".
</TestCase.ExpectedResult>
<NumberTestCase />
</TestCase>
</FixtureSet>
);
},
});

export default NumberInputs;
11 changes: 11 additions & 0 deletions scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -837,6 +837,8 @@ src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js
* should set className to empty string instead of null
* should remove property properly for boolean properties
* should remove property properly even with different name
* should update an empty attribute to zero
* should always assign the value attribute for non-inputs
* should remove attributes for normal properties
* should not remove attributes for special properties
* should not leave all options selected when deleting multiple
Expand Down Expand Up @@ -1409,6 +1411,10 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
* should allow setting `value` to `objToString`
* should not incur unnecessary DOM mutations
* should properly control a value of number `0`
* should properly control 0.0 for a text input
* should properly control 0.0 for a number input
* should properly transition from an empty value to 0
* should properly transition from 0 to an empty value
* should have the correct target value
* should not set a value for submit buttons unnecessarily
* should control radio buttons
Expand Down Expand Up @@ -1442,6 +1448,11 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
* sets value properly with type coming later in props
* does not raise a validation warning when it switches types
* resets value of date/time input to fix bugs in iOS Safari
* always sets the attribute when values change on text inputs
* does not set the value attribute on number inputs if focused
* sets the value attribute on number inputs on blur
* an uncontrolled number input will not update the value attribute on blur
* an uncontrolled text input will not update the value attribute on blur

src/renderers/dom/shared/wrappers/__tests__/ReactDOMOption-test.js
* should flatten children to a string
Expand Down
28 changes: 18 additions & 10 deletions src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,11 +138,8 @@ var ReactDOMInput = {
? props.checked
: props.defaultChecked,
initialValue: props.value != null ? props.value : defaultValue,
controlled: isControlled(props),
};

if (__DEV__) {
node._wrapperState.controlled = isControlled(props);
}
},

updateWrapper: function(element: Element, props: Object) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I wonder why this was dev-only in the first place?

Copy link
Collaborator

@sebmarkbage sebmarkbage Mar 30, 2017

Choose a reason for hiding this comment

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

It was only used for warnings. We intentionally want to minimize state and memory usage associated with controlled components. The goal is to get rid of the entire wrapper object. Making this a prod dependency makes that harder.

Expand Down Expand Up @@ -195,13 +192,24 @@ var ReactDOMInput = {

var value = props.value;
if (value != null) {
// Cast `value` to a string to ensure the value is set correctly. While
// browsers typically do this as necessary, jsdom doesn't.
var newValue = '' + value;
if (value === 0 && node.value === '') {
node.value = '0';
// Note: IE9 reports a number inputs as 'text', so check props instead.
} else if (props.type === 'number') {
// Simulate `input.valueAsNumber`. IE9 does not support it
var valueAsNumber = parseFloat(node.value, 10) || 0;

// To avoid side effects (such as losing text selection), only set value if changed
if (newValue !== node.value) {
node.value = newValue;
// eslint-disable-next-line
if (value != valueAsNumber) {
// Cast `value` to a string to ensure the value is set correctly. While
// browsers typically do this as necessary, jsdom doesn't.
node.value = '' + value;
}
// eslint-disable-next-line
} else if (value != node.value) {
// Cast `value` to a string to ensure the value is set correctly. While
// browsers typically do this as necessary, jsdom doesn't.
node.value = '' + value;
}
} else {
if (props.value == null && props.defaultValue != null) {
Expand Down
28 changes: 28 additions & 0 deletions src/renderers/dom/shared/HTMLDOMPropertyConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,34 @@ var HTMLDOMPropertyConfig = {
httpEquiv: 'http-equiv',
},
DOMPropertyNames: {},
DOMMutationMethods: {
value: function(node, value) {
if (value == null) {
return node.removeAttribute('value');
}

// Number inputs get special treatment due to some edge cases in
// Chrome. Let everything else assign the value attribute as normal.
// https://github.com/facebook/react/issues/7253#issuecomment-236074326
if (node.type !== 'number' || node.hasAttribute('value') === false) {
node.setAttribute('value', '' + value);
} else if (
node.validity &&
!node.validity.badInput &&
node.ownerDocument.activeElement !== node
) {
// Don't assign an attribute if validation reports bad
// input. Chrome will clear the value. Additionally, don't
// operate on inputs that have focus, otherwise Chrome might
// strip off trailing decimal places and cause the user's
// cursor position to jump to the beginning of the input.
//
// In ReactDOMInput, we have an onBlur event that will trigger
// this function again when focus is lost.
node.setAttribute('value', '' + value);
}
},
},
};

module.exports = HTMLDOMPropertyConfig;
29 changes: 29 additions & 0 deletions src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,35 @@ describe('DOMPropertyOperations', () => {
});
});

describe('value mutation method', function() {
it('should update an empty attribute to zero', function() {
var stubNode = document.createElement('input');
var stubInstance = {_debugID: 1};
ReactDOMComponentTree.precacheNode(stubInstance, stubNode);

stubNode.setAttribute('type', 'radio');

DOMPropertyOperations.setValueForProperty(stubNode, 'value', '');
spyOn(stubNode, 'setAttribute');
DOMPropertyOperations.setValueForProperty(stubNode, 'value', 0);

expect(stubNode.setAttribute.calls.count()).toBe(1);
});

it('should always assign the value attribute for non-inputs', function() {
var stubNode = document.createElement('progress');
var stubInstance = {_debugID: 1};
ReactDOMComponentTree.precacheNode(stubInstance, stubNode);

spyOn(stubNode, 'setAttribute');

DOMPropertyOperations.setValueForProperty(stubNode, 'value', 30);
DOMPropertyOperations.setValueForProperty(stubNode, 'value', '30');

expect(stubNode.setAttribute.calls.count()).toBe(2);
});
});

describe('deleteValueForProperty', () => {
var stubNode;
var stubInstance;
Expand Down
Loading