Skip to content

Commit 323efbc

Browse files
nhunzakergaearon
nhunzaker
authored and
gaearon
committed
Ensure value and defaultValue do not assign functions and symbols (#11741)
* 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
1 parent 2091c62 commit 323efbc

File tree

5 files changed

+234
-42
lines changed

5 files changed

+234
-42
lines changed

‎fixtures/attribute-behavior/AttributeTableSnapshot.md

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2593,8 +2593,8 @@
25932593
| `defaultValue=(string 'false')`| (changed)| `"false"` |
25942594
| `defaultValue=(string 'on')`| (changed)| `"on"` |
25952595
| `defaultValue=(string 'off')`| (changed)| `"off"` |
2596-
| `defaultValue=(symbol)`| (changed, error, warning, ssr error)| `` |
2597-
| `defaultValue=(function)`| (changed, ssr warning)| `"function f() {}"` |
2596+
| `defaultValue=(symbol)`| (initial, ssr error, ssr mismatch)| `<empty string>` |
2597+
| `defaultValue=(function)`| (initial, ssr mismatch)| `<empty string>` |
25982598
| `defaultValue=(null)`| (initial, ssr warning)| `<empty string>` |
25992599
| `defaultValue=(undefined)`| (initial)| `<empty string>` |
26002600

@@ -11768,8 +11768,8 @@
1176811768
| `value=(string 'false')`| (changed)| `"false"` |
1176911769
| `value=(string 'on')`| (changed)| `"on"` |
1177011770
| `value=(string 'off')`| (changed)| `"off"` |
11771-
| `value=(symbol)`| (changed, error, warning, ssr error)| `` |
11772-
| `value=(function)`| (changed, warning, ssr warning)| `"function f() {}"` |
11771+
| `value=(symbol)`| (initial, warning, ssr error, ssr mismatch)| `<empty string>` |
11772+
| `value=(function)`| (initial, warning, ssr mismatch)| `<empty string>` |
1177311773
| `value=(null)`| (initial, warning, ssr warning)| `<empty string>` |
1177411774
| `value=(undefined)`| (initial)| `<empty string>` |
1177511775

@@ -11793,8 +11793,8 @@
1179311793
| `value=(string 'false')`| (changed)| `"false"` |
1179411794
| `value=(string 'on')`| (changed)| `"on"` |
1179511795
| `value=(string 'off')`| (changed)| `"off"` |
11796-
| `value=(symbol)`| (changed, error, warning, ssr error)| `` |
11797-
| `value=(function)`| (changed, warning)| `"function f() {}"` |
11796+
| `value=(symbol)`| (initial, warning, ssr error, ssr mismatch)| `<empty string>` |
11797+
| `value=(function)`| (initial, warning, ssr mismatch)| `<empty string>` |
1179811798
| `value=(null)`| (initial, warning, ssr warning)| `<empty string>` |
1179911799
| `value=(undefined)`| (initial)| `<empty string>` |
1180011800

@@ -11818,7 +11818,7 @@
1181811818
| `value=(string 'false')`| (initial)| `<empty string>` |
1181911819
| `value=(string 'on')`| (initial)| `<empty string>` |
1182011820
| `value=(string 'off')`| (initial)| `<empty string>` |
11821-
| `value=(symbol)`| (changed, error, warning, ssr error)| `` |
11821+
| `value=(symbol)`| (initial, warning, ssr error, ssr mismatch)| `<empty string>` |
1182211822
| `value=(function)`| (initial, warning)| `<empty string>` |
1182311823
| `value=(null)`| (initial, warning, ssr warning)| `<empty string>` |
1182411824
| `value=(undefined)`| (initial)| `<empty string>` |

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

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,23 @@ describe('ReactDOMInput', () => {
248248
}
249249
});
250250

