From 094ba200d8c923eeb5f9a85daae5118f0abb18a1 Mon Sep 17 00:00:00 2001 From: Nathan Hunzaker Date: Tue, 11 Jul 2017 07:31:43 -0400 Subject: [PATCH] Controlled inputs do not synchronize value or checked attribute This commit is a follow up to prior work to resolve issues with number inputs in React. Inputs keep their value/checked attribute in sync with the value/checked property. This is a React behavior. Traditionally browser DOM manipulation does not rely on keeping the value/checked attribute in sync. It's also very problematic for number inputs. After discussion, it was decided to make a breaking change to no longer sync up the value/checked attribute with it's assoicated property. For this to work, I made the following changes: - The value, defaultValue, checked and defaultChecked properties are now maintained within the HTML property config. - This required adding a filter to strip out the value property on selects and textareas - The logic to defer assignment of the value attribute has been removed form ChangeEventPlugin - defaultValue and defaultChecked are aliased to `value` and `checked` so that uncontrolled input attribute assignment works as intended. --- scripts/fiber/tests-passing.txt | 10 +-- .../dom/fiber/ReactDOMFiberComponent.js | 4 ++ .../dom/fiber/wrappers/ReactDOMFiberInput.js | 12 ++-- .../dom/fiber/wrappers/ReactDOMFiberSelect.js | 1 + .../dom/shared/HTMLDOMPropertyConfig.js | 33 ++-------- .../__tests__/DOMPropertyOperations-test.js | 29 --------- .../shared/eventPlugins/ChangeEventPlugin.js | 25 -------- .../wrappers/__tests__/ReactDOMInput-test.js | 61 ++----------------- .../stack/client/wrappers/ReactDOMInput.js | 12 ++-- .../stack/client/wrappers/ReactDOMSelect.js | 1 + .../shared/server/ReactPartialRenderer.js | 2 + 11 files changed, 35 insertions(+), 155 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index f642283bdbb9e..f57a3639266a8 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -717,8 +717,6 @@ src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js * should set className to empty string instead of null * should remove property properly for boolean properties * should remove property properly even with different name -* should update an empty attribute to zero -* should always assign the value attribute for non-inputs * should remove attributes for normal properties * should not remove attributes for special properties * should not leave all options selected when deleting multiple @@ -1805,7 +1803,7 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js * should control values in reentrant events with different targets * does change the number 2 to "2.0" with no change handler * does change the string "2" to "2.0" with no change handler -* changes the number 2 to "2.0" using a change handler +* changes the value property from number 2 to "2.0" using a change handler * does change the string ".98" to "0.98" with no change handler * distinguishes precision for extra zeroes in string number values * should display `defaultValue` of number 0 @@ -1867,11 +1865,7 @@ src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js * sets value properly with type coming later in props * does not raise a validation warning when it switches types * resets value of date/time input to fix bugs in iOS Safari -* always sets the attribute when values change on text inputs -* does not set the value attribute on number inputs if focused -* sets the value attribute on number inputs on blur -* an uncontrolled number input will not update the value attribute on blur -* an uncontrolled text input will not update the value attribute on blur +* does not set the attribute when values change on text inputs src/renderers/dom/shared/wrappers/__tests__/ReactDOMOption-test.js * should flatten children to a string diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 9bc7e30a618e8..cf897006b3fec 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -846,8 +846,10 @@ var ReactDOMFiberComponent = { // Controlled attributes are not validated // TODO: Only ignore them on controlled tags. case 'value': + case 'defaultValue': break; case 'checked': + case 'defaultChecked': break; case 'selected': break; @@ -901,7 +903,9 @@ var ReactDOMFiberComponent = { // Controlled attributes are not validated // TODO: Only ignore them on controlled tags. propKey === 'value' || + propKey === 'defaultValue' || propKey === 'checked' || + propKey === 'defaultChecked' || propKey === 'selected' ) { // Noop diff --git a/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js b/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js index 6f89df4b1b3de..af4d581001804 100644 --- a/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js +++ b/src/renderers/dom/fiber/wrappers/ReactDOMFiberInput.js @@ -79,10 +79,14 @@ var ReactDOMInput = { }, props, { - defaultChecked: undefined, - defaultValue: undefined, - value: value != null ? value : node._wrapperState.initialValue, - checked: checked != null ? checked : node._wrapperState.initialChecked, + defaultValue: value == null + ? props.defaultValue + : node._wrapperState.initialValue, + defaultChecked: checked == null + ? props.defaultChecked + : node._wrapperState.initialChecked, + value: undefined, + checked: undefined, }, ); diff --git a/src/renderers/dom/fiber/wrappers/ReactDOMFiberSelect.js b/src/renderers/dom/fiber/wrappers/ReactDOMFiberSelect.js index 43d203c02eebb..4b532dc016133 100644 --- a/src/renderers/dom/fiber/wrappers/ReactDOMFiberSelect.js +++ b/src/renderers/dom/fiber/wrappers/ReactDOMFiberSelect.js @@ -133,6 +133,7 @@ var ReactDOMSelect = { getHostProps: function(element: Element, props: Object) { return Object.assign({}, props, { value: undefined, + defaultValue: undefined, }); }, diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index 6c0f387873d1e..6139362480a40 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -49,6 +49,7 @@ var HTMLDOMPropertyConfig = { charSet: 0, challenge: 0, checked: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE, + defaultChecked: HAS_BOOLEAN_VALUE, cite: 0, classID: 0, className: 0, @@ -159,6 +160,7 @@ var HTMLDOMPropertyConfig = { type: 0, useMap: 0, value: 0, + defaultValue: 0, width: 0, wmode: 0, wrap: 0, @@ -211,36 +213,11 @@ var HTMLDOMPropertyConfig = { className: 'class', htmlFor: 'for', httpEquiv: 'http-equiv', + defaultValue: 'value', + defaultChecked: 'checked', }, DOMPropertyNames: {}, - DOMMutationMethods: { - value: function(node, value) { - if (value == null) { - return node.removeAttribute('value'); - } - - // Number inputs get special treatment due to some edge cases in - // Chrome. Let everything else assign the value attribute as normal. - // https://github.com/facebook/react/issues/7253#issuecomment-236074326 - if (node.type !== 'number' || node.hasAttribute('value') === false) { - node.setAttribute('value', '' + value); - } else if ( - node.validity && - !node.validity.badInput && - node.ownerDocument.activeElement !== node - ) { - // Don't assign an attribute if validation reports bad - // input. Chrome will clear the value. Additionally, don't - // operate on inputs that have focus, otherwise Chrome might - // strip off trailing decimal places and cause the user's - // cursor position to jump to the beginning of the input. - // - // In ReactDOMInput, we have an onBlur event that will trigger - // this function again when focus is lost. - node.setAttribute('value', '' + value); - } - }, - }, + DOMMutationMethods: {}, }; module.exports = HTMLDOMPropertyConfig; diff --git a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js index c0607871e55ad..dcd53e7f71a75 100644 --- a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js +++ b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js @@ -297,35 +297,6 @@ describe('DOMPropertyOperations', () => { }); }); - describe('value mutation method', function() { - it('should update an empty attribute to zero', function() { - var stubNode = document.createElement('input'); - var stubInstance = {_debugID: 1}; - ReactDOMComponentTree.precacheNode(stubInstance, stubNode); - - stubNode.setAttribute('type', 'radio'); - - DOMPropertyOperations.setValueForProperty(stubNode, 'value', ''); - spyOn(stubNode, 'setAttribute'); - DOMPropertyOperations.setValueForProperty(stubNode, 'value', 0); - - expect(stubNode.setAttribute.calls.count()).toBe(1); - }); - - it('should always assign the value attribute for non-inputs', function() { - var stubNode = document.createElement('progress'); - var stubInstance = {_debugID: 1}; - ReactDOMComponentTree.precacheNode(stubInstance, stubNode); - - spyOn(stubNode, 'setAttribute'); - - DOMPropertyOperations.setValueForProperty(stubNode, 'value', 30); - DOMPropertyOperations.setValueForProperty(stubNode, 'value', '30'); - - expect(stubNode.setAttribute.calls.count()).toBe(2); - }); - }); - describe('deleteValueForProperty', () => { var stubNode; var stubInstance; diff --git a/src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js b/src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js index f60e2d44a32a4..ac21b4c57ef16 100644 --- a/src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js +++ b/src/renderers/dom/shared/eventPlugins/ChangeEventPlugin.js @@ -226,26 +226,6 @@ function getTargetInstForInputOrChangeEvent(topLevelType, targetInst) { } } -function handleControlledInputBlur(inst, node) { - // TODO: In IE, inst is occasionally null. Why? - if (inst == null) { - return; - } - - // Fiber and ReactDOM keep wrapper state in separate places - let state = inst._wrapperState || node._wrapperState; - - if (!state || !state.controlled || node.type !== 'number') { - return; - } - - // If controlled, assign the value attribute to the current value on blur - let value = '' + node.value; - if (node.getAttribute('value') !== value) { - node.setAttribute('value', value); - } -} - /** * This plugin creates an `onChange` event that normalizes change events * across form elements. This event fires at a time when it's possible to @@ -300,11 +280,6 @@ var ChangeEventPlugin = { if (handleEventFunc) { handleEventFunc(topLevelType, targetNode, targetInst); } - - // When blurring, set the value attribute for number inputs - if (topLevelType === 'topBlur') { - handleControlledInputBlur(targetInst, targetNode); - } }, }; diff --git a/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js b/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js index b90923c772138..3f444726837d6 100644 --- a/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js +++ b/src/renderers/dom/shared/wrappers/__tests__/ReactDOMInput-test.js @@ -207,7 +207,7 @@ describe('ReactDOMInput', () => { expect(node.value).toBe('2'); }); - it('changes the number 2 to "2.0" using a change handler', () => { + it('changes the value property from number 2 to "2.0" using a change handler', () => { class Stub extends React.Component { state = { value: 2, @@ -229,7 +229,7 @@ describe('ReactDOMInput', () => { ReactTestUtils.Simulate.change(node); - expect(node.getAttribute('value')).toBe('2.0'); + expect(node.getAttribute('value')).toBe('2'); expect(node.value).toBe('2.0'); }); }); @@ -1283,67 +1283,14 @@ describe('ReactDOMInput', () => { }; } - it('always sets the attribute when values change on text inputs', function() { + it('does not set the attribute when values change on text inputs', function() { var Input = getTestInput(); var stub = ReactTestUtils.renderIntoDocument(); var node = ReactDOM.findDOMNode(stub); ReactTestUtils.Simulate.change(node, {target: {value: '2'}}); - expect(node.getAttribute('value')).toBe('2'); - }); - - it('does not set the value attribute on number inputs if focused', () => { - var Input = getTestInput(); - var stub = ReactTestUtils.renderIntoDocument( - , - ); - var node = ReactDOM.findDOMNode(stub); - - node.focus(); - - ReactTestUtils.Simulate.change(node, {target: {value: '2'}}); - - expect(node.getAttribute('value')).toBe('1'); - }); - - it('sets the value attribute on number inputs on blur', () => { - var Input = getTestInput(); - var stub = ReactTestUtils.renderIntoDocument( - , - ); - var node = ReactDOM.findDOMNode(stub); - - ReactTestUtils.Simulate.change(node, {target: {value: '2'}}); - ReactTestUtils.SimulateNative.blur(node); - - expect(node.getAttribute('value')).toBe('2'); - }); - - it('an uncontrolled number input will not update the value attribute on blur', () => { - var stub = ReactTestUtils.renderIntoDocument( - , - ); - var node = ReactDOM.findDOMNode(stub); - - node.value = 4; - - ReactTestUtils.SimulateNative.blur(node); - - expect(node.getAttribute('value')).toBe('1'); - }); - - it('an uncontrolled text input will not update the value attribute on blur', () => { - var stub = ReactTestUtils.renderIntoDocument( - , - ); - var node = ReactDOM.findDOMNode(stub); - - node.value = 4; - - ReactTestUtils.SimulateNative.blur(node); - - expect(node.getAttribute('value')).toBe('1'); + expect(node.getAttribute('value')).toBe(''); }); }); }); diff --git a/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js b/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js index 8ac425f52ed16..bb65a20b6c420 100644 --- a/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js +++ b/src/renderers/dom/stack/client/wrappers/ReactDOMInput.js @@ -70,10 +70,14 @@ var ReactDOMInput = { }, props, { - defaultChecked: undefined, - defaultValue: undefined, - value: value != null ? value : inst._wrapperState.initialValue, - checked: checked != null ? checked : inst._wrapperState.initialChecked, + defaultValue: value == null + ? props.defaultValue + : inst._wrapperState.initialValue, + defaultChecked: checked == null + ? props.defaultChecked + : inst._wrapperState.initialChecked, + value: undefined, + checked: undefined, }, ); diff --git a/src/renderers/dom/stack/client/wrappers/ReactDOMSelect.js b/src/renderers/dom/stack/client/wrappers/ReactDOMSelect.js index 21fea02bb6417..2340618755487 100644 --- a/src/renderers/dom/stack/client/wrappers/ReactDOMSelect.js +++ b/src/renderers/dom/stack/client/wrappers/ReactDOMSelect.js @@ -127,6 +127,7 @@ function updateOptions(inst, multiple, propValue) { var ReactDOMSelect = { getHostProps: function(inst, props) { return Object.assign({}, props, { + defaultValue: undefined, value: undefined, }); }, diff --git a/src/renderers/shared/server/ReactPartialRenderer.js b/src/renderers/shared/server/ReactPartialRenderer.js index 2061ce4b664f5..a460b9884086f 100644 --- a/src/renderers/shared/server/ReactPartialRenderer.js +++ b/src/renderers/shared/server/ReactPartialRenderer.js @@ -534,6 +534,7 @@ class ReactDOMServerRenderer { props = Object.assign({}, props, { value: undefined, + defaultValue: undefined, children: '' + initialValue, }); } else if (tag === 'select') { @@ -590,6 +591,7 @@ class ReactDOMServerRenderer { : props.defaultValue; props = Object.assign({}, props, { value: undefined, + defaultValue: undefined, }); } else if (tag === 'option') { var selected = null;