From e7ed5d6d783fa86ae137f1f4afb2d12efac87efc Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Thu, 30 Nov 2017 22:32:41 -0500 Subject: [PATCH 01/15] Ensure value and defaultValue do not assign functions and symbols --- .../src/__tests__/ReactDOMInput-test.js | 80 +++++++++++++++++++ .../src/client/ReactDOMFiberInput.js | 56 +++++++++---- 2 files changed, 119 insertions(+), 17 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index 4e8948d49b5a2..8e0f7dbf2847f 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -1585,4 +1585,84 @@ describe('ReactDOMInput', () => { } }); }); + + describe('When given a Symbol value', function() { + it('does not allow initial assignment of Symbols to value', function() { + var container = document.createElement('div'); + ReactDOM.render(, container); + var node = container.firstChild; + + expect(node.value).toBe(''); + expect(node.getAttribute('value')).toBe(null); + }); + + it('does not update the value to a Symbol', function() { + var container = document.createElement('div'); + ReactDOM.render(, container); + ReactDOM.render(, container); + var node = container.firstChild; + + expect(node.value).toBe(''); + expect(node.getAttribute('value')).toBe(''); + }); + + it('does not allow initial assignment of Symbols to defaultValue', function() { + var container = document.createElement('div'); + ReactDOM.render(, container); + var node = container.firstChild; + + expect(node.value).toBe(''); + expect(node.getAttribute('value')).toBe(null); + }); + + it('does not update defaultValue to a Symbol', function() { + var container = document.createElement('div'); + ReactDOM.render(, container); + ReactDOM.render(, container); + var node = container.firstChild; + + expect(node.value).toBe(''); + expect(node.getAttribute('value')).toBe(''); + }); + }); + + describe('When given a function value', function() { + it('does not allow initial assignment of functions to value', function() { + var container = document.createElement('div'); + ReactDOM.render( {}} />, container); + var node = container.firstChild; + + expect(node.value).toBe(''); + expect(node.getAttribute('value')).toBe(null); + }); + + it('does not update the value to a function', function() { + var container = document.createElement('div'); + ReactDOM.render(, container); + ReactDOM.render( {}} />, container); + var node = container.firstChild; + + expect(node.value).toBe(''); + expect(node.getAttribute('value')).toBe(''); + }); + + it('does not allow initial assignment of functions to defaultValue', function() { + var container = document.createElement('div'); + ReactDOM.render( {}} />, container); + var node = container.firstChild; + + expect(node.value).toBe(''); + expect(node.getAttribute('value')).toBe(null); + }); + + it('does not update defaultValue to a function', function() { + var container = document.createElement('div'); + ReactDOM.render(, container); + ReactDOM.render( {}} />, container); + var node = container.firstChild; + + expect(node.value).toBe(''); + expect(node.getAttribute('value')).toBe(''); + }); + }); }); diff --git a/packages/react-dom/src/client/ReactDOMFiberInput.js b/packages/react-dom/src/client/ReactDOMFiberInput.js index 22cf89c400b04..340967fe7cd45 100644 --- a/packages/react-dom/src/client/ReactDOMFiberInput.js +++ b/packages/react-dom/src/client/ReactDOMFiberInput.js @@ -178,7 +178,7 @@ export function updateWrapper(element: Element, props: Object) { var value = props.value; if (value != null) { if (value === 0 && node.value === '') { - node.value = '0'; + assignProperty(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 @@ -190,14 +190,10 @@ export function updateWrapper(element: Element, props: Object) { // eslint-disable-next-line (value == valueAsNumber && node.value != 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; + assignProperty(node, 'value', value); } - } else if (node.value !== '' + 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 { + assignProperty(node, 'value', value); } synchronizeDefaultValue(node, props.type, value); } else if ( @@ -220,13 +216,13 @@ export function postMountWrapper(element: Element, props: Object) { // Do not assign value if it is already set. This prevents user text input // from being lost during SSR hydration. if (node.value === '') { - node.value = initialValue; + assignProperty(node, 'value', initialValue); } // value must be assigned before defaultValue. This fixes an issue where the // visually displayed value of date inputs disappears on mobile Safari and Chrome: // https://github.com/facebook/react/issues/7233 - node.defaultValue = initialValue; + assignProperty(node, 'defaultValue', initialValue); } // Normally, we'd just do `node.checked = node.checked` upon initial mount, less this bug @@ -314,13 +310,39 @@ export function synchronizeDefaultValue( ) { if ( // Focused number inputs synchronize on blur. See ChangeEventPlugin.js - (type !== 'number' || node.ownerDocument.activeElement !== node) && - node.defaultValue !== '' + value + type !== 'number' || + node.ownerDocument.activeElement !== node ) { - if (value != null) { - node.defaultValue = '' + value; - } else { - node.defaultValue = node._wrapperState.initialValue; - } + assignProperty( + node, + 'defaultValue', + value == null ? node._wrapperState.initialValue : value, + ); + } +} + +function assignProperty( + node: InputWithWrapperState, + property: string, + value: *, +) { + // TODO: This should use DOMProperty.shouldSetAttribute, however the current + // behavior is such that invalid attributes do not update the current attribute, + // instead of removing it. + switch (typeof value) { + case 'boolean': + case 'number': + case 'string': + case 'object': + // Use hasOwnProperty instead of checking for '' to ensure properties + // are set for the first time. This is important for defaultValue, which + // otherwise does not set an empty value attribute + if (!node.hasOwnProperty(property) || node[property] !== '' + value) { + // Cast `value` to a string to ensure the value is set correctly. While + // browsers typically do this as necessary, jsdom doesn't. + // TODO: Is this still true with reasonably modern JSDOM? + node[property] = '' + value; + } + break; } } From bf28fa3d54246cca7f39470a03f4e8f78166d95e Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 1 Dec 2017 09:55:07 -0500 Subject: [PATCH 02/15] Eliminate assignProperty method from ReactDOMInput --- .../src/client/ReactDOMFiberInput.js | 61 +++++++------------ .../react-dom/src/events/ChangeEventPlugin.js | 1 + packages/react-dom/src/shared/DOMProperty.js | 5 +- 3 files changed, 26 insertions(+), 41 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOMFiberInput.js b/packages/react-dom/src/client/ReactDOMFiberInput.js index 340967fe7cd45..1918dd255e4bf 100644 --- a/packages/react-dom/src/client/ReactDOMFiberInput.js +++ b/packages/react-dom/src/client/ReactDOMFiberInput.js @@ -16,6 +16,7 @@ import * as DOMPropertyOperations from './DOMPropertyOperations'; import {getFiberCurrentPropsFromNode} from './ReactDOMComponentTree'; import ReactControlledValuePropTypes from '../shared/ReactControlledValuePropTypes'; import * as inputValueTracking from './inputValueTracking'; +import {shouldSetAttribute} from '../shared/DOMProperty.js'; type InputWithWrapperState = HTMLInputElement & { _wrapperState: { @@ -175,10 +176,14 @@ export function updateWrapper(element: Element, props: Object) { updateChecked(element, props); - var value = props.value; + var value = shouldSetAttribute('value', props.value) ? props.value : null; + var defaultValue = shouldSetAttribute('value', props.defaultValue) + ? props.defaultValue + : null; + if (value != null) { if (value === 0 && node.value === '') { - assignProperty(node, 'value', '0'); + 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 @@ -190,17 +195,17 @@ export function updateWrapper(element: Element, props: Object) { // eslint-disable-next-line (value == valueAsNumber && node.value != value) ) { - assignProperty(node, 'value', value); + node.value = '' + value; } - } else { - assignProperty(node, 'value', value); + } else if (node.value !== '' + value) { + node.value = '' + value; } synchronizeDefaultValue(node, props.type, value); } else if ( props.hasOwnProperty('value') || props.hasOwnProperty('defaultValue') ) { - synchronizeDefaultValue(node, props.type, props.defaultValue); + synchronizeDefaultValue(node, props.type, defaultValue); } if (props.checked == null && props.defaultChecked != null) { @@ -215,14 +220,16 @@ export function postMountWrapper(element: Element, props: Object) { if (props.value != null || props.defaultValue != null) { // Do not assign value if it is already set. This prevents user text input // from being lost during SSR hydration. - if (node.value === '') { - assignProperty(node, 'value', initialValue); + if (node.value === '' && shouldSetAttribute('value', initialValue)) { + node.value = '' + initialValue; } // value must be assigned before defaultValue. This fixes an issue where the // visually displayed value of date inputs disappears on mobile Safari and Chrome: // https://github.com/facebook/react/issues/7233 - assignProperty(node, 'defaultValue', initialValue); + if (shouldSetAttribute('value', initialValue)) { + node.defaultValue = '' + initialValue; + } } // Normally, we'd just do `node.checked = node.checked` upon initial mount, less this bug @@ -313,36 +320,10 @@ export function synchronizeDefaultValue( type !== 'number' || node.ownerDocument.activeElement !== node ) { - assignProperty( - node, - 'defaultValue', - value == null ? node._wrapperState.initialValue : value, - ); - } -} - -function assignProperty( - node: InputWithWrapperState, - property: string, - value: *, -) { - // TODO: This should use DOMProperty.shouldSetAttribute, however the current - // behavior is such that invalid attributes do not update the current attribute, - // instead of removing it. - switch (typeof value) { - case 'boolean': - case 'number': - case 'string': - case 'object': - // Use hasOwnProperty instead of checking for '' to ensure properties - // are set for the first time. This is important for defaultValue, which - // otherwise does not set an empty value attribute - if (!node.hasOwnProperty(property) || node[property] !== '' + value) { - // Cast `value` to a string to ensure the value is set correctly. While - // browsers typically do this as necessary, jsdom doesn't. - // TODO: Is this still true with reasonably modern JSDOM? - node[property] = '' + value; - } - break; + var nextValue = + '' + (value == null ? node._wrapperState.initialValue : value); + if (nextValue !== node.defaultValue) { + node.defaultValue = nextValue; + } } } diff --git a/packages/react-dom/src/events/ChangeEventPlugin.js b/packages/react-dom/src/events/ChangeEventPlugin.js index cb98d19a9b9fa..b3194a9b5df4e 100644 --- a/packages/react-dom/src/events/ChangeEventPlugin.js +++ b/packages/react-dom/src/events/ChangeEventPlugin.js @@ -229,6 +229,7 @@ function handleControlledInputBlur(inst, node) { } // Fiber and ReactDOM keep wrapper state in separate places + // TODO: Is this still necessary now that Stack is gone? let state = inst._wrapperState || node._wrapperState; if (!state || !state.controlled || node.type !== 'number') { diff --git a/packages/react-dom/src/shared/DOMProperty.js b/packages/react-dom/src/shared/DOMProperty.js index 4a11eaf34c548..16086824d2cf9 100644 --- a/packages/react-dom/src/shared/DOMProperty.js +++ b/packages/react-dom/src/shared/DOMProperty.js @@ -12,8 +12,11 @@ import warning from 'fbjs/lib/warning'; var RESERVED_PROPS = { children: true, dangerouslySetInnerHTML: true, - defaultValue: true, defaultChecked: true, + // TODO: This prevents the assignment of defaultValue to regular + // elements (not just inputs). Now that ReactDOMInput assigns to the + // defaultValue property -- do we need this? + defaultValue: true, innerHTML: true, suppressContentEditableWarning: true, suppressHydrationWarning: true, From 5510b99a3f10a5d157a9d20a125c9bfafc7ede7d Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 1 Dec 2017 09:57:25 -0500 Subject: [PATCH 03/15] Restore original placement of defaultValue reservedProp --- packages/react-dom/src/shared/DOMProperty.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-dom/src/shared/DOMProperty.js b/packages/react-dom/src/shared/DOMProperty.js index 16086824d2cf9..a6e7a09778ab9 100644 --- a/packages/react-dom/src/shared/DOMProperty.js +++ b/packages/react-dom/src/shared/DOMProperty.js @@ -12,11 +12,11 @@ import warning from 'fbjs/lib/warning'; var RESERVED_PROPS = { children: true, dangerouslySetInnerHTML: true, - defaultChecked: true, // TODO: This prevents the assignment of defaultValue to regular // elements (not just inputs). Now that ReactDOMInput assigns to the // defaultValue property -- do we need this? defaultValue: true, + defaultChecked: true, innerHTML: true, suppressContentEditableWarning: true, suppressHydrationWarning: true, From 570a1a150d903f54dd0f3e3a8d2bae79af37e10a Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 1 Dec 2017 10:27:46 -0500 Subject: [PATCH 04/15] Reduce branching. Make assignment more consistent --- .../src/__tests__/ReactDOMInput-test.js | 8 ++--- .../src/client/ReactDOMFiberInput.js | 35 ++++++++++--------- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index 8e0f7dbf2847f..4927bd875085e 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -1593,7 +1593,7 @@ describe('ReactDOMInput', () => { var node = container.firstChild; expect(node.value).toBe(''); - expect(node.getAttribute('value')).toBe(null); + expect(node.getAttribute('value')).toBe(''); }); it('does not update the value to a Symbol', function() { @@ -1612,7 +1612,7 @@ describe('ReactDOMInput', () => { var node = container.firstChild; expect(node.value).toBe(''); - expect(node.getAttribute('value')).toBe(null); + expect(node.getAttribute('value')).toBe(''); }); it('does not update defaultValue to a Symbol', function() { @@ -1633,7 +1633,7 @@ describe('ReactDOMInput', () => { var node = container.firstChild; expect(node.value).toBe(''); - expect(node.getAttribute('value')).toBe(null); + expect(node.getAttribute('value')).toBe(''); }); it('does not update the value to a function', function() { @@ -1652,7 +1652,7 @@ describe('ReactDOMInput', () => { var node = container.firstChild; expect(node.value).toBe(''); - expect(node.getAttribute('value')).toBe(null); + expect(node.getAttribute('value')).toBe(''); }); it('does not update defaultValue to a function', function() { diff --git a/packages/react-dom/src/client/ReactDOMFiberInput.js b/packages/react-dom/src/client/ReactDOMFiberInput.js index 1918dd255e4bf..d4e0d1909947c 100644 --- a/packages/react-dom/src/client/ReactDOMFiberInput.js +++ b/packages/react-dom/src/client/ReactDOMFiberInput.js @@ -118,11 +118,17 @@ export function initWrapperState(element: Element, props: Object) { } var defaultValue = props.defaultValue == null ? '' : props.defaultValue; + var initialValue = props.value != null ? props.value : defaultValue; var node = ((element: any): InputWithWrapperState); + + if (!shouldSetAttribute('value', initialValue)) { + initialValue = ''; + } + node._wrapperState = { initialChecked: props.checked != null ? props.checked : props.defaultChecked, - initialValue: props.value != null ? props.value : defaultValue, + initialValue: initialValue, controlled: isControlled(props), }; } @@ -200,11 +206,11 @@ export function updateWrapper(element: Element, props: Object) { } else if (node.value !== '' + value) { node.value = '' + value; } + } + + if (props.hasOwnProperty('value')) { synchronizeDefaultValue(node, props.type, value); - } else if ( - props.hasOwnProperty('value') || - props.hasOwnProperty('defaultValue') - ) { + } else if (props.hasOwnProperty('defaultValue')) { synchronizeDefaultValue(node, props.type, defaultValue); } @@ -215,21 +221,18 @@ export function updateWrapper(element: Element, props: Object) { export function postMountWrapper(element: Element, props: Object) { var node = ((element: any): InputWithWrapperState); - var initialValue = node._wrapperState.initialValue; - if (props.value != null || props.defaultValue != null) { + if (props.hasOwnProperty('value') || props.hasOwnProperty('defaultValue')) { // Do not assign value if it is already set. This prevents user text input // from being lost during SSR hydration. - if (node.value === '' && shouldSetAttribute('value', initialValue)) { - node.value = '' + initialValue; + if (node.value === '') { + node.value = '' + node._wrapperState.initialValue; } // value must be assigned before defaultValue. This fixes an issue where the // visually displayed value of date inputs disappears on mobile Safari and Chrome: // https://github.com/facebook/react/issues/7233 - if (shouldSetAttribute('value', initialValue)) { - node.defaultValue = '' + initialValue; - } + node.defaultValue = '' + node._wrapperState.initialValue; } // Normally, we'd just do `node.checked = node.checked` upon initial mount, less this bug @@ -320,10 +323,10 @@ export function synchronizeDefaultValue( type !== 'number' || node.ownerDocument.activeElement !== node ) { - var nextValue = - '' + (value == null ? node._wrapperState.initialValue : value); - if (nextValue !== node.defaultValue) { - node.defaultValue = nextValue; + if (value == null) { + node.defaultValue = '' + node._wrapperState.initialValue; + } else if (node.defaultValue !== '' + value) { + node.defaultValue = '' + value; } } } From d99d97bb10f2a21bdad2049fdae6653f2e193eec Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 1 Dec 2017 12:47:08 -0500 Subject: [PATCH 05/15] Control for warnings in symbol/function tests --- .../src/__tests__/ReactDOMInput-test.js | 44 ++++++++++++++++--- 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index 4927bd875085e..b10c8686b681d 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -1588,27 +1588,43 @@ describe('ReactDOMInput', () => { describe('When given a Symbol value', function() { it('does not allow initial assignment of Symbols to value', function() { + spyOnDev(console, 'error'); var container = document.createElement('div'); - ReactDOM.render(, container); + ReactDOM.render(, container); var node = container.firstChild; expect(node.value).toBe(''); expect(node.getAttribute('value')).toBe(''); + + if (__DEV__) { + expect(console.error.calls.count()).toBe(1); + expect(console.error.calls.argsFor(0)[0]).toContain( + 'Invalid value for prop `value`', + ); + } }); it('does not update the value to a Symbol', function() { + spyOnDev(console, 'error'); var container = document.createElement('div'); - ReactDOM.render(, container); - ReactDOM.render(, container); + ReactDOM.render(, container); + ReactDOM.render(, container); var node = container.firstChild; expect(node.value).toBe(''); expect(node.getAttribute('value')).toBe(''); + + if (__DEV__) { + expect(console.error.calls.count()).toBe(1); + expect(console.error.calls.argsFor(0)[0]).toContain( + 'Invalid value for prop `value`', + ); + } }); it('does not allow initial assignment of Symbols to defaultValue', function() { var container = document.createElement('div'); - ReactDOM.render(, container); + ReactDOM.render(, container); var node = container.firstChild; expect(node.value).toBe(''); @@ -1628,15 +1644,24 @@ describe('ReactDOMInput', () => { describe('When given a function value', function() { it('does not allow initial assignment of functions to value', function() { + spyOnDev(console, 'error'); var container = document.createElement('div'); - ReactDOM.render( {}} />, container); + ReactDOM.render( {}} readOnly />, container); var node = container.firstChild; expect(node.value).toBe(''); expect(node.getAttribute('value')).toBe(''); + + if (__DEV__) { + expect(console.error.calls.count()).toBe(1); + expect(console.error.calls.argsFor(0)[0]).toContain( + 'Invalid value for prop `value`', + ); + } }); it('does not update the value to a function', function() { + spyOnDev(console, 'error'); var container = document.createElement('div'); ReactDOM.render(, container); ReactDOM.render( {}} />, container); @@ -1644,11 +1669,18 @@ describe('ReactDOMInput', () => { expect(node.value).toBe(''); expect(node.getAttribute('value')).toBe(''); + + if (__DEV__) { + expect(console.error.calls.count()).toBe(1); + expect(console.error.calls.argsFor(0)[0]).toContain( + 'Invalid value for prop `value`', + ); + } }); it('does not allow initial assignment of functions to defaultValue', function() { var container = document.createElement('div'); - ReactDOM.render( {}} />, container); + ReactDOM.render( {}} readOnly />, container); var node = container.firstChild; expect(node.value).toBe(''); From ab9e93245cd768c4e27e8c136d1974a8fd37dc31 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 1 Dec 2017 13:00:30 -0500 Subject: [PATCH 06/15] Add boolean to readOnly assignments --- .../src/__tests__/ReactDOMInput-test.js | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index b10c8686b681d..ffa6fbc434283 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -1590,7 +1590,10 @@ describe('ReactDOMInput', () => { it('does not allow initial assignment of Symbols to value', function() { spyOnDev(console, 'error'); var container = document.createElement('div'); - ReactDOM.render(, container); + ReactDOM.render( + , + container, + ); var node = container.firstChild; expect(node.value).toBe(''); @@ -1607,8 +1610,11 @@ describe('ReactDOMInput', () => { it('does not update the value to a Symbol', function() { spyOnDev(console, 'error'); var container = document.createElement('div'); - ReactDOM.render(, container); - ReactDOM.render(, container); + ReactDOM.render(, container); + ReactDOM.render( + , + container, + ); var node = container.firstChild; expect(node.value).toBe(''); @@ -1646,7 +1652,7 @@ describe('ReactDOMInput', () => { it('does not allow initial assignment of functions to value', function() { spyOnDev(console, 'error'); var container = document.createElement('div'); - ReactDOM.render( {}} readOnly />, container); + ReactDOM.render( {}} readOnly={true} />, container); var node = container.firstChild; expect(node.value).toBe(''); @@ -1680,7 +1686,7 @@ describe('ReactDOMInput', () => { it('does not allow initial assignment of functions to defaultValue', function() { var container = document.createElement('div'); - ReactDOM.render( {}} readOnly />, container); + ReactDOM.render( {}} />, container); var node = container.firstChild; expect(node.value).toBe(''); From 787cf4c5c0fc926294ed8b5dd987d3c96f2596f1 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 1 Dec 2017 19:20:33 +0000 Subject: [PATCH 07/15] Tweak the tests --- .../src/__tests__/ReactDOMInput-test.js | 58 +++++++++++-------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index ffa6fbc434283..cfa5cfb945d1e 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -1587,11 +1587,11 @@ describe('ReactDOMInput', () => { }); describe('When given a Symbol value', function() { - it('does not allow initial assignment of Symbols to value', function() { + it('treats initial Symbol value as an empty string', function() { spyOnDev(console, 'error'); var container = document.createElement('div'); ReactDOM.render( - , + {}} readOnly={true} />, container, ); var node = container.firstChild; @@ -1607,18 +1607,21 @@ describe('ReactDOMInput', () => { } }); - it('does not update the value to a Symbol', function() { + it('treats updated Symbol value as an empty string', function() { spyOnDev(console, 'error'); var container = document.createElement('div'); - ReactDOM.render(, container); ReactDOM.render( - , + {}} readOnly={true} />, + container, + ); + ReactDOM.render( + {}} readOnly={true} />, container, ); var node = container.firstChild; - expect(node.value).toBe(''); - expect(node.getAttribute('value')).toBe(''); + expect(node.value).toBe('foo'); + expect(node.getAttribute('value')).toBe('foo'); if (__DEV__) { expect(console.error.calls.count()).toBe(1); @@ -1628,31 +1631,35 @@ describe('ReactDOMInput', () => { } }); - it('does not allow initial assignment of Symbols to defaultValue', function() { + it('treats initial Symbol defaultValue as an empty string', function() { var container = document.createElement('div'); ReactDOM.render(, container); var node = container.firstChild; expect(node.value).toBe(''); expect(node.getAttribute('value')).toBe(''); + // TODO: we should warn here. }); - it('does not update defaultValue to a Symbol', function() { + it('treats updated Symbol defaultValue as an empty string', function() { var container = document.createElement('div'); - ReactDOM.render(, container); + ReactDOM.render(, container); ReactDOM.render(, container); var node = container.firstChild; - expect(node.value).toBe(''); - expect(node.getAttribute('value')).toBe(''); + expect(node.value).toBe('foo'); + expect(node.getAttribute('value')).toBe('foo'); }); }); describe('When given a function value', function() { - it('does not allow initial assignment of functions to value', function() { + it('treats initial function value as an empty string', function() { spyOnDev(console, 'error'); var container = document.createElement('div'); - ReactDOM.render( {}} readOnly={true} />, container); + ReactDOM.render( + {}} onChange={() => {}} readOnly={true} />, + container, + ); var node = container.firstChild; expect(node.value).toBe(''); @@ -1666,15 +1673,18 @@ describe('ReactDOMInput', () => { } }); - it('does not update the value to a function', function() { + it('treats updated function value as an empty string', function() { spyOnDev(console, 'error'); var container = document.createElement('div'); - ReactDOM.render(, container); - ReactDOM.render( {}} />, container); + ReactDOM.render( {}} />, container); + ReactDOM.render( + {}} onChange={() => {}} />, + container, + ); var node = container.firstChild; - expect(node.value).toBe(''); - expect(node.getAttribute('value')).toBe(''); + expect(node.value).toBe('foo'); + expect(node.getAttribute('value')).toBe('foo'); if (__DEV__) { expect(console.error.calls.count()).toBe(1); @@ -1684,7 +1694,7 @@ describe('ReactDOMInput', () => { } }); - it('does not allow initial assignment of functions to defaultValue', function() { + it('treats initial function defaultValue as an empty string', function() { var container = document.createElement('div'); ReactDOM.render( {}} />, container); var node = container.firstChild; @@ -1693,14 +1703,14 @@ describe('ReactDOMInput', () => { expect(node.getAttribute('value')).toBe(''); }); - it('does not update defaultValue to a function', function() { + it('treats updated function defaultValue as an empty string', function() { var container = document.createElement('div'); - ReactDOM.render(, container); + ReactDOM.render(, container); ReactDOM.render( {}} />, container); var node = container.firstChild; - expect(node.value).toBe(''); - expect(node.getAttribute('value')).toBe(''); + expect(node.value).toBe('foo'); + expect(node.getAttribute('value')).toBe('foo'); }); }); }); From 7215c8e71c9f64c374a440c97e2956266b63e9a0 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 1 Dec 2017 15:11:51 -0500 Subject: [PATCH 08/15] Invalid value attributes should convert to an empty string --- .../src/__tests__/ReactDOMInput-test.js | 20 +++++++++---------- .../src/client/ReactDOMFiberInput.js | 4 ++-- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index cfa5cfb945d1e..76016a64df5dd 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -1591,7 +1591,7 @@ describe('ReactDOMInput', () => { spyOnDev(console, 'error'); var container = document.createElement('div'); ReactDOM.render( - {}} readOnly={true} />, + {}} />, container, ); var node = container.firstChild; @@ -1611,17 +1611,17 @@ describe('ReactDOMInput', () => { spyOnDev(console, 'error'); var container = document.createElement('div'); ReactDOM.render( - {}} readOnly={true} />, + {}} />, container, ); ReactDOM.render( - {}} readOnly={true} />, + {}} />, container, ); var node = container.firstChild; - expect(node.value).toBe('foo'); - expect(node.getAttribute('value')).toBe('foo'); + expect(node.value).toBe(''); + expect(node.getAttribute('value')).toBe(''); if (__DEV__) { expect(console.error.calls.count()).toBe(1); @@ -1648,7 +1648,7 @@ describe('ReactDOMInput', () => { var node = container.firstChild; expect(node.value).toBe('foo'); - expect(node.getAttribute('value')).toBe('foo'); + expect(node.getAttribute('value')).toBe(''); }); }); @@ -1657,7 +1657,7 @@ describe('ReactDOMInput', () => { spyOnDev(console, 'error'); var container = document.createElement('div'); ReactDOM.render( - {}} onChange={() => {}} readOnly={true} />, + {}} onChange={() => {}} />, container, ); var node = container.firstChild; @@ -1683,8 +1683,8 @@ describe('ReactDOMInput', () => { ); var node = container.firstChild; - expect(node.value).toBe('foo'); - expect(node.getAttribute('value')).toBe('foo'); + expect(node.value).toBe(''); + expect(node.getAttribute('value')).toBe(''); if (__DEV__) { expect(console.error.calls.count()).toBe(1); @@ -1710,7 +1710,7 @@ describe('ReactDOMInput', () => { var node = container.firstChild; expect(node.value).toBe('foo'); - expect(node.getAttribute('value')).toBe('foo'); + expect(node.getAttribute('value')).toBe(''); }); }); }); diff --git a/packages/react-dom/src/client/ReactDOMFiberInput.js b/packages/react-dom/src/client/ReactDOMFiberInput.js index d4e0d1909947c..e56e2e9d68b42 100644 --- a/packages/react-dom/src/client/ReactDOMFiberInput.js +++ b/packages/react-dom/src/client/ReactDOMFiberInput.js @@ -182,10 +182,10 @@ export function updateWrapper(element: Element, props: Object) { updateChecked(element, props); - var value = shouldSetAttribute('value', props.value) ? props.value : null; + var value = shouldSetAttribute('value', props.value) ? props.value : ''; var defaultValue = shouldSetAttribute('value', props.defaultValue) ? props.defaultValue - : null; + : ''; if (value != null) { if (value === 0 && node.value === '') { From 80b5d92c6d8c93f44450104ce278c54bae322244 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 1 Dec 2017 15:25:15 -0500 Subject: [PATCH 09/15] Revert ChangeEventPlugin update. See #11746 --- packages/react-dom/src/events/ChangeEventPlugin.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/react-dom/src/events/ChangeEventPlugin.js b/packages/react-dom/src/events/ChangeEventPlugin.js index b3194a9b5df4e..fc76c88b79a1c 100644 --- a/packages/react-dom/src/events/ChangeEventPlugin.js +++ b/packages/react-dom/src/events/ChangeEventPlugin.js @@ -17,7 +17,6 @@ import getEventTarget from './getEventTarget'; import isEventSupported from './isEventSupported'; import {getNodeFromInstance} from '../client/ReactDOMComponentTree'; import * as inputValueTracking from '../client/inputValueTracking'; -import {synchronizeDefaultValue} from '../client/ReactDOMFiberInput'; var eventTypes = { change: { @@ -229,7 +228,6 @@ function handleControlledInputBlur(inst, node) { } // Fiber and ReactDOM keep wrapper state in separate places - // TODO: Is this still necessary now that Stack is gone? let state = inst._wrapperState || node._wrapperState; if (!state || !state.controlled || node.type !== 'number') { From ec65674bda6a0a0b9b559c08ad8658db5644fdfd Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Fri, 1 Dec 2017 15:33:30 -0500 Subject: [PATCH 10/15] Format --- packages/react-dom/src/__tests__/ReactDOMInput-test.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index 76016a64df5dd..95a97512b1725 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -1610,10 +1610,7 @@ describe('ReactDOMInput', () => { it('treats updated Symbol value as an empty string', function() { spyOnDev(console, 'error'); var container = document.createElement('div'); - ReactDOM.render( - {}} />, - container, - ); + ReactDOM.render( {}} />, container); ReactDOM.render( {}} />, container, From a44c34f0e03fd1cd28298c3c25ab4099aa73a33d Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Sat, 2 Dec 2017 12:19:01 -0500 Subject: [PATCH 11/15] 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. --- .../src/client/ReactDOMFiberInput.js | 36 +++++++++++-------- .../react-dom/src/events/ChangeEventPlugin.js | 3 +- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOMFiberInput.js b/packages/react-dom/src/client/ReactDOMFiberInput.js index e56e2e9d68b42..9c3c85e76f8b1 100644 --- a/packages/react-dom/src/client/ReactDOMFiberInput.js +++ b/packages/react-dom/src/client/ReactDOMFiberInput.js @@ -117,18 +117,15 @@ export function initWrapperState(element: Element, props: Object) { } } - var defaultValue = props.defaultValue == null ? '' : props.defaultValue; - var initialValue = props.value != null ? props.value : defaultValue; var node = ((element: any): InputWithWrapperState); - - if (!shouldSetAttribute('value', initialValue)) { - initialValue = ''; - } + var defaultValue = props.defaultValue == null ? '' : props.defaultValue; node._wrapperState = { initialChecked: props.checked != null ? props.checked : props.defaultChecked, - initialValue: initialValue, + initialValue: getSafeValue( + props.value != null ? props.value : defaultValue, + ), controlled: isControlled(props), }; } @@ -182,10 +179,7 @@ export function updateWrapper(element: Element, props: Object) { updateChecked(element, props); - var value = shouldSetAttribute('value', props.value) ? props.value : ''; - var defaultValue = shouldSetAttribute('value', props.defaultValue) - ? props.defaultValue - : ''; + var value = getSafeValue(props.value); if (value != null) { if (value === 0 && node.value === '') { @@ -209,9 +203,9 @@ export function updateWrapper(element: Element, props: Object) { } if (props.hasOwnProperty('value')) { - synchronizeDefaultValue(node, props.type, value); + setDefaultValue(node, props.type, value); } else if (props.hasOwnProperty('defaultValue')) { - synchronizeDefaultValue(node, props.type, defaultValue); + setDefaultValue(node, props.type, getSafeValue(props.defaultValue)); } if (props.checked == null && props.defaultChecked != null) { @@ -313,7 +307,7 @@ function updateNamedCousins(rootNode, props) { // when the user is inputting text // // https://github.com/facebook/react/issues/7253 -export function synchronizeDefaultValue( +export function setDefaultValue( node: InputWithWrapperState, type: ?string, value: *, @@ -330,3 +324,17 @@ export function synchronizeDefaultValue( } } } + +function getSafeValue(value: *): * { + switch (typeof value) { + case 'boolean': + case 'number': + case 'object': + case 'string': + case 'undefined': + return value; + default: + // function, symbol are assigned as empty strings + return ''; + } +} diff --git a/packages/react-dom/src/events/ChangeEventPlugin.js b/packages/react-dom/src/events/ChangeEventPlugin.js index fc76c88b79a1c..9d759907b5ef8 100644 --- a/packages/react-dom/src/events/ChangeEventPlugin.js +++ b/packages/react-dom/src/events/ChangeEventPlugin.js @@ -17,6 +17,7 @@ import getEventTarget from './getEventTarget'; import isEventSupported from './isEventSupported'; import {getNodeFromInstance} from '../client/ReactDOMComponentTree'; import * as inputValueTracking from '../client/inputValueTracking'; +import {setDefaultValue} from '../client/ReactDOMFiberInput'; var eventTypes = { change: { @@ -235,7 +236,7 @@ function handleControlledInputBlur(inst, node) { } // If controlled, assign the value attribute to the current value on blur - synchronizeDefaultValue(node, 'number', node.value); + setDefaultValue(node, 'number', node.value); } /** From d843cfe7a565b85de0d14251c4d9fb40b7c126d3 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Sat, 2 Dec 2017 12:33:10 -0500 Subject: [PATCH 12/15] Remove unused import --- packages/react-dom/src/client/ReactDOMFiberInput.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/react-dom/src/client/ReactDOMFiberInput.js b/packages/react-dom/src/client/ReactDOMFiberInput.js index 9c3c85e76f8b1..e12cd44c62b1f 100644 --- a/packages/react-dom/src/client/ReactDOMFiberInput.js +++ b/packages/react-dom/src/client/ReactDOMFiberInput.js @@ -16,7 +16,6 @@ import * as DOMPropertyOperations from './DOMPropertyOperations'; import {getFiberCurrentPropsFromNode} from './ReactDOMComponentTree'; import ReactControlledValuePropTypes from '../shared/ReactControlledValuePropTypes'; import * as inputValueTracking from './inputValueTracking'; -import {shouldSetAttribute} from '../shared/DOMProperty.js'; type InputWithWrapperState = HTMLInputElement & { _wrapperState: { From a65e25f34f826a607329f70a2261edaac8f6f35a Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Mon, 4 Dec 2017 08:44:06 -0500 Subject: [PATCH 13/15] 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 --- .../src/__tests__/ReactDOMInput-test.js | 55 +++++++++++++++++++ .../src/client/ReactDOMFiberInput.js | 13 +---- 2 files changed, 58 insertions(+), 10 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index 95a97512b1725..67c0c1f2b770b 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -248,6 +248,23 @@ describe('ReactDOMInput', () => { } }); + it('performs a state change from "" to 0', () => { + class Stub extends React.Component { + state = { + value: '', + }; + render() { + return ; + } + } + + var stub = ReactTestUtils.renderIntoDocument(); + var node = ReactDOM.findDOMNode(stub); + stub.setState({value: 0}); + + expect(node.value).toEqual('0'); + }); + it('distinguishes precision for extra zeroes in string number values', () => { spyOnDev(console, 'error'); class Stub extends React.Component { @@ -595,6 +612,7 @@ describe('ReactDOMInput', () => { var node = container.firstChild; expect(node.value).toBe('0'); + expect(node.defaultValue).toBe('0'); }); it('should properly transition from 0 to an empty value', function() { @@ -606,6 +624,43 @@ describe('ReactDOMInput', () => { var node = container.firstChild; expect(node.value).toBe(''); + expect(node.defaultValue).toBe(''); + }); + + it('should properly transition a text input from 0 to an empty 0.0', function() { + var container = document.createElement('div'); + + ReactDOM.render(, container); + ReactDOM.render(, container); + + var node = container.firstChild; + + expect(node.value).toBe('0.0'); + expect(node.defaultValue).toBe('0.0'); + }); + + it('should properly transition a number input from "" to 0', function() { + var container = document.createElement('div'); + + ReactDOM.render(, container); + ReactDOM.render(, container); + + var node = container.firstChild; + + expect(node.value).toBe('0'); + expect(node.defaultValue).toBe('0'); + }); + + it('should properly transition a number input from "" to "0"', function() { + var container = document.createElement('div'); + + ReactDOM.render(, container); + ReactDOM.render(, container); + + var node = container.firstChild; + + expect(node.value).toBe('0'); + expect(node.defaultValue).toBe('0'); }); it('should have the correct target value', () => { diff --git a/packages/react-dom/src/client/ReactDOMFiberInput.js b/packages/react-dom/src/client/ReactDOMFiberInput.js index e12cd44c62b1f..b82a4dcb0e7ac 100644 --- a/packages/react-dom/src/client/ReactDOMFiberInput.js +++ b/packages/react-dom/src/client/ReactDOMFiberInput.js @@ -181,18 +181,11 @@ export function updateWrapper(element: Element, props: Object) { var value = getSafeValue(props.value); if (value != null) { - 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) || 0; - + if (props.type === 'number') { if ( + (value === 0 && node.value === '') || // eslint-disable-next-line - value != valueAsNumber || - // eslint-disable-next-line - (value == valueAsNumber && node.value != value) + node.value != value ) { node.value = '' + value; } From ca66bd5ac44311a98d48a17dafb58fb357274beb Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 4 Dec 2017 14:28:41 +0000 Subject: [PATCH 14/15] Add todos --- packages/react-dom/src/__tests__/ReactDOMInput-test.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js index 67c0c1f2b770b..c89802c202917 100644 --- a/packages/react-dom/src/__tests__/ReactDOMInput-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMInput-test.js @@ -1701,6 +1701,7 @@ describe('ReactDOMInput', () => { expect(node.value).toBe('foo'); expect(node.getAttribute('value')).toBe(''); + // TODO: we should warn here. }); }); @@ -1753,6 +1754,7 @@ describe('ReactDOMInput', () => { expect(node.value).toBe(''); expect(node.getAttribute('value')).toBe(''); + // TODO: we should warn here. }); it('treats updated function defaultValue as an empty string', function() { @@ -1763,6 +1765,7 @@ describe('ReactDOMInput', () => { expect(node.value).toBe('foo'); expect(node.getAttribute('value')).toBe(''); + // TODO: we should warn here. }); }); }); From 343a334ded2eaab383e0db2bb21001e6ce3612ef Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 4 Dec 2017 14:36:30 +0000 Subject: [PATCH 15/15] Update the attribute table --- .../attribute-behavior/AttributeTableSnapshot.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/fixtures/attribute-behavior/AttributeTableSnapshot.md b/fixtures/attribute-behavior/AttributeTableSnapshot.md index a54c608c6e23e..8a36ac9e87eb4 100644 --- a/fixtures/attribute-behavior/AttributeTableSnapshot.md +++ b/fixtures/attribute-behavior/AttributeTableSnapshot.md @@ -2593,8 +2593,8 @@ | `defaultValue=(string 'false')`| (changed)| `"false"` | | `defaultValue=(string 'on')`| (changed)| `"on"` | | `defaultValue=(string 'off')`| (changed)| `"off"` | -| `defaultValue=(symbol)`| (changed, error, warning, ssr error)| `` | -| `defaultValue=(function)`| (changed, ssr warning)| `"function f() {}"` | +| `defaultValue=(symbol)`| (initial, ssr error, ssr mismatch)| `` | +| `defaultValue=(function)`| (initial, ssr mismatch)| `` | | `defaultValue=(null)`| (initial, ssr warning)| `` | | `defaultValue=(undefined)`| (initial)| `` | @@ -11768,8 +11768,8 @@ | `value=(string 'false')`| (changed)| `"false"` | | `value=(string 'on')`| (changed)| `"on"` | | `value=(string 'off')`| (changed)| `"off"` | -| `value=(symbol)`| (changed, error, warning, ssr error)| `` | -| `value=(function)`| (changed, warning, ssr warning)| `"function f() {}"` | +| `value=(symbol)`| (initial, warning, ssr error, ssr mismatch)| `` | +| `value=(function)`| (initial, warning, ssr mismatch)| `` | | `value=(null)`| (initial, warning, ssr warning)| `` | | `value=(undefined)`| (initial)| `` | @@ -11793,8 +11793,8 @@ | `value=(string 'false')`| (changed)| `"false"` | | `value=(string 'on')`| (changed)| `"on"` | | `value=(string 'off')`| (changed)| `"off"` | -| `value=(symbol)`| (changed, error, warning, ssr error)| `` | -| `value=(function)`| (changed, warning)| `"function f() {}"` | +| `value=(symbol)`| (initial, warning, ssr error, ssr mismatch)| `` | +| `value=(function)`| (initial, warning, ssr mismatch)| `` | | `value=(null)`| (initial, warning, ssr warning)| `` | | `value=(undefined)`| (initial)| `` | @@ -11818,7 +11818,7 @@ | `value=(string 'false')`| (initial)| `` | | `value=(string 'on')`| (initial)| `` | | `value=(string 'off')`| (initial)| `` | -| `value=(symbol)`| (changed, error, warning, ssr error)| `` | +| `value=(symbol)`| (initial, warning, ssr error, ssr mismatch)| `` | | `value=(function)`| (initial, warning)| `` | | `value=(null)`| (initial, warning, ssr warning)| `` | | `value=(undefined)`| (initial)| `` |