251+
it('performs a state change from "" to 0', () => {
252+
class Stub extends React.Component {
253+
state = {
254+
value: '',
255+
};
256+
render() {
257+
return <input type="number" value={this.state.value} readOnly={true} />;
258+
}
259+
}
260+
261+
var stub = ReactTestUtils.renderIntoDocument(<Stub />);
262+
var node = ReactDOM.findDOMNode(stub);
263+
stub.setState({value: 0});
264+
265+
expect(node.value).toEqual('0');
266+
});
267+
251268
it('distinguishes precision for extra zeroes in string number values', () => {
252269
spyOnDev(console, 'error');
253270
class Stub extends React.Component {
@@ -595,6 +612,7 @@ describe('ReactDOMInput', () => {
595612
var node = container.firstChild;
596613

597614
expect(node.value).toBe('0');
615+
expect(node.defaultValue).toBe('0');
598616
});
599617

600618
it('should properly transition from 0 to an empty value', function() {
@@ -606,6 +624,43 @@ describe('ReactDOMInput', () => {
606624
var node = container.firstChild;
607625

608626
expect(node.value).toBe('');
627+
expect(node.defaultValue).toBe('');
628+
});
629+
630+
it('should properly transition a text input from 0 to an empty 0.0', function() {
631+
var container = document.createElement('div');
632+
633+
ReactDOM.render(<input type="text" value={0} />, container);
634+
ReactDOM.render(<input type="text" value="0.0" />, container);
635+
636+
var node = container.firstChild;
637+
638+
expect(node.value).toBe('0.0');
639+
expect(node.defaultValue).toBe('0.0');
640+
});
641+
642+
it('should properly transition a number input from "" to 0', function() {
643+
var container = document.createElement('div');
644+
645+
ReactDOM.render(<input type="number" value="" />, container);
646+
ReactDOM.render(<input type="number" value={0} />, container);
647+
648+
var node = container.firstChild;
649+
650+
expect(node.value).toBe('0');
651+
expect(node.defaultValue).toBe('0');
652+
});
653+
654+
it('should properly transition a number input from "" to "0"', function() {
655+
var container = document.createElement('div');
656+
657+
ReactDOM.render(<input type="number" value="" />, container);
658+
ReactDOM.render(<input type="number" value="0" />, container);
659+
660+
var node = container.firstChild;
661+
662+
expect(node.value).toBe('0');
663+
expect(node.defaultValue).toBe('0');
609664
});
610665

611666
it('should have the correct target value', () => {
@@ -1585,4 +1640,132 @@ describe('ReactDOMInput', () => {
15851640
}
15861641
});
15871642
});
1643+
1644+
describe('When given a Symbol value', function() {
1645+
it('treats initial Symbol value as an empty string', function() {
1646+
spyOnDev(console, 'error');
1647+
var container = document.createElement('div');
1648+
ReactDOM.render(
1649+
<input value={Symbol('foobar')} onChange={() => {}} />,
1650+
container,
1651+
);
1652+
var node = container.firstChild;
1653+
1654+
expect(node.value).toBe('');
1655+
expect(node.getAttribute('value')).toBe('');
1656+
1657+
if (__DEV__) {
1658+
expect(console.error.calls.count()).toBe(1);
1659+
expect(console.error.calls.argsFor(0)[0]).toContain(
1660+
'Invalid value for prop `value`',
1661+
);
1662+
}
1663+
});
1664+
1665+
it('treats updated Symbol value as an empty string', function() {
1666+
spyOnDev(console, 'error');
1667+
var container = document.createElement('div');
1668+
ReactDOM.render(<input value="foo" onChange={() => {}} />, container);
1669+
ReactDOM.render(
1670+
<input value={Symbol('foobar')} onChange={() => {}} />,
1671+
container,
1672+
);
1673+
var node = container.firstChild;
1674+
1675+
expect(node.value).toBe('');
1676+
expect(node.getAttribute('value')).toBe('');
1677+
1678+
if (__DEV__) {
1679+
expect(console.error.calls.count()).toBe(1);
1680+
expect(console.error.calls.argsFor(0)[0]).toContain(
1681+
'Invalid value for prop `value`',
1682+
);
1683+
}
1684+
});
1685+
1686+
it('treats initial Symbol defaultValue as an empty string', function() {
1687+
var container = document.createElement('div');
1688+
ReactDOM.render(<input defaultValue={Symbol('foobar')} />, container);
1689+
var node = container.firstChild;
1690+
1691+
expect(node.value).toBe('');
1692+
expect(node.getAttribute('value')).toBe('');
1693+
// TODO: we should warn here.
1694+
});
1695+
1696+
it('treats updated Symbol defaultValue as an empty string', function() {
1697+
var container = document.createElement('div');
1698+
ReactDOM.render(<input defaultValue="foo" />, container);
1699+
ReactDOM.render(<input defaultValue={Symbol('foobar')} />, container);
1700+
var node = container.firstChild;
1701+
1702+
expect(node.value).toBe('foo');
1703+
expect(node.getAttribute('value')).toBe('');
1704+
// TODO: we should warn here.
1705+
});
1706+
});
1707+
1708+
describe('When given a function value', function() {
1709+
it('treats initial function value as an empty string', function() {
1710+
spyOnDev(console, 'error');
1711+
var container = document.createElement('div');
1712+
ReactDOM.render(
1713+
<input value={() => {}} onChange={() => {}} />,
1714+
container,
1715+
);
1716+
var node = container.firstChild;
1717+
1718+
expect(node.value).toBe('');
1719+
expect(node.getAttribute('value')).toBe('');
1720+
1721+
if (__DEV__) {
1722+
expect(console.error.calls.count()).toBe(1);
1723+
expect(console.error.calls.argsFor(0)[0]).toContain(
1724+
'Invalid value for prop `value`',
1725+
);
1726+
}
1727+
});
1728+
1729+
it('treats updated function value as an empty string', function() {
1730+
spyOnDev(console, 'error');
1731+
var container = document.createElement('div');
1732+
ReactDOM.render(<input value="foo" onChange={() => {}} />, container);
1733+
ReactDOM.render(
1734+
<input value={() => {}} onChange={() => {}} />,
1735+
container,
1736+
);
1737+
var node = container.firstChild;
1738+
1739+
expect(node.value).toBe('');
1740+
expect(node.getAttribute('value')).toBe('');
1741+
1742+
if (__DEV__) {
1743+
expect(console.error.calls.count()).toBe(1);
1744+
expect(console.error.calls.argsFor(0)[0]).toContain(
1745+
'Invalid value for prop `value`',
1746+
);
1747+
}
1748+
});
1749+
1750+
it('treats initial function defaultValue as an empty string', function() {
1751+
var container = document.createElement('div');
1752+
ReactDOM.render(<input defaultValue={() => {}} />, container);
1753+
var node = container.firstChild;
1754+
1755+
expect(node.value).toBe('');
1756+
expect(node.getAttribute('value')).toBe('');
1757+
// TODO: we should warn here.
1758+
});
1759+
1760+
it('treats updated function defaultValue as an empty string', function() {
1761+
var container = document.createElement('div');
1762+
ReactDOM.render(<input defaultValue="foo" />, container);
1763+
ReactDOM.render(<input defaultValue={() => {}} />, container);
1764+
var node = container.firstChild;
1765+
1766+
expect(node.value).toBe('foo');
1767+
expect(node.getAttribute('value')).toBe('');
1768+
// TODO: we should warn here.
1769+
});
1770+
});
15881771
});

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

Lines changed: 39 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -116,12 +116,15 @@ export function initWrapperState(element: Element, props: Object) {
116116
}
117117
}
118118

119-
var defaultValue = props.defaultValue == null ? '' : props.defaultValue;
120119
var node = ((element: any): InputWithWrapperState);
120+
var defaultValue = props.defaultValue == null ? '' : props.defaultValue;
121+
121122
node._wrapperState = {
122123
initialChecked:
123124
props.checked != null ? props.checked : props.defaultChecked,
124-
initialValue: props.value != null ? props.value : defaultValue,
125+
initialValue: getSafeValue(
126+
props.value != null ? props.value : defaultValue,
127+
),
125128
controlled: isControlled(props),
126129
};
127130
}
@@ -175,36 +178,26 @@ export function updateWrapper(element: Element, props: Object) {
175178

176179
updateChecked(element, props);
177180

178-
var value = props.value;
179-
if (value != null) {
180-
if (value === 0 && node.value === '') {
181-
node.value = '0';
182-
// Note: IE9 reports a number inputs as 'text', so check props instead.
183-
} else if (props.type === 'number') {
184-
// Simulate `input.valueAsNumber`. IE9 does not support it
185-
var valueAsNumber = parseFloat(node.value) || 0;
181+
var value = getSafeValue(props.value);
186182

183+
if (value != null) {
184+
if (props.type === 'number') {
187185
if (
186+
(value === 0 && node.value === '') ||
188187
// eslint-disable-next-line
189-
value != valueAsNumber ||
190-
// eslint-disable-next-line
191-
(value == valueAsNumber && node.value != value)
188+
node.value != value
192189
) {
193-
// Cast `value` to a string to ensure the value is set correctly. While
194-
// browsers typically do this as necessary, jsdom doesn't.
195190
node.value = '' + value;
196191
}
197192
} else if (node.value !== '' + value) {
198-
// Cast `value` to a string to ensure the value is set correctly. While
199-
// browsers typically do this as necessary, jsdom doesn't.
200193
node.value = '' + value;
201194
}
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);
195+
}
196+
197+
if (props.hasOwnProperty('value')) {
198+
setDefaultValue(node, props.type, value);
199+
} else if (props.hasOwnProperty('defaultValue')) {
200+
setDefaultValue(node, props.type, getSafeValue(props.defaultValue));
208201
}
209202

210203
if (props.checked == null && props.defaultChecked != null) {
@@ -214,19 +207,18 @@ export function updateWrapper(element: Element, props: Object) {
214207

215208
export function postMountWrapper(element: Element, props: Object) {
216209
var node = ((element: any): InputWithWrapperState);
217-
var initialValue = node._wrapperState.initialValue;
218210

219-
if (props.value != null || props.defaultValue != null) {
211+
if (props.hasOwnProperty('value') || props.hasOwnProperty('defaultValue')) {
220212
// Do not assign value if it is already set. This prevents user text input
221213
// from being lost during SSR hydration.
222214
if (node.value === '') {
223-
node.value = initialValue;
215+
node.value = '' + node._wrapperState.initialValue;
224216
}
225217

226218
// value must be assigned before defaultValue. This fixes an issue where the
227219
// visually displayed value of date inputs disappears on mobile Safari and Chrome:
228220
// https://github.com/facebook/react/issues/7233
229-
node.defaultValue = initialValue;
221+
node.defaultValue = '' + node._wrapperState.initialValue;
230222
}
231223

232224
// Normally, we'd just do `node.checked = node.checked` upon initial mount, less this bug
@@ -307,20 +299,34 @@ function updateNamedCousins(rootNode, props) {
307299
// when the user is inputting text
308300
//
309301
// https://github.com/facebook/react/issues/7253
310-
export function synchronizeDefaultValue(
302+
export function setDefaultValue(
311303
node: InputWithWrapperState,
312304
type: ?string,
313305
value: *,
314306
) {
315307
if (
316308
// Focused number inputs synchronize on blur. See ChangeEventPlugin.js
317-
(type !== 'number' || node.ownerDocument.activeElement !== node) &&
318-
node.defaultValue !== '' + value
309+
type !== 'number' ||
310+
node.ownerDocument.activeElement !== node
319311
) {
320-
if (value != null) {
312+
if (value == null) {
313+
node.defaultValue = '' + node._wrapperState.initialValue;
314+
} else if (node.defaultValue !== '' + value) {
321315
node.defaultValue = '' + value;
322-
} else {
323-
node.defaultValue = node._wrapperState.initialValue;
324316
}
325317
}
326318
}
319+
320+
function getSafeValue(value: *): * {
321+
switch (typeof value) {
322+
case 'boolean':
323+
case 'number':
324+
case 'object':
325+
case 'string':
326+
case 'undefined':
327+
return value;
328+
default:
329+
// function, symbol are assigned as empty strings
330+
return '';
331+
}
332+
}

‎packages/react-dom/src/events/ChangeEventPlugin.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import getEventTarget from './getEventTarget';
1717
import isEventSupported from './isEventSupported';
1818
import {getNodeFromInstance} from '../client/ReactDOMComponentTree';
1919
import * as inputValueTracking from '../client/inputValueTracking';
20-
import {synchronizeDefaultValue} from '../client/ReactDOMFiberInput';
20+
import {setDefaultValue} from '../client/ReactDOMFiberInput';
2121

2222
var eventTypes = {
2323
change: {
@@ -236,7 +236,7 @@ function handleControlledInputBlur(inst, node) {
236236
}
237237

238238
// If controlled, assign the value attribute to the current value on blur
239-
synchronizeDefaultValue(node, 'number', node.value);
239+
setDefaultValue(node, 'number', node.value);
240240
}
241241

242242
/**

0 commit comments

Comments
 (0)