fix(select): properly handle empty & unknown options without ngOptions

Previously only when ngOptions was used, we correctly handled situations
when model was set to an unknown value. With this change, we'll add/remove
extra unknown option or reuse an existing empty option (option with value
set to "") when model is undefined.

src/ng/directive/select.js

@@ -22,16 +22,10 @@
  * be nested into the `<select>` element. This element will then represent `null` or "not selected"
  * option. See example below for demonstration.
  *
- * Note: `ngOptions` provides iterator facility for `<option>` element which must be used instead
+ * Note: `ngOptions` provides iterator facility for `<option>` element which should be used instead
- * of {@link angular.module.ng.$compileProvider.directive.ngRepeat ngRepeat}. `ngRepeat` is not
+ * of {@link angular.module.ng.$compileProvider.directive.ngRepeat ngRepeat} when you want the
- * suitable for use with `<option>` element because of the following reasons:
+ * `select` model to be bound to a non-string value. This is because an option element can currently
- *
+ * be bound to string values only.
- *   * value attribute of the option element that we need to bind to requires a string, but the
- *     source of data for the iteration might be in a form of array containing objects instead of
- *     strings
- *   * {@link angular.module.ng.$compileProvider.directive.ngRepeat ngRepeat} unrolls after the
-  *    select binds causing incorect rendering on most browsers.
- *   * binding to a value not in list confuses most browsers.
  *
  * @param {string} name assignable expression to data-bind to.
  * @param {string=} required The control is considered valid only if value is entered.
@@ -92,11 +86,11 @@
           <select ng-model="color" ng-options="c.name for c in colors"></select><br>
 
           Color (null allowed):
-          <div  class="nullable">
+          <span  class="nullable">
             <select ng-model="color" ng-options="c.name for c in colors">
               <option value="">-- chose color --</option>
             </select>
-          </div><br/>
+          </span><br/>
 
           Color grouped by shade:
           <select ng-model="color" ng-options="c.name group by c.shade for c in colors">
