fix(select, ngOptions): make the handling of unknown / empty options consistent

Narretz · Narretz · commit 2785ad72599c · 2016-08-26T14:16:59.000+02:00
- when the model does not match any option, the select generates an unknown option with the value
'? hashedModelVal ?' (select) or simply '?' (ngOptions) and inserts it into the select. This
option is then auto-selected by the browser. Once the user selects an option / the application sets
a matching model, the unknown option is removed from the select. It is therefore not user-selectable.

Alternatively, the application can provide an option with the value='', the so-called empty option.
The empty option takes over the role of the unknown option, in that it is selected when there's no
matching model, but it is not removed when a option and model match, and it can also be selected by
the user. The empty option has the value='' and sets the model to  if selected.

Previously, these concepts where a bit mushy, especially in ngOptions.

This fix now delegates the bulk of the work to the selectCtrl, as the concept of unknown and empty
option are the same for both directives. Only the generation of the empty option is different.

The commit also adjusts a test for an officially unsupported use-case: multiple empty options.
The new behavior is that the empty option that is registered last is used. The other one is still
present. Otherwise, this behavior is unspecified, and the docs are clear that only a single empty
option is allowed. We support dynamically generated empty options, but not multiple ones. I've left
the test to highlight that this might be someone's use case, and the we are aware of it.
diff --git a/src/ng/directive/ngOptions.js b/src/ng/directive/ngOptions.js
@@ -418,15 +418,14 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
 
       // The emptyOption allows the application developer to provide their own custom "empty"
       // option when the viewValue does not match any of the option values.
-      var emptyOption;
       for (var i = 0, children = selectElement.children(), ii = children.length; i < ii; i++) {
         if (children[i].value === '') {
-          emptyOption = children.eq(i);
+          selectCtrl.emptyOption = children.eq(i);
           break;
         }
       }
 
-      var providedEmptyOption = !!emptyOption;
+      var providedEmptyOption = !!selectCtrl.emptyOption;
 
       var unknownOption = jqLite(optionTemplate.cloneNode(false));
       unknownOption.val('?');
@@ -438,32 +437,9 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
       // we only need to create it once.
       var listFragment = $document[0].createDocumentFragment();
 
-      var renderEmptyOption = function() {
-        if (!providedEmptyOption) {
-          selectElement.prepend(emptyOption);
-        }
-        selectElement.val('');
-        emptyOption.prop('selected', true); // needed for IE
-        emptyOption.attr('selected', true);
-      };
-
-      var removeEmptyOption = function() {
-        if (!providedEmptyOption) {
-          emptyOption.remove();
-        } else {
-          emptyOption.removeAttr('selected');
-        }
-      };
-
-      var renderUnknownOption = function() {
-        selectElement.prepend(unknownOption);
-        selectElement.val('?');
-        unknownOption.prop('selected', true); // needed for IE
-        unknownOption.attr('selected', true);
-      };
-
-      var removeUnknownOption = function() {
-        unknownOption.remove();
+      // Overwrite the implementation. ngOptions doesn't use hashes
+      selectCtrl.generateUnknownOptionValue = function(val) {
+        return '?';
       };
 
       // Update the controller methods for multiple selectable options
@@ -484,21 +460,22 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
             // set always
 
             if (selectElement[0].value !== option.selectValue) {
-              removeUnknownOption();
-              removeEmptyOption();
+              selectCtrl.removeUnknownOption();
+              selectCtrl.unselectEmptyOption();
 
               selectElement[0].value = option.selectValue;
               option.element.selected = true;
             }
 
             option.element.setAttribute('selected', 'selected');
           } else {
-            if (value === null || providedEmptyOption) {
-              removeUnknownOption();
-              renderEmptyOption();
+
+            if (providedEmptyOption) {
+              selectCtrl.selectEmptyOption();
+            } else if (selectCtrl.unknownOption.parent().length) {
+              selectCtrl.updateUnknownOption(value);
             } else {
-              removeEmptyOption();
-              renderUnknownOption();
+              selectCtrl.renderUnknownOption(value);
             }
           }
         };
@@ -508,8 +485,8 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
           var selectedOption = options.selectValueMap[selectElement.val()];
 
           if (selectedOption && !selectedOption.disabled) {
-            removeEmptyOption();
-            removeUnknownOption();
+            selectCtrl.unselectEmptyOption();
+            selectCtrl.removeUnknownOption();
             return options.getViewValueFromOption(selectedOption);
           }
           return null;
@@ -575,21 +552,18 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
         }
       }
 
