From d25f3a79ff91a6024299dda34cf23187e6cb903d 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 +- scripts/rollup/results.json | 118 +++++++++--------- .../dom/fiber/ReactDOMFiberComponent.js | 5 + .../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 + 12 files changed, 95 insertions(+), 214 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/scripts/rollup/results.json b/scripts/rollup/results.json index 40c2dded5f539..b5508c2863cca 100644 --- a/scripts/rollup/results.json +++ b/scripts/rollup/results.json @@ -25,36 +25,36 @@ "gzip": 7214 }, "react-dom.development.js (UMD_DEV)": { - "size": 613141, - "gzip": 140395 + "size": 621770, + "gzip": 142030 }, "react-dom.production.min.js (UMD_PROD)": { - "size": 126584, - "gzip": 39853 + "size": 125762, + "gzip": 39791 }, "react-dom.development.js (NODE_DEV)": { - "size": 570841, - "gzip": 130520 + "size": 580431, + "gzip": 132387 }, "react-dom.production.min.js (NODE_PROD)": { - "size": 122880, - "gzip": 38546 + "size": 122127, + "gzip": 38480 }, "ReactDOMFiber-dev.js (FB_DEV)": { - "size": 570125, - "gzip": 130563 + "size": 579715, + "gzip": 132422 }, "ReactDOMFiber-prod.js (FB_PROD)": { - "size": 428502, - "gzip": 96996 + "size": 425463, + "gzip": 96331 }, "react-dom-test-utils.development.js (NODE_DEV)": { - "size": 53025, - "gzip": 13685 + "size": 53430, + "gzip": 13782 }, "ReactTestUtils-dev.js (FB_DEV)": { - "size": 52904, - "gzip": 13646 + "size": 53309, + "gzip": 13741 }, "ReactDOMServerStack-dev.js (FB_DEV)": { "size": 460810, @@ -65,20 +65,20 @@ "gzip": 81957 }, "react-dom-server.development.js (UMD_DEV)": { - "size": 308329, - "gzip": 77617 + "size": 307012, + "gzip": 76850 }, "react-dom-server.production.min.js (UMD_PROD)": { - "size": 66111, - "gzip": 22613 + "size": 65184, + "gzip": 22155 }, "react-dom-server.development.js (NODE_DEV)": { - "size": 266194, - "gzip": 67866 + "size": 265858, + "gzip": 67373 }, "react-dom-server.production.min.js (NODE_PROD)": { - "size": 62380, - "gzip": 21260 + "size": 61522, + "gzip": 20857 }, "ReactDOMServerStream-dev.js (FB_DEV)": { "size": 264750, @@ -89,64 +89,64 @@ "gzip": 51047 }, "react-art.development.js (UMD_DEV)": { - "size": 362062, - "gzip": 80236 + "size": 359303, + "gzip": 79940 }, "react-art.production.min.js (UMD_PROD)": { - "size": 99126, - "gzip": 30132 + "size": 97521, + "gzip": 29904 }, "react-art.development.js (NODE_DEV)": { - "size": 283458, - "gzip": 60201 + "size": 280721, + "gzip": 59867 }, "react-art.production.min.js (NODE_PROD)": { - "size": 60504, - "gzip": 18189 + "size": 58905, + "gzip": 17961 }, "ReactARTFiber-dev.js (FB_DEV)": { - "size": 282891, - "gzip": 60125 + "size": 280154, + "gzip": 59786 }, "ReactARTFiber-prod.js (FB_PROD)": { - "size": 220185, - "gzip": 45704 + "size": 215532, + "gzip": 44949 }, "ReactNativeStack-dev.js (RN_DEV)": { "size": 197039, - "gzip": 36193 + "gzip": 36189 }, "ReactNativeStack-prod.js (RN_PROD)": { "size": 136606, "gzip": 25990 }, "ReactNativeFiber-dev.js (RN_DEV)": { - "size": 301278, - "gzip": 51431 + "size": 298654, + "gzip": 51338 }, "ReactNativeFiber-prod.js (RN_PROD)": { - "size": 221863, - "gzip": 38015 + "size": 218380, + "gzip": 37833 }, "react-test-renderer.development.js (NODE_DEV)": { - "size": 280651, - "gzip": 59110 + "size": 277864, + "gzip": 58787 }, "ReactTestRendererFiber-dev.js (FB_DEV)": { - "size": 280075, - "gzip": 59030 + "size": 277288, + "gzip": 58707 }, "react-test-renderer-shallow.development.js (NODE_DEV)": { - "size": 8179, - "gzip": 2288 + "size": 8437, + "gzip": 2308 }, "ReactShallowRenderer-dev.js (FB_DEV)": { - "size": 8080, - "gzip": 2237 + "size": 8338, + "gzip": 2255 }, "react-noop-renderer.development.js (NODE_DEV)": { - "size": 274713, - "gzip": 57491 + "size": 272012, + "gzip": 57213 }, "ReactHTMLString-dev.js (FB_DEV)": { "size": 265654, @@ -181,20 +181,20 @@ "gzip": 50920 }, "ReactDOMServer-dev.js (FB_DEV)": { - "size": 265645, - "gzip": 67788 + "size": 265309, + "gzip": 67297 }, "ReactDOMServer-prod.js (FB_PROD)": { - "size": 197859, - "gzip": 51191 + "size": 195492, + "gzip": 50325 }, "react-dom-node-stream.development.js (NODE_DEV)": { - "size": 265427, - "gzip": 67670 + "size": 267552, + "gzip": 67871 }, "react-dom-node-stream.production.min.js (NODE_PROD)": { - "size": 62695, - "gzip": 21279 + "size": 62459, + "gzip": 21187 }, "ReactDOMNodeStream-dev.js (FB_DEV)": { "size": 264918, diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 9bc7e30a618e8..5f5a757859ddb 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -811,6 +811,7 @@ var ReactDOMFiberComponent = { break; case 'select': ReactDOMFiberSelect.initWrapperState(domElement, rawProps); + rawProps = ReactDOMFiberSelect.getHostProps(domElement, rawProps); trapBubbledEventsLocal(domElement, tag); // For controlled components we always need to ensure we're listening // to onChange. Even if there is no listener. @@ -846,8 +847,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 +904,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;