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.

Author: nhunzaker

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

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

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,
       },
     );
 

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,
     });
   },
 

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;

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;

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);
-    }
   },
 };
 

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(<Input type="text" />);
       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(
-        <Input type="number" value="1" />,
-      );
-      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(
-        <Input type="number" value="1" />,
-      );
-      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(
-        <input type="number" defaultValue="1" />,
-      );
-      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(
-        <input type="text" defaultValue="1" />,
-      );
-      var node = ReactDOM.findDOMNode(stub);
-
-      node.value = 4;
-
-      ReactTestUtils.SimulateNative.blur(node);
-
-      expect(node.getAttribute('value')).toBe('1');
+      expect(node.getAttribute('value')).toBe('');
     });
   });
 });

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,
       },
     );
 

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,
     });
   },