From 9f89b2f0ec21061f954cfbf0ed44111b42e17209 Mon Sep 17 00:00:00 2001
From: Shahar Talmi <shahar.talmi@gmail.com>
Date: Sat, 4 Oct 2014 02:13:52 +0300
Subject: [PATCH] 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
---
 src/ng/directive/select.js      | 21 +++++++--
 test/ng/directive/selectSpec.js | 84 +++++++++++++++++++++++++++------
 2 files changed, 88 insertions(+), 17 deletions(-)

diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js
index fe17c903c522..d671cb1cbf45 100644
--- a/src/ng/directive/select.js
+++ b/src/ng/directive/select.js
@@ -203,7 +203,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;
         }
       };
@@ -501,6 +501,11 @@ var selectDirective = ['$compile', '$parse', function($compile,   $parse) {
           }
         }
 
+        function updateLabelMap(labelMap, label, added) {
+          labelMap[label] = labelMap[label] || 0;
+          labelMap[label] += (added ? 1 : -1);
+        }
+
         function render() {
           renderScheduled = false;
 
@@ -518,6 +523,7 @@ var selectDirective = ['$compile', '$parse', function($compile,   $parse) {
               value,
               groupLength, length,
               groupIndex, index,
+              labelMap = {},
               selected,
               isSelected = createIsSelectedFn(viewValue),
               anySelected = false,
@@ -600,6 +606,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) {
@@ -639,7 +647,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 {
@@ -652,9 +660,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) {
diff --git a/test/ng/directive/selectSpec.js b/test/ng/directive/selectSpec.js
index 4685cc9f6b64..f04e526e3416 100644
--- a/test/ng/directive/selectSpec.js
+++ b/test/ng/directive/selectSpec.js
@@ -407,21 +407,37 @@ 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();
@@ -429,9 +445,21 @@ describe('select', function() {
 
             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,21 +544,37 @@ 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();
@@ -538,9 +582,21 @@ describe('select', function() {
 
         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);