Skip to content

Commit 29d9710

Browse files
nhunzakeraweary
nhunzaker
authored and
aweary
committed
Fix Chrome number input backspace and invalid input issue (#7359)
* 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
1 parent c51411c commit 29d9710

File tree

11 files changed

+463
-20
lines changed

11 files changed

+463
-20
lines changed

‎fixtures/dom/src/components/Header.js

+1
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ const Header = React.createClass({
4444
<option value="/">Select a Fixture</option>
4545
<option value="/range-inputs">Range Inputs</option>
4646
<option value="/text-inputs">Text Inputs</option>
47+
<option value="/number-inputs">Number Input</option>
4748
<option value="/selects">Selects</option>
4849
<option value="/textareas">Textareas</option>
4950
<option value="/input-change-events">Input change events</option>

‎fixtures/dom/src/components/fixtures/index.js

+3
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import TextInputFixtures from './text-inputs';
44
import SelectFixtures from './selects';
55
import TextAreaFixtures from './textareas';
66
import InputChangeEvents from './input-change-events';
7+
import NumberInputFixtures from './number-inputs/';
78

89
/**
910
* A simple routing component that renders the appropriate
@@ -22,6 +23,8 @@ const FixturesPage = React.createClass({
2223
return <TextAreaFixtures />;
2324
case '/input-change-events':
2425
return <InputChangeEvents />;
26+
case '/number-inputs':
27+
return <NumberInputFixtures />;
2528
default:
2629
return <p>Please select a test fixture.</p>;
2730
}

‎fixtures/dom/src/components/fixtures/number-inputs/NumberTestCase.js

+37
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
const React = window.React;
2+
3+
import Fixture from '../../Fixture';
4+
5+
const NumberTestCase = React.createClass({
6+
getInitialState() {
7+
return { value: '' };
8+
},
9+
onChange(event) {
10+
const parsed = parseFloat(event.target.value, 10)
11+
const value = isNaN(parsed) ? '' : parsed
12+
13+
this.setState({ value })
14+
},
15+
render() {
16+
return (
17+
<Fixture>
18+
<div>{this.props.children}</div>
19+
20+
<div className="control-box">
21+
<fieldset>
22+
<legend>Controlled</legend>
23+
<input type="number" value={this.state.value} onChange={this.onChange} />
24+
<span className="hint"> Value: {JSON.stringify(this.state.value)}</span>
25+
</fieldset>
26+
27+
<fieldset>
28+
<legend>Uncontrolled</legend>
29+
<input type="number" defaultValue={0.5} />
30+
</fieldset>
31+
</div>
32+
</Fixture>
33+
);
34+
},
35+
});
36+
37+
export default NumberTestCase;

‎fixtures/dom/src/components/fixtures/number-inputs/index.js

+167
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
const React = window.React;
2+
3+
import FixtureSet from '../../FixtureSet';
4+
import TestCase from '../../TestCase';
5+
import NumberTestCase from './NumberTestCase';
6+
7+
const NumberInputs = React.createClass({
8+
render() {
9+
return (
10+
<FixtureSet
11+
title="Number inputs"
12+
description="Number inputs inconsistently assign and report the value
13+
property depending on the browser."
14+
>
15+
<TestCase
16+
title="Backspacing"
17+
description="The decimal place should not be lost"
18+
>
19+
<TestCase.Steps>
20+
<li>Type "3.1"</li>
21+
<li>Press backspace, eliminating the "1"</li>
22+
</TestCase.Steps>
23+
24+
<TestCase.ExpectedResult>
25+
The field should read "3.", preserving the decimal place
26+
</TestCase.ExpectedResult>
27+
28+
<NumberTestCase />
29+
30+
<p className="footnote">
31+
<b>Notes:</b> Chrome and Safari clear trailing
32+
decimals on blur. React makes this concession so that the
33+
value attribute remains in sync with the value property.
34+
</p>
35+
</TestCase>
36+
37+
<TestCase
38+
title="Decimal precision"
39+
description="Supports decimal precision greater than 2 places"
40+
>
41+
<TestCase.Steps>
42+
<li>Type "0.01"</li>
43+
</TestCase.Steps>
44+
45+
<TestCase.ExpectedResult>
46+
The field should read "0.01"
47+
</TestCase.ExpectedResult>
48+
49+
<NumberTestCase />
50+
</TestCase>
51+
52+
<TestCase
53+
title="Exponent form"
54+
description="Supports exponent form ('2e4')"
55+
>
56+
<TestCase.Steps>
57+
<li>Type "2e"</li>
58+
<li>Type 4, to read "2e4"</li>
59+
</TestCase.Steps>
60+
61+
<TestCase.ExpectedResult>
62+
The field should read "2e4". The parsed value should read "20000"
63+
</TestCase.ExpectedResult>
64+
65+
<NumberTestCase />
66+
</TestCase>
67+
68+
<TestCase
69+
title="Exponent Form"
70+
description="Pressing 'e' at the end"
71+
>
72+
<TestCase.Steps>
73+
<li>Type "3.14"</li>
74+
<li>Press "e", so that the input reads "3.14e"</li>
75+
</TestCase.Steps>
76+
77+
<TestCase.ExpectedResult>
78+
The field should read "3.14e", the parsed value should be empty
79+
</TestCase.ExpectedResult>
80+
81+
<NumberTestCase />
82+
</TestCase>
83+
84+
<TestCase
85+
title="Exponent Form"
86+
description="Supports pressing 'ee' in the middle of a number"
87+
>
88+
<TestCase.Steps>
89+
<li>Type "3.14"</li>
90+
<li>Move the text cursor to after the decimal place</li>
91+
<li>Press "e" twice, so that the value reads "3.ee14"</li>
92+
</TestCase.Steps>
93+
94+
<TestCase.ExpectedResult>
95+
The field should read "3.ee14"
96+
</TestCase.ExpectedResult>
97+
98+
<NumberTestCase />
99+
</TestCase>
100+
101+
<TestCase
102+
title="Trailing Zeroes"
103+
description="Typing '3.0' preserves the trailing zero"
104+
>
105+
<TestCase.Steps>
106+
<li>Type "3.0"</li>
107+
</TestCase.Steps>
108+
109+
<TestCase.ExpectedResult>
110+
The field should read "3.0"
111+
</TestCase.ExpectedResult>
112+
113+
<NumberTestCase />
114+
</TestCase>
115+
116+
<TestCase
117+
title="Inserting decimals precision"
118+
description="Inserting '.' in to '300' maintains the trailing zeroes"
119+
>
120+
<TestCase.Steps>
121+
<li>Type "300"</li>
122+
<li>Move the cursor to after the "3"</li>
123+
<li>Type "."</li>
124+
</TestCase.Steps>
125+
126+
<TestCase.ExpectedResult>
127+
The field should read "3.00", not "3"
128+
</TestCase.ExpectedResult>
129+
<NumberTestCase />
130+
</TestCase>
131+
132+
<TestCase
133+
title="Replacing numbers with -"
134+
description="Replacing a number with the '-' sign should not clear the value"
135+
>
136+
<TestCase.Steps>
137+
<li>Type "3"</li>
138+
<li>Select the entire value"</li>
139+
<li>Type '-' to replace '3' with '-'</li>
140+
</TestCase.Steps>
141+
142+
<TestCase.ExpectedResult>
143+
The field should read "-", not be blank.
144+
</TestCase.ExpectedResult>
145+
<NumberTestCase />
146+
</TestCase>
147+
148+
<TestCase
149+
title="Negative numbers"
150+
description="Typing minus when inserting a negative number should work"
151+
>
152+
<TestCase.Steps>
153+
<li>Type "-"</li>
154+
<li>Type '3'</li>
155+
</TestCase.Steps>
156+
157+
<TestCase.ExpectedResult>
158+
The field should read "-3".
159+
</TestCase.ExpectedResult>
160+
<NumberTestCase />
161+
</TestCase>
162+
</FixtureSet>
163+
);
164+
},
165+
});
166+
167+
export default NumberInputs;

‎scripts/fiber/tests-passing.txt

+11
Original file line numberDiff line numberDiff line change
@@ -837,6 +837,8 @@ src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js
837837
* should set className to empty string instead of null
838838
* should remove property properly for boolean properties
839839
* should remove property properly even with different name
840+
* should update an empty attribute to zero
841+
* should always assign the value attribute for non-inputs
840842
* should remove attributes for normal properties
841843
* should not remove attributes for special properties
842844
* should not leave all options selected when deleting multiple
@@ -1449,6 +1451,10 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
14491451
* should allow setting `value` to `objToString`
14501452
* should not incur unnecessary DOM mutations
14511453
* should properly control a value of number `0`
1454+
* should properly control 0.0 for a text input
1455+
* should properly control 0.0 for a number input
1456+
* should properly transition from an empty value to 0
1457+
* should properly transition from 0 to an empty value
14521458
* should have the correct target value
14531459
* should not set a value for submit buttons unnecessarily
14541460
* should control radio buttons
@@ -1482,6 +1488,11 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js
14821488
* sets value properly with type coming later in props
14831489
* does not raise a validation warning when it switches types
14841490
* resets value of date/time input to fix bugs in iOS Safari
1491+
* always sets the attribute when values change on text inputs
1492+
* does not set the value attribute on number inputs if focused
1493+
* sets the value attribute on number inputs on blur
1494+
* an uncontrolled number input will not update the value attribute on blur
1495+
* an uncontrolled text input will not update the value attribute on blur
14851496

14861497
src/renderers/dom/shared/wrappers/__tests__/ReactDOMOption-test.js
14871498
* should flatten children to a string

‎src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js

+18-10
Original file line numberDiff line numberDiff line change
@@ -138,11 +138,8 @@ var ReactDOMInput = {
138138
? props.checked
139139
: props.defaultChecked,
140140
initialValue: props.value != null ? props.value : defaultValue,
141+
controlled: isControlled(props),
141142
};
142-
143-
if (__DEV__) {
144-
node._wrapperState.controlled = isControlled(props);
145-
}
146143
},
147144

148145
updateWrapper: function(element: Element, props: Object) {
@@ -195,13 +192,24 @@ var ReactDOMInput = {
195192

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

202-
// To avoid side effects (such as losing text selection), only set value if changed
203-
if (newValue !== node.value) {
204-
node.value = newValue;
202+
// eslint-disable-next-line
203+
if (value != valueAsNumber) {
204+
// Cast `value` to a string to ensure the value is set correctly. While
205+
// browsers typically do this as necessary, jsdom doesn't.
206+
node.value = '' + value;
207+
}
208+
// eslint-disable-next-line
209+
} else if (value != node.value) {
210+
// Cast `value` to a string to ensure the value is set correctly. While
211+
// browsers typically do this as necessary, jsdom doesn't.
212+
node.value = '' + value;
205213
}
206214
} else {
207215
if (props.value == null && props.defaultValue != null) {

‎src/renderers/dom/shared/HTMLDOMPropertyConfig.js

+28
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,34 @@ var HTMLDOMPropertyConfig = {
210210
httpEquiv: 'http-equiv',
211211
},
212212
DOMPropertyNames: {},
213+
DOMMutationMethods: {
214+
value: function(node, value) {
215+
if (value == null) {
216+
return node.removeAttribute('value');
217+
}
218+
219+
// Number inputs get special treatment due to some edge cases in
220+
// Chrome. Let everything else assign the value attribute as normal.
221+
// https://github.com/facebook/react/issues/7253#issuecomment-236074326
222+
if (node.type !== 'number' || node.hasAttribute('value') === false) {
223+
node.setAttribute('value', '' + value);
224+
} else if (
225+
node.validity &&
226+
!node.validity.badInput &&
227+
node.ownerDocument.activeElement !== node
228+
) {
229+
// Don't assign an attribute if validation reports bad
230+
// input. Chrome will clear the value. Additionally, don't
231+
// operate on inputs that have focus, otherwise Chrome might
232+
// strip off trailing decimal places and cause the user's
233+
// cursor position to jump to the beginning of the input.
234+
//
235+
// In ReactDOMInput, we have an onBlur event that will trigger
236+
// this function again when focus is lost.
237+
node.setAttribute('value', '' + value);
238+
}
239+
},
240+
},
213241
};
214242

215243
module.exports = HTMLDOMPropertyConfig;

‎src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js

+29
Original file line numberDiff line numberDiff line change
@@ -296,6 +296,35 @@ describe('DOMPropertyOperations', () => {
296296
});
297297
});
298298

299+
describe('value mutation method', function() {
300+
it('should update an empty attribute to zero', function() {
301+
var stubNode = document.createElement('input');
302+
var stubInstance = {_debugID: 1};
303+
ReactDOMComponentTree.precacheNode(stubInstance, stubNode);
304+
305+
stubNode.setAttribute('type', 'radio');
306+
307+
DOMPropertyOperations.setValueForProperty(stubNode, 'value', '');
308+
spyOn(stubNode, 'setAttribute');
309+
DOMPropertyOperations.setValueForProperty(stubNode, 'value', 0);
310+
311+
expect(stubNode.setAttribute.calls.count()).toBe(1);
312+
});
313+
314+
it('should always assign the value attribute for non-inputs', function() {
315+
var stubNode = document.createElement('progress');
316+
var stubInstance = {_debugID: 1};
317+
ReactDOMComponentTree.precacheNode(stubInstance, stubNode);
318+
319+
spyOn(stubNode, 'setAttribute');
320+
321+
DOMPropertyOperations.setValueForProperty(stubNode, 'value', 30);
322+
DOMPropertyOperations.setValueForProperty(stubNode, 'value', '30');
323+
324+
expect(stubNode.setAttribute.calls.count()).toBe(2);
325+
});
326+
});
327+
299328
describe('deleteValueForProperty', () => {
300329
var stubNode;
301330
var stubInstance;

0 commit comments

Comments
 (0)