From 4b4098bfcae64f69c70a22393de1f3d9a0d3dc46 Mon Sep 17 00:00:00 2001 From: Juan Gabriel Jimenez Campos Date: Tue, 21 Oct 2014 16:08:21 +0200 Subject: [PATCH] fix(select): assign result of track exp to element value Fixes a regression where the option/select values would always be set to the key or index of a value within the corresponding collection. Prior to some 1.3.0 refactoring, the result of the track expression would be bound to the value, but this behavior was not documented or explicitly tested. A cache was added in order to improve performance getting the associated value for a given track expression. This commit adds one explicit test for this behavior, and changes several other trackBy tests to reflect the desired behavior as well. Closes #9718 Fixes #9592 --- src/ng/directive/select.js | 16 +++++-- test/ng/directive/selectSpec.js | 79 ++++++++++++++++++++++++--------- 2 files changed, 70 insertions(+), 25 deletions(-) diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js index 9150d26cdb09..f65f3a663c2d 100644 --- a/src/ng/directive/select.js +++ b/src/ng/directive/select.js @@ -355,6 +355,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { valuesFn = $parse(match[7]), track = match[8], trackFn = track ? $parse(match[8]) : null, + trackKeysCache = {}, // This is an array of array of existing option groups in DOM. // We try to reuse these if possible // - optionGroupsCache[0] is the options with no option group @@ -405,10 +406,11 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { if (multiple) { viewValue = []; forEach(selectElement.val(), function(selectedKey) { + selectedKey = trackFn ? trackKeysCache[selectedKey] : selectedKey; viewValue.push(getViewValue(selectedKey, collection[selectedKey])); }); } else { - var selectedKey = selectElement.val(); + var selectedKey = trackFn ? trackKeysCache[selectElement.val()] : selectElement.val(); viewValue = getViewValue(selectedKey, collection[selectedKey]); } ctrl.$setViewValue(viewValue); @@ -530,7 +532,10 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { anySelected = false, lastElement, element, - label; + label, + optionId; + + trackKeysCache = {}; // We now build up the list of options we need (we merge later) for (index = 0; length = keys.length, index < length; index++) { @@ -554,9 +559,14 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) { // doing displayFn(scope, locals) || '' overwrites zero values label = isDefined(label) ? label : ''; + optionId = trackFn ? trackFn(scope, locals) : (keyName ? keys[index] : index); + if (trackFn) { + trackKeysCache[optionId] = key; + } + optionGroup.push({ // either the index into array or key from object - id: (keyName ? keys[index] : index), + id: optionId, 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 3e918fc80309..7aa91b97fcfb 100644 --- a/test/ng/directive/selectSpec.js +++ b/test/ng/directive/selectSpec.js @@ -722,10 +722,45 @@ describe('select', function() { describe('trackBy 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'}}; + scope.obj = {'1': {score: 10, label: 'ten'}, '2': {score: 20, label: 'twenty'}}; }); + it('should set the result of track by expression to element value', 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('10'); + + scope.$apply(function() { + scope.arr[0] = {id: 10, label: 'new ten'}; + }); + expect(element.val()).toBe('10'); + + element.children()[1].selected = 'selected'; + browserTrigger(element, 'change'); + expect(scope.selected).toEqual(scope.arr[1]); + }); + + + it('should use the tracked expression as option value', function() { + createSelect({ + 'ng-model': 'selected', + 'ng-options': 'item.label for item in arr track by item.id' + }); + + var options = element.find('option'); + expect(options.length).toEqual(3); + expect(sortedHtml(options[0])).toEqual(''); + expect(sortedHtml(options[1])).toEqual(''); + expect(sortedHtml(options[2])).toEqual(''); + }); + it('should preserve value even when reference has changed (single&array)', function() { createSelect({ 'ng-model': 'selected', @@ -735,12 +770,12 @@ describe('select', function() { scope.$apply(function() { scope.selected = scope.arr[0]; }); - expect(element.val()).toBe('0'); + expect(element.val()).toBe('10'); scope.$apply(function() { scope.arr[0] = {id: 10, label: 'new ten'}; }); - expect(element.val()).toBe('0'); + expect(element.val()).toBe('10'); element.children()[1].selected = 1; browserTrigger(element, 'change'); @@ -758,12 +793,12 @@ describe('select', function() { scope.$apply(function() { scope.selected = scope.arr; }); - expect(element.val()).toEqual(['0','1']); + expect(element.val()).toEqual(['10','20']); scope.$apply(function() { scope.arr[0] = {id: 10, label: 'new ten'}; }); - expect(element.val()).toEqual(['0','1']); + expect(element.val()).toEqual(['10','20']); element.children()[0].selected = false; browserTrigger(element, 'change'); @@ -778,18 +813,18 @@ describe('select', function() { }); scope.$apply(function() { - scope.selected = scope.obj['10']; + scope.selected = scope.obj['1']; }); expect(element.val()).toBe('10'); scope.$apply(function() { - scope.obj['10'] = {score: 10, label: 'ten'}; + scope.obj['1'] = {score: 10, label: 'ten'}; }); expect(element.val()).toBe('10'); element.val('20'); browserTrigger(element, 'change'); - expect(scope.selected).toBe(scope.obj[20]); + expect(scope.selected).toBe(scope.obj['2']); }); @@ -801,18 +836,18 @@ describe('select', function() { }); scope.$apply(function() { - scope.selected = [scope.obj['10']]; + scope.selected = [scope.obj['1']]; }); expect(element.val()).toEqual(['10']); scope.$apply(function() { - scope.obj['10'] = {score: 10, label: 'ten'}; + scope.obj['1'] = {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]]); + expect(scope.selected).toEqual([scope.obj['1'], scope.obj['2']]); }); }); @@ -840,16 +875,16 @@ describe('select', function() { scope.$apply(function() { scope.selected = scope.arr[0].subItem; }); - expect(element.val()).toEqual('0'); + expect(element.val()).toEqual('10'); scope.$apply(function() { scope.selected = scope.arr[1].subItem; }); - expect(element.val()).toEqual('1'); + expect(element.val()).toEqual('20'); // Now test view -> model - element.val('0'); + element.val('10'); browserTrigger(element, 'change'); expect(scope.selected).toBe(scope.arr[0].subItem); @@ -861,7 +896,7 @@ describe('select', function() { subItem: {label: 'new twenty', id: 20} }]; }); - expect(element.val()).toBe('0'); + expect(element.val()).toBe('10'); expect(scope.selected.id).toBe(10); }); @@ -879,12 +914,12 @@ describe('select', function() { scope.$apply(function() { scope.selected = [scope.arr[0].subItem]; }); - expect(element.val()).toEqual(['0']); + expect(element.val()).toEqual(['10']); scope.$apply(function() { scope.selected = [scope.arr[1].subItem]; }); - expect(element.val()).toEqual(['1']); + expect(element.val()).toEqual(['20']); // Now test view -> model @@ -901,7 +936,7 @@ describe('select', function() { subItem: {label: 'new twenty', id: 20} }]; }); - expect(element.val()).toEqual(['0']); + expect(element.val()).toEqual(['10']); expect(scope.selected[0].id).toEqual(10); expect(scope.selected.length).toBe(1); }); @@ -1338,20 +1373,20 @@ describe('select', function() { scope.selected = scope.values[1]; }); - expect(element.val()).toEqual('1'); + expect(element.val()).toEqual('2'); var first = jqLite(element.find('option')[0]); expect(first.text()).toEqual('first'); - expect(first.attr('value')).toEqual('0'); + expect(first.attr('value')).toEqual('1'); var forth = jqLite(element.find('option')[3]); expect(forth.text()).toEqual('forth'); - expect(forth.attr('value')).toEqual('3'); + expect(forth.attr('value')).toEqual('4'); scope.$apply(function() { scope.selected = scope.values[3]; }); - expect(element.val()).toEqual('3'); + expect(element.val()).toEqual('4'); });