Skip to content

Commit 4b4098b

Browse files
juangabreiljeffbcross
juangabreil
authored and
jeffbcross
committed
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 angular#9718 Fixes angular#9592
1 parent c3b3b90 commit 4b4098b

2 files changed

Lines changed: 70 additions & 25 deletions

File tree

‎src/ng/directive/select.js‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,7 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
355355
valuesFn = $parse(match[7]),
356356
track = match[8],
357357
trackFn = track ? $parse(match[8]) : null,
358+
trackKeysCache = {},
358359
// This is an array of array of existing option groups in DOM.
359360
// We try to reuse these if possible
360361
// - optionGroupsCache[0] is the options with no option group
@@ -405,10 +406,11 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
405406
if (multiple) {
406407
viewValue = [];
407408
forEach(selectElement.val(), function(selectedKey) {
409+
selectedKey = trackFn ? trackKeysCache[selectedKey] : selectedKey;
408410
viewValue.push(getViewValue(selectedKey, collection[selectedKey]));
409411
});
410412
} else {
411-
var selectedKey = selectElement.val();
413+
var selectedKey = trackFn ? trackKeysCache[selectElement.val()] : selectElement.val();
412414
viewValue = getViewValue(selectedKey, collection[selectedKey]);
413415
}
414416
ctrl.$setViewValue(viewValue);
@@ -530,7 +532,10 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
530532
anySelected = false,
531533
lastElement,
532534
element,
533-
label;
535+
label,
536+
optionId;
537+
538+
trackKeysCache = {};
534539

