Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Commit be3a2b9

Browse files
NarretzNarretz
committed
fix(select): re-define ngModelCtrl.$render in the select postLink fn
Previously, the $render function was re-defined in the select directive's preLink function. When a select element is compiled, every option element inside it is linked and registered with the selectCtrl, which calls $render to update the selected option.$render calls selectCtrl.writeValue, which adds an unknown option in case no option is selected. In cases where optgroup elements are followed by a line-break, adding the unknown option confuses the html compiler and makes it call the link function of the following option with a wrong element, which means this option is not correctly registered. Since manipulation of the DOM in the preLink function is wrong API usage, the problem cannot be fixed in the compiler. With this commit, the $render function is not re-defined until the select postLink function, at which point all option elements have been linked already. The commit also changes the toEqualSelectWithOptions matcher to take selected options in groups into account. Closes #13583
1 parent dec8a0e commit be3a2b9

File tree

2 files changed

+113
-33
lines changed

2 files changed

+113
-33
lines changed

‎src/ng/directive/select.js

+19-8
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,8 @@ var selectDirective = function() {
356356
controller: SelectController,
357357
priority: 1,
358358
link: {
359-
pre: selectPreLink
359+
pre: selectPreLink,
360+
post: selectPostLink
360361
}
361362
};
362363

@@ -370,13 +371,6 @@ var selectDirective = function() {
370371

371372
selectCtrl.ngModelCtrl = ngModelCtrl;
372373

373-
// We delegate rendering to the `writeValue` method, which can be changed
374-
// if the select can have multiple selected values or if the options are being
375-
// generated by `ngOptions`
376-
ngModelCtrl.$render = function() {
377-
selectCtrl.writeValue(ngModelCtrl.$viewValue);
378-
};
379-
380374
// When the selected item(s) changes we delegate getting the value of the select control
381375
// to the `readValue` method, which can be changed if the select can have multiple
382376
// selected values or if the options are being generated by `ngOptions`
@@ -430,6 +424,23 @@ var selectDirective = function() {
430424

431425
}
432426
}
427+
428+
function selectPostLink(scope, element, attrs, ctrls) {
429+
// if ngModel is not defined, we don't need to do anything
430+
var ngModelCtrl = ctrls[1];
431+
if (!ngModelCtrl) return;
432+
433+
var selectCtrl = ctrls[0];
434+
435+
// We delegate rendering to the `writeValue` method, which can be changed
436+
// if the select can have multiple selected values or if the options are being
437+
// generated by `ngOptions`.
438+
// This must be done in the postLink fn to prevent $render to be called before
439+
// all nodes have been linked correctly.
440+
ngModelCtrl.$render = function() {
441+
selectCtrl.writeValue(ngModelCtrl.$viewValue);
442+
};
443+
}
433444
};
434445

435446

‎test/ng/directive/selectSpec.js

+94-25
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
'use strict';
22

