fix(select): manage select controller options correctly

This fixes a regression that was introduced in 2bcd02d. Basically, the problem was that render() removed the wrong option from the select controller since it assumed that the option that was removed has the same label as the excessive option in existingOptions, but this is only correct if the option was popped from the end of the array. We now remember for each label whether it was added or removed (or removed at some point and then added at a different point) and report to the select controller only about options that were actually removed or added, ignoring any options that just moved.

Closes #9418

Author: shahata

src/ng/directive/select.js

@@ -200,7 +200,7 @@ var selectDirective = ['$compile', '$parse', function($compile,   $parse) {
         // Workaround for https://code.google.com/p/chromium/issues/detail?id=381459
         // Adding an <option selected="selected"> element to a <select required="required"> should
         // automatically select the new element
-        if (element[0].hasAttribute('selected')) {
+        if (element && element[0].hasAttribute('selected')) {
           element[0].selected = true;
         }
       };
@@ -498,6 +498,23 @@ var selectDirective = ['$compile', '$parse', function($compile,   $parse) {
           }
         }
 
+        /**
+         * A new labelMap is created with each render.
+         * This function is called for each existing option with added=false,
+         * and each new option with added=true.
+         * - Labels that are passed to this method twice,
+         * (once with added=true and once with added=false) will end up with a value of 0, and
+         * will cause no change to happen to the corresponding option.
+         * - Labels that are passed to this method only once with added=false will end up with a
+         * value of -1 and will eventually be passed to selectCtrl.removeOption()
+         * - Labels that are passed to this method only once with added=true will end up with a
+         * value of 1 and will eventually be passed to selectCtrl.addOption()
+        */
+        function updateLabelMap(labelMap, label, added) {
+          labelMap[label] = labelMap[label] || 0;
+          labelMap[label] += (added ? 1 : -1);
+        }
+
         function render() {
           renderScheduled = false;
 
@@ -515,6 +532,7 @@ var selectDirective = ['$compile', '$parse', function($compile,   $parse) {
               value,
               groupLength, length,
               groupIndex, index,
+              labelMap = {},
               selected,
               isSelected = createIsSelectedFn(viewValue),
               anySelected = false,
@@ -597,6 +615,8 @@ var selectDirective = ['$compile', '$parse', function($compile,   $parse) {
                 // reuse elements
                 lastElement = existingOption.element;
                 if (existingOption.label !== option.label) {
+                  updateLabelMap(labelMap, existingOption.label, false);
+                  updateLabelMap(labelMap, option.label, true);
                   lastElement.text(existingOption.label = option.label);
                 }
                 if (existingOption.id !== option.id) {
@@ -636,7 +656,7 @@ var selectDirective = ['$compile', '$parse', function($compile,   $parse) {
                     id: option.id,
                     selected: option.selected
                 });
-                selectCtrl.addOption(option.label, element);
+                updateLabelMap(labelMap, option.label, true);
                 if (lastElement) {
                   lastElement.after(element);
                 } else {
@@ -649,9 +669,16 @@ var selectDirective = ['$compile', '$parse', function($compile,   $parse) {
             index++; // increment since the existingOptions[0] is parent element not OPTION
             while(existingOptions.length > index) {
               option = existingOptions.pop();
-              selectCtrl.removeOption(option.label);
+              updateLabelMap(labelMap, option.label, false);
               option.element.remove();
             }
+            forEach(labelMap, function (count, label) {
+              if (count > 0) {
+                selectCtrl.addOption(label);
+              } else if (count < 0) {
+                selectCtrl.removeOption(label);
+              }
+            });
           }
           // remove any excessive OPTGROUPs from select
           while(optionGroupsCache.length > groupIndex) {

test/ng/directive/selectSpec.js

@@ -407,31 +407,59 @@ describe('select', function() {
         });
 
         describe('selectController.hasOption', function() {
-          it('should return true for options added via ngOptions', function() {
+          it('should return false for options shifted via ngOptions', function() {
             scope.robots = [
-              {key: 1, value: 'c3p0'},
-              {key: 2, value: 'r2d2'}
+              {value: 1, label: 'c3p0'},
+              {value: 2, label: 'r2d2'}
             ];
-            scope.robot = 'r2d2';
 
             compile('<select ng-model="robot" ' +
-                      'ng-options="item.key as item.value for item in robots">' +
+                      'ng-options="item.value as item.label for item in robots">' +
                     '</select>');
 
             var selectCtrl = element.data().$selectController;
 
-            expect(selectCtrl.hasOption('c3p0')).toBe(true);
+            scope.$apply(function() {
+              scope.robots.shift();
+            });
+
+            expect(selectCtrl.hasOption('c3p0')).toBe(false);
             expect(selectCtrl.hasOption('r2d2')).toBe(true);
+          });
+
+          it('should return false for options popped via ngOptions', function() {
+            scope.robots = [
+              {value: 1, label: 'c3p0'},
+              {value: 2, label: 'r2d2'}
+            ];
+
+            compile('<select ng-model="robot" ' +
+                      'ng-options="item.value as item.label for item in robots">' +
+                    '</select>');
+
+            var selectCtrl = element.data().$selectController;
 
             scope.$apply(function() {
               scope.robots.pop();
             });
 
             expect(selectCtrl.hasOption('c3p0')).toBe(true);
             expect(selectCtrl.hasOption('r2d2')).toBe(false);
+          });
+
+          it('should return true for options added via ngOptions', function() {
+            scope.robots = [
+              {value: 2, label: 'r2d2'}
+            ];
+
+            compile('<select ng-model="robot" ' +
+                      'ng-options="item.value as item.label for item in robots">' +
+                    '</select>');
+
+            var selectCtrl = element.data().$selectController;
 
             scope.$apply(function() {
-              scope.robots.push({key: 2, value: 'r2d2'});
+              scope.robots.unshift({value: 1, label: 'c3p0'});
             });
 
             expect(selectCtrl.hasOption('c3p0')).toBe(true);
@@ -516,31 +544,59 @@ describe('select', function() {
     });
 
     describe('selectController.hasOption', function() {
-      it('should return true for options added via ngOptions', function() {
+      it('should return false for options shifted via ngOptions', function() {
         scope.robots = [
-          {key: 1, value: 'c3p0'},
-          {key: 2, value: 'r2d2'}
+          {value: 1, label: 'c3p0'},
+          {value: 2, label: 'r2d2'}
         ];
-        scope.robot = 'r2d2';
 
         compile('<select ng-model="robot" multiple ' +
-                  'ng-options="item.key as item.value for item in robots">' +
+                  'ng-options="item.value as item.label for item in robots">' +
                 '</select>');
 
         var selectCtrl = element.data().$selectController;
 
-        expect(selectCtrl.hasOption('c3p0')).toBe(true);
+        scope.$apply(function() {
+          scope.robots.shift();
+        });
+
+        expect(selectCtrl.hasOption('c3p0')).toBe(false);
         expect(selectCtrl.hasOption('r2d2')).toBe(true);
+      });
+
+      it('should return false for options popped via ngOptions', function() {
+        scope.robots = [
+          {value: 1, label: 'c3p0'},
+          {value: 2, label: 'r2d2'}
+        ];
+
+        compile('<select ng-model="robot" multiple ' +
+                  'ng-options="item.value as item.label for item in robots">' +
+                '</select>');
+
+        var selectCtrl = element.data().$selectController;
 
         scope.$apply(function() {
           scope.robots.pop();
         });
 
         expect(selectCtrl.hasOption('c3p0')).toBe(true);
         expect(selectCtrl.hasOption('r2d2')).toBe(false);
+      });
+
+      it('should return true for options added via ngOptions', function() {
+        scope.robots = [
+          {value: 2, label: 'r2d2'}
+        ];
+
+        compile('<select ng-model="robot" multiple ' +
+                  'ng-options="item.value as item.label for item in robots">' +
+                '</select>');
+
+        var selectCtrl = element.data().$selectController;
 
         scope.$apply(function() {
-          scope.robots.push({key: 2, value: 'r2d2'});
+          scope.robots.unshift({value: 1, label: 'c3p0'});
         });
 
         expect(selectCtrl.hasOption('c3p0')).toBe(true);