-
       if (providedEmptyOption) {
 
         // we need to remove it before calling selectElement.empty() because otherwise IE will
         // remove the label from the element. wtf?
-        emptyOption.remove();
+        selectCtrl.emptyOption.remove();
 
         // compile the element since there might be bindings in it
-        $compile(emptyOption)(scope);
+        $compile(selectCtrl.emptyOption)(scope);
 
         // remove the class, which is added automatically because we recompile the element and it
         // becomes the compilation root
-        emptyOption.removeClass('ng-scope');
-      } else {
-        emptyOption = jqLite(optionTemplate.cloneNode(false));
+        selectCtrl.emptyOption.removeClass('ng-scope');
       }
 
       selectElement.empty();
@@ -651,7 +625,7 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
 
         // Ensure that the empty option is always there if it was explicitly provided
         if (providedEmptyOption) {
-          selectElement.prepend(emptyOption);
+          selectElement.prepend(selectCtrl.emptyOption);
         }
 
         options.items.forEach(function addOption(option) {
diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js
@@ -31,28 +31,57 @@ var SelectController =
   // We can't just jqLite('<option>') since jqLite is not smart enough
   // to create it in <select> and IE barfs otherwise.
   self.unknownOption = jqLite(window.document.createElement('option'));
+
   self.renderUnknownOption = function(val) {
-    var unknownVal = '? ' + hashKey(val) + ' ?';
+    var unknownVal = self.generateUnknownOptionValue(val);
     self.unknownOption.val(unknownVal);
     $element.prepend(self.unknownOption);
+    self.unknownOption.prop('selected', true); // needed for IE
+    self.unknownOption.attr('selected', true);
     $element.val(unknownVal);
   };
 
   self.updateUnknownOption = function(val) {
-    var unknownVal = '? ' + hashKey(val) + ' ?';
+    var unknownVal = self.generateUnknownOptionValue(val);
     self.unknownOption.val(unknownVal);
+    self.unknownOption.prop('selected', true); // needed for IE
+    self.unknownOption.attr('selected', true);
     $element.val(unknownVal);
   };
 
-  $scope.$on('$destroy', function() {
-    // disable unknown option so that we don't do work when the whole select is being destroyed
-    self.renderUnknownOption = noop;
-  });
+  self.generateUnknownOptionValue = function(val) {
+    return '? ' + hashKey(val) + ' ?';
+  };
 
   self.removeUnknownOption = function() {
     if (self.unknownOption.parent()) self.unknownOption.remove();
   };
 
+  // The empty option is an option with the value '' that te application developer can
+  // provide inside the select. When the model changes to a value that doesn't match an option,
+  // it is selected - so if an empty option is provided, no unknown option is generated.
+  // However, the empty option is not removed when the model matches an option. It is always selectable
+  // and indicates that a "null" selection has been made.
+  self.emptyOption = undefined;
+
+  self.selectEmptyOption = function() {
+    if (self.emptyOption) {
+      $element.val('');
+      self.emptyOption.prop('selected', true); // needed for IE
+      self.emptyOption.attr('selected', true);
+    }
+  };
+
+  self.unselectEmptyOption = function() {
+    if (self.emptyOption) {
+      self.emptyOption.removeAttr('selected');
+    }
+  };
+
+  $scope.$on('$destroy', function() {
+    // disable unknown option so that we don't do work when the whole select is being destroyed
+    self.renderUnknownOption = noop;
+  });
 
   // Read the value of the select control, the implementation of this changes depending
   // upon whether the select can have multiple values and whether ngOptions is at work.
@@ -74,14 +103,15 @@ var SelectController =
   self.writeValue = function writeSingleValue(value) {
     if (self.hasOption(value)) {
       self.removeUnknownOption();
+
       var hashedVal = hashKey(value);
       $element.val(hashedVal in self.selectValueMap ? hashedVal : value);
 
       if (value === '') self.emptyOption.prop('selected', true); // to make IE9 happy
     } else {
       if (value == null && self.emptyOption) {
         self.removeUnknownOption();
-        $element.val('');
+        self.selectEmptyOption();
       } else if (self.unknownOption.parent().length) {
         self.updateUnknownOption(value);
       } else {
diff --git a/test/helpers/matchers.js b/test/helpers/matchers.js
@@ -319,6 +319,47 @@ beforeEach(function() {
           }
         };
       }
+    },
+    toBeMarkedAsSelected: function() {
+      // Selected is special because the element property and attribute reflect each other's state.
+      // IE9 will wrongly report hasAttribute('selected') === true when the property is
+      // undefined or null, and the dev tools show that no attribute is set
+      return {
+        compare: function(actual) {
+          var errors = [];
+          if (actual.selected === null || typeof actual.selected === 'undefined' || actual.selected === false) {
+            errors.push('Expected option property "selected" to be truthy');
+          }
+
+          if (msie !== 9 && actual.hasAttribute('selected') === false) {
+            errors.push('Expected option to have attribute "selected"');
+          }
+
+          var result = {
+            pass: errors.length === 0,
+            message: errors.join('\n')
+          };
+
+          return result;
+        },
+        negativeCompare: function(actual) {
+          var errors = [];
+          if (actual.selected) {
+            errors.push('Expected option property "selected" to be falsy');
+          }
+
+          if (msie !== 9 && actual.hasAttribute('selected')) {
+            errors.push('Expected option not to have attribute "selected"');
+          }
+
+          var result = {
+            pass: errors.length === 0,
+            message: errors.join('\n')
+          };
+
+          return result;
+        }
+      };
     }
   });
 });