33
describe('select', function() {
4-
var scope, formElement, element, $compile;
4+
var scope, formElement, element, $compile, ngModelCtrl, selectCtrl, renderSpy;
55

66
function compile(html) {
77
formElement = jqLite('<form name="form">' + html + '</form>');
@@ -10,10 +10,49 @@ describe('select', function() {
1010
scope.$apply();
1111
}
1212

13+
function compileRepeatedOptions() {
14+
compile('<select ng-model="robot">' +
15+
'<option value="{{item.value}}" ng-repeat="item in robots">{{item.label}}</option>' +
16+
'</select>');
17+
}
18+
19+
function compileGroupedOptions() {
20+
compile(
21+
'<select ng-model="mySelect">' +
22+
'<option ng-repeat="item in values">{{item.name}}</option>' +
23+
'<optgroup ng-repeat="group in groups" label="{{group.name}}">' +
24+
'<option ng-repeat="item in group.values">{{item.name}}</option>' +
25+
'</optgroup>' +
26+
'</select>');
27+
}
28+
1329
function unknownValue(value) {
1430
return '? ' + hashKey(value) + ' ?';
1531
}
1632

33+
beforeEach(module(function($compileProvider) {
34+
$compileProvider.directive('captureSelectCtrl', function() {
35+
return {
36+
require: 'select',
37+
link: {
38+
pre: function(scope, element, attrs, ctrl) {
39+
selectCtrl = ctrl;
40+
renderSpy = jasmine.createSpy('renderSpy');
41+
dump('fakePre', selectCtrl.ngModelCtrl.$render);
42+
selectCtrl.ngModelCtrl.$render = renderSpy.andCallFake(selectCtrl.ngModelCtrl.$render);
43+
spyOn(selectCtrl, 'writeValue').andCallThrough();
44+
},
45+
post: function(scope, element, attrs, ctrl) {
46+
dump('fakePost')
47+
// expect(selectCtrl.ngModelCtrl.$render).not.toHaveBeenCalled();
48+
// dump('fakePost', selectCtrl.ngModelCtrl.$render)
49+
// spyOn(selectCtrl.ngModelCtrl, '$render').andCallThrough();
50+
},
51+
}
52+
};
53+
});
54+
}));
55+
1756
beforeEach(inject(function($rootScope, _$compile_) {
1857
scope = $rootScope.$new(); //create a child scope because the root scope can't be $destroy-ed
1958
$compile = _$compile_;
@@ -47,12 +86,14 @@ describe('select', function() {
4786
toEqualSelectWithOptions: function(expected) {
4887
var actualValues = {};
4988
var optionGroup;
89+
var optionValue;
5090

5191
forEach(this.actual.find('option'), function(option) {
5292
optionGroup = option.parentNode.label || '';
5393
actualValues[optionGroup] = actualValues[optionGroup] || [];
5494
// IE9 doesn't populate the label property from the text property like other browsers
55-
actualValues[optionGroup].push(option.label || option.text);
95+
optionValue = option.label || option.text;
96+
actualValues[optionGroup].push(option.selected ? [optionValue] : optionValue);
5697
});
5798

5899
this.message = function() {
@@ -198,6 +239,50 @@ describe('select', function() {
198239
});
199240

200241

242+
it('should select options in a group when there is a linebreak before an option', function() {
243+
scope.mySelect = 'B';
244+
scope.$apply();
245+
246+
var select = jqLite(
247+
'<select ng-model="mySelect">' +
248+
'<optgroup label="first">' +
249+
'<option value="A">A</option>' +
250+
'</optgroup>' +
251+
'<optgroup label="second">' + '\n' +
252+
'<option value="B">B</option>' +
253+
'</optgroup> ' +
254+
'</select>');
255+
256+
$compile(select)(scope);
257+
scope.$apply();
258+
259+
expect(select).toEqualSelectWithOptions({'first':['A'], 'second': [['B']]});
260+
dealoc(select);
261+
});
262+
263+
264+
it('should only call selectCtrl.writeValue after a digest has occured', function() {
265+
scope.mySelect = 'B';
266+
scope.$apply();
267+
268+
var select = jqLite(
269+
'<select capture-select-ctrl ng-model="mySelect">' +
270+
'<optgroup label="first">' +
271+
'<option value="A">A</option>' +
272+
'</optgroup>' +
273+
'<optgroup label="second">' + '\n' +
274+
'<option value="B">B</option>' +
275+
'</optgroup> ' +
276+
'</select>');
277+
278+
$compile(select)(scope);
279+
expect(selectCtrl.writeValue).not.toHaveBeenCalled();
280+
281+
scope.$digest();
282+
expect(selectCtrl.writeValue).toHaveBeenCalledOnce();
283+
dealoc(select);
284+
});
285+
201286
describe('empty option', function() {
202287

203288
it('should allow empty option to be added and removed dynamically', function() {
@@ -538,22 +623,6 @@ describe('select', function() {
538623

539624
describe('selectController.hasOption', function() {
540625

541-
function compileRepeatedOptions() {
542-
compile('<select ng-model="robot">' +
543-
'<option value="{{item.value}}" ng-repeat="item in robots">{{item.label}}</option>' +
544-
'</select>');
545-
}
546-
547-
function compileGroupedOptions() {
548-
compile(
549-
'<select ng-model="mySelect">' +
550-
'<option ng-repeat="item in values">{{item.name}}</option>' +
551-
'<optgroup ng-repeat="group in groups" label="{{group.name}}">' +
552-
'<option ng-repeat="item in group.values">{{item.name}}</option>' +
553-
'</optgroup>' +
554-
'</select>');
555-
}
556-
557626
describe('flat options', function() {
558627
it('should return false for options shifted via ngRepeat', function() {
559628
scope.robots = [
@@ -624,7 +693,7 @@ describe('select', function() {
624693
expect(selectCtrl.hasOption('A')).toBe(true);
625694
expect(selectCtrl.hasOption('B')).toBe(true);
626695
expect(selectCtrl.hasOption('C')).toBe(true);
627-
expect(element).toEqualSelectWithOptions({'': ['A', 'B', 'C']});
696+
expect(element).toEqualSelectWithOptions({'': ['A', 'B', ['C']]});
628697
});
629698
});
630699

@@ -665,7 +734,7 @@ describe('select', function() {
665734
expect(selectCtrl.hasOption('C')).toBe(true);
666735
expect(selectCtrl.hasOption('D')).toBe(true);
667736
expect(selectCtrl.hasOption('E')).toBe(true);
668-
expect(element).toEqualSelectWithOptions({'': [''], 'first':['B', 'C'], 'second': ['D', 'E']});
737+
expect(element).toEqualSelectWithOptions({'': [['']], 'first':['B', 'C'], 'second': ['D', 'E']});
669738
});
670739

671740

@@ -702,7 +771,7 @@ describe('select', function() {
702771
expect(selectCtrl.hasOption('C')).toBe(true);
703772
expect(selectCtrl.hasOption('D')).toBe(true);
704773
expect(selectCtrl.hasOption('E')).toBe(true);
705-
expect(element).toEqualSelectWithOptions({'': [''], 'first':['B', 'C', 'D'], 'second': ['E']});
774+
expect(element).toEqualSelectWithOptions({'': [['']], 'first':['B', 'C', 'D'], 'second': ['E']});
706775
});
707776

708777

@@ -741,7 +810,7 @@ describe('select', function() {
741810
expect(selectCtrl.hasOption('D')).toBe(true);
742811
expect(selectCtrl.hasOption('E')).toBe(true);
743812
expect(selectCtrl.hasOption('F')).toBe(false);
744-
expect(element).toEqualSelectWithOptions({'': ['', 'A'], 'first':['B', 'C'], 'second': ['D', 'E']});
813+
expect(element).toEqualSelectWithOptions({'': [[''], 'A'], 'first':['B', 'C'], 'second': ['D', 'E']});
745814
});
746815

747816

@@ -778,7 +847,7 @@ describe('select', function() {
778847
expect(selectCtrl.hasOption('C')).toBe(false);
779848
expect(selectCtrl.hasOption('D')).toBe(true);
780849
expect(selectCtrl.hasOption('E')).toBe(true);
781-
expect(element).toEqualSelectWithOptions({'': ['', 'A'], 'first':['B', 'D'], 'second': ['E']});
850+
expect(element).toEqualSelectWithOptions({'': [[''], 'A'], 'first':['B', 'D'], 'second': ['E']});
782851
});
783852

784853

@@ -813,7 +882,7 @@ describe('select', function() {
813882
expect(selectCtrl.hasOption('C')).toBe(true);
814883
expect(selectCtrl.hasOption('D')).toBe(false);
815884
expect(selectCtrl.hasOption('E')).toBe(true);
816-
expect(element).toEqualSelectWithOptions({'': ['', 'A'], 'first':['B', 'C'], 'second': ['E']});
885+
expect(element).toEqualSelectWithOptions({'': [[''], 'A'], 'first':['B', 'C'], 'second': ['E']});
817886
});
818887

819888

@@ -848,7 +917,7 @@ describe('select', function() {
848917
expect(selectCtrl.hasOption('C')).toBe(true);
849918
expect(selectCtrl.hasOption('D')).toBe(false);
850919
expect(selectCtrl.hasOption('E')).toBe(false);
851-
expect(element).toEqualSelectWithOptions({'': ['', 'A'], 'first':['B', 'C']});
920+
expect(element).toEqualSelectWithOptions({'': [[''], 'A'], 'first':['B', 'C']});
852921
});
853922
});
854923
});

0 commit comments

Comments
 (0)