From 6db02e94ea1ba10752304ce3c6b39f00bfa776e7 Mon Sep 17 00:00:00 2001 From: Geoff Hayes Date: Tue, 15 Dec 2015 01:04:04 -0800 Subject: [PATCH 1/2] Have `defaultValue` reach DOM node for html input box for #4618 --- .../dom/client/wrappers/ReactDOMInput.js | 17 +-- .../dom/client/wrappers/ReactDOMTextarea.js | 90 +++++++------- .../wrappers/__tests__/ReactDOMInput-test.js | 86 ++++++++++++++ .../__tests__/ReactDOMTextarea-test.js | 112 ++++++++++++++++-- .../dom/shared/HTMLDOMPropertyConfig.js | 4 +- .../__tests__/ReactDOMComponent-test.js | 20 ++-- 6 files changed, 254 insertions(+), 75 deletions(-) diff --git a/src/renderers/dom/client/wrappers/ReactDOMInput.js b/src/renderers/dom/client/wrappers/ReactDOMInput.js index 881e8c146b4..c7b39488d9a 100644 --- a/src/renderers/dom/client/wrappers/ReactDOMInput.js +++ b/src/renderers/dom/client/wrappers/ReactDOMInput.js @@ -74,7 +74,7 @@ var ReactDOMInput = { }, props, { defaultChecked: undefined, defaultValue: undefined, - value: value != null ? value : inst._wrapperState.initialValue, + value: value != null ? value : props.defaultValue, checked: checked != null ? checked : inst._wrapperState.initialChecked, onChange: inst._wrapperState.onChange, }); @@ -137,10 +137,8 @@ var ReactDOMInput = { warnIfValueIsNull(props); } - var defaultValue = props.defaultValue; inst._wrapperState = { initialChecked: props.defaultChecked || false, - initialValue: defaultValue != null ? defaultValue : null, listeners: null, onChange: _handleChange.bind(inst), }; @@ -165,13 +163,16 @@ var ReactDOMInput = { var value = LinkedValueUtils.getValue(props); if (value != null) { + var node = ReactDOMComponentTree.getNodeFromInstance(inst); + // 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; + } } }, }; diff --git a/src/renderers/dom/client/wrappers/ReactDOMTextarea.js b/src/renderers/dom/client/wrappers/ReactDOMTextarea.js index ed5ac820f30..89e71129c5d 100644 --- a/src/renderers/dom/client/wrappers/ReactDOMTextarea.js +++ b/src/renderers/dom/client/wrappers/ReactDOMTextarea.js @@ -11,7 +11,6 @@ 'use strict'; -var DOMPropertyOperations = require('DOMPropertyOperations'); var LinkedValueUtils = require('LinkedValueUtils'); var ReactDOMComponentTree = require('ReactDOMComponentTree'); var ReactUpdates = require('ReactUpdates'); @@ -66,12 +65,46 @@ var ReactDOMTextarea = { '`dangerouslySetInnerHTML` does not make sense on , container); + + expect(node.value).toBe('1'); + }); + + it('should not incur unnecessary DOM mutations', function() { + var container = document.createElement('div'); + ReactDOM.render(; - stub = renderTextarea(stub, container); + stub = renderTextarea(stub, container, true); var node = ReactDOM.findDOMNode(stub); expect(console.error.argsForCall.length).toBe(1); expect(node.value).toBe('giraffe'); - // Changing children should do nothing, it functions like `defaultValue`. + // Changing children should cause value to change (new behavior of `defaultValue`) stub = ReactDOM.render(, container); - expect(node.value).toEqual('giraffe'); + expect(node.value).toEqual('gorilla'); + }); + + it('should not keep value when switching to uncontrolled element if not changed', function() { + var container = document.createElement('div'); + + var stub = renderTextarea(, container); + + expect(node.value).toEqual('gorilla'); + }); + + it('should keep value when switching to uncontrolled element if changed', function() { + var container = document.createElement('div'); + + var stub = renderTextarea(, container); + + expect(node.value).toBe('puppies'); + + ReactDOM.render(, container); + + expect(node.value).toEqual('puppies'); }); it('should allow numbers as children', function() { @@ -297,4 +386,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 c76f2c536ac..376258471bf 100644 --- a/src/renderers/dom/shared/HTMLDOMPropertyConfig.js +++ b/src/renderers/dom/shared/HTMLDOMPropertyConfig.js @@ -17,7 +17,6 @@ var ExecutionEnvironment = require('ExecutionEnvironment'); var MUST_USE_ATTRIBUTE = DOMProperty.injection.MUST_USE_ATTRIBUTE; 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; @@ -81,6 +80,7 @@ var HTMLDOMPropertyConfig = { data: null, // For `` acts as `src`. dateTime: MUST_USE_ATTRIBUTE, default: HAS_BOOLEAN_VALUE, + defaultValue: MUST_USE_PROPERTY, defer: HAS_BOOLEAN_VALUE, dir: MUST_USE_ATTRIBUTE, disabled: MUST_USE_ATTRIBUTE | HAS_BOOLEAN_VALUE, @@ -169,7 +169,7 @@ var HTMLDOMPropertyConfig = { // Setting .type throws on non- tags type: MUST_USE_ATTRIBUTE, useMap: null, - value: MUST_USE_PROPERTY | HAS_SIDE_EFFECTS, + value: MUST_USE_ATTRIBUTE, width: MUST_USE_ATTRIBUTE, wmode: MUST_USE_ATTRIBUTE, wrap: MUST_USE_ATTRIBUTE, diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 66178eb2362..bee1b6290b7 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -575,25 +575,25 @@ describe('ReactDOMComponent', function() { it('should not incur unnecessary DOM mutations', function() { var container = document.createElement('div'); - ReactDOM.render(
, container); + ReactDOM.render(
, container); var node = container.firstChild; - var nodeValue = ''; // node.value always returns undefined - var nodeValueSetter = jest.genMockFn(); - Object.defineProperty(node, 'value', { + var nodeId = ''; // node.value always returns undefined + var nodeIdSetter = jest.genMockFn(); + Object.defineProperty(node, 'id', { get: function() { - return nodeValue; + return nodeId; }, - set: nodeValueSetter.mockImplementation(function(newValue) { - nodeValue = newValue; + set: nodeIdSetter.mockImplementation(function(newValue) { + nodeId = newValue; }), }); - ReactDOM.render(
, container); - expect(nodeValueSetter.mock.calls.length).toBe(0); + ReactDOM.render(
, container); + expect(nodeIdSetter.mock.calls.length).toBe(0); ReactDOM.render(
, container); - expect(nodeValueSetter.mock.calls.length).toBe(1); + expect(nodeIdSetter.mock.calls.length).toBe(1); }); it('should ignore attribute whitelist for elements with the "is: attribute', function() { From d7886fd83b0d6b61a3148884d57e30723cd486c5 Mon Sep 17 00:00:00 2001 From: Geoff Hayes Date: Wed, 13 Jan 2016 01:19:53 -0800 Subject: [PATCH 2/2] Update specs to use ReactDOM.render() directly and remove specs with now undefined behavior --- .../wrappers/__tests__/ReactDOMInput-test.js | 51 ++----------- .../__tests__/ReactDOMTextarea-test.js | 71 +++++++------------ 2 files changed, 32 insertions(+), 90 deletions(-) diff --git a/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js b/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js index 3e204596be7..2b8bc5d5f8f 100644 --- a/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js +++ b/src/renderers/dom/client/wrappers/__tests__/ReactDOMInput-test.js @@ -59,8 +59,7 @@ describe('ReactDOMInput', function() { it('should update `defaultValue` for uncontrolled input', function() { var container = document.createElement('div'); - var el = ReactDOM.render(, container); - var node = ReactDOM.findDOMNode(el); + var node = ReactDOM.render(, container); expect(node.value).toBe('0'); @@ -69,47 +68,10 @@ describe('ReactDOMInput', function() { expect(node.value).toBe('1'); }); - it('should take `defaultValue` for a controlled input', function() { - var container = document.createElement('div'); - - var el = ReactDOM.render(, container); - var node = ReactDOM.findDOMNode(el); - - expect(node.value).toBe('0'); - - ReactDOM.render(, container); - - expect(node.value).toBe('1'); - - ReactDOM.render(, container); - - expect(node.value).toBe('3'); - - ReactDOM.render(, container); - - expect(node.value).toBe('5'); - expect(node.getAttribute('value')).toBe('5'); - expect(node.defaultValue).toBe('5'); - }); - - it('should update `value` when changing to controlled input', function() { - var container = document.createElement('div'); - - var el = ReactDOM.render(, container); - var node = ReactDOM.findDOMNode(el); - - expect(node.value).toBe('0'); - - ReactDOM.render(, container); - - expect(node.value).toBe('1'); - }); - it('should take `defaultValue` when changing to uncontrolled input', function() { var container = document.createElement('div'); - var el = ReactDOM.render(, container); - var node = ReactDOM.findDOMNode(el); + var node = ReactDOM.render(, container); expect(node.value).toBe('0'); @@ -143,8 +105,7 @@ describe('ReactDOMInput', function() { it('should allow setting `value` to `true`', function() { var container = document.createElement('div'); var stub = ; - stub = ReactDOM.render(stub, container); - var node = ReactDOM.findDOMNode(stub); + var node = ReactDOM.render(stub, container); expect(node.value).toBe('yolo'); @@ -158,8 +119,7 @@ describe('ReactDOMInput', function() { it('should allow setting `value` to `false`', function() { var container = document.createElement('div'); var stub = ; - stub = ReactDOM.render(stub, container); - var node = ReactDOM.findDOMNode(stub); + var node = ReactDOM.render(stub, container); expect(node.value).toBe('yolo'); @@ -173,8 +133,7 @@ describe('ReactDOMInput', function() { it('should allow setting `value` to `objToString`', function() { var container = document.createElement('div'); var stub = ; - stub = ReactDOM.render(stub, container); - var node = ReactDOM.findDOMNode(stub); + var node = ReactDOM.render(stub, container); expect(node.value).toBe('foo'); diff --git a/src/renderers/dom/client/wrappers/__tests__/ReactDOMTextarea-test.js b/src/renderers/dom/client/wrappers/__tests__/ReactDOMTextarea-test.js index 18645e026b3..9117c9330a2 100644 --- a/src/renderers/dom/client/wrappers/__tests__/ReactDOMTextarea-test.js +++ b/src/renderers/dom/client/wrappers/__tests__/ReactDOMTextarea-test.js @@ -31,47 +31,43 @@ describe('ReactDOMTextarea', function() { if (!container) { container = document.createElement('div'); } - var stub = ReactDOM.render(component, container); - var node = ReactDOM.findDOMNode(stub); + var node = ReactDOM.render(component, container); if (!skipReplace) { // Fixing jsdom's quirky behavior -- in reality, the parser should strip // off the leading newline but we need to do it by hand here. node.value = node.innerHTML.replace(/^\n/, ''); } - return stub; + return node; }; }); it('should allow setting `defaultValue`', function() { var container = document.createElement('div'); - var stub = renderTextarea(; - stub = renderTextarea(stub, container, true); - var node = ReactDOM.findDOMNode(stub); + var node = renderTextarea(stub, container, true); expect(console.error.argsForCall.length).toBe(1); expect(node.value).toBe('giraffe'); @@ -251,8 +236,7 @@ describe('ReactDOMTextarea', function() { it('should not keep value when switching to uncontrolled element if not changed', function() { var container = document.createElement('div'); - var stub = 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'); }); @@ -299,7 +282,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'); }); @@ -317,7 +300,7 @@ describe('ReactDOMTextarea', function() { var node; expect(function() { - node = ReactDOM.findDOMNode(renderTextarea()); + node = renderTextarea(); }).not.toThrow(); expect(node.value).toBe('[object Object]'); @@ -337,12 +320,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');