Merge pull request #2241 from syranide/selectvalue

ReactDOMSelect makeover, fix edge-case inconsistencies and remove state

Author: zpao

src/browser/ui/dom/components/ReactDOMSelect.js

@@ -22,11 +22,14 @@ var assign = require('Object.assign');
 
 var select = ReactElement.createFactory('select');
 
-function updateWithPendingValueIfMounted() {
+function updateOptionsIfPendingUpdateAndMounted() {
   /*jshint validthis:true */
-  if (this.isMounted()) {
-    this.setState({value: this._pendingValue});
-    this._pendingValue = 0;
+  if (this._pendingUpdate) {
+    this._pendingUpdate = false;
+    var value = LinkedValueUtils.getValue(this);
+    if (value != null && this.isMounted()) {
+      updateOptions(this, value);
+    }
   }
 }
 
@@ -56,40 +59,43 @@ function selectValueType(props, propName, componentName) {
 }
 
 /**
- * If `value` is supplied, updates <option> elements on mount and update.
  * @param {ReactComponent} component Instance of ReactDOMSelect
- * @param {?*} propValue For uncontrolled components, null/undefined. For
- * controlled components, a string (or with `multiple`, a list of strings).
+ * @param {*} propValue A stringable (with `multiple`, a list of stringables).
  * @private
  */
 function updateOptions(component, propValue) {
-  var multiple = component.props.multiple;
-  var value = propValue != null ? propValue : component.state.value;
-  var options = component.getDOMNode().options;
   var selectedValue, i, l;
-  if (multiple) {
+  var options = component.getDOMNode().options;
+
+  if (component.props.multiple) {
     selectedValue = {};
-    for (i = 0, l = value.length; i < l; ++i) {
-      selectedValue['' + value[i]] = true;
+    for (i = 0, l = propValue.length; i < l; i++) {
+      selectedValue['' + propValue[i]] = true;
+    }
+    for (i = 0, l = options.length; i < l; i++) {
+      var selected = selectedValue.hasOwnProperty(options[i].value);
+      if (options[i].selected !== selected) {
+        options[i].selected = selected;
+      }
     }
   } else {
-    selectedValue = '' + value;
-  }
-  for (i = 0, l = options.length; i < l; i++) {
-    var selected = multiple ?
-      selectedValue.hasOwnProperty(options[i].value) :
-      options[i].value === selectedValue;
-
-    if (selected !== options[i].selected) {
-      options[i].selected = selected;
+    // Do not set `select.value` as exact behavior isn't consistent across all
+    // browsers for all cases.
+    selectedValue = '' + propValue;
+    for (i = 0, l = options.length; i < l; i++) {
+      if (options[i].value === selectedValue) {
+        options[i].selected = true;
+        return;
+      }
     }
+    options[0].selected = true;
   }
 }
 
 /**
  * Implements a <select> native component that allows optionally setting the
  * props `value` and `defaultValue`. If `multiple` is false, the prop must be a
- * string. If `multiple` is true, the prop must be an array of strings.
+ * stringable. If `multiple` is true, the prop must be an array of stringables.
  *
  * If `value` is not supplied (or null/undefined), user actions that change the
  * selected option will trigger updates to the rendered options.
@@ -111,22 +117,6 @@ var ReactDOMSelect = ReactClass.createClass({
     value: selectValueType
   },
 
-  getInitialState: function() {
-    return {value: this.props.defaultValue || (this.props.multiple ? [] : '')};
-  },
-
-  componentWillMount: function() {
-    this._pendingValue = null;
-  },
-
-  componentWillReceiveProps: function(nextProps) {
-    if (!this.props.multiple && nextProps.multiple) {
-      this.setState({value: [this.state.value]});
-    } else if (this.props.multiple && !nextProps.multiple) {
-      this.setState({value: this.state.value[0]});
-    }
-  },
-
   render: function() {
     // Clone `this.props` so we don't mutate the input.
     var props = assign({}, this.props);
@@ -137,16 +127,32 @@ var ReactDOMSelect = ReactClass.createClass({
     return select(props, this.props.children);
   },
 
+  componentWillMount: function() {
+    this._pendingUpdate = false;
+  },
+
   componentDidMount: function() {
-    updateOptions(this, LinkedValueUtils.getValue(this));
+    var value = LinkedValueUtils.getValue(this);
+    if (value != null) {
+      updateOptions(this, value);
+    } else if (this.props.defaultValue != null) {
+      updateOptions(this, this.props.defaultValue);
+    }
   },
 
   componentDidUpdate: function(prevProps) {
     var value = LinkedValueUtils.getValue(this);
-    var prevMultiple = !!prevProps.multiple;
-    var multiple = !!this.props.multiple;
-    if (value != null || prevMultiple !== multiple) {
+    if (value != null) {
+      this._pendingUpdate = false;
       updateOptions(this, value);
+    } else if (!prevProps.multiple !== !this.props.multiple) {
+      // For simplicity, reapply `defaultValue` if `multiple` is toggled.
+      if (this.props.defaultValue != null) {
+        updateOptions(this, this.props.defaultValue);
+      } else {
+        // Revert the select back to its default unselected state.
+        updateOptions(this, this.props.multiple ? [] : '');
+      }
     }
   },
 
@@ -157,21 +163,8 @@ var ReactDOMSelect = ReactClass.createClass({
       returnValue = onChange.call(this, event);
     }
 
-    var selectedValue;
-    if (this.props.multiple) {
-      selectedValue = [];
-      var options = event.target.options;
-      for (var i = 0, l = options.length; i < l; i++) {
-        if (options[i].selected) {
-          selectedValue.push(options[i].value);
-        }
-      }
-    } else {
-      selectedValue = event.target.value;
-    }
-
-    this._pendingValue = selectedValue;
-    ReactUpdates.asap(updateWithPendingValueIfMounted, this);
+    this._pendingUpdate = true;
+    ReactUpdates.asap(updateOptionsIfPendingUpdateAndMounted, this);
     return returnValue;
   }
 

src/browser/ui/dom/components/__tests__/ReactDOMSelect-test.js

@@ -137,6 +137,27 @@ describe('ReactDOMSelect', function() {
     expect(node.options[2].selected).toBe(true);  // twelve
   });
 
+  it('should reset child options selected when they are changed and `value` is set', function() {
+    var stub =
+      <select multiple={true} value={["a", "b"]}>
+      </select>
+    stub = ReactTestUtils.renderIntoDocument(stub);
+
+    stub.setProps({
+      children: [
+        <option value="a">a</option>,
+        <option value="b">b</option>,
+        <option value="c">c</option>
+      ]
+    })
+
+    var node = stub.getDOMNode()
+
+    expect(node.options[0].selected).toBe(true);  // a
+    expect(node.options[1].selected).toBe(true);  // b
+    expect(node.options[2].selected).toBe(false);  // c
+  });
+
   it('should allow setting `value` with `objectToString`', function() {
     var objectToString = {
       animal: "giraffe",
@@ -181,12 +202,12 @@ describe('ReactDOMSelect', function() {
     expect(node.options[1].selected).toBe(true);  // giraffe
     expect(node.options[2].selected).toBe(false);  // gorilla
 
-    // When making it multiple, giraffe should still be selected
-    stub.setProps({multiple: true, defaultValue: null});
+    // When making it multiple, giraffe and gorilla should be selected
+    stub.setProps({multiple: true, defaultValue: ['giraffe', 'gorilla']});
 
     expect(node.options[0].selected).toBe(false);  // monkey
     expect(node.options[1].selected).toBe(true);  // giraffe
-    expect(node.options[2].selected).toBe(false);  // gorilla
+    expect(node.options[2].selected).toBe(true);  // gorilla
   });
 
   it('should allow switching from multiple', function() {
@@ -203,13 +224,57 @@ describe('ReactDOMSelect', function() {
     expect(node.options[1].selected).toBe(true);  // giraffe
     expect(node.options[2].selected).toBe(true);  // gorilla
 
-    // When removing multiple, giraffe should still be selected (but gorilla
-    // will no longer be)
-    stub.setProps({multiple: false, defaultValue: null});
+    // When removing multiple, defaultValue is applied again, being omitted
+    // means that "monkey" will be selected
+    stub.setProps({multiple: false, defaultValue: 'gorilla'});
+
+    expect(node.options[0].selected).toBe(false);  // monkey
+    expect(node.options[1].selected).toBe(false);  // giraffe
+    expect(node.options[2].selected).toBe(true);  // gorilla
+  });
+
+  it('should remember value when switching to uncontrolled', function() {
+    var stub =
+      <select value={'giraffe'}>
+        <option value="monkey">A monkey!</option>
+        <option value="giraffe">A giraffe!</option>
+        <option value="gorilla">A gorilla!</option>
+      </select>;
+    stub = ReactTestUtils.renderIntoDocument(stub);
+    var node = stub.getDOMNode();
 
     expect(node.options[0].selected).toBe(false);  // monkey
     expect(node.options[1].selected).toBe(true);  // giraffe
     expect(node.options[2].selected).toBe(false);  // gorilla
+
+    stub.setProps({value: null});
+
+    expect(node.options[0].selected).toBe(false);  // monkey
+    expect(node.options[1].selected).toBe(true);  // giraffe
+    expect(node.options[2].selected).toBe(false);  // gorilla
+  });
+
+  it('should remember updated value when switching to uncontrolled', function() {
+    var stub =
+      <select value={'giraffe'}>
+        <option value="monkey">A monkey!</option>
+        <option value="giraffe">A giraffe!</option>
+        <option value="gorilla">A gorilla!</option>
+      </select>;
+    stub = ReactTestUtils.renderIntoDocument(stub);
+    var node = stub.getDOMNode();
+
+    stub.setProps({value: 'gorilla'});
+
+    expect(node.options[0].selected).toBe(false);  // monkey
+    expect(node.options[1].selected).toBe(false);  // giraffe
+    expect(node.options[2].selected).toBe(true);  // gorilla
+
+    stub.setProps({value: null});
+
+    expect(node.options[0].selected).toBe(false);  // monkey
+    expect(node.options[1].selected).toBe(false);  // giraffe
+    expect(node.options[2].selected).toBe(true);  // gorilla
   });
 
   it('should support ReactLink', function() {