Merge pull request facebook#6449 from spicyj/option-value

Set value using attribute only on initial option render
zpao · Apr 8, 2016 · 2b1bd1d · 2b1bd1d
2 parents 516c1d8 + 5cb4a62
commit 2b1bd1d
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 29 deletions.
diff --git a/src/renderers/dom/client/wrappers/ReactDOMOption.js b/src/renderers/dom/client/wrappers/ReactDOMOption.js
@@ -12,6 +12,7 @@
 'use strict';
 
 var ReactChildren = require('ReactChildren');
+var ReactDOMComponentTree = require('ReactDOMComponentTree');
 var ReactDOMSelect = require('ReactDOMSelect');
 
 var warning = require('warning');
@@ -57,6 +58,15 @@ var ReactDOMOption = {
     inst._wrapperState = {selected: selected};
   },
 
+  postMountWrapper: function(inst) {
+    // value="" should make a value attribute (#6219)
+    var props = inst._currentElement.props;
+    if (props.value != null) {
+      var node = ReactDOMComponentTree.getNodeFromInstance(inst);
+      node.setAttribute('value', props.value);
+    }
+  },
+
   getNativeProps: function(inst, props) {
     var nativeProps = Object.assign({selected: undefined, children: undefined}, props);
 

diff --git a/src/renderers/dom/client/wrappers/__tests__/ReactDOMOption-test.js b/src/renderers/dom/client/wrappers/__tests__/ReactDOMOption-test.js
@@ -71,4 +71,15 @@ describe('ReactDOMOption', function() {
     var node = ReactDOM.findDOMNode(stub);
     expect(node.innerHTML).toBe('foobar');
   });
+
+  it('should set attribute for empty value', function() {
+    var container = document.createElement('div');
+    var option = ReactDOM.render(<option value="" />, container);
+    expect(option.hasAttribute('value')).toBe(true);
+    expect(option.getAttribute('value')).toBe('');
+
+    ReactDOM.render(<option value="lava" />, container);
+    expect(option.hasAttribute('value')).toBe(true);
+    expect(option.getAttribute('value')).toBe('lava');
+  });
 });
diff --git a/src/renderers/dom/shared/DOMPropertyOperations.js b/src/renderers/dom/shared/DOMPropertyOperations.js
@@ -149,10 +149,8 @@ var DOMPropertyOperations = {
         var propName = propertyInfo.propertyName;
         // Must explicitly cast values for HAS_SIDE_EFFECTS-properties to the
         // property type before comparing; only `value` does and is string.
-        // Must set `value` property if it is not null and not yet set.
         if (!propertyInfo.hasSideEffects ||
-            ('' + node[propName]) !== ('' + value) ||
-            !node.hasAttribute(propertyInfo.attributeName)) {
+            ('' + node[propName]) !== ('' + value)) {
           // Contrary to `setAttribute`, object properties are properly
           // `toString`ed by IE8/9.
           node[propName] = value;

diff --git a/src/renderers/dom/shared/ReactDOMComponent.js b/src/renderers/dom/shared/ReactDOMComponent.js
@@ -235,6 +235,11 @@ function putListener() {
   );
 }
 
+function optionPostMount() {
+  var inst = this;
+  ReactDOMOption.postMountWrapper(inst);
+}
+
 // There are so many media events, it makes sense to just
 // maintain a list rather than create a `trapBubbledEvent` for each
 var mediaEvents = {
@@ -600,6 +605,11 @@ ReactDOMComponent.Mixin = {
           );
         }
         break;
+      case 'option':
+        transaction.getReactMountReady().enqueue(
+          optionPostMount,
+          this
+        );
     }
 
     return mountImage;

diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
@@ -15,13 +15,11 @@
 describe('ReactDOMComponent', function() {
   var React;
   var ReactDOM;
-  var ReactDOMFeatureFlags;
   var ReactDOMServer;
 
   beforeEach(function() {
     jest.resetModuleRegistry();
     React = require('React');
-    ReactDOMFeatureFlags = require('ReactDOMFeatureFlags')
     ReactDOM = require('ReactDOM');
     ReactDOMServer = require('ReactDOMServer');
   });
@@ -503,30 +501,14 @@ describe('ReactDOMComponent', function() {
         expect(nodeValueSetter.mock.calls.length).toBe(expected);
       }
 
-      if (ReactDOMFeatureFlags.useCreateElement) {
-        renderWithValueAndExpect(undefined, 0);
-        renderWithValueAndExpect('', 1);
-        renderWithValueAndExpect('foo', 2);
-        renderWithValueAndExpect('foo', 2);
-        renderWithValueAndExpect(undefined, 3);
-        renderWithValueAndExpect(null, 3);
-        renderWithValueAndExpect('', 4);
-        renderWithValueAndExpect(undefined, 4);
-      } else {
-        renderWithValueAndExpect(undefined, 0);
-        // This differs because we will have created a node with the value
-        // attribute set. This means it will hasAttribute, so we won't try to
-        // set the value.
-        renderWithValueAndExpect('', 0);
-        renderWithValueAndExpect('foo', 1);
-        renderWithValueAndExpect('foo', 1);
-        renderWithValueAndExpect(undefined, 2);
-        renderWithValueAndExpect(null, 2);
-        // Again, much like the initial update case, we will always have the
-        // attribute set so we won't set the value.
-        renderWithValueAndExpect('', 2);
-        renderWithValueAndExpect(undefined, 2);
-      }
+      renderWithValueAndExpect(undefined, 0);
+      renderWithValueAndExpect('', 0);
+      renderWithValueAndExpect('foo', 1);
+      renderWithValueAndExpect('foo', 1);
+      renderWithValueAndExpect(undefined, 2);
+      renderWithValueAndExpect(null, 2);
+      renderWithValueAndExpect('', 2);
+      renderWithValueAndExpect(undefined, 2);
     });
 
     it('should not incur unnecessary DOM mutations for boolean properties', function() {