Refactor DOM attribute code (#11804)

* Harden tests around init/addition/update/removal of aliased attributes

I noticed some patterns weren't being tested.

* Call setValueForProperty() for null and undefined

The branching before the call is unnecessary because setValueForProperty() already
has an internal branch that delegates to deleteValueForProperty() for null and
undefined through the shouldIgnoreValue() check.

The goal is to start unifying these methods because their separation doesn't
reflect the current behavior (e.g. for unknown properties) anymore, and obscures
what actually happens with different inputs.

* Inline deleteValueForProperty() into setValueForProperty()

Now we don't read propertyInfo twice in this case.

I also dropped a few early returns. I added them a while ago when we had
Stack-only tracking of DOM operations, and some operations were being
counted twice because of how this code is structured. This isn't a problem
anymore (both because we don't track operations, and because I've just
inlined this method call).

* Inline deleteValueForAttribute() into setValueForAttribute()

The special cases for null and undefined already exist in setValueForAttribute().

* Delete some dead code

* Make setValueForAttribute() a branch of setValueForProperty()

Their naming is pretty confusing by now. For example setValueForProperty()
calls setValueForAttribute() when shouldSetAttribute() is false (!). I want
to refactor (as in, inline and then maybe factor it out differently) the relation
between them. For now, I'm consolidating the callers to use setValueForProperty().

* Make it more obvious where we skip and when we reset attributes

The naming of these methods is still very vague and conflicting in some cases.
Will need further work.

* Rewrite setValueForProperty() with early exits

This makes the flow clearer in my opinion.

* Move shouldIgnoreValue() into DOMProperty

It was previously duplicated.

It's also suspiciously similar in purpose to shouldTreatAttributeValueAsNull()
so I want to see if there is a way to unify them.

* Use more specific methods for testing validity

* Unify shouldTreatAttributeValueAsNull() and shouldIgnoreValue()

* Remove shouldSetAttribute()

Its naming was confusing and it was used all over the place instead of more specific checks.
Now that we only have one call site, we might as well inline and get rid of it.

* Remove unnecessary condition

* Remove another unnecessary condition

* Add Flow coverage

* Oops

* Fix lint (ESLint complains about Flow suppression)

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

@@ -442,6 +442,97 @@ describe('ReactDOMComponent', () => {
       expect(container.firstChild.className).toEqual('');
     });
 
+    it('should not set null/undefined attributes', () => {
+      var container = document.createElement('div');
+      // Initial render.
+      ReactDOM.render(<img src={null} data-foo={undefined} />, container);
+      var node = container.firstChild;
+      expect(node.hasAttribute('src')).toBe(false);
+      expect(node.hasAttribute('data-foo')).toBe(false);
+      // Update in one direction.
+      ReactDOM.render(<img src={undefined} data-foo={null} />, container);
+      expect(node.hasAttribute('src')).toBe(false);
+      expect(node.hasAttribute('data-foo')).toBe(false);
+      // Update in another direction.
+      ReactDOM.render(<img src={null} data-foo={undefined} />, container);
+      expect(node.hasAttribute('src')).toBe(false);
+      expect(node.hasAttribute('data-foo')).toBe(false);
+      // Removal.
+      ReactDOM.render(<img />, container);
+      expect(node.hasAttribute('src')).toBe(false);
+      expect(node.hasAttribute('data-foo')).toBe(false);
+      // Addition.
+      ReactDOM.render(<img src={undefined} data-foo={null} />, container);
+      expect(node.hasAttribute('src')).toBe(false);
+      expect(node.hasAttribute('data-foo')).toBe(false);
+    });
+
+    it('should apply React-specific aliases to HTML elements', () => {
+      var container = document.createElement('div');
+      ReactDOM.render(<form acceptCharset="foo" />, container);
+      var node = container.firstChild;
+      // Test attribute initialization.
+      expect(node.getAttribute('accept-charset')).toBe('foo');
+      expect(node.hasAttribute('acceptCharset')).toBe(false);
+      // Test attribute update.
+      ReactDOM.render(<form acceptCharset="boo" />, container);
+      expect(node.getAttribute('accept-charset')).toBe('boo');
+      expect(node.hasAttribute('acceptCharset')).toBe(false);
+      // Test attribute removal by setting to null.
+      ReactDOM.render(<form acceptCharset={null} />, container);
+      expect(node.hasAttribute('accept-charset')).toBe(false);
+      expect(node.hasAttribute('acceptCharset')).toBe(false);
+      // Restore.
+      ReactDOM.render(<form acceptCharset="foo" />, container);
+      expect(node.getAttribute('accept-charset')).toBe('foo');
+      expect(node.hasAttribute('acceptCharset')).toBe(false);
+      // Test attribute removal by setting to undefined.
+      ReactDOM.render(<form acceptCharset={undefined} />, container);
+      expect(node.hasAttribute('accept-charset')).toBe(false);
+      expect(node.hasAttribute('acceptCharset')).toBe(false);
+      // Restore.
+      ReactDOM.render(<form acceptCharset="foo" />, container);
+      expect(node.getAttribute('accept-charset')).toBe('foo');
+      expect(node.hasAttribute('acceptCharset')).toBe(false);
+      // Test attribute removal.
+      ReactDOM.render(<form />, container);
+      expect(node.hasAttribute('accept-charset')).toBe(false);
+      expect(node.hasAttribute('acceptCharset')).toBe(false);
+    });
+
+    it('should apply React-specific aliases to SVG elements', () => {
+      var container = document.createElement('div');
+      ReactDOM.render(<svg arabicForm="foo" />, container);
+      var node = container.firstChild;
+      // Test attribute initialization.
+      expect(node.getAttribute('arabic-form')).toBe('foo');
+      expect(node.hasAttribute('arabicForm')).toBe(false);
+      // Test attribute update.
+      ReactDOM.render(<svg arabicForm="boo" />, container);
+      expect(node.getAttribute('arabic-form')).toBe('boo');
+      expect(node.hasAttribute('arabicForm')).toBe(false);
+      // Test attribute removal by setting to null.
+      ReactDOM.render(<svg arabicForm={null} />, container);
+      expect(node.hasAttribute('arabic-form')).toBe(false);
+      expect(node.hasAttribute('arabicForm')).toBe(false);
+      // Restore.
+      ReactDOM.render(<svg arabicForm="foo" />, container);
+      expect(node.getAttribute('arabic-form')).toBe('foo');
+      expect(node.hasAttribute('arabicForm')).toBe(false);
+      // Test attribute removal by setting to undefined.
+      ReactDOM.render(<svg arabicForm={undefined} />, container);
+      expect(node.hasAttribute('arabic-form')).toBe(false);
+      expect(node.hasAttribute('arabicForm')).toBe(false);
+      // Restore.
+      ReactDOM.render(<svg arabicForm="foo" />, container);
+      expect(node.getAttribute('arabic-form')).toBe('foo');
+      expect(node.hasAttribute('arabicForm')).toBe(false);
+      // Test attribute removal.
+      ReactDOM.render(<svg />, container);
+      expect(node.hasAttribute('arabic-form')).toBe(false);
+      expect(node.hasAttribute('arabicForm')).toBe(false);
+    });
+
     it('should properly update custom attributes on custom elements', () => {
       const container = document.createElement('div');
       ReactDOM.render(<some-custom-element foo="bar" />, container);
@@ -451,6 +542,25 @@ describe('ReactDOMComponent', () => {
       expect(node.getAttribute('bar')).toBe('buzz');
     });
 
+    it('should not apply React-specific aliases to custom elements', () => {
+      var container = document.createElement('div');
+      ReactDOM.render(<some-custom-element arabicForm="foo" />, container);
+      var node = container.firstChild;
+      // Should not get transformed to arabic-form as SVG would be.
+      expect(node.getAttribute('arabicForm')).toBe('foo');
+      expect(node.hasAttribute('arabic-form')).toBe(false);
+      // Test attribute update.
+      ReactDOM.render(<some-custom-element arabicForm="boo" />, container);
+      expect(node.getAttribute('arabicForm')).toBe('boo');
+      // Test attribute removal and addition.
+      ReactDOM.render(<some-custom-element acceptCharset="buzz" />, container);
+      // Verify the previous attribute was removed.
+      expect(node.hasAttribute('arabicForm')).toBe(false);
+      // Should not get transformed to accept-charset as HTML would be.
+      expect(node.getAttribute('acceptCharset')).toBe('buzz');
+      expect(node.hasAttribute('accept-charset')).toBe(false);
+    });
+
     it('should clear a single style prop when changing `style`', () => {
       let styles = {display: 'none', color: 'red'};
       const container = document.createElement('div');

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

@@ -3,51 +3,33 @@
  *
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
+ *
+ * @flow
  */
 
 import {
-  ID_ATTRIBUTE_NAME,
-  ROOT_ATTRIBUTE_NAME,
   getPropertyInfo,
-  shouldSetAttribute,
+  shouldSkipAttribute,
+  shouldTreatAttributeValueAsNull,
   isAttributeNameSafe,
 } from '../shared/DOMProperty';
 
-// shouldIgnoreValue() is currently duplicated in DOMMarkupOperations.
-// TODO: Find a better place for this.
-function shouldIgnoreValue(propertyInfo, value) {
-  return (
-    value == null ||
-    (propertyInfo.hasBooleanValue && !value) ||
-    (propertyInfo.hasNumericValue && isNaN(value)) ||
-    (propertyInfo.hasPositiveNumericValue && value < 1) ||
-    (propertyInfo.hasOverloadedBooleanValue && value === false)
-  );
-}
-
-/**
- * Operations for dealing with DOM properties.
- */
-
-export function setAttributeForID(node, id) {
-  node.setAttribute(ID_ATTRIBUTE_NAME, id);
-}
-
-export function setAttributeForRoot(node) {
-  node.setAttribute(ROOT_ATTRIBUTE_NAME, '');
-}
-
 /**
  * Get the value for a property on a node. Only used in DEV for SSR validation.
  * The "expected" argument is used as a hint of what the expected value is.
  * Some properties have multiple equivalent values.
  */
-export function getValueForProperty(node, name, expected) {
+export function getValueForProperty(
+  node: Element,
+  name: string,
+  expected: mixed,
+): mixed {
   if (__DEV__) {
     const propertyInfo = getPropertyInfo(name);
     if (propertyInfo) {
       if (propertyInfo.mustUseProperty) {
-        return node[propertyInfo.propertyName];
+        const {propertyName} = propertyInfo;
+        return (node: any)[propertyName];
       } else {
         const attributeName = propertyInfo.attributeName;
 
@@ -59,16 +41,16 @@ export function getValueForProperty(node, name, expected) {
             if (value === '') {
               return true;
             }
-            if (shouldIgnoreValue(propertyInfo, expected)) {
+            if (shouldTreatAttributeValueAsNull(name, expected, false)) {
               return value;
             }
-            if (value === '' + expected) {
+            if (value === '' + (expected: any)) {
               return expected;
             }
             return value;
           }
         } else if (node.hasAttribute(attributeName)) {
-          if (shouldIgnoreValue(propertyInfo, expected)) {
+          if (shouldTreatAttributeValueAsNull(name, expected, false)) {
             // We had an attribute but shouldn't have had one, so read it
             // for the error message.
             return node.getAttribute(attributeName);
@@ -85,9 +67,9 @@ export function getValueForProperty(node, name, expected) {
           stringValue = node.getAttribute(attributeName);
         }
 
-        if (shouldIgnoreValue(propertyInfo, expected)) {
+        if (shouldTreatAttributeValueAsNull(name, expected, false)) {
           return stringValue === null ? expected : stringValue;
-        } else if (stringValue === '' + expected) {
+        } else if (stringValue === '' + (expected: any)) {
           return expected;
         } else {
           return stringValue;
@@ -102,7 +84,11 @@ export function getValueForProperty(node, name, expected) {
  * The third argument is used as a hint of what the expected value is. Some
  * attributes have multiple equivalent values.
  */
-export function getValueForAttribute(node, name, expected) {
+export function getValueForAttribute(
+  node: Element,
+  name: string,
+  expected: mixed,
+): mixed {
   if (__DEV__) {
     if (!isAttributeNameSafe(name)) {
       return;
@@ -111,7 +97,7 @@ export function getValueForAttribute(node, name, expected) {
       return expected === undefined ? undefined : null;
     }
     const value = node.getAttribute(name);
-    if (value === '' + expected) {
+    if (value === '' + (expected: any)) {
       return expected;
     }
     return value;
@@ -125,84 +111,64 @@ export function getValueForAttribute(node, name, expected) {
  * @param {string} name
  * @param {*} value
  */
-export function setValueForProperty(node, name, value) {
-  const propertyInfo = getPropertyInfo(name);
-
-  if (propertyInfo && shouldSetAttribute(name, value)) {
-    if (shouldIgnoreValue(propertyInfo, value)) {
-      deleteValueForProperty(node, name);
-      return;
-    } else if (propertyInfo.mustUseProperty) {
-      // Contrary to `setAttribute`, object properties are properly
-      // `toString`ed by IE8/9.
-      node[propertyInfo.propertyName] = value;
-    } else {
-      const attributeName = propertyInfo.attributeName;
-      const namespace = propertyInfo.attributeNamespace;
-      // `setAttribute` with objects becomes only `[object]` in IE8/9,
-      // ('' + value) makes it output the correct toString()-value.
-      if (namespace) {
-        node.setAttributeNS(namespace, attributeName, '' + value);
-      } else if (
-        propertyInfo.hasBooleanValue ||
-        (propertyInfo.hasOverloadedBooleanValue && value === true)
-      ) {
-        node.setAttribute(attributeName, '');
+export function setValueForProperty(
+  node: Element,
+  name: string,
+  value: mixed,
+  isCustomComponentTag: boolean,
+) {
+  if (shouldSkipAttribute(name, isCustomComponentTag)) {
+    return;
+  }
+  const propertyInfo = isCustomComponentTag ? null : getPropertyInfo(name);
+  if (shouldTreatAttributeValueAsNull(name, value, isCustomComponentTag)) {
+    value = null;
+  }
+  // If the prop isn't in the special list, treat it as a simple attribute.
+  if (!propertyInfo) {
+    if (isAttributeNameSafe(name)) {
+      const attributeName = name;
+      if (value == null) {
+        node.removeAttribute(attributeName);
       } else {
-        node.setAttribute(attributeName, '' + value);
+        node.setAttribute(attributeName, '' + (value: any));
       }
     }
-  } else {
-    setValueForAttribute(
-      node,
-      name,
-      shouldSetAttribute(name, value) ? value : null,
-    );
     return;
   }
-}
-
-export function setValueForAttribute(node, name, value) {
-  if (!isAttributeNameSafe(name)) {
+  const {
+    hasBooleanValue,
+    hasOverloadedBooleanValue,
+    mustUseProperty,
+  } = propertyInfo;
+  if (mustUseProperty) {
+    const {propertyName} = propertyInfo;
+    if (value === null) {
+      (node: any)[propertyName] = hasBooleanValue ? false : '';
+    } else {
+      // Contrary to `setAttribute`, object properties are properly
+      // `toString`ed by IE8/9.
+      (node: any)[propertyName] = value;
+    }
     return;
   }
-  if (value == null) {
-    node.removeAttribute(name);
+  // The rest are treated as attributes with special cases.
+  const {attributeName, attributeNamespace} = propertyInfo;
+  if (value === null) {
+    node.removeAttribute(attributeName);
   } else {
-    node.setAttribute(name, '' + value);
-  }
-}
-
-/**
- * Deletes an attributes from a node.
- *
- * @param {DOMElement} node
- * @param {string} name
- */
-export function deleteValueForAttribute(node, name) {
-  node.removeAttribute(name);
-}
-
-/**
- * Deletes the value for a property on a node.
- *
- * @param {DOMElement} node
- * @param {string} name
- */
-export function deleteValueForProperty(node, name) {
-  const propertyInfo = getPropertyInfo(name);
-  if (propertyInfo) {
-    if (propertyInfo.mustUseProperty) {
-      const propName = propertyInfo.propertyName;
-      if (propertyInfo.hasBooleanValue) {
-        node[propName] = false;
-      } else {
-        node[propName] = '';
-      }
+    let attributeValue;
+    if (hasBooleanValue || (hasOverloadedBooleanValue && value === true)) {
+      attributeValue = '';
+    } else {
+      // `setAttribute` with objects becomes only `[object]` in IE8/9,
+      // ('' + value) makes it output the correct toString()-value.
+      attributeValue = '' + (value: any);
+    }
+    if (attributeNamespace) {
+      node.setAttributeNS(attributeNamespace, attributeName, attributeValue);
     } else {
-      node.removeAttribute(propertyInfo.attributeName);
+      node.setAttribute(attributeName, attributeValue);
     }
-  } else {
-    node.removeAttribute(name);
   }
 }