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

Set value using attribute only on initial option render
(cherry picked from commit 2b1bd1d)

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

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');
+ });
 });

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;

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;

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() {