Skip to content

Commit 25f736b

Browse files
committed
fix(ngOptions): explicitly override selectCtrl default option handling
When ngOptions is present on a select, the option directive should not be able to add options to the selectCtrl. This will cause errors during the ngOptions lifecycle. This can happen in the following case: - there's a blank option inside the select - another directive on the select element compiles the contents of it before ngOptions is linked. Now this happens: - the option directive is compiled and adds an element $destroy listener that calls ngModel.$render - when ngOptions processes the blank option, it removes the element, and triggers the $destroy listener - ngModel.$render delegates to selectCtrl.writeValue, which accesses the options - in that phase, the options aren't yet set Fixes angular#11685
1 parent e51174b commit 25f736b

File tree

3 files changed

+105
-10
lines changed

3 files changed

+105
-10
lines changed

src/ng/directive/ngOptions.js

+11-6
Original file line numberDiff line numberDiff line change
@@ -392,11 +392,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
392392
var optionTemplate = document.createElement('option'),
393393
optGroupTemplate = document.createElement('optgroup');
394394

395-
return {
396-
restrict: 'A',
397-
terminal: true,
398-
require: ['select', 'ngModel'],
399-
link: function(scope, selectElement, attr, ctrls) {
395+
function ngOptionsPostLink(scope, selectElement, attr, ctrls) {
400396

401397
var selectCtrl = ctrls[0];
402398
var ngModelCtrl = ctrls[1];
@@ -448,7 +444,6 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
448444
unknownOption.remove();
449445
};
450446

451-
452447
// Update the controller methods for multiple selectable options
453448
if (!multiple) {
454449

@@ -726,7 +721,17 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
726721
}
727722

728723
}
724+
}
729725

726+
return {
727+
restrict: 'A',
728+
terminal: true,
729+
require: ['select', 'ngModel'],
730+
link: {
731+
pre: function ngOptionsPreLink(scope, selectElement, attr, ctrls) {
732+
ctrls[0].override();
733+
},
734+
post: ngOptionsPostLink
730735
}
731736
};
732737
}];

src/ng/directive/select.js

+17-3
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,15 @@ var SelectController =
9898
self.hasOption = function(value) {
9999
return !!optionsMap.get(value);
100100
};
101+
102+
// Directives that provide their own option adding mechanism call this to prevent options
103+
// from being added in the standard way
104+
self.overriden = false;
105+
self.override = function() {
106+
self.overridden = true;
107+
self.addOption = noop;
108+
self.removeOption = noop;
109+
};
101110
}];
102111

103112
/**
@@ -308,7 +317,13 @@ var selectDirective = function() {
308317
restrict: 'E',
309318
require: ['select', '?ngModel'],
310319
controller: SelectController,
311-
link: function(scope, element, attr, ctrls) {
320+
priority: 1,
321+
link: {
322+
pre: selectPreLink
323+
}
324+
};
325+
326+
function selectPreLink(scope, element, attr, ctrls) {
312327

313328
// if ngModel is not defined, we don't need to do anything
314329
var ngModelCtrl = ctrls[1];
@@ -378,7 +393,6 @@ var selectDirective = function() {
378393

379394
}
380395
}
381-
};
382396
};
383397

384398

@@ -430,7 +444,7 @@ var optionDirective = ['$interpolate', function($interpolate) {
430444

431445
// Only update trigger option updates if this is an option within a `select`
432446
// that also has `ngModel` attached
433-
if (selectCtrl && selectCtrl.ngModelCtrl) {
447+
if (selectCtrl && selectCtrl.ngModelCtrl && !selectCtrl.overridden) {
434448

435449
if (valueInterpolated) {
436450
// The value attribute is interpolated

test/ng/directive/ngOptionsSpec.js

+77-1
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
describe('ngOptions', function() {
44

5-
var scope, formElement, element, $compile;
5+
var scope, formElement, element, $compile, linkLog;
66

77
function compile(html) {
88
formElement = jqLite('<form name="form">' + html + '</form>');
@@ -104,6 +104,53 @@ describe('ngOptions', function() {
104104
});
105105
});
106106

107+
beforeEach(module(function($compileProvider, $provide) {
108+
linkLog = [];
109+
110+
$compileProvider
111+
.directive('customSelect', function() {
112+
return {
113+
restrict: "E",
114+
replace: true,
115+
scope: {
116+
ngModel: '=',
117+
options: '='
118+
},
119+
templateUrl: 'select_template.html',
120+
link: function(scope, $element, attributes) {
121+
scope.selectable_options = scope.options;
122+
}
123+
};
124+
})
125+
126+
.directive('oCompileContents', function() {
127+
return {
128+
link: function(scope, element) {
129+
linkLog.push('linkCompileContents');
130+
$compile(element.contents())(scope);
131+
}
132+
};
133+
});
134+
135+
$provide.decorator('ngOptionsDirective', function($delegate) {
136+
137+
var origPreLink = $delegate[0].link.pre;
138+
var origPostLink = $delegate[0].link.post;
139+
140+
$delegate[0].compile = function() {
141+
return {
142+
pre: origPreLink,
143+
post: function() {
144+
linkLog.push('linkNgOptions');
145+
origPostLink.apply(this, arguments);
146+
}
147+
};
148+
};
149+
150+
return $delegate;
151+
});
152+
}));
153+
107154
beforeEach(inject(function($rootScope, _$compile_) {
108155
scope = $rootScope.$new(); //create a child scope because the root scope can't be $destroy-ed
109156
$compile = _$compile_;
@@ -2119,6 +2166,35 @@ describe('ngOptions', function() {
21192166
option = element.find('option').eq(0);
21202167
expect(option.text()).toBe('A');
21212168
});
2169+
2170+
2171+
it('should not throw when a directive compiles the blank option before ngOptions is linked', function() {
2172+
expect(function() {
2173+
createSelect({
2174+
'o-compile-contents': '',
2175+
'name': 'select',
2176+
'ng-model': 'value',
2177+
'ng-options': 'item for item in items'
2178+
}, true);
2179+
}).not.toThrow();
2180+
2181+
expect(linkLog).toEqual(['linkCompileContents', 'linkNgOptions']);
2182+
});
2183+
2184+
2185+
it('should not throw with a directive that replaces', inject(function($templateCache, $httpBackend) {
2186+
$templateCache.put('select_template.html', '<select ng-options="option as option for option in selectable_options"> <option value="">This is a test</option> </select>');
2187+
2188+
scope.options = ['a', 'b', 'c', 'd'];
2189+
2190+
expect(function() {
2191+
element = $compile('<custom-select ng-model="value" options="options"></custom-select>')(scope);
2192+
scope.$digest();
2193+
}).not.toThrow();
2194+
2195+
dealoc(element);
2196+
}));
2197+
21222198
});
21232199

21242200

0 commit comments

Comments
 (0)