Skip to content

Commit fd69c23

Browse files
nhunzakergaearon
nhunzaker
authored and
gaearon
committed
Use defaultValue instead of setAttribute('value') (#11534)
* Use defaultValue instead of setAttribute('value') This commit replaces the method of synchronizing an input's value attribute from using setAttribute to assigning defaultValue. This has several benefits: - Fixes issue where IE10+ and Edge password icon disappears (#7328) - Fixes issue where toggling input types hides display value on dates in Safari (unreported) - Removes mutationMethod behaviors from DOMPropertyOperations * initialValue in Input wrapperState is always a string * The value property is assigned before the value attribute. Fix related tests. * Remove initial value tests in ReactDOMInput I added these tests after removing the `value` mutation method. However they do not add any additional value over existing tests. * Improve clarity of value checks in ReactDOMInput.postMountWrapper * Remove value and defaultValue from InputWithWrapperState type They are already included in the type definition for HTMLInputElement * Inline stringification of value in ReactDOMInput Avoids eagier stringification and makes usage more consistent. * Use consistent value/defaultValue presence in postMountHook Other methods in ReactDOMInput check for null instead of hasOwnProperty. * Add missing semicolon * Remove unused value argument in ReactDOMInput test * Address cases where a value switches to undefined When a controlled input value switches to undefined, it reverts back to the initial state of the controlled input. We didn't have test coverage for this case, so I've added two describe blocks to cover both null and undefined.
1 parent 3f736c3 commit fd69c23

File tree

6 files changed

+147
-133
lines changed

6 files changed

+147
-133
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,9 @@ function NumberInputs() {
2727
<NumberTestCase />
2828

2929
<p className="footnote">
30-
<b>Notes:</b> Chrome and Safari clear trailing decimals on blur. React
31-
makes this concession so that the value attribute remains in sync with
32-
the value property.
30+
<b>Notes:</b> Modern Chrome and Safari {'<='} 6 clear trailing
31+
decimals on blur. React makes this concession so that the value
32+
attribute remains in sync with the value property.
3333
</p>
3434
</TestCase>
3535

‎packages/react-dom/src/__tests__/ReactDOMInput-test.js

Lines changed: 86 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1249,15 +1249,20 @@ describe('ReactDOMInput', () => {
12491249
var originalCreateElement = document.createElement;
12501250
spyOnDevAndProd(document, 'createElement').and.callFake(function(type) {
12511251
var el = originalCreateElement.apply(this, arguments);
1252+
var value = '';
1253+
12521254
if (type === 'input') {
12531255
Object.defineProperty(el, 'value', {
1254-
get: function() {},
1255-
set: function() {
1256-
log.push('set value');
1256+
get: function() {
1257+
return value;
1258+
},
1259+
set: function(val) {
1260+
value = '' + val;
1261+
log.push('set property value');
12571262
},
12581263
});
1259-
spyOnDevAndProd(el, 'setAttribute').and.callFake(function(name, value) {
1260-
log.push('set ' + name);
1264+
spyOnDevAndProd(el, 'setAttribute').and.callFake(function(name) {
1265+
log.push('set attribute ' + name);
12611266
});
12621267
}
12631268
return el;
@@ -1267,14 +1272,14 @@ describe('ReactDOMInput', () => {
12671272
<input value="0" type="range" min="0" max="100" step="1" />,
12681273
);
12691274
expect(log).toEqual([
1270-
'set type',
1271-
'set step',
1272-
'set min',
1273-
'set max',
1274-
'set value',
1275-
'set value',
1276-
'set checked',
1277-
'set checked',
1275+
'set attribute type',
1276+
'set attribute min',
1277+
'set attribute max',
1278+
'set attribute step',
1279+
'set property value',
1280+
'set attribute value',
1281+
'set attribute checked',
1282+
'set attribute checked',
12781283
]);
12791284
});
12801285

@@ -1313,9 +1318,14 @@ describe('ReactDOMInput', () => {
13131318
var originalCreateElement = document.createElement;
13141319
spyOnDevAndProd(document, 'createElement').and.callFake(function(type) {
13151320
var el = originalCreateElement.apply(this, arguments);
1321+
var value = '';
13161322
if (type === 'input') {
13171323
Object.defineProperty(el, 'value', {
1324+
get: function() {
1325+
return value;
1326+
},
13181327
set: function(val) {
1328+
value = '' + val;
13191329
log.push(`node.value = ${strify(val)}`);
13201330
},
13211331
});
@@ -1331,9 +1341,8 @@ describe('ReactDOMInput', () => {
13311341
);
13321342
expect(log).toEqual([
13331343
'node.setAttribute("type", "date")',
1344+
'node.value = "1980-01-01"',
13341345
'node.setAttribute("value", "1980-01-01")',
1335-
'node.value = ""',
1336-
'node.value = ""',
13371346
'node.setAttribute("checked", "")',
13381347
'node.setAttribute("checked", "")',
13391348
]);
@@ -1420,4 +1429,66 @@ describe('ReactDOMInput', () => {
14201429
expect(node.getAttribute('value')).toBe('1');
14211430
});
14221431
});
1432+
1433+
describe('setting a controlled input to undefined', () => {
1434+
var input;
1435+
1436+
beforeEach(() => {
1437+
class Input extends React.Component {
1438+
state = {value: 'first'};
1439+
render() {
1440+
return (
1441+
<input
1442+
onChange={e => this.setState({value: e.target.value})}
1443+
value={this.state.value}
1444+
/>
1445+
);
1446+
}
1447+
}
1448+
1449+
var stub = ReactTestUtils.renderIntoDocument(<Input />);
1450+
input = ReactDOM.findDOMNode(stub);
1451+
ReactTestUtils.Simulate.change(input, {target: {value: 'latest'}});
1452+
ReactTestUtils.Simulate.change(input, {target: {value: undefined}});
1453+
});
1454+
1455+
it('reverts the value attribute to the initial value', () => {
1456+
expect(input.getAttribute('value')).toBe('first');
1457+
});
1458+
1459+
it('preserves the value property', () => {
1460+
expect(input.value).toBe('latest');
1461+
});
1462+
});
1463+
1464+
describe('setting a controlled input to null', () => {
1465+
var input;
1466+
1467+
beforeEach(() => {
1468+
class Input extends React.Component {
1469+
state = {value: 'first'};
1470+
render() {
1471+
return (
1472+
<input
1473+
onChange={e => this.setState({value: e.target.value})}
1474+
value={this.state.value}
1475+
/>
1476+
);
1477+
}
1478+
}
1479+
1480+
var stub = ReactTestUtils.renderIntoDocument(<Input />);
1481+
input = ReactDOM.findDOMNode(stub);
1482+
ReactTestUtils.Simulate.change(input, {target: {value: 'latest'}});
1483+
ReactTestUtils.Simulate.change(input, {target: {value: null}});
1484+
});
1485+
1486+
it('reverts the value attribute to the initial value', () => {
1487+
expect(input.getAttribute('value')).toBe('first');
1488+
});
1489+
1490+
it('preserves the value property', () => {
1491+
expect(input.value).toBe('latest');
1492+
});
1493+
});
14231494
});

‎packages/react-dom/src/client/DOMPropertyOperations.js

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,7 @@ export function getValueForProperty(node, name, expected) {
7373
if (__DEV__) {
7474
var propertyInfo = getPropertyInfo(name);
7575
if (propertyInfo) {
76-
var mutationMethod = propertyInfo.mutationMethod;
77-
if (mutationMethod || propertyInfo.mustUseProperty) {
76+
if (propertyInfo.mustUseProperty) {
7877
return node[propertyInfo.propertyName];
7978
} else {
8079
var attributeName = propertyInfo.attributeName;
@@ -157,10 +156,7 @@ export function setValueForProperty(node, name, value) {
157156
var propertyInfo = getPropertyInfo(name);
158157

159158
if (propertyInfo && shouldSetAttribute(name, value)) {
160-
var mutationMethod = propertyInfo.mutationMethod;
161-
if (mutationMethod) {
162-
mutationMethod(node, value);
163-
} else if (shouldIgnoreValue(propertyInfo, value)) {
159+
if (shouldIgnoreValue(propertyInfo, value)) {
164160
deleteValueForProperty(node, name);
165161
return;
166162
} else if (propertyInfo.mustUseProperty) {
@@ -233,10 +229,7 @@ export function deleteValueForAttribute(node, name) {
233229
export function deleteValueForProperty(node, name) {
234230
var propertyInfo = getPropertyInfo(name);
235231
if (propertyInfo) {
236-
var mutationMethod = propertyInfo.mutationMethod;
237-
if (mutationMethod) {
238-
mutationMethod(node, undefined);
239-
} else if (propertyInfo.mustUseProperty) {
232+
if (propertyInfo.mustUseProperty) {
240233
var propName = propertyInfo.propertyName;
241234
if (propertyInfo.hasBooleanValue) {
242235
node[propName] = false;

‎packages/react-dom/src/client/ReactDOMFiberInput.js

Lines changed: 55 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import * as inputValueTracking from './inputValueTracking';
1919

2020
type InputWithWrapperState = HTMLInputElement & {
2121
_wrapperState: {
22-
initialValue: ?string,
22+
initialValue: string,
2323
initialChecked: ?boolean,
2424
controlled?: boolean,
2525
},
@@ -58,30 +58,14 @@ function isControlled(props) {
5858

5959
export function getHostProps(element: Element, props: Object) {
6060
var node = ((element: any): InputWithWrapperState);
61-
var value = props.value;
6261
var checked = props.checked;
6362

64-
var hostProps = Object.assign(
65-
{
66-
// Make sure we set .type before any other properties (setting .value
67-
// before .type means .value is lost in IE11 and below)
68-
type: undefined,
69-
// Make sure we set .step before .value (setting .value before .step
70-
// means .value is rounded on mount, based upon step precision)
71-
step: undefined,
72-
// Make sure we set .min & .max before .value (to ensure proper order
73-
// in corner cases such as min or max deriving from value, e.g. Issue #7170)
74-
min: undefined,
75-
max: undefined,
76-
},
77-
props,
78-
{
79-
defaultChecked: undefined,
80-
defaultValue: undefined,
81-
value: value != null ? value : node._wrapperState.initialValue,
82-
checked: checked != null ? checked : node._wrapperState.initialChecked,
83-
},
84-
);
63+
var hostProps = Object.assign({}, props, {
64+
defaultChecked: undefined,
65+
defaultValue: undefined,
66+
value: undefined,
67+
checked: checked != null ? checked : node._wrapperState.initialChecked,
68+
});
8569

8670
return hostProps;
8771
}
@@ -132,7 +116,7 @@ export function initWrapperState(element: Element, props: Object) {
132116
}
133117
}
134118

135-
var defaultValue = props.defaultValue;
119+
var defaultValue = props.defaultValue == null ? '' : props.defaultValue;
136120
var node = ((element: any): InputWithWrapperState);
137121
node._wrapperState = {
138122
initialChecked:
@@ -215,54 +199,34 @@ export function updateWrapper(element: Element, props: Object) {
215199
// browsers typically do this as necessary, jsdom doesn't.
216200
node.value = '' + value;
217201
}
218-
} else {
219-
if (props.value == null && props.defaultValue != null) {
220-
// In Chrome, assigning defaultValue to certain input types triggers input validation.
221-
// For number inputs, the display value loses trailing decimal points. For email inputs,
222-
// Chrome raises "The specified value <x> is not a valid email address".
223-
//
224-
// Here we check to see if the defaultValue has actually changed, avoiding these problems
225-
// when the user is inputting text
226-
//
227-
// https://github.com/facebook/react/issues/7253
228-
if (node.defaultValue !== '' + props.defaultValue) {
229-
node.defaultValue = '' + props.defaultValue;
230-
}
231-
}
232-
if (props.checked == null && props.defaultChecked != null) {
233-
node.defaultChecked = !!props.defaultChecked;
234-
}
202+
synchronizeDefaultValue(node, props.type, value);
203+
} else if (
204+
props.hasOwnProperty('value') ||
205+
props.hasOwnProperty('defaultValue')
206+
) {
207+
synchronizeDefaultValue(node, props.type, props.defaultValue);
208+
}
209+
210+
if (props.checked == null && props.defaultChecked != null) {
211+
node.defaultChecked = !!props.defaultChecked;
235212
}
236213
}
237214

238215
export function postMountWrapper(element: Element, props: Object) {
239216
var node = ((element: any): InputWithWrapperState);
217+
var initialValue = node._wrapperState.initialValue;
240218

241-
// Detach value from defaultValue. We won't do anything if we're working on
242-
// submit or reset inputs as those values & defaultValues are linked. They
243-
// are not resetable nodes so this operation doesn't matter and actually
244-
// removes browser-default values (eg "Submit Query") when no value is
245-
// provided.
219+
if (props.value != null || props.defaultValue != null) {
220+
// Do not assign value if it is already set. This prevents user text input
221+
// from being lost during SSR hydration.
222+
if (node.value === '') {
223+
node.value = initialValue;
224+
}
246225

247-
switch (props.type) {
248-
case 'submit':
249-
case 'reset':
250-
break;
251-
case 'color':
252-
case 'date':
253-
case 'datetime':
254-
case 'datetime-local':
255-
case 'month':
256-
case 'time':
257-
case 'week':
258-
// This fixes the no-show issue on iOS Safari and Android Chrome:
259-
// https://github.com/facebook/react/issues/7233
260-
node.value = '';
261-
node.value = node.defaultValue;
262-
break;
263-
default:
264-
node.value = node.value;
265-
break;
226+
// value must be assigned before defaultValue. This fixes an issue where the
227+
// visually displayed value of date inputs disappears on mobile Safari and Chrome:
228+
// https://github.com/facebook/react/issues/7233
229+
node.defaultValue = initialValue;
266230
}
267231

268232
// Normally, we'd just do `node.checked = node.checked` upon initial mount, less this bug
@@ -334,3 +298,29 @@ function updateNamedCousins(rootNode, props) {
334298
}
335299
}
336300
}
301+
302+
// In Chrome, assigning defaultValue to certain input types triggers input validation.
303+
// For number inputs, the display value loses trailing decimal points. For email inputs,
304+
// Chrome raises "The specified value <x> is not a valid email address".
305+
//
306+
// Here we check to see if the defaultValue has actually changed, avoiding these problems
307+
// when the user is inputting text
308+
//
309+
// https://github.com/facebook/react/issues/7253
310+
function synchronizeDefaultValue(
311+
node: InputWithWrapperState,
312+
type: ?string,
313+
value: *,
314+
) {
315+
if (
316+
// Focused number inputs synchronize on blur. See ChangeEventPlugin.js
317+
(type !== 'number' || node.ownerDocument.activeElement !== node) &&
318+
node.defaultValue !== '' + value
319+
) {
320+
if (value != null) {
321+
node.defaultValue = '' + value;
322+
} else {
323+
node.defaultValue = node._wrapperState.initialValue;
324+
}
325+
}
326+
}

‎packages/react-dom/src/shared/DOMProperty.js

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -54,17 +54,13 @@ var DOMPropertyInjection = {
5454
* DOMPropertyNames: similar to DOMAttributeNames but for DOM properties.
5555
* Property names not specified use the normalized name.
5656
*
57-
* DOMMutationMethods: Properties that require special mutation methods. If
58-
* `value` is undefined, the mutation method should unset the property.
59-
*
6057
* @param {object} domPropertyConfig the config as described above.
6158
*/
6259
injectDOMPropertyConfig: function(domPropertyConfig) {
6360
var Injection = DOMPropertyInjection;
6461
var Properties = domPropertyConfig.Properties || {};
6562
var DOMAttributeNamespaces = domPropertyConfig.DOMAttributeNamespaces || {};
6663
var DOMAttributeNames = domPropertyConfig.DOMAttributeNames || {};
67-
var DOMMutationMethods = domPropertyConfig.DOMMutationMethods || {};
6864

6965
for (var propName in Properties) {
7066
invariant(
@@ -83,7 +79,6 @@ var DOMPropertyInjection = {
8379
attributeName: lowerCased,
8480
attributeNamespace: null,
8581
propertyName: propName,
86-
mutationMethod: null,
8782

8883
mustUseProperty: checkMask(propConfig, Injection.MUST_USE_PROPERTY),
8984
hasBooleanValue: checkMask(propConfig, Injection.HAS_BOOLEAN_VALUE),
@@ -121,10 +116,6 @@ var DOMPropertyInjection = {
121116
propertyInfo.attributeNamespace = DOMAttributeNamespaces[propName];
122117
}
123118

124-
if (DOMMutationMethods.hasOwnProperty(propName)) {
125-
propertyInfo.mutationMethod = DOMMutationMethods[propName];
126-
}
127-
128119
// Downcase references to whitelist properties to check for membership
129120
// without case-sensitivity. This allows the whitelist to pick up
130121
// `allowfullscreen`, which should be written using the property configuration
@@ -154,9 +145,6 @@ export const ROOT_ATTRIBUTE_NAME = 'data-reactroot';
154145
* propertyName:
155146
* Used on DOM node instances. (This includes properties that mutate due to
156147
* external factors.)
157-
* mutationMethod:
158-
* If non-null, used instead of the property or `setAttribute()` after
159-
* initial render.
160148
* mustUseProperty:
161149
* Whether the property must be accessed and mutated as an object property.
162150
* hasBooleanValue:

0 commit comments

Comments
 (0)