@@ -126,49 +120,136 @@
 var ngOptionsDirective = valueFn({ terminal: true });
 var selectDirective = ['$compile', '$parse', function($compile,   $parse) {
                          //00001111100000000000222200000000000000000000003333000000000000044444444444444444000000000555555555555555550000000666666666666666660000000000000007777
-  var NG_OPTIONS_REGEXP = /^\s*(.*?)(?:\s+as\s+(.*?))?(?:\s+group\s+by\s+(.*))?\s+for\s+(?:([\$\w][\$\w\d]*)|(?:\(\s*([\$\w][\$\w\d]*)\s*,\s*([\$\w][\$\w\d]*)\s*\)))\s+in\s+(.*)$/;
+  var NG_OPTIONS_REGEXP = /^\s*(.*?)(?:\s+as\s+(.*?))?(?:\s+group\s+by\s+(.*))?\s+for\s+(?:([\$\w][\$\w\d]*)|(?:\(\s*([\$\w][\$\w\d]*)\s*,\s*([\$\w][\$\w\d]*)\s*\)))\s+in\s+(.*)$/,
+      nullModelCtrl = {$setViewValue: noop};
 
   return {
     restrict: 'E',
-    require: '?ngModel',
+    require: ['select', '?ngModel'],
-    link: function(scope, element, attr, ctrl) {
+    controller: ['$element', '$scope', function($element, $scope) {
-      if (!ctrl) return;
+      var self = this,
+          optionsMap = {},
+          ngModelCtrl = nullModelCtrl,
+          nullOption,
+          unknownOption;
 
-      var multiple = attr.multiple,
+      self.init = function(ngModelCtrl_, nullOption_, unknownOption_) {
-          optionsExp = attr.ngOptions;
+        ngModelCtrl = ngModelCtrl_;
+        nullOption = nullOption_;
+        unknownOption = unknownOption_;
+      }
+
+
+      self.addOption = function(value) {
+        optionsMap[value] = true;
+
+        if (ngModelCtrl.$viewValue == value) {
+          $element.val(value);
+          if (unknownOption.parent()) unknownOption.remove();
+        }
+      };
+
+
+      self.removeOption = function(value) {
+        if (this.hasOption(value)) {
+          delete optionsMap[value];
+          if (ngModelCtrl.$viewValue == value) {
+            this.renderUnknownOption(value);
+          }
+        }
+      };
+
+
+      self.renderUnknownOption = function(val) {
+        var unknownVal = '? ' + hashKey(val) + ' ?';
+        unknownOption.val(unknownVal);
+        $element.prepend(unknownOption);
+        $element.val(unknownVal);
+        unknownOption.prop('selected', true); // needed for IE
+      }
+
+
+      self.hasOption = function(value) {
+        return optionsMap.hasOwnProperty(value);
+      }
+
+      $scope.$on('$destroy', function() {
+        // disable unknown option so that we don't do work when the whole select is being destroyed
+        self.renderUnknownOption = noop;
+      });
+    }],
+
+    link: function(scope, element, attr, ctrls) {
+      // if ngModel is not defined, we don't need to do anything
+      if (!ctrls[1]) return;
+
+      var selectCtrl = ctrls[0],
+          ngModelCtrl = ctrls[1],
+          multiple = attr.multiple,
+          optionsExp = attr.ngOptions,
+          nullOption = false, // if false, user will not be able to select it (used by ngOptions)
+          emptyOption,
+          // we can't just jqLite('<option>') since jqLite is not smart enough
+          // to create it in <select> and IE barfs otherwise.
+          optionTemplate = jqLite(document.createElement('option')),
+          optGroupTemplate =jqLite(document.createElement('optgroup')),
+          unknownOption = optionTemplate.clone();
+
+      // find "null" option
+      for(var i = 0, children = element.children(), ii = children.length; i < ii; i++) {
+        if (children[i].value == '') {
+          emptyOption = nullOption = children.eq(i);
+          break;
+        }
+      }
+
+      selectCtrl.init(ngModelCtrl, nullOption, unknownOption);
 
       // required validator
       if (multiple && (attr.required || attr.ngRequired)) {
         var requiredValidator = function(value) {
-          ctrl.$setValidity('required', !attr.required || (value && value.length));
+          ngModelCtrl.$setValidity('required', !attr.required || (value && value.length));
           return value;
         };
 
-        ctrl.$parsers.push(requiredValidator);
+        ngModelCtrl.$parsers.push(requiredValidator);
-        ctrl.$formatters.unshift(requiredValidator);
+        ngModelCtrl.$formatters.unshift(requiredValidator);
 
         attr.$observe('required', function() {
-          requiredValidator(ctrl.$viewValue);
+          requiredValidator(ngModelCtrl.$viewValue);
         });
       }
 
-      if (optionsExp) Options(scope, element, ctrl);
+      if (optionsExp) Options(scope, element, ngModelCtrl);
-      else if (multiple) Multiple(scope, element, ctrl);
+      else if (multiple) Multiple(scope, element, ngModelCtrl);
-      else Single(scope, element, ctrl);
+      else Single(scope, element, ngModelCtrl, selectCtrl);
 
 
       ////////////////////////////
 
 
 
-      function Single(scope, selectElement, ctrl) {
+      function Single(scope, selectElement, ngModelCtrl, selectCtrl) {
-        ctrl.$render = function() {
+        ngModelCtrl.$render = function() {
-          selectElement.val(ctrl.$viewValue);
+          var viewValue = ngModelCtrl.$viewValue;
+
+          if (selectCtrl.hasOption(viewValue)) {
+            if (unknownOption.parent()) unknownOption.remove();
+            selectElement.val(viewValue);
+            if (viewValue === '') emptyOption.prop('selected', true); // to make IE9 happy
+          } else {
+            if (isUndefined(viewValue) && emptyOption) {
+              selectElement.val('');
+            } else {
+              selectCtrl.renderUnknownOption(viewValue);
+            }
+          }
         };
 
         selectElement.bind('change', function() {
           scope.$apply(function() {
-            ctrl.$setViewValue(selectElement.val());
+            if (unknownOption.parent()) unknownOption.remove();
+            ngModelCtrl.$setViewValue(selectElement.val());
           });
         });
       }
@@ -219,26 +300,26 @@ var selectDirective = ['$compile', '$parse', function($compile,   $parse) {
             groupByFn = $parse(match[3] || ''),
             valueFn = $parse(match[2] ? match[1] : valueName),
             valuesFn = $parse(match[7]),
-            // we can't just jqLite('<option>') since jqLite is not smart enough
-            // to create it in <select> and IE barfs otherwise.
-            optionTemplate = jqLite(document.createElement('option')),
-            optGroupTemplate = jqLite(document.createElement('optgroup')),
-            nullOption = false, // if false then user will not be able to select it
             // This is an array of array of existing option groups in DOM. We try to reuse these if possible
             // optionGroupsCache[0] is the options with no option group
             // optionGroupsCache[?][0] is the parent: either the SELECT or OPTGROUP element
             optionGroupsCache = [[{element: selectElement, label:''}]];
 
-        // find existing special options
+        if (nullOption) {
-        forEach(selectElement.children(), function(option) {
+          // compile the element since there might be bindings in it
-          if (option.value == '') {
+          $compile(nullOption)(scope);
-            // developer declared null option, so user should be able to select it
+
-            nullOption = jqLite(option).remove();
+          // remove the class, which is added automatically because we recompile the element and it
-            // compile the element since there might be bindings in it
+          // becomes the compilation root
-            $compile(nullOption)(scope);
+          nullOption.removeClass('ng-scope');
-          }
+
-        });
+          // we need to remove it before calling selectElement.html('') because otherwise IE will
-        selectElement.html(''); // clear contents
+          // remove the label from the element. wtf?
+          nullOption.remove();
+        }
+
+        // clear contents, we'll add what's needed based on the model
+        selectElement.html('');
 
         selectElement.bind('change', function() {
           scope.$apply(function() {
@@ -250,8 +331,8 @@ var selectDirective = ['$compile', '$parse', function($compile,   $parse) {
             if (multiple) {
               value = [];
               for (groupIndex = 0, groupLength = optionGroupsCache.length;
-              groupIndex < groupLength;
+                   groupIndex < groupLength;
-              groupIndex++) {
+                   groupIndex++) {
                 // list of options for that group. (first item has the parent)
                 optionGroup = optionGroupsCache[groupIndex];
 
@@ -365,7 +446,7 @@ var selectDirective = ['$compile', '$parse', function($compile,   $parse) {
               }
             }
 
-            lastElement = null;  // start at the begining
+            lastElement = null;  // start at the beginning
             for(index = 0, length = optionGroup.length; index < length; index++) {
               option = optionGroup[index];
               if ((existingOption = existingOptions[index+1])) {
@@ -431,19 +512,34 @@ var optionDirective = ['$interpolate', function($interpolate) {
   return {
     restrict: 'E',
     priority: 100,
+    require: '^select',
     compile: function(element, attr) {
       if (isUndefined(attr.value)) {
         var interpolateFn = $interpolate(element.text(), true);
-        if (interpolateFn) {
+        if (!interpolateFn) {
-          return function (scope, element, attr) {
-            scope.$watch(interpolateFn, function(value) {
-              attr.$set('value', value);
-            });
-          }
-        } else {
           attr.$set('value', element.text());
         }
       }
+
+      // For some reason Opera defaults to true and if not overridden this messes up the repeater.
+      // We don't want the view to drive the initialization of the model anyway.
+      element.prop('selected', false);
+
+      return function (scope, element, attr, selectCtrl) {
+        if (interpolateFn) {
+          scope.$watch(interpolateFn, function(newVal, oldVal) {
+            attr.$set('value', newVal);
+            if (newVal !== oldVal) selectCtrl.removeOption(oldVal);
+            selectCtrl.addOption(newVal);
+          });
+        } else {
+          selectCtrl.addOption(attr.value);
+        }
+
+        element.bind('$destroy', function() {
+          selectCtrl.removeOption(attr.value);
+        });
+      };
     }
   }
 }];

test/ng/directive/selectSpec.js

@@ -10,14 +10,19 @@ describe('select', function() {
     scope.$apply();
   }
 
-
+  beforeEach(inject(function($rootScope, _$compile_) {
-  beforeEach(inject(function($injector, $rootScope) {
+    scope = $rootScope.$new(); //create a child scope because the root scope can't be $destroy-ed
-    scope = $rootScope;
+    $compile = _$compile_;
-    $compile = $injector.get('$compile');
     formElement = element = null;
   }));
 
 
+  afterEach(function() {
+    scope.$destroy(); //disables unknown option work during destruction
+    dealoc(formElement);
+  });
+
+
   beforeEach(function() {
     this.addMatchers({
       toEqualSelect: function(expected){
@@ -38,11 +43,6 @@ describe('select', function() {
   });
 
 
-  afterEach(function() {
-    dealoc(formElement);
-  });
-
-
   describe('select-one', function() {
 
     it('should compile children of a select without a ngModel, but not create a model for it',
@@ -108,6 +108,267 @@ describe('select', function() {
       expect(element).toBeValid();
       expect(element).toBePristine();
     });
+
+
+    it('should work with repeated value options', function() {
+      scope.robots = ['c3p0', 'r2d2'];
+      scope.robot = 'r2d2';
+      compile('<select ng-model="robot">' +
+                '<option ng-repeat="r in robots">{{r}}</option>' +
+              '</select>');
+      expect(element).toEqualSelect('c3p0', ['r2d2']);
+
+      browserTrigger(element.find('option').eq(0));
+      expect(element).toEqualSelect(['c3p0'], 'r2d2');
+      expect(scope.robot).toBe('c3p0');
+
+      scope.$apply(function() {
+        scope.robots.unshift('wallee');
+      });
+      expect(element).toEqualSelect('wallee', ['c3p0'], 'r2d2');
+      expect(scope.robot).toBe('c3p0');
+
+      scope.$apply(function() {
+        scope.robots = ['c3p0+', 'r2d2+'];
+        scope.robot = 'r2d2+';
+      });
+      expect(element).toEqualSelect('c3p0+', ['r2d2+']);
+      expect(scope.robot).toBe('r2d2+');
+    });
+
+
+    describe('empty option', function() {
+
+      it('should select the empty option when model is undefined', function() {
+        compile('<select ng-model="robot">' +
+                  '<option value="">--select--</option>' +
+                  '<option value="x">robot x</option>' +
+                  '<option value="y">robot y</option>' +
+                '</select>');
+
+        expect(element).toEqualSelect([''], 'x', 'y');
+      });
+
+
+      it('should support defining an empty option anywhere in the option list', function() {
+        compile('<select ng-model="robot">' +
+                  '<option value="x">robot x</option>' +
+                  '<option value="">--select--</option>' +
+                  '<option value="y">robot y</option>' +
+                '</select>');
+
+        expect(element).toEqualSelect('x', [''], 'y');
+      });
+
+
+      it('should set the model to empty string when empty option is selected', function() {
+        scope.robot = 'x';
+        compile('<select ng-model="robot">' +
+                  '<option value="">--select--</option>' +
+                  '<option value="x">robot x</option>' +
+                  '<option value="y">robot y</option>' +
+                '</select>');
+        expect(element).toEqualSelect('', ['x'], 'y');
+
+        browserTrigger(element.find('option').eq(0));
+        expect(element).toEqualSelect([''], 'x', 'y');
+        expect(scope.robot).toBe('');
+      });
+
+
+      describe('interactions with repeated options', function() {
+
+        it('should select empty option when model is undefined', function() {
+          scope.robots = ['c3p0', 'r2d2'];
+          compile('<select ng-model="robot">' +
+                    '<option value="">--select--</option>' +
+                    '<option ng-repeat="r in robots">{{r}}</option>' +
+                  '</select>');
+          expect(element).toEqualSelect([''], 'c3p0', 'r2d2');
+        });
+
+
+        it('should set model to empty string when selected', function() {
+          scope.robots = ['c3p0', 'r2d2'];
+          compile('<select ng-model="robot">' +
+                    '<option value="">--select--</option>' +
+                    '<option ng-repeat="r in robots">{{r}}</option>' +
+                  '</select>');
+
+          browserTrigger(element.find('option').eq(1));
+          expect(element).toEqualSelect('', ['c3p0'], 'r2d2');
+          expect(scope.robot).toBe('c3p0');
+
+          browserTrigger(element.find('option').eq(0));
+          expect(element).toEqualSelect([''], 'c3p0', 'r2d2');
+          expect(scope.robot).toBe('');
+        });
+
+
+        it('should not break if both the select and repeater models change at once', function() {
+          scope.robots = ['c3p0', 'r2d2'];
+          scope.robot = 'c3p0'
+          compile('<select ng-model="robot">' +
+                    '<option value="">--select--</option>' +
+                    '<option ng-repeat="r in robots">{{r}}</option>' +
+                  '</select>');
+          expect(element).toEqualSelect('', ['c3p0'], 'r2d2');
+
+          scope.$apply(function() {
+            scope.robots = ['wallee'];
+            scope.robot = '';
+          });
+
+          expect(element).toEqualSelect([''], 'wallee');
+        });
+      });
+    });
+
+
+    describe('unknown option', function() {
+
+      it("should insert&select temporary unknown option when no options-model match", function() {
+        compile('<select ng-model="robot">' +
+                  '<option>c3p0</option>' +
+                  '<option>r2d2</option>' +
+                '</select>');
+
+        expect(element).toEqualSelect(['? undefined:undefined ?'], 'c3p0', 'r2d2');
+
+        scope.$apply(function() {
+          scope.robot = 'r2d2';
+        });
+        expect(element).toEqualSelect('c3p0', ['r2d2']);
+
+
+        scope.$apply(function() {
+          scope.robot = "wallee";
+        });
+        expect(element).toEqualSelect(['? string:wallee ?'], 'c3p0', 'r2d2');
+      });
+
+
+      it("should NOT insert temporary unknown option when model is undefined and empty options " +
+          "is present", function() {
+        compile('<select ng-model="robot">' +
+                  '<option value="">--select--</option>' +
+                  '<option>c3p0</option>' +
+                  '<option>r2d2</option>' +
+                '</select>');
+
+        expect(element).toEqualSelect([''], 'c3p0', 'r2d2');
+        expect(scope.robot).toBeUndefined();
+
+        scope.$apply(function() {
+          scope.robot = null;
+        });
+        expect(element).toEqualSelect(['? object:null ?'], '', 'c3p0', 'r2d2');
+
+        scope.$apply(function() {
+          scope.robot = 'r2d2';
+        });
+        expect(element).toEqualSelect('', 'c3p0', ['r2d2']);
+
+        scope.$apply(function() {
+          delete scope.robot;
+        });
+        expect(element).toEqualSelect([''], 'c3p0', 'r2d2');
+      });
+
+
+      it("should insert&select temporary unknown option when no options-model match, empty " +
+          "option is present and model is defined", function() {
+        scope.robot = 'wallee';
+        compile('<select ng-model="robot">' +
+                  '<option value="">--select--</option>' +
+                  '<option>c3p0</option>' +
+                  '<option>r2d2</option>' +
+                '</select>');
+
+        expect(element).toEqualSelect(['? string:wallee ?'], '', 'c3p0', 'r2d2');
+
+        scope.$apply(function() {
+          scope.robot = 'r2d2';
+        });
+        expect(element).toEqualSelect('', 'c3p0', ['r2d2']);
+      });
+
+
+      describe('interactions with repeated options', function() {
+
+        it('should work with repeated options', function() {
+          compile('<select ng-model="robot">' +
+                    '<option ng-repeat="r in robots">{{r}}</option>' +
+                  '</select>');
+          expect(element).toEqualSelect(['? undefined:undefined ?']);
+          expect(scope.robot).toBeUndefined();
+
+          scope.$apply(function() {
+            scope.robot = 'r2d2';
+          });
+          expect(element).toEqualSelect(['? string:r2d2 ?']);
+          expect(scope.robot).toBe('r2d2');
+
+          scope.$apply(function() {
+            scope.robots = ['c3p0', 'r2d2'];
+          });
+          expect(element).toEqualSelect('c3p0', ['r2d2']);
+          expect(scope.robot).toBe('r2d2');
+        });
+
+
+        it('should work with empty option and repeated options', function() {
+          compile('<select ng-model="robot">' +
+                    '<option value="">--select--</option>' +
+                    '<option ng-repeat="r in robots">{{r}}</option>' +
+                  '</select>');
+          expect(element).toEqualSelect(['']);
+          expect(scope.robot).toBeUndefined();
+
+          scope.$apply(function() {
+            scope.robot = 'r2d2';
+          });
+          expect(element).toEqualSelect(['? string:r2d2 ?'], '');
+          expect(scope.robot).toBe('r2d2');
+
+          scope.$apply(function() {
+            scope.robots = ['c3p0', 'r2d2'];
+          });
+          expect(element).toEqualSelect('', 'c3p0', ['r2d2']);
+          expect(scope.robot).toBe('r2d2');
+        });
+
+
+        it('should insert unknown element when repeater shrinks and selected option is unavailable',
+            function() {
+          scope.robots = ['c3p0', 'r2d2'];
+          scope.robot = 'r2d2';
+          compile('<select ng-model="robot">' +
+                    '<option ng-repeat="r in robots">{{r}}</option>' +
+                  '</select>');
+          expect(element).toEqualSelect('c3p0', ['r2d2']);
+          expect(scope.robot).toBe('r2d2');
+
+          scope.$apply(function() {
+            scope.robots.pop();
+          });
+          expect(element).toEqualSelect(['? string:r2d2 ?'], 'c3p0');
+          expect(scope.robot).toBe('r2d2');
+
+          scope.$apply(function() {
+            scope.robots.unshift('r2d2');
+          });
+          expect(element).toEqualSelect(['r2d2'], 'c3p0');
+          expect(scope.robot).toBe('r2d2');
+
+          scope.$apply(function() {
+            delete scope.robots;
+          });
+          expect(element).toEqualSelect(['? string:r2d2 ?']);
+          expect(scope.robot).toBe('r2d2');
+        });
+      });
+    });
   });
 
 
@@ -840,24 +1101,18 @@ describe('select', function() {
 
     it('should populate value attribute on OPTION', function() {
       compile('<select ng-model="x"><option selected>abc</option></select>');
-      expect(element).toEqualSelect('abc');
+      expect(element).toEqualSelect(['? undefined:undefined ?'], 'abc');
     });
 
     it('should ignore value if already exists', function() {
       compile('<select ng-model="x"><option value="abc">xyz</option></select>');
-      expect(element).toEqualSelect('abc');
+      expect(element).toEqualSelect(['? undefined:undefined ?'], 'abc');
-    });
-
-    it('should set value even if newlines present', function() {
-      compile('<select ng-model="x"><option attr="\ntext\n" \n>\nabc\n</option></select>');
-      expect(element).toEqualSelect('\nabc\n');
     });
 
     it('should set value even if self closing HTML', function() {
-      // IE removes the \n from option, which makes this test pointless
+      scope.x = 'hello'
-      if (msie) return;
+      compile('<select ng-model="x"><option>hello</select>');
-      compile('<select ng-model="x"><option>\n</option></select>');
+      expect(element).toEqualSelect(['hello']);
-      expect(element).toEqualSelect('\n');
     });
   });
 });