535540
// We now build up the list of options we need (we merge later)
536541
for (index = 0; length = keys.length, index < length; index++) {
@@ -554,9 +559,14 @@ var selectDirective = ['$compile', '$parse', function($compile, $parse) {
554559

555560
// doing displayFn(scope, locals) || '' overwrites zero values
556561
label = isDefined(label) ? label : '';
562+
optionId = trackFn ? trackFn(scope, locals) : (keyName ? keys[index] : index);
563+
if (trackFn) {
564+
trackKeysCache[optionId] = key;
565+
}
566+
557567
optionGroup.push({
558568
// either the index into array or key from object
559-
id: (keyName ? keys[index] : index),
569+
id: optionId,
560570
label: label,
561571
selected: selected // determine if we should be selected
562572
});

‎test/ng/directive/selectSpec.js‎

Lines changed: 57 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -722,10 +722,45 @@ describe('select', function() {
722722
describe('trackBy expression', function() {
723723
beforeEach(function() {
724724
scope.arr = [{id: 10, label: 'ten'}, {id:20, label: 'twenty'}];
725-
scope.obj = {'10': {score: 10, label: 'ten'}, '20': {score: 20, label: 'twenty'}};
725+
scope.obj = {'1': {score: 10, label: 'ten'}, '2': {score: 20, label: 'twenty'}};
726726
});
727727

728728

729+
it('should set the result of track by expression to element value', function() {
730+
createSelect({
731+
'ng-model': 'selected',
732+
'ng-options': 'item.label for item in arr track by item.id'
733+
});
734+
735+
scope.$apply(function() {
736+
scope.selected = scope.arr[0];
737+
});
738+
expect(element.val()).toBe('10');
739+
740+
scope.$apply(function() {
741+
scope.arr[0] = {id: 10, label: 'new ten'};
742+
});
743+
expect(element.val()).toBe('10');
744+
745+
element.children()[1].selected = 'selected';
746+
browserTrigger(element, 'change');
747+
expect(scope.selected).toEqual(scope.arr[1]);
748+
});
749+
750+
751+
it('should use the tracked expression as option value', function() {
752+
createSelect({
753+
'ng-model': 'selected',
754+
'ng-options': 'item.label for item in arr track by item.id'
755+
});
756+
757+
var options = element.find('option');
758+
expect(options.length).toEqual(3);
759+
expect(sortedHtml(options[0])).toEqual('<option value="?"></option>');
760+
expect(sortedHtml(options[1])).toEqual('<option value="10">ten</option>');
761+
expect(sortedHtml(options[2])).toEqual('<option value="20">twenty</option>');
762+
});
763+
729764
it('should preserve value even when reference has changed (single&array)', function() {
730765
createSelect({
731766
'ng-model': 'selected',
@@ -735,12 +770,12 @@ describe('select', function() {
735770
scope.$apply(function() {
736771
scope.selected = scope.arr[0];
737772
});
738-
expect(element.val()).toBe('0');
773+
expect(element.val()).toBe('10');
739774

740775
scope.$apply(function() {
741776
scope.arr[0] = {id: 10, label: 'new ten'};
742777
});
743-
expect(element.val()).toBe('0');
778+
expect(element.val()).toBe('10');
744779

745780
element.children()[1].selected = 1;
746781
browserTrigger(element, 'change');
@@ -758,12 +793,12 @@ describe('select', function() {
758793
scope.$apply(function() {
759794
scope.selected = scope.arr;
760795
});
761-
expect(element.val()).toEqual(['0','1']);
796+
expect(element.val()).toEqual(['10','20']);
762797

763798
scope.$apply(function() {
764799
scope.arr[0] = {id: 10, label: 'new ten'};
765800
});
766-
expect(element.val()).toEqual(['0','1']);
801+
expect(element.val()).toEqual(['10','20']);
767802

768803
element.children()[0].selected = false;
769804
browserTrigger(element, 'change');
@@ -778,18 +813,18 @@ describe('select', function() {
778813
});
779814

780815
scope.$apply(function() {
781-
scope.selected = scope.obj['10'];
816+
scope.selected = scope.obj['1'];
782817
});
783818
expect(element.val()).toBe('10');
784819

785820
scope.$apply(function() {
786-
scope.obj['10'] = {score: 10, label: 'ten'};
821+
scope.obj['1'] = {score: 10, label: 'ten'};
787822
});
788823
expect(element.val()).toBe('10');
789824

790825
element.val('20');
791826
browserTrigger(element, 'change');
792-
expect(scope.selected).toBe(scope.obj[20]);
827+
expect(scope.selected).toBe(scope.obj['2']);
793828
});
794829

795830

@@ -801,18 +836,18 @@ describe('select', function() {
801836
});
802837

803838
scope.$apply(function() {
804-
scope.selected = [scope.obj['10']];
839+
scope.selected = [scope.obj['1']];
805840
});
806841
expect(element.val()).toEqual(['10']);
807842

808843
scope.$apply(function() {
809-
scope.obj['10'] = {score: 10, label: 'ten'};
844+
scope.obj['1'] = {score: 10, label: 'ten'};
810845
});
811846
expect(element.val()).toEqual(['10']);
812847

813848
element.children()[1].selected = 'selected';
814849
browserTrigger(element, 'change');
815-
expect(scope.selected).toEqual([scope.obj[10], scope.obj[20]]);
850+
expect(scope.selected).toEqual([scope.obj['1'], scope.obj['2']]);
816851
});
817852
});
818853

@@ -840,16 +875,16 @@ describe('select', function() {
840875
scope.$apply(function() {
841876
scope.selected = scope.arr[0].subItem;
842877
});
843-
expect(element.val()).toEqual('0');
878+
expect(element.val()).toEqual('10');
844879

845880
scope.$apply(function() {
846881
scope.selected = scope.arr[1].subItem;
847882
});
848-
expect(element.val()).toEqual('1');
883+
expect(element.val()).toEqual('20');
849884

850885
// Now test view -> model
851886

852-
element.val('0');
887+
element.val('10');
853888
browserTrigger(element, 'change');
854889
expect(scope.selected).toBe(scope.arr[0].subItem);
855890

@@ -861,7 +896,7 @@ describe('select', function() {
861896
subItem: {label: 'new twenty', id: 20}
862897
}];
863898
});
864-
expect(element.val()).toBe('0');
899+
expect(element.val()).toBe('10');
865900
expect(scope.selected.id).toBe(10);
866901
});
867902

@@ -879,12 +914,12 @@ describe('select', function() {
879914
scope.$apply(function() {
880915
scope.selected = [scope.arr[0].subItem];
881916
});
882-
expect(element.val()).toEqual(['0']);
917+
expect(element.val()).toEqual(['10']);
883918

884919
scope.$apply(function() {
885920
scope.selected = [scope.arr[1].subItem];
886921
});
887-
expect(element.val()).toEqual(['1']);
922+
expect(element.val()).toEqual(['20']);
888923

889924
// Now test view -> model
890925

@@ -901,7 +936,7 @@ describe('select', function() {
901936
subItem: {label: 'new twenty', id: 20}
902937
}];
903938
});
904-
expect(element.val()).toEqual(['0']);
939+
expect(element.val()).toEqual(['10']);
905940
expect(scope.selected[0].id).toEqual(10);
906941
expect(scope.selected.length).toBe(1);
907942
});
@@ -1338,20 +1373,20 @@ describe('select', function() {
13381373
scope.selected = scope.values[1];
13391374
});
13401375

1341-
expect(element.val()).toEqual('1');
1376+
expect(element.val()).toEqual('2');
13421377

13431378
var first = jqLite(element.find('option')[0]);
13441379
expect(first.text()).toEqual('first');
1345-
expect(first.attr('value')).toEqual('0');
1380+
expect(first.attr('value')).toEqual('1');
13461381
var forth = jqLite(element.find('option')[3]);
13471382
expect(forth.text()).toEqual('forth');
1348-
expect(forth.attr('value')).toEqual('3');
1383+
expect(forth.attr('value')).toEqual('4');
13491384

13501385
scope.$apply(function() {
13511386
scope.selected = scope.values[3];
13521387
});
13531388

1354-
expect(element.val()).toEqual('3');
1389+
expect(element.val()).toEqual('4');
13551390
});
13561391

13571392

0 commit comments

Comments
 (0)