From 67115f39eec4261bfe7768e44effeee162d15880 Mon Sep 17 00:00:00 2001 From: Jeff Cross Date: Fri, 3 Oct 2014 13:12:54 -0700 Subject: [PATCH 1/3] fix(select): make ngOptions support selectAs and trackBy together This commit implements two functions, "isSelected()" and "getViewValue()" to properly compute an option's selected state and the model controller's viewValue respectively. These functions give proper precedence to "track by" and "select as" parts of the ngOptions comprehension expression, which were previously inconsistent and incompatible. Fixes #6564 --- src/ng/directive/select.js | 144 ++++++++----- test/ng/directive/selectSpec.js | 358 +++++++++++++++++++++++++++++++- 2 files changed, 447 insertions(+), 55 deletions(-) diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js index 46d38717c37b..a79fbf0cc43a 100644 --- a/src/ng/directive/select.js +++ b/src/ng/directive/select.js @@ -35,6 +35,12 @@ var ngOptionsMinErr = minErr('ngOptions'); * be bound to string values at present. * * + *
+ * **Note:** Using `selectAs` will bind the result of the `selectAs` expression to the model, but + * the value of the `select` and `option` elements will be either the index (for array data sources) + * or property name (for object data sources) of the value within the collection. + *
+ * * @param {string} ngModel Assignable angular expression to data-bind to. * @param {string=} name Property name of the form under which the control is published. * @param {string=} required The control is considered valid only if value is entered. @@ -69,7 +75,8 @@ var ngOptionsMinErr = minErr('ngOptions'); * DOM element. * * `trackexpr`: Used when working with an array of objects. The result of this expression will be * used to identify the objects in the array. The `trackexpr` will most likely refer to the - * `value` variable (e.g. `value.propertyName`). + * `value` variable (e.g. `value.propertyName`). With this the selection is preserved + * even when the options are recreated (e.g. reloaded from the server). * * @example @@ -326,6 +333,8 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { var displayFn = $parse(match[2] || match[1]), valueName = match[4] || match[6], + selectAs = / as /.test(match[0]) && match[1], + selectAsFn = selectAs ? $parse(selectAs) : null, keyName = match[5], groupByFn = $parse(match[3] || ''), valueFn = $parse(match[2] ? match[1] : valueName), @@ -371,41 +380,12 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { for(index = 1, length = optionGroup.length; index < length; index++) { if ((optionElement = optionGroup[index].element)[0].selected) { - key = optionElement.val(); - if (keyName) locals[keyName] = key; - if (trackFn) { - for (trackIndex = 0; trackIndex < collection.length; trackIndex++) { - locals[valueName] = collection[trackIndex]; - if (trackFn(scope, locals) == key) break; - } - } else { - locals[valueName] = collection[key]; - } - value.push(valueFn(scope, locals)); + value.push(getViewValue(optionElement, locals, collection)); } } } } else { - key = selectElement.val(); - if (key == '?') { - value = undefined; - } else if (key === ''){ - value = null; - } else { - if (trackFn) { - for (trackIndex = 0; trackIndex < collection.length; trackIndex++) { - locals[valueName] = collection[trackIndex]; - if (trackFn(scope, locals) == key) { - value = valueFn(scope, locals); - break; - } - } - } else { - locals[valueName] = collection[key]; - if (keyName) locals[keyName] = key; - value = valueFn(scope, locals); - } - } + value = getViewValue(selectElement, locals, collection); } ctrl.$setViewValue(value); render(); @@ -437,7 +417,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { var selectedSet = false; if (multiple) { var viewValue = ctrl.$viewValue; - if (trackFn && isArray(viewValue)) { + if (trackFn && isArray(viewValue) && !selectAs) { selectedSet = new HashMap([]); var locals = {}; for (var trackIndex = 0; trackIndex < viewValue.length; trackIndex++) { @@ -451,6 +431,79 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { return selectedSet; } + function getViewValue (el, locals, collection) { + var key = el.val(); + var value; + var calculateViewValue; + + if (selectAsFn || trackFn) { + calculateViewValue = function () { + var getterFn = selectAsFn || trackFn; + var wrappedCollectionValue = {}; + wrappedCollectionValue[valueName] = collection[key]; + wrappedCollectionValue[keyName] = key; + + for (var i in collection) { + if (collection.hasOwnProperty(i)) { + locals[valueName] = collection[i]; + if (keyName) locals[keyName] = i; + if (getterFn(scope, locals) == + getterFn(scope, wrappedCollectionValue)) { + /* + * trackBy should not be used for final calculation, because it doesn't + * necessarily return the expected value. + */ + return (selectAsFn||valueFn)(scope, locals); + } + } + } + }; + } else { + calculateViewValue = function() { + locals[valueName] = collection[key]; + if (keyName) locals[keyName] = key; + return valueFn(scope, locals); + }; + } + + if (multiple) { + if (keyName) locals[keyName] = key; + calculateViewValue(); + return (selectAsFn || valueFn)(scope, locals); + } + + if (key == '?') { + value = undefined; + } else if (key === ''){ + value = null; + } else { + value = calculateViewValue(); + } + + return value; + } + + function isSelected(viewValue, locals, selectedSet) { + var compareValueFn; + if (selectAsFn) { + compareValueFn = selectAsFn; + } else if (trackFn) { + var withValueName = {}; + withValueName[valueName] = viewValue; + compareValueFn = trackFn; + + viewValue = trackFn(withValueName); + } else { + compareValueFn = valueFn; + } + + var optionValue = compareValueFn(scope, locals); + + if (multiple) { + return isDefined(selectedSet.remove(optionValue)); + } + return viewValue === optionValue; + } function scheduleRendering() { if (!renderScheduled) { @@ -483,10 +536,8 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { element, label; - // We now build up the list of options we need (we merge later) for (index = 0; length = keys.length, index < length; index++) { - key = index; if (keyName) { key = keys[index]; @@ -501,27 +552,20 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { optionGroup = optionGroups[optionGroupName] = []; optionGroupNames.push(optionGroupName); } - if (multiple) { - selected = isDefined( - selectedSet.remove(trackFn ? trackFn(scope, locals) : valueFn(scope, locals)) - ); - } else { - if (trackFn) { - var modelCast = {}; - modelCast[valueName] = viewValue; - selected = trackFn(scope, modelCast) === trackFn(scope, locals); - } else { - selected = viewValue === valueFn(scope, locals); - } - selectedSet = selectedSet || selected; // see if at least one item is selected - } + + selected = isSelected( + viewValue, + locals, + selectedSet); + selectedSet = selectedSet || selected; + label = displayFn(scope, locals); // what will be seen by the user // doing displayFn(scope, locals) || '' overwrites zero values label = isDefined(label) ? label : ''; optionGroup.push({ // either the index into array or key from object - id: trackFn ? trackFn(scope, locals) : (keyName ? keys[index] : index), + id: (keyName ? keys[index] : index), label: label, selected: selected // determine if we should be selected }); diff --git a/test/ng/directive/selectSpec.js b/test/ng/directive/selectSpec.js index fb423d516622..a15d6dcb3fc2 100644 --- a/test/ng/directive/selectSpec.js +++ b/test/ng/directive/selectSpec.js @@ -583,6 +583,354 @@ describe('select', function() { }, blank, unknown); } + describe('selectAs expression', function() { + beforeEach(function() { + scope.arr = [{id: 10, label: 'ten'}, {id:20, label: 'twenty'}]; + scope.obj = {'10': {score: 10, label: 'ten'}, '20': {score: 20, label: 'twenty'}}; + }); + + it('should support single select with array source', function() { + createSelect({ + 'ng-model': 'selected', + 'ng-options': 'item.id as item.label for item in arr' + }); + + scope.$apply(function() { + scope.selected = 10; + }); + expect(element.val()).toBe('0'); + + element.val('1'); + browserTrigger(element, 'change'); + expect(scope.selected).toBe(20); + }); + + + it('should support multi select with array source', function() { + createSelect({ + 'ng-model': 'selected', + 'multiple': true, + 'ng-options': 'item.id as item.label for item in arr' + }); + + scope.$apply(function() { + scope.selected = [10,20]; + }); + expect(element.val()).toEqual(['0','1']); + expect(scope.selected).toEqual([10,20]); + + element.children()[0].selected = false; + browserTrigger(element, 'change'); + expect(scope.selected).toEqual([20]); + expect(element.val()).toEqual(['1']); + }); + + + it('should support single select with object source', function() { + createSelect({ + 'ng-model': 'selected', + 'ng-options': 'val.score as val.label for (key, val) in obj' + }); + + scope.$apply(function() { + scope.selected = 10; + }); + expect(element.val()).toBe('10'); + + element.val('20'); + browserTrigger(element, 'change'); + expect(scope.selected).toBe(20); + }); + + + it('should support multi select with object source', function() { + createSelect({ + 'ng-model': 'selected', + 'multiple': true, + 'ng-options': 'val.score as val.label for (key, val) in obj' + }); + + scope.$apply(function() { + scope.selected = [10,20]; + }); + expect(element.val()).toEqual(['10','20']); + + element.children()[0].selected = false; + browserTrigger(element, 'change'); + expect(scope.selected).toEqual([20]); + expect(element.val()).toEqual(['20']); + }); + }); + + + describe('trackBy', function() { + beforeEach(function() { + scope.arr = [{id: 10, label: 'ten'}, {id:'20', label: 'twenty'}]; + scope.obj = {'10': {score: 10, label: 'ten'}, '20': {score: 20, label: 'twenty'}}; + }); + + + it('should preserve value even when reference has changed (single&array)', function() { + createSelect({ + 'ng-model': 'selected', + 'ng-options': 'item.label for item in arr track by item.id' + }); + + scope.$apply(function() { + scope.selected = scope.arr[0]; + }); + expect(element.val()).toBe('0'); + + scope.$apply(function() { + scope.arr[0] = {id: 10, label: 'new ten'}; + }); + expect(element.val()).toBe('0'); + + element.children()[1].selected = 1; + browserTrigger(element, 'change'); + expect(scope.selected).toEqual(scope.arr[1]); + }); + + + it('should preserve value even when reference has changed (multi&array)', function() { + createSelect({ + 'ng-model': 'selected', + 'multiple': true, + 'ng-options': 'item.label for item in arr track by item.id' + }); + + scope.$apply(function() { + scope.selected = scope.arr; + }); + expect(element.val()).toEqual(['0','1']); + + scope.$apply(function() { + scope.arr[0] = {id: 10, label: 'new ten'}; + }); + expect(element.val()).toEqual(['0','1']); + + element.children()[0].selected = false; + browserTrigger(element, 'change'); + expect(scope.selected).toEqual([scope.arr[1]]); + }); + + + it('should preserve value even when reference has changed (single&object)', function() { + createSelect({ + 'ng-model': 'selected', + 'ng-options': 'val.label for (key, val) in obj track by val.score' + }); + + scope.$apply(function() { + scope.selected = scope.obj['10']; + }); + expect(element.val()).toBe('10'); + + scope.$apply(function() { + scope.obj['10'] = {score: 10, label: 'ten'}; + }); + expect(element.val()).toBe('10'); + + element.val('20'); + browserTrigger(element, 'change'); + expect(scope.selected).toBe(scope.obj[20]); + }); + + + it('should preserve value even when reference has changed (multi&object)', function() { + createSelect({ + 'ng-model': 'selected', + 'multiple': true, + 'ng-options': 'val.label for (key, val) in obj track by val.score' + }); + + scope.$apply(function() { + scope.selected = [scope.obj['10']]; + }); + expect(element.val()).toEqual(['10']); + + scope.$apply(function() { + scope.obj['10'] = {score: 10, label: 'ten'}; + }); + expect(element.val()).toEqual(['10']); + + element.children()[1].selected = 'selected'; + browserTrigger(element, 'change'); + expect(scope.selected).toEqual([scope.obj[10], scope.obj[20]]); + }); + }); + + + describe('selectAs+trackBy', function() { + beforeEach(function() { + scope.arr = [{id: 10, label: 'ten'}, {id:'20', label: 'twenty'}]; + scope.obj = {'10': {score: 10, label: 'ten'}, '20': {score: 20, label: 'twenty'}}; + }); + + + it('should bind selectAs expression result to scope (array&single)', function() { + createSelect({ + 'ng-model': 'selected', + 'ng-options': 'item.id as item.name for item in values track by item.id' + }); + + scope.$apply(function() { + scope.values = [{id: 10, name: 'A'}, {id: 20, name: 'B'}]; + scope.selected = 10; + }); + expect(element.val()).toEqual('0'); + + scope.$apply(function() { + scope.selected = 20; + }); + expect(element.val()).toEqual('1'); + + element.val('0'); + browserTrigger(element, 'change'); + expect(scope.selected).toBe(10); + }); + + + it('should bind selectAs expression result to scope (array&multiple)',function() { + createSelect({ + 'ng-model': 'selected', + 'multiple': true, + 'ng-options': 'item.id as item.name for item in values track by item.id' + }); + + scope.$apply(function() { + scope.values = [{id: 10, name: 'A'}, {id: 20, name: 'B'}]; + scope.selected = [10]; + }); + expect(element.val()).toEqual(['0']); + + scope.$apply(function() { + scope.selected = [20]; + }); + expect(element.val()).toEqual(['1']); + + element.children(0).attr('selected', 'selected'); + element.children(1).attr('selected', 'selected'); + browserTrigger(element, 'change'); + expect(scope.selected).toEqual([10, 20]); + }); + + + it('should bind selectAs expression result to scope (object&single)', function() { + createSelect({ + 'ng-model': 'selected', + 'ng-options': 'value.score as value.label for (key, value) in obj track by value.score' + }); + + scope.$apply(function() { + scope.selected = 10; + }); + expect(element.val()).toEqual('10'); + + scope.$apply(function() { + scope.selected = 20; + }); + expect(element.val()).toEqual('20'); + + element.val('10'); + browserTrigger(element, 'change'); + expect(scope.selected).toBe(10); + }); + + + it('should bind selectAs expression result to scope (object&multiple)', function() { + createSelect({ + 'ng-model': 'selected', + 'multiple': true, + 'ng-options': 'value.score as value.label for (key, value) in obj track by value.score' + }); + + scope.$apply(function() { + scope.selected = [10]; + }); + expect(element.val()).toEqual(['10']); + + scope.$apply(function() { + scope.selected = [20]; + }); + expect(element.val()).toEqual(['20']); + + element.find('option')[0].selected = 'selected'; + browserTrigger(element, 'change'); + expect(scope.selected).toEqual([10, 20]); + }); + + + it('should correctly assign model if track & select expressions differ (array&single)', function() { + createSelect({ + 'ng-model': 'selected', + 'ng-options': 'item.label as item.label for item in arr track by item.id' + }); + + scope.$apply(function() { + scope.selected = 'ten'; + }); + expect(element.val()).toBe('0'); + + element.val('1'); + browserTrigger(element, 'change'); + expect(scope.selected).toBe('twenty'); + }); + + + it('should correctly assign model if track & select expressions differ (array&multiple)', function() { + createSelect({ + 'ng-model': 'selected', + 'multiple': true, + 'ng-options': 'item.label as item.label for item in arr track by item.id' + }); + + scope.$apply(function() { + scope.selected = ['ten']; + }); + expect(element.val()).toEqual(['0']); + + element.find('option')[1].selected = 'selected'; + browserTrigger(element, 'change'); + expect(scope.selected).toEqual(['ten', 'twenty']); + }); + + + it('should correctly assign model if track & select expressions differ (object&single)', function() { + createSelect({ + 'ng-model': 'selected', + 'ng-options': 'val.label as val.label for (key, val) in obj track by val.score' + }); + + scope.$apply(function() { + scope.selected = 'ten'; + }); + expect(element.val()).toBe('10'); + + element.val('20'); + browserTrigger(element, 'change'); + expect(scope.selected).toBe('twenty'); + }); + + + it('should correctly assign model if track & select expressions differ (object&multiple)', function() { + createSelect({ + 'ng-model': 'selected', + 'multiple': true, + 'ng-options': 'val.label as val.label for (key, val) in obj track by val.score' + }); + + scope.$apply(function() { + scope.selected = ['ten']; + }); + expect(element.val()).toEqual(['10']); + + element.find('option')[1].selected = 'selected'; + browserTrigger(element, 'change'); + expect(scope.selected).toEqual(['ten', 'twenty']); + }); + }); + it('should throw when not formated "? for ? in ?"', function() { expect(function() { @@ -924,23 +1272,23 @@ describe('select', function() { {id: 2, name: 'second'}, {id: 3, name: 'third'}, {id: 4, name: 'forth'}]; - scope.selected = {id: 2}; + scope.selected = scope.values[1]; }); - expect(element.val()).toEqual('2'); + expect(element.val()).toEqual('1'); var first = jqLite(element.find('option')[0]); expect(first.text()).toEqual('first'); - expect(first.attr('value')).toEqual('1'); + expect(first.attr('value')).toEqual('0'); var forth = jqLite(element.find('option')[3]); expect(forth.text()).toEqual('forth'); - expect(forth.attr('value')).toEqual('4'); + expect(forth.attr('value')).toEqual('3'); scope.$apply(function() { scope.selected = scope.values[3]; }); - expect(element.val()).toEqual('4'); + expect(element.val()).toEqual('3'); }); From a68223b05ad2f667a32c4732c6da52c43c31f5ca Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Tue, 7 Oct 2014 16:42:39 -0700 Subject: [PATCH 2/3] refactor(select): reduce duplication and reorder functions --- src/ng/directive/select.js | 238 +++++++++++++++----------------- test/ng/directive/selectSpec.js | 23 ++- 2 files changed, 130 insertions(+), 131 deletions(-) diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js index a79fbf0cc43a..d7ea94d487a1 100644 --- a/src/ng/directive/select.js +++ b/src/ng/directive/select.js @@ -77,6 +77,24 @@ var ngOptionsMinErr = minErr('ngOptions'); * used to identify the objects in the array. The `trackexpr` will most likely refer to the * `value` variable (e.g. `value.propertyName`). With this the selection is preserved * even when the options are recreated (e.g. reloaded from the server). + + *
+ * **Note:** Using `selectAs` together with `trackexpr` is not possible (and will throw). + * TODO: Add some nice reasoning here, add a minErr and a nice error page. + * reasoning: + * - Example: ' + ); + + scope.$apply(function() { + scope.values = {a: {id: 10, name: 'A'}, b: {id: 20, name: 'B'}}; + scope.selected = scope.values.a.id; + }); + + scope.$apply(function() { + scope.values.a.name = 'C'; + }); + + var options = element.find('option'); + expect(options.length).toEqual(2); + expect(sortedHtml(options[0])).toEqual(''); + expect(sortedHtml(options[1])).toEqual(''); + }); + + it('should bind to object key', function() { createSelect({ 'ng-model': 'selected', From 6e8bf21ff550ed67274977072af4e56b318585d7 Mon Sep 17 00:00:00 2001 From: Jeff Cross Date: Wed, 8 Oct 2014 11:36:15 -0700 Subject: [PATCH 3/3] fix(select): throw for selectAs and trackBy trackBy and selectAs have never worked together, and are fundamentally incompatible since model changes cannot deterministically be reflected back to the view. This change throws an error to help developers better understand this scenario. --- docs/content/error/ngOptions/trkslct.ngdoc | 28 ++++ src/ng/directive/select.js | 10 +- test/ng/directive/selectSpec.js | 171 ++------------------- 3 files changed, 46 insertions(+), 163 deletions(-) create mode 100644 docs/content/error/ngOptions/trkslct.ngdoc diff --git a/docs/content/error/ngOptions/trkslct.ngdoc b/docs/content/error/ngOptions/trkslct.ngdoc new file mode 100644 index 000000000000..836fce31df4a --- /dev/null +++ b/docs/content/error/ngOptions/trkslct.ngdoc @@ -0,0 +1,28 @@ +@ngdoc error +@name ngOptions:trkslct +@fullName Comprehension expression cannot contain both selectAs and trackBy expressions. +@description +This error occurs when 'ngOptions' is passed a comprehension expression that contains both a +`select as` expression and a `track by` expression. These two expressions are fundamentally +incompatible. + + * Example of bad expression: ` +``` + +For more information on valid expression syntax, see 'ngOptions' in {@link ng.directive:select select} directive docs. diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js index d7ea94d487a1..c840716cc6f2 100644 --- a/src/ng/directive/select.js +++ b/src/ng/directive/select.js @@ -80,8 +80,7 @@ var ngOptionsMinErr = minErr('ngOptions'); *
* **Note:** Using `selectAs` together with `trackexpr` is not possible (and will throw). - * TODO: Add some nice reasoning here, add a minErr and a nice error page. - * reasoning: + * Reasoning: * - Example: