fix(ngOptions): ensure that tracked properties are always watched

Commit 47f9fc3 failed to account for changes to
the tracked value of model items in a collection where the select was `multiple`.

See angular#11743 (comment)

Author: petebacondarwin

src/ng/directive/ngOptions.js

@@ -261,9 +261,13 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
     // Get the value by which we are going to track the option
     // if we have a trackFn then use that (passing scope and locals)
     // otherwise just hash the given viewValue
-    var getTrackByValue = trackBy ?
-                              function(viewValue, locals) { return trackByFn(scope, locals); } :
-                              function getHashOfValue(viewValue) { return hashKey(viewValue); };
+    var getTrackByValueFn = trackBy ?
+                              function(value, locals) { return trackByFn(scope, locals); } :
+                              function getHashOfValue(value) { return hashKey(value); };
+    var getTrackByValue = function(value, key) {
+      return getTrackByValueFn(value, getLocals(value, key));
+    };
+
     var displayFn = $parse(match[2] || match[1]);
     var groupByFn = $parse(match[3] || '');
     var disableWhenFn = $parse(match[4] || '');
@@ -290,6 +294,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
 
     return {
       trackBy: trackBy,
+      getTrackByValue: getTrackByValue,
       getWatchables: $parse(valuesFn, function(values) {
         // Create a collection of things that we would like to watch (watchedArray)
         // so that they can all be watched using a single $watchCollection
@@ -299,7 +304,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
 
         Object.keys(values).forEach(function getWatchable(key) {
           var locals = getLocals(values[key], key);
-          var selectValue = getTrackByValue(values[key], locals);
+          var selectValue = getTrackByValueFn(values[key], locals);
           watchedArray.push(selectValue);
 
           // Only need to watch the displayFn if there is a specific label expression
@@ -347,7 +352,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
           var value = optionValues[key];
           var locals = getLocals(value, key);
           var viewValue = viewValueFn(scope, locals);
-          var selectValue = getTrackByValue(viewValue, locals);
+          var selectValue = getTrackByValueFn(viewValue, locals);
           var label = displayFn(scope, locals);
           var group = groupByFn(scope, locals);
           var disabled = disableWhenFn(scope, locals);
@@ -361,7 +366,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
           items: optionItems,
           selectValueMap: selectValueMap,
           getOptionFromViewValue: function(value) {
-            return selectValueMap[getTrackByValue(value, getLocals(value))];
+            return selectValueMap[getTrackByValue(value)];
           },
           getViewValueFromOption: function(option) {
             // If the viewValue could be an object that may be mutated by the application,
@@ -439,44 +444,54 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
       };
 
 
-      selectCtrl.writeValue = function writeNgOptionsValue(value) {
-        var option = options.getOptionFromViewValue(value);
+      // Update the controller methods for multiple selectable options
+      if (!multiple) {
 
-        if (option && !option.disabled) {
-          if (selectElement[0].value !== option.selectValue) {
-            removeUnknownOption();
-            removeEmptyOption();
+        selectCtrl.writeValue = function writeNgOptionsValue(value) {
+          var option = options.getOptionFromViewValue(value);
 
-            selectElement[0].value = option.selectValue;
-            option.element.selected = true;
-            option.element.setAttribute('selected', 'selected');
-          }
-        } else {
-          if (value === null || providedEmptyOption) {
-            removeUnknownOption();
-            renderEmptyOption();
+          if (option && !option.disabled) {
+            if (selectElement[0].value !== option.selectValue) {
+              removeUnknownOption();
+              removeEmptyOption();
+
+              selectElement[0].value = option.selectValue;
+              option.element.selected = true;
+              option.element.setAttribute('selected', 'selected');
+            }
           } else {
-            removeEmptyOption();
-            renderUnknownOption();
+            if (value === null || providedEmptyOption) {
+              removeUnknownOption();
+              renderEmptyOption();
+            } else {
+              removeEmptyOption();
+              renderUnknownOption();
+            }
           }
-        }
-      };
+        };
 
-      selectCtrl.readValue = function readNgOptionsValue() {
+        selectCtrl.readValue = function readNgOptionsValue() {
 
-        var selectedOption = options.selectValueMap[selectElement.val()];
+          var selectedOption = options.selectValueMap[selectElement.val()];
 
-        if (selectedOption && !selectedOption.disabled) {
-          removeEmptyOption();
-          removeUnknownOption();
-          return options.getViewValueFromOption(selectedOption);
-        }
-        return null;
-      };
+          if (selectedOption && !selectedOption.disabled) {
+            removeEmptyOption();
+            removeUnknownOption();
+            return options.getViewValueFromOption(selectedOption);
+          }
+          return null;
+        };
 
+        // If we are using `track by` then we must watch the tracked value on the model
+        // since ngModel only watches for object identity change
+        if (ngOptions.trackBy) {
+          scope.$watch(
+            function() { return ngOptions.getTrackByValue(ngModelCtrl.$viewValue); },
+            function() { ngModelCtrl.$render(); }
+          );
+        }
 
-      // Update the controller methods for multiple selectable options
-      if (multiple) {
+      } else {
 
         ngModelCtrl.$isEmpty = function(value) {
           return !value || value.length === 0;
@@ -508,6 +523,22 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
 
           return selections;
         };
+
+        // If we are using `track by` then we must watch these tracked values on the model
+        // since ngModel only watches for object identity change
+        if (ngOptions.trackBy) {
+
+          scope.$watchCollection(function() {
+            if (isArray(ngModelCtrl.$viewValue)) {
+              return ngModelCtrl.$viewValue.map(function(value) {
+                return ngOptions.getTrackByValue(value);
+              });
+            }
+          }, function() {
+            ngModelCtrl.$render();
+          });
+
+        }
       }
 
 
@@ -534,11 +565,6 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
       // We will re-render the option elements if the option values or labels change
       scope.$watchCollection(ngOptions.getWatchables, updateOptions);
 
-      // We also need to watch to see if the internals of the model changes, since
-      // ngModel only watches for object identity change
-      if (ngOptions.trackBy) {
-        scope.$watchCollection(attr.ngModel, function() { ngModelCtrl.$render(); });
-      }
       // ------------------------------------------------------------------ //
 
 

test/ng/directive/ngOptionsSpec.js

@@ -988,9 +988,9 @@ describe('ngOptions', function() {
 
       expect(element.val()).toEqual(['10']);
 
-      // Update the properties on the object in the selected array, rather than replacing the whole object
+      // Update the tracked property on the object in the selected array, rather than replacing the whole object
       scope.$apply(function() {
-        scope.selected[0] = {id: 20, label: 'new twenty'};
+        scope.selected[0].id = 20;
       });
 
       // The value of the select should change since the id property changed
@@ -1130,21 +1130,21 @@ describe('ngOptions', function() {
       }).not.toThrow();
     });
 
-    it('should re-render if a propery of the model is changed when using trackBy', function() {
+    it('should re-render if the tracked property of the model is changed when using trackBy', function() {
 
       createSelect({
         'ng-model': 'selected',
         'ng-options': 'item for item in arr track by item.id'
       });
 
       scope.$apply(function() {
-        scope.selected = scope.arr[0];
+        scope.selected = {id: 10, label: 'ten'};
       });
 
       spyOn(element.controller('ngModel'), '$render');
 
       scope.$apply(function() {
-        scope.selected.label = 'changed';
+        scope.arr[0].id = 20;
       });
 
       // update render due to equality watch