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
facebook · Nov 12, 2017 · 8c1343d · 8c1343d
1 parent acabf11
commit 8c1343d
Show file tree

Hide file tree

Showing 6 changed files with 103 additions and 129 deletions.
diff --git a/fixtures/dom/src/components/fixtures/number-inputs/index.js b/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>
 

diff --git a/packages/react-dom/src/__tests__/ReactDOMInput-test.js b/packages/react-dom/src/__tests__/ReactDOMInput-test.js
@@ -1152,15 +1152,20 @@ describe('ReactDOMInput', () => {
     var originalCreateElement = document.createElement;
     spyOn(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');
           },
         });
-        spyOn(el, 'setAttribute').and.callFake(function(name, value) {
-          log.push('set ' + name);
+        spyOn(el, 'setAttribute').and.callFake(function(name) {
+          log.push('set attribute ' + name);
         });
       }
       return el;
@@ -1170,14 +1175,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 attribute value',
+      'set property value',
+      'set attribute checked',
+      'set attribute checked',
     ]);
   });
 
@@ -1216,9 +1221,14 @@ describe('ReactDOMInput', () => {
     var originalCreateElement = document.createElement;
     spyOn(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)}`);
           },
         });
@@ -1235,8 +1245,7 @@ describe('ReactDOMInput', () => {
     expect(log).toEqual([
       'node.setAttribute("type", "date")',
       'node.setAttribute("value", "1980-01-01")',
-      'node.value = ""',
-      'node.value = ""',
+      'node.value = "1980-01-01"',
       'node.setAttribute("checked", "")',
       'node.setAttribute("checked", "")',
     ]);
@@ -1270,6 +1279,36 @@ describe('ReactDOMInput', () => {
       expect(node.getAttribute('value')).toBe('2');
     });
 
+    it('initially sets the value attribute on mount', () => {
+      var Input = getTestInput();
+      var stub = ReactTestUtils.renderIntoDocument(
+        <Input type="number" value="1" />,
+      );
+      var node = ReactDOM.findDOMNode(stub);
+
+      expect(node.getAttribute('value')).toBe('1');
+    });
+
+    it('initially sets the value attribute for submit on mount', () => {
+      var Input = getTestInput();
+      var stub = ReactTestUtils.renderIntoDocument(
+        <Input type="submit" value="1" />,
+      );
+      var node = ReactDOM.findDOMNode(stub);
+
+      expect(node.getAttribute('value')).toBe('1');
+    });
+
+    it('initially sets the value attribute for reset on mount', () => {
+      var Input = getTestInput();
+      var stub = ReactTestUtils.renderIntoDocument(
+        <Input type="reset" value="1" />,
+      );
+      var node = ReactDOM.findDOMNode(stub);
+
+      expect(node.getAttribute('value')).toBe('1');
+    });
+
     it('does not set the value attribute on number inputs if focused', () => {
       var Input = getTestInput();
       var stub = ReactTestUtils.renderIntoDocument(

diff --git a/packages/react-dom/src/client/DOMPropertyOperations.js b/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;

diff --git a/packages/react-dom/src/client/ReactDOMFiberInput.js b/packages/react-dom/src/client/ReactDOMFiberInput.js
@@ -57,30 +57,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;
 }
@@ -131,7 +115,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:
@@ -190,6 +174,7 @@ export function updateWrapper(element: Element, props: Object) {
   }
 
   var value = props.value;
+  var valueAsString = '' + props.value;
   if (value != null) {
     if (value === 0 && node.value === '') {
       node.value = '0';
@@ -206,26 +191,17 @@ export function updateWrapper(element: Element, props: Object) {
       ) {
         // Cast `value` to a string to ensure the value is set correctly. While
         // browsers typically do this as necessary, jsdom doesn't.
-        node.value = '' + value;
+        node.value = valueAsString;
       }
-    } else if (node.value !== '' + value) {
+    } else if (node.value !== valueAsString) {
       // Cast `value` to a string to ensure the value is set correctly. While
       // browsers typically do this as necessary, jsdom doesn't.
-      node.value = '' + value;
+      node.value = valueAsString;
     }
+    synchronizeDefaultValue(node, props.type, valueAsString);
   } 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;
-      }
+      synchronizeDefaultValue(node, props.type, '' + props.defaultValue);
     }
     if (props.checked == null && props.defaultChecked != null) {
       node.defaultChecked = !!props.defaultChecked;
@@ -235,32 +211,20 @@ export function updateWrapper(element: Element, props: Object) {
 
 export function postMountWrapper(element: Element, props: Object) {
   var node = ((element: any): InputWithWrapperState);
+  var hasUserInput = node.value !== '';
+  var value = 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 (value !== '' || props.hasOwnProperty('value')) {
+    // Do not assign value if it is already set. This prevents user text input
+    // from being lost during SSR hydration.
+    if (!hasUserInput) {
+      node.value = value;
+    }
 
-  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 = value;
   }
 
   // Normally, we'd just do `node.checked = node.checked` upon initial mount, less this bug
@@ -327,3 +291,21 @@ 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: Element, type: ?string, value: string) {
+  if (
+    // Focused number inputs synchronize on blur. See ChangeEventPlugin.js
+    (type !== 'number' || node.ownerDocument.activeElement !== node) &&
+    node.defaultValue !== value
+  ) {
+    node.defaultValue = value;
+  }
+}
diff --git a/packages/react-dom/src/shared/DOMProperty.js b/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:

diff --git a/packages/react-dom/src/shared/HTMLDOMPropertyConfig.js b/packages/react-dom/src/shared/HTMLDOMPropertyConfig.js
@@ -83,34 +83,6 @@ var HTMLDOMPropertyConfig = {
     htmlFor: 'for',
     httpEquiv: 'http-equiv',
   },
-  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);
-      }
-    },
-  },
 };
 
 export default HTMLDOMPropertyConfig;