Use defaultValue instead of setAttribute('value') (#11534)

* Use defaultValue instead of setAttribute('value')

This commit replaces the method of synchronizing an input's value
attribute from using setAttribute to assigning defaultValue. This has
several benefits:

- Fixes issue where IE10+ and Edge password icon disappears (#7328)
- Fixes issue where toggling input types hides display value on dates
  in Safari (unreported)
- Removes mutationMethod behaviors from DOMPropertyOperations

* initialValue in Input wrapperState is always a string

* The value property is assigned before the value attribute. Fix related tests.

* Remove initial value tests in ReactDOMInput

I added these tests after removing the `value` mutation
method. However they do not add any additional value over existing
tests.

* Improve clarity of value checks in ReactDOMInput.postMountWrapper

* Remove value and defaultValue from InputWithWrapperState type

They are already included in the type definition for HTMLInputElement

* Inline stringification of value in ReactDOMInput

Avoids eagier stringification and makes usage more consistent.

* Use consistent value/defaultValue presence in postMountHook

Other methods in ReactDOMInput check for null instead of
hasOwnProperty.

* Add missing semicolon

* Remove unused value argument in ReactDOMInput test

* Address cases where a value switches to undefined

When a controlled input value switches to undefined, it reverts back
to the initial state of the controlled input.

We didn't have test coverage for this case, so I've added two describe
blocks to cover both null and undefined.

fixtures/dom/src/components/fixtures/number-inputs/index.js

@@ -27,9 +27,9 @@ function NumberInputs() {
         <NumberTestCase />
 
         <p className="footnote">
-          <b>Notes:</b> Chrome and Safari clear trailing decimals on blur. React
-          makes this concession so that the value attribute remains in sync with
-          the value property.
+          <b>Notes:</b> Modern Chrome and Safari {'<='} 6 clear trailing
+          decimals on blur. React makes this concession so that the value
+          attribute remains in sync with the value property.
         </p>
       </TestCase>
 

packages/react-dom/src/__tests__/ReactDOMInput-test.js

@@ -1249,15 +1249,20 @@ describe('ReactDOMInput', () => {
     var originalCreateElement = document.createElement;
     spyOnDevAndProd(document, 'createElement').and.callFake(function(type) {
       var el = originalCreateElement.apply(this, arguments);
+      var value = '';
+
       if (type === 'input') {
         Object.defineProperty(el, 'value', {
-          get: function() {},
-          set: function() {
-            log.push('set value');
+          get: function() {
+            return value;
+          },
+          set: function(val) {
+            value = '' + val;
+            log.push('set property value');
           },
         });
-        spyOnDevAndProd(el, 'setAttribute').and.callFake(function(name, value) {
-          log.push('set ' + name);
+        spyOnDevAndProd(el, 'setAttribute').and.callFake(function(name) {
+          log.push('set attribute ' + name);
         });
       }
       return el;
@@ -1267,14 +1272,14 @@ describe('ReactDOMInput', () => {
       <input value="0" type="range" min="0" max="100" step="1" />,
     );
     expect(log).toEqual([
-      'set type',
-      'set step',
-      'set min',
-      'set max',
-      'set value',
-      'set value',
-      'set checked',
-      'set checked',
+      'set attribute type',
+      'set attribute min',
+      'set attribute max',
+      'set attribute step',
+      'set property value',
+      'set attribute value',
+      'set attribute checked',
+      'set attribute checked',
     ]);
   });
 
@@ -1313,9 +1318,14 @@ describe('ReactDOMInput', () => {
     var originalCreateElement = document.createElement;
     spyOnDevAndProd(document, 'createElement').and.callFake(function(type) {
       var el = originalCreateElement.apply(this, arguments);
+      var value = '';
       if (type === 'input') {
         Object.defineProperty(el, 'value', {
+          get: function() {
+            return value;
+          },
           set: function(val) {
+            value = '' + val;
             log.push(`node.value = ${strify(val)}`);
           },
         });
@@ -1331,9 +1341,8 @@ describe('ReactDOMInput', () => {
     );
     expect(log).toEqual([
       'node.setAttribute("type", "date")',
+      'node.value = "1980-01-01"',
       'node.setAttribute("value", "1980-01-01")',
-      'node.value = ""',
-      'node.value = ""',
       'node.setAttribute("checked", "")',
       'node.setAttribute("checked", "")',
     ]);
@@ -1420,4 +1429,66 @@ describe('ReactDOMInput', () => {
       expect(node.getAttribute('value')).toBe('1');
     });
   });
+
+  describe('setting a controlled input to undefined', () => {
+    var input;
+
+    beforeEach(() => {
+      class Input extends React.Component {
+        state = {value: 'first'};
+        render() {
+          return (
+            <input
+              onChange={e => this.setState({value: e.target.value})}
+              value={this.state.value}
+            />
+          );
+        }
+      }
+
+      var stub = ReactTestUtils.renderIntoDocument(<Input />);
+      input = ReactDOM.findDOMNode(stub);
+      ReactTestUtils.Simulate.change(input, {target: {value: 'latest'}});
+      ReactTestUtils.Simulate.change(input, {target: {value: undefined}});
+    });
+
+    it('reverts the value attribute to the initial value', () => {
+      expect(input.getAttribute('value')).toBe('first');
+    });
+
+    it('preserves the value property', () => {
+      expect(input.value).toBe('latest');
+    });
+  });
+
+  describe('setting a controlled input to null', () => {
+    var input;
+
+    beforeEach(() => {
+      class Input extends React.Component {
+        state = {value: 'first'};
+        render() {
+          return (
+            <input
+              onChange={e => this.setState({value: e.target.value})}
+              value={this.state.value}
+            />
+          );
+        }
+      }
+
+      var stub = ReactTestUtils.renderIntoDocument(<Input />);
+      input = ReactDOM.findDOMNode(stub);
+      ReactTestUtils.Simulate.change(input, {target: {value: 'latest'}});
+      ReactTestUtils.Simulate.change(input, {target: {value: null}});
+    });
+
+    it('reverts the value attribute to the initial value', () => {
+      expect(input.getAttribute('value')).toBe('first');
+    });
+
+    it('preserves the value property', () => {
+      expect(input.value).toBe('latest');
+    });
+  });
 });

packages/react-dom/src/client/DOMPropertyOperations.js

@@ -73,8 +73,7 @@ export function getValueForProperty(node, name, expected) {
   if (__DEV__) {
     var propertyInfo = getPropertyInfo(name);
     if (propertyInfo) {
-      var mutationMethod = propertyInfo.mutationMethod;
-      if (mutationMethod || propertyInfo.mustUseProperty) {
+      if (propertyInfo.mustUseProperty) {
         return node[propertyInfo.propertyName];
       } else {
         var attributeName = propertyInfo.attributeName;
@@ -157,10 +156,7 @@ export function setValueForProperty(node, name, value) {
   var propertyInfo = getPropertyInfo(name);
 
   if (propertyInfo && shouldSetAttribute(name, value)) {
-    var mutationMethod = propertyInfo.mutationMethod;
-    if (mutationMethod) {
-      mutationMethod(node, value);
-    } else if (shouldIgnoreValue(propertyInfo, value)) {
+    if (shouldIgnoreValue(propertyInfo, value)) {
       deleteValueForProperty(node, name);
       return;
     } else if (propertyInfo.mustUseProperty) {
@@ -233,10 +229,7 @@ export function deleteValueForAttribute(node, name) {
 export function deleteValueForProperty(node, name) {
   var propertyInfo = getPropertyInfo(name);
   if (propertyInfo) {
-    var mutationMethod = propertyInfo.mutationMethod;
-    if (mutationMethod) {
-      mutationMethod(node, undefined);
-    } else if (propertyInfo.mustUseProperty) {
+    if (propertyInfo.mustUseProperty) {
       var propName = propertyInfo.propertyName;
       if (propertyInfo.hasBooleanValue) {
         node[propName] = false;

packages/react-dom/src/client/ReactDOMFiberInput.js

@@ -19,7 +19,7 @@ import * as inputValueTracking from './inputValueTracking';
 
 type InputWithWrapperState = HTMLInputElement & {
   _wrapperState: {
-    initialValue: ?string,
+    initialValue: string,
     initialChecked: ?boolean,
     controlled?: boolean,
   },
@@ -58,30 +58,14 @@ function isControlled(props) {
 
 export function getHostProps(element: Element, props: Object) {
   var node = ((element: any): InputWithWrapperState);
-  var value = props.value;
   var checked = props.checked;
 
-  var hostProps = Object.assign(
-    {
-      // Make sure we set .type before any other properties (setting .value
-      // before .type means .value is lost in IE11 and below)
-      type: undefined,
-      // Make sure we set .step before .value (setting .value before .step
-      // means .value is rounded on mount, based upon step precision)
-      step: undefined,
-      // Make sure we set .min & .max before .value (to ensure proper order
-      // in corner cases such as min or max deriving from value, e.g. Issue #7170)
-      min: undefined,
-      max: undefined,
-    },
-    props,
-    {
-      defaultChecked: undefined,
-      defaultValue: undefined,
-      value: value != null ? value : node._wrapperState.initialValue,
-      checked: checked != null ? checked : node._wrapperState.initialChecked,
-    },
-  );
+  var hostProps = Object.assign({}, props, {
+    defaultChecked: undefined,
+    defaultValue: undefined,
+    value: undefined,
+    checked: checked != null ? checked : node._wrapperState.initialChecked,
+  });
 
   return hostProps;
 }
@@ -132,7 +116,7 @@ export function initWrapperState(element: Element, props: Object) {
     }
   }
 
-  var defaultValue = props.defaultValue;
+  var defaultValue = props.defaultValue == null ? '' : props.defaultValue;
   var node = ((element: any): InputWithWrapperState);
   node._wrapperState = {
     initialChecked:
@@ -215,54 +199,34 @@ export function updateWrapper(element: Element, props: Object) {
       // browsers typically do this as necessary, jsdom doesn't.
       node.value = '' + value;
     }
-  } else {
-    if (props.value == null && props.defaultValue != null) {
-      // In Chrome, assigning defaultValue to certain input types triggers input validation.
-      // For number inputs, the display value loses trailing decimal points. For email inputs,
-      // Chrome raises "The specified value <x> is not a valid email address".
-      //
-      // Here we check to see if the defaultValue has actually changed, avoiding these problems
-      // when the user is inputting text
-      //
-      // https://github.com/facebook/react/issues/7253
-      if (node.defaultValue !== '' + props.defaultValue) {
-        node.defaultValue = '' + props.defaultValue;
-      }
-    }
-    if (props.checked == null && props.defaultChecked != null) {
-      node.defaultChecked = !!props.defaultChecked;
-    }
+    synchronizeDefaultValue(node, props.type, value);
+  } else if (
+    props.hasOwnProperty('value') ||
+    props.hasOwnProperty('defaultValue')
+  ) {
+    synchronizeDefaultValue(node, props.type, props.defaultValue);
+  }
+
+  if (props.checked == null && props.defaultChecked != null) {
+    node.defaultChecked = !!props.defaultChecked;
   }
 }
 
 export function postMountWrapper(element: Element, props: Object) {
   var node = ((element: any): InputWithWrapperState);
+  var initialValue = node._wrapperState.initialValue;
 
-  // Detach value from defaultValue. We won't do anything if we're working on
-  // submit or reset inputs as those values & defaultValues are linked. They
-  // are not resetable nodes so this operation doesn't matter and actually
-  // removes browser-default values (eg "Submit Query") when no value is
-  // provided.
+  if (props.value != null || props.defaultValue != null) {
+    // Do not assign value if it is already set. This prevents user text input
+    // from being lost during SSR hydration.
+    if (node.value === '') {
+      node.value = initialValue;
+    }
 
-  switch (props.type) {
-    case 'submit':
-    case 'reset':
-      break;
-    case 'color':
-    case 'date':
-    case 'datetime':
-    case 'datetime-local':
-    case 'month':
-    case 'time':
-    case 'week':
-      // This fixes the no-show issue on iOS Safari and Android Chrome:
-      // https://github.com/facebook/react/issues/7233
-      node.value = '';
-      node.value = node.defaultValue;
-      break;
-    default:
-      node.value = node.value;
-      break;
+    // value must be assigned before defaultValue. This fixes an issue where the
+    // visually displayed value of date inputs disappears on mobile Safari and Chrome:
+    // https://github.com/facebook/react/issues/7233
+    node.defaultValue = initialValue;
   }
 
   // Normally, we'd just do `node.checked = node.checked` upon initial mount, less this bug
@@ -334,3 +298,29 @@ function updateNamedCousins(rootNode, props) {
     }
   }
 }
+
+// In Chrome, assigning defaultValue to certain input types triggers input validation.
+// For number inputs, the display value loses trailing decimal points. For email inputs,
+// Chrome raises "The specified value <x> is not a valid email address".
+//
+// Here we check to see if the defaultValue has actually changed, avoiding these problems
+// when the user is inputting text
+//
+// https://github.com/facebook/react/issues/7253
+function synchronizeDefaultValue(
+  node: InputWithWrapperState,
+  type: ?string,
+  value: *,
+) {
+  if (
+    // Focused number inputs synchronize on blur. See ChangeEventPlugin.js
+    (type !== 'number' || node.ownerDocument.activeElement !== node) &&
+    node.defaultValue !== '' + value
+  ) {
+    if (value != null) {
+      node.defaultValue = '' + value;
+    } else {
+      node.defaultValue = node._wrapperState.initialValue;
+    }
+  }
+}

packages/react-dom/src/shared/DOMProperty.js

@@ -54,17 +54,13 @@ var DOMPropertyInjection = {
    * DOMPropertyNames: similar to DOMAttributeNames but for DOM properties.
    * Property names not specified use the normalized name.
    *
-   * DOMMutationMethods: Properties that require special mutation methods. If
-   * `value` is undefined, the mutation method should unset the property.
-   *
    * @param {object} domPropertyConfig the config as described above.
    */
   injectDOMPropertyConfig: function(domPropertyConfig) {
     var Injection = DOMPropertyInjection;
     var Properties = domPropertyConfig.Properties || {};
     var DOMAttributeNamespaces = domPropertyConfig.DOMAttributeNamespaces || {};
     var DOMAttributeNames = domPropertyConfig.DOMAttributeNames || {};
-    var DOMMutationMethods = domPropertyConfig.DOMMutationMethods || {};
 
     for (var propName in Properties) {
       invariant(
@@ -83,7 +79,6 @@ var DOMPropertyInjection = {
         attributeName: lowerCased,
         attributeNamespace: null,
         propertyName: propName,
-        mutationMethod: null,
 
         mustUseProperty: checkMask(propConfig, Injection.MUST_USE_PROPERTY),
         hasBooleanValue: checkMask(propConfig, Injection.HAS_BOOLEAN_VALUE),
@@ -121,10 +116,6 @@ var DOMPropertyInjection = {
         propertyInfo.attributeNamespace = DOMAttributeNamespaces[propName];
       }
 
-      if (DOMMutationMethods.hasOwnProperty(propName)) {
-        propertyInfo.mutationMethod = DOMMutationMethods[propName];
-      }
-
       // Downcase references to whitelist properties to check for membership
       // without case-sensitivity. This allows the whitelist to pick up
       // `allowfullscreen`, which should be written using the property configuration
@@ -154,9 +145,6 @@ export const ROOT_ATTRIBUTE_NAME = 'data-reactroot';
  * propertyName:
  *   Used on DOM node instances. (This includes properties that mutate due to
  *   external factors.)
- * mutationMethod:
- *   If non-null, used instead of the property or `setAttribute()` after
- *   initial render.
  * mustUseProperty:
  *   Whether the property must be accessed and mutated as an object property.
  * hasBooleanValue: