From f98e6c195568641fbca099e3cd2632a0d37d364f Mon Sep 17 00:00:00 2001 From: Jim Date: Tue, 24 May 2016 18:28:01 -0700 Subject: [PATCH] Properly set value and defaultValue for input and textarea (#6406) * Have `defaultValue` reach DOM node for html input box for #4618 * Cleanup and bug fixes for merge. (cherry picked from commit 4338c8db4b03cb1ba5f2e229b1baa5a848fe65ab) --- .../dom/client/wrappers/ReactDOMInput.js | 45 ++++- .../dom/client/wrappers/ReactDOMTextarea.js | 99 ++++++---- .../wrappers/__tests__/ReactDOMInput-test.js | 89 ++++++++- .../__tests__/ReactDOMTextarea-test.js | 177 ++++++++++++++---- .../dom/shared/HTMLDOMPropertyConfig.js | 3 +- src/renderers/dom/shared/ReactDOMComponent.js | 25 ++- .../__tests__/ReactDOMComponent-test.js | 46 ++--- .../__tests__/ReactStatelessComponent-test.js | 2 +- 8 files changed, 369 insertions(+), 117 deletions(-) diff --git a/src/renderers/dom/client/wrappers/ReactDOMInput.js b/src/renderers/dom/client/wrappers/ReactDOMInput.js index f539a73b1c8aa..2ce3826c6a17e 100644 --- a/src/renderers/dom/client/wrappers/ReactDOMInput.js +++ b/src/renderers/dom/client/wrappers/ReactDOMInput.js @@ -149,8 +149,8 @@ var ReactDOMInput = { var defaultValue = props.defaultValue; inst._wrapperState = { - initialChecked: props.defaultChecked || false, - initialValue: defaultValue != null ? defaultValue : null, + initialChecked: props.checked != null ? props.checked : props.defaultChecked, + initialValue: props.value != null ? props.value : defaultValue, listeners: null, onChange: _handleChange.bind(inst), }; @@ -166,13 +166,12 @@ var ReactDOMInput = { if (__DEV__) { warnIfValueIsNull(props); - var initialValue = inst._wrapperState.initialChecked || inst._wrapperState.initialValue; var defaultValue = props.defaultChecked || props.defaultValue; var controlled = props.checked !== undefined || props.value !== undefined; var owner = inst._currentElement._owner; if ( - (initialValue || !inst._wrapperState.controlled) && + !inst._wrapperState.controlled && controlled && !didWarnUncontrolledToControlled ) { warning( @@ -214,17 +213,45 @@ var ReactDOMInput = { ); } + var node = ReactDOMComponentTree.getNodeFromInstance(inst); var value = LinkedValueUtils.getValue(props); if (value != null) { + // Cast `value` to a string to ensure the value is set correctly. While // browsers typically do this as necessary, jsdom doesn't. - DOMPropertyOperations.setValueForProperty( - ReactDOMComponentTree.getNodeFromInstance(inst), - 'value', - '' + value - ); + var newValue = '' + value; + + // To avoid side effects (such as losing text selection), only set value if changed + if (newValue !== node.value) { + node.value = newValue; + } + } else { + if (props.value == null && props.defaultValue != null) { + node.defaultValue = '' + props.defaultValue; + } + if (props.checked == null && props.defaultChecked != null) { + node.defaultChecked = !!props.defaultChecked; + } } }, + + postMountWrapper: function(inst) { + // This is in postMount because we need access to the DOM node, which is not + // available until after the component has mounted. + var node = ReactDOMComponentTree.getNodeFromInstance(inst); + node.value = node.value; // Detach value from defaultValue + + // Normally, we'd just do `node.checked = node.checked` upon initial mount, less this bug + // this is needed to work around a chrome bug where setting defaultChecked + // will sometimes influence the value of checked (even after detachment). + // Reference: https://bugs.chromium.org/p/chromium/issues/detail?id=608416 + // We need to temporarily unset name to avoid disrupting radio button groups. + var name = node.name; + node.name = undefined; + node.defaultChecked = !node.defaultChecked; + node.defaultChecked = !node.defaultChecked; + node.name = name; + }, }; function _handleChange(event) { diff --git a/src/renderers/dom/client/wrappers/ReactDOMTextarea.js b/src/renderers/dom/client/wrappers/ReactDOMTextarea.js index 73b24315d6df2..11190eb3f9c6b 100644 --- a/src/renderers/dom/client/wrappers/ReactDOMTextarea.js +++ b/src/renderers/dom/client/wrappers/ReactDOMTextarea.js @@ -12,7 +12,6 @@ 'use strict'; var DisabledInputUtils = require('DisabledInputUtils'); -var DOMPropertyOperations = require('DOMPropertyOperations'); var LinkedValueUtils = require('LinkedValueUtils'); var ReactDOMComponentTree = require('ReactDOMComponentTree'); var ReactUpdates = require('ReactUpdates'); @@ -67,11 +66,14 @@ var ReactDOMTextarea = { ); // Always set children to the same thing. In IE9, the selection range will - // get reset if `textContent` is mutated. + // get reset if `textContent` is mutated. We could add a check in setTextContent + // to only set the value if/when the value differs from the node value (which would + // completely solve this IE9 bug), but Sebastian+Ben seemed to like this solution. + // The value can be a boolean or object so that's why it's forced to be a string. var hostProps = Object.assign({}, DisabledInputUtils.getHostProps(inst, props), { - defaultValue: undefined, value: undefined, - children: inst._wrapperState.initialValue, + defaultValue: undefined, + children: '' + inst._wrapperState.initialValue, onChange: inst._wrapperState.onChange, }); @@ -110,41 +112,45 @@ var ReactDOMTextarea = { warnIfValueIsNull(props); } - var defaultValue = props.defaultValue; - // TODO (yungsters): Remove support for children content in , container); + + expect(node.value).toBe('0'); + }); + + it('should not incur unnecessary DOM mutations', function() { + var container = document.createElement('div'); + ReactDOM.render(; - stub = renderTextarea(stub, container); - var node = ReactDOM.findDOMNode(stub); + var node = renderTextarea(stub, container); expect(console.error.argsForCall.length).toBe(1); expect(node.value).toBe('giraffe'); @@ -189,16 +265,44 @@ describe('ReactDOMTextarea', function() { expect(node.value).toEqual('giraffe'); }); + it('should keep value when switching to uncontrolled element if not changed', function() { + var container = document.createElement('div'); + + var node = renderTextarea(, container); + + expect(node.value).toEqual('kitten'); + }); + + it('should keep value when switching to uncontrolled element if changed', function() { + var container = document.createElement('div'); + + var node = renderTextarea(, container); + + expect(node.value).toBe('puppies'); + + ReactDOM.render(, container); + + expect(node.value).toEqual('puppies'); + }); + it('should allow numbers as children', function() { spyOn(console, 'error'); - var node = ReactDOM.findDOMNode(renderTextarea()); + var node = renderTextarea(); expect(console.error.argsForCall.length).toBe(1); expect(node.value).toBe('17'); }); it('should allow booleans as children', function() { spyOn(console, 'error'); - var node = ReactDOM.findDOMNode(renderTextarea()); + var node = renderTextarea(); expect(console.error.argsForCall.length).toBe(1); expect(node.value).toBe('false'); }); @@ -210,7 +314,7 @@ describe('ReactDOMTextarea', function() { return 'sharkswithlasers'; }, }; - var node = ReactDOM.findDOMNode(renderTextarea()); + var node = renderTextarea(); expect(console.error.argsForCall.length).toBe(1); expect(node.value).toBe('sharkswithlasers'); }); @@ -228,7 +332,7 @@ describe('ReactDOMTextarea', function() { var node; expect(function() { - node = ReactDOM.findDOMNode(renderTextarea()); + node = renderTextarea(); }).not.toThrow(); expect(node.value).toBe('[object Object]'); @@ -248,12 +352,12 @@ describe('ReactDOMTextarea', function() { ); - expect(ReactDOM.findDOMNode(instance).value).toBe('yolo'); + expect(instance.value).toBe('yolo'); expect(link.value).toBe('yolo'); expect(link.requestChange.mock.calls.length).toBe(0); - ReactDOM.findDOMNode(instance).value = 'test'; - ReactTestUtils.Simulate.change(ReactDOM.findDOMNode(instance)); + instance.value = 'test'; + ReactTestUtils.Simulate.change(instance); expect(link.requestChange.mock.calls.length).toBe(1); expect(link.requestChange.mock.calls[0][0]).toEqual('test'); @@ -297,4 +401,5 @@ describe('ReactDOMTextarea', function() { ); expect(console.error.argsForCall.length).toBe(1); }); + }); diff --git a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js index ce5e93cea6e1a..f5e0258d293a6 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -15,7 +15,6 @@ var DOMProperty = require('DOMProperty'); var MUST_USE_PROPERTY = DOMProperty.injection.MUST_USE_PROPERTY; var HAS_BOOLEAN_VALUE = DOMProperty.injection.HAS_BOOLEAN_VALUE; -var HAS_SIDE_EFFECTS = DOMProperty.injection.HAS_SIDE_EFFECTS; var HAS_NUMERIC_VALUE = DOMProperty.injection.HAS_NUMERIC_VALUE; var HAS_POSITIVE_NUMERIC_VALUE = DOMProperty.injection.HAS_POSITIVE_NUMERIC_VALUE; @@ -153,7 +152,7 @@ var HTMLDOMPropertyConfig = { // Setting .type throws on non- tags type: 0, useMap: 0, - value: MUST_USE_PROPERTY | HAS_SIDE_EFFECTS, + value: 0, width: 0, wmode: 0, wrap: 0, diff --git a/src/renderers/dom/shared/ReactDOMComponent.js b/src/renderers/dom/shared/ReactDOMComponent.js index 41550b3a5f158..042f6b109704f 100644 --- a/src/renderers/dom/shared/ReactDOMComponent.js +++ b/src/renderers/dom/shared/ReactDOMComponent.js @@ -240,6 +240,16 @@ function putListener() { ); } +function inputPostMount() { + var inst = this; + ReactDOMInput.postMountWrapper(inst); +} + +function textareaPostMount() { + var inst = this; + ReactDOMTextarea.postMountWrapper(inst); +} + function optionPostMount() { var inst = this; ReactDOMOption.postMountWrapper(inst); @@ -634,10 +644,20 @@ ReactDOMComponent.Mixin = { } switch (this._tag) { - case 'button': case 'input': - case 'select': + transaction.getReactMountReady().enqueue( + inputPostMount, + this + ); + break; case 'textarea': + transaction.getReactMountReady().enqueue( + textareaPostMount, + this + ); + break; + case 'button': + case 'select': if (props.autoFocus) { transaction.getReactMountReady().enqueue( AutoFocusUtils.focusDOMComponent, @@ -650,6 +670,7 @@ ReactDOMComponent.Mixin = { optionPostMount, this ); + break; } return mountImage; diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 3e0d48ebeb3ce..6efab43deea2e 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -496,30 +496,32 @@ describe('ReactDOMComponent', function() { ReactDOM.render(
, container); var node = container.firstChild; - var nodeValue = ''; // node.value always returns undefined - var nodeValueSetter = jest.fn(); - Object.defineProperty(node, 'value', { - get: function() { - return nodeValue; - }, - set: nodeValueSetter.mockImplementation(function(newValue) { - nodeValue = newValue; - }), - }); - function renderWithValueAndExpect(value, expected) { - ReactDOM.render(
, container); - expect(nodeValueSetter.mock.calls.length).toBe(expected); - } + var nodeValueSetter = jest.genMockFn(); + + var oldSetAttribute = node.setAttribute.bind(node); + node.setAttribute = function(key, value) { + oldSetAttribute(key, value); + nodeValueSetter(key, value); + }; + + ReactDOM.render(
, container); + expect(nodeValueSetter.mock.calls.length).toBe(1); - renderWithValueAndExpect(undefined, 0); - renderWithValueAndExpect('', 0); - renderWithValueAndExpect('foo', 1); - renderWithValueAndExpect('foo', 1); - renderWithValueAndExpect(undefined, 2); - renderWithValueAndExpect(null, 2); - renderWithValueAndExpect('', 2); - renderWithValueAndExpect(undefined, 2); + ReactDOM.render(
, container); + expect(nodeValueSetter.mock.calls.length).toBe(1); + + ReactDOM.render(
, container); + expect(nodeValueSetter.mock.calls.length).toBe(1); + + ReactDOM.render(
, container); + expect(nodeValueSetter.mock.calls.length).toBe(1); + + ReactDOM.render(
, container); + expect(nodeValueSetter.mock.calls.length).toBe(2); + + ReactDOM.render(
, container); + expect(nodeValueSetter.mock.calls.length).toBe(2); }); it('should not incur unnecessary DOM mutations for boolean properties', function() { diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactStatelessComponent-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactStatelessComponent-test.js index 56eceb67f2425..f8dda28fc8629 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactStatelessComponent-test.js @@ -108,7 +108,7 @@ describe('ReactStatelessComponent', function() { }).toThrow(); expect(console.error.calls.length).toBe(1); expect(console.error.argsForCall[0][0]).toContain( - 'NotAComponent(...): A valid React element (or null) must be returned. '+ + 'NotAComponent(...): A valid React element (or null) must be returned. ' + 'You may have returned undefined, an array or some other invalid object.' ); });