From 210c18418469e9e74a16f1627a265d4f3d6b47cc Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Wed, 28 Jan 2015 13:36:20 +0000 Subject: [PATCH 1/2] style(ngOptionsSpec): ensure two newlines between specs --- test/ng/directive/ngOptionsSpec.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/ng/directive/ngOptionsSpec.js b/test/ng/directive/ngOptionsSpec.js index 62b3be328734..38ac81275138 100644 --- a/test/ng/directive/ngOptionsSpec.js +++ b/test/ng/directive/ngOptionsSpec.js @@ -362,6 +362,7 @@ describe('ngOptions', function() { expect(options.eq(2)).toEqualOption(scope.values[2], 'D'); }); + it('should preserve pre-existing empty option', function() { createSingleSelect(true); @@ -855,6 +856,7 @@ describe('ngOptions', function() { expect(options.eq(2)).toEqualTrackedOption(20, 'twenty'); }); + it('should preserve value even when reference has changed (single&array)', function() { createSelect({ 'ng-model': 'selected', From 6a03ca274314352052c3082163367a146bb11c2d Mon Sep 17 00:00:00 2001 From: Peter Bacon Darwin Date: Wed, 28 Jan 2015 22:01:52 +0000 Subject: [PATCH 2/2] fix(ngOptions): fix model<->option interaction when using track by This problem is beset by the problem of `ngModel` expecting models to be atomic things (primitives/objects). > When it was first invented it was expected that ngModel would only be a primitive, e.g. a string or a number. Later when things like ngList and ngOptions were added or became more complex then various hacks were put in place to make it look like it worked well with those but it doesn't. ------------- Just to be clear what is happening, lets name the objects: ```js var option1 = { uid: 1, name: 'someName1' }; var option2 = { uid: 2, name: 'someName2' }; var option3 = { uid: 3, name: 'someName3' }; var initialItem = { uid: 1, name: 'someName1' }; model { options: [option1, option2, option3], selected: initialItem }; ``` Now when we begin we have: ```js expect(model.selected).toBe(initialItem); expect(model.selected.uid).toEqual(option1.uid); expect(model.selected).not.toBe(option1); ``` So although `ngOptions` has found a match between an option and the modelValue, these are not the same object. Now if we change the properties of the `model.selected` object, we are effectively changing the `initialItem` object. ```js model.selected.uid = 3; model.selected.name = 'someName3'; expect(model.selected).toBe(initialItem); expect(model.selected.uid).toEqual(option3.uid); expect(model.selected).not.toBe(option3); ``` At the moment `ngModel` only watches for changes to the object identity and so it doesn't trigger an update to the `ngOptions` directive. This commit fixes this in `ngOptions` by adding a **deep** watch on the `attr.ngModel` expression... ```js scope.$watch(attr.ngModel, updateOptions, true); ``` You can see that in this Plunker: http://plnkr.co/edit/0PE7qN5FXIA23y4RwyN0?p=preview ------- But this isn't the end of the story. Since `ngModel` and `ngOptions` did not make copies between the model and the view, we can't go around just changing the properties of the `model.selected` object. This is particularly important in the situation where the user has actually chosen an option, since the `model.selected` points directly to one of the option objects: ```js // User selects "someName2" option expect(model.selected).toBe(option2); expect(model.selected.uid).toEqual(option2.uid); expect(model.selected).not.toBe(initialOption); ``` If we now change the `model.selected` object's properties we are actually changing the `option2` object: ```js expect(model.selected).toBe(option2); model.selected.uid = 3; model.selected.name = 'someName3'; expect(model.selected).toBe(option2); expect(model.selected).not.toBe(option3); expect(option2.uid).toEqual(3); expect(option2.name).toEqual('someName3'); ``` which means that the options are now broken: ```js expect(model.options).toEqual([ { uid: 1, name: 'someName1' }, { uid: 3, name: 'someName3' }, { uid: 3, name: 'someName3' } ]); ``` This commit fixes this in `ngOptions` by making copies when reading the value if `track by` is being used. If we are not using `track by` then we really do care about the identity of the object and should not be copying... You can see this in the Plunker here: http://plnkr.co/edit/YEzEf4dxHTnoW5pbeJDp?p=preview Closes #10869 Closes #10893 --- src/ng/directive/ngOptions.js | 13 ++++- test/ng/directive/ngOptionsSpec.js | 90 ++++++++++++++++++++++++++++-- 2 files changed, 96 insertions(+), 7 deletions(-) diff --git a/src/ng/directive/ngOptions.js b/src/ng/directive/ngOptions.js index 85a725069ee0..476da491d755 100644 --- a/src/ng/directive/ngOptions.js +++ b/src/ng/directive/ngOptions.js @@ -335,6 +335,11 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) { selectValueMap: selectValueMap, getOptionFromViewValue: function(value) { return selectValueMap[getTrackByValue(value, getLocals(value))]; + }, + getViewValueFromOption: function(option) { + // If the viewValue could be an object that may be mutated by the application, + // we need to make a copy and not return the reference to the value on the option. + return trackBy ? angular.copy(option.viewValue) : option.viewValue; } }; } @@ -428,7 +433,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) { if (selectedOption && !selectedOption.disabled) { removeEmptyOption(); removeUnknownOption(); - return selectedOption.viewValue; + return options.getViewValueFromOption(selectedOption); } return null; }; @@ -462,7 +467,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) { forEach(selectedValues, function(value) { var option = options.selectValueMap[value]; - if (!option.disabled) selections.push(option.viewValue); + if (!option.disabled) selections.push(options.getViewValueFromOption(option)); }); return selections; @@ -493,6 +498,10 @@ 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 + scope.$watch(attr.ngModel, function() { ngModelCtrl.$render(); }, true); + // ------------------------------------------------------------------ // diff --git a/test/ng/directive/ngOptionsSpec.js b/test/ng/directive/ngOptionsSpec.js index 38ac81275138..ceb3e312f494 100644 --- a/test/ng/directive/ngOptionsSpec.js +++ b/test/ng/directive/ngOptionsSpec.js @@ -856,7 +856,87 @@ describe('ngOptions', function() { expect(options.eq(2)).toEqualTrackedOption(20, 'twenty'); }); - + + it('should update the selected option even if only the tracked property on the selected object changes (single)', function() { + createSelect({ + 'ng-model': 'selected', + 'ng-options': 'item.label for item in arr track by item.id' + }); + + scope.$apply(function() { + scope.selected = {id: 10, label: 'ten'}; + }); + + expect(element.val()).toEqual('10'); + + // Update the properties on the selected object, rather than replacing the whole object + scope.$apply(function() { + scope.selected.id = 20; + scope.selected.label = 'new twenty'; + }); + + // The value of the select should change since the id property changed + expect(element.val()).toEqual('20'); + + // But the label of the selected option does not change + var option = element.find('option').eq(1); + expect(option.prop('selected')).toEqual(true); + expect(option.text()).toEqual('twenty'); // not 'new twenty' + }); + + + it('should update the selected options even if only the tracked properties on the objects in the ' + + 'selected collection change (multi)', function() { + createSelect({ + 'ng-model': 'selected', + 'multiple': true, + 'ng-options': 'item.label for item in arr track by item.id' + }); + + scope.$apply(function() { + scope.selected = [{id: 10, label: 'ten'}]; + }); + + expect(element.val()).toEqual(['10']); + + // Update the properties on the object in the selected array, rather than replacing the whole object + scope.$apply(function() { + scope.selected[0].id = 20; + scope.selected[0].label = 'new twenty'; + }); + + // The value of the select should change since the id property changed + expect(element.val()).toEqual(['20']); + + // But the label of the selected option does not change + var option = element.find('option').eq(1); + expect(option.prop('selected')).toEqual(true); + expect(option.text()).toEqual('twenty'); // not 'new twenty' + }); + + + it('should prevent changes to the selected object from modifying the options objects (single)', function() { + + createSelect({ + 'ng-model': 'selected', + 'ng-options': 'item.label for item in arr track by item.id' + }); + + element.val('10'); + browserTrigger(element, 'change'); + + expect(scope.selected).toEqual(scope.arr[0]); + + scope.$apply(function() { + scope.selected.id = 20; + }); + + expect(scope.selected).not.toEqual(scope.arr[0]); + expect(element.val()).toEqual('20'); + expect(scope.arr).toEqual([{id: 10, label: 'ten'}, {id:20, label: 'twenty'}]); + }); + + it('should preserve value even when reference has changed (single&array)', function() { createSelect({ 'ng-model': 'selected', @@ -919,7 +999,7 @@ describe('ngOptions', function() { expect(element.val()).toBe('10'); setSelectValue(element, 1); - expect(scope.selected).toBe(scope.obj['2']); + expect(scope.selected).toEqual(scope.obj['2']); }); @@ -998,7 +1078,7 @@ describe('ngOptions', function() { element.val('10'); browserTrigger(element, 'change'); - expect(scope.selected).toBe(scope.arr[0].subItem); + expect(scope.selected).toEqual(scope.arr[0].subItem); // Now reload the array scope.$apply(function() { @@ -1577,7 +1657,7 @@ describe('ngOptions', function() { scope.values.pop(); }); - expect(element.val()).toEqualUnknownValue(); + expect(element.val()).toEqual(''); expect(scope.selected).toEqual(null); // Check after model change @@ -1591,7 +1671,7 @@ describe('ngOptions', function() { scope.values.pop(); }); - expect(element.val()).toEqualUnknownValue(); + expect(element.val()).toEqual(''); expect(scope.selected).toEqual(null); });