diff --git a/test/ng/directive/ngOptionsSpec.js b/test/ng/directive/ngOptionsSpec.js
@@ -120,47 +120,6 @@ describe('ngOptions', function() {
             return { pass: errors.length === 0, message: message };
           }
         };
-      },
-      toBeMarkedAsSelected: function() {
-        // Selected is special because the element property and attribute reflect each other's state.
-        // IE9 will wrongly report hasAttribute('selected') === true when the property is
-        // undefined or null, and the dev tools show that no attribute is set
-        return {
-          compare: function(actual) {
-            var errors = [];
-            if (actual.selected === null || typeof actual.selected === 'undefined' || actual.selected === false) {
-              errors.push('Expected option property "selected" to be truthy');
-            }
-
-            if (msie !== 9 && actual.hasAttribute('selected') === false) {
-              errors.push('Expected option to have attribute "selected"');
-            }
-
-            var result = {
-              pass: errors.length === 0,
-              message: errors.join('\n')
-            };
-
-            return result;
-          },
-          negativeCompare: function(actual) {
-            var errors = [];
-            if (actual.selected) {
-              errors.push('Expected option property "selected" to be falsy');
-            }
-
-            if (msie !== 9 && actual.hasAttribute('selected')) {
-              errors.push('Expected option not to have attribute "selected"');
-            }
-
-            var result = {
-              pass: errors.length === 0,
-              message: errors.join('\n')
-            };
-
-            return result;
-          }
-        };
       }
     });
   });
@@ -2129,14 +2088,34 @@ describe('ngOptions', function() {
       expect(options.eq(1).prop('disabled')).toEqual(false);
     });
 
-    it('should insert a blank option if bound to null', function() {
+    it('should insert the unknown option if bound to null', function() {
       createSingleSelect();
 
       scope.$apply(function() {
         scope.values = [{name: 'A'}];
         scope.selected = null;
       });
 
+      expect(element.find('option').length).toEqual(2);
+      expect(element.val()).toEqual('?');
+      expect(jqLite(element.find('option')[0]).val()).toEqual('?');
+
+      scope.$apply(function() {
+        scope.selected = scope.values[0];
+      });
+
+      expect(element).toEqualSelectValue(scope.selected);
+      expect(element.find('option').length).toEqual(1);
+    });
+
+    it('should select the provided empty option if bound to null', function() {
+      createSingleSelect(true);
+
+      scope.$apply(function() {
+        scope.values = [{name: 'A'}];
+        scope.selected = null;
+      });
+
       expect(element.find('option').length).toEqual(2);
       expect(element.val()).toEqual('');
       expect(jqLite(element.find('option')[0]).val()).toEqual('');
@@ -2146,7 +2125,8 @@ describe('ngOptions', function() {
       });
 
       expect(element).toEqualSelectValue(scope.selected);
-      expect(element.find('option').length).toEqual(1);
+      expect(jqLite(element.find('option')[0]).val()).toEqual('');
+      expect(element.find('option').length).toEqual(2);
     });
 
 
@@ -2314,7 +2294,7 @@ describe('ngOptions', function() {
         scope.values.pop();
       });
 
-      expect(element.val()).toEqual('');
+      expect(element.val()).toEqual('?');
       expect(scope.selected).toEqual(null);
 
       // Check after model change
@@ -2328,7 +2308,7 @@ describe('ngOptions', function() {
         scope.values.pop();
       });
 
-      expect(element.val()).toEqual('');
+      expect(element.val()).toEqual('?');
       expect(scope.selected).toEqual(null);
     });
 
@@ -2377,7 +2357,7 @@ describe('ngOptions', function() {
   });
 
 
-  describe('blank option', function() {
+  describe('empty option', function() {
 
     it('should be compiled as template, be watched and updated', function() {
       var option;
diff --git a/test/ng/directive/selectSpec.js b/test/ng/directive/selectSpec.js
