From 45eb52f5720bd62680d8dbe455079a678b2acbfe Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Mon, 28 Sep 2015 23:39:21 +0200 Subject: [PATCH] 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 #11685 --- src/ng/directive/ngOptions.js | 17 ++++--- src/ng/directive/select.js | 20 ++++++-- test/ng/directive/ngOptionsSpec.js | 78 +++++++++++++++++++++++++++++- 3 files changed, 105 insertions(+), 10 deletions(-) diff --git a/src/ng/directive/ngOptions.js b/src/ng/directive/ngOptions.js index 282dff3aedd2..eea156ce207a 100644 --- a/src/ng/directive/ngOptions.js +++ b/src/ng/directive/ngOptions.js @@ -392,11 +392,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) { var optionTemplate = document.createElement('option'), optGroupTemplate = document.createElement('optgroup'); - return { - restrict: 'A', - terminal: true, - require: ['select', 'ngModel'], - link: function(scope, selectElement, attr, ctrls) { + function ngOptionsPostLink(scope, selectElement, attr, ctrls) { var selectCtrl = ctrls[0]; var ngModelCtrl = ctrls[1]; @@ -448,7 +444,6 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) { unknownOption.remove(); }; - // Update the controller methods for multiple selectable options if (!multiple) { @@ -726,7 +721,17 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) { } } + } + return { + restrict: 'A', + terminal: true, + require: ['select', 'ngModel'], + link: { + pre: function ngOptionsPreLink(scope, selectElement, attr, ctrls) { + ctrls[0].override(); + }, + post: ngOptionsPostLink } }; }]; diff --git a/src/ng/directive/select.js b/src/ng/directive/select.js index df2bcee32437..f0669ae075c0 100644 --- a/src/ng/directive/select.js +++ b/src/ng/directive/select.js @@ -98,6 +98,15 @@ var SelectController = self.hasOption = function(value) { return !!optionsMap.get(value); }; + + // Directives that provide their own option adding mechanism call this to prevent options + // from being added in the standard way + self.overriden = false; + self.override = function() { + self.overridden = true; + self.addOption = noop; + self.removeOption = noop; + }; }]; /** @@ -308,7 +317,13 @@ var selectDirective = function() { restrict: 'E', require: ['select', '?ngModel'], controller: SelectController, - link: function(scope, element, attr, ctrls) { + priority: 1, + link: { + pre: selectPreLink + } + }; + + function selectPreLink(scope, element, attr, ctrls) { // if ngModel is not defined, we don't need to do anything var ngModelCtrl = ctrls[1]; @@ -378,7 +393,6 @@ var selectDirective = function() { } } - }; }; @@ -430,7 +444,7 @@ var optionDirective = ['$interpolate', function($interpolate) { // Only update trigger option updates if this is an option within a `select` // that also has `ngModel` attached - if (selectCtrl && selectCtrl.ngModelCtrl) { + if (selectCtrl && selectCtrl.ngModelCtrl && !selectCtrl.overridden) { if (valueInterpolated) { // The value attribute is interpolated diff --git a/test/ng/directive/ngOptionsSpec.js b/test/ng/directive/ngOptionsSpec.js index bc50bcbef818..b96f8acc8026 100644 --- a/test/ng/directive/ngOptionsSpec.js +++ b/test/ng/directive/ngOptionsSpec.js @@ -2,7 +2,7 @@ describe('ngOptions', function() { - var scope, formElement, element, $compile; + var scope, formElement, element, $compile, linkLog; function compile(html) { formElement = jqLite('
' + html + '
'); @@ -104,6 +104,53 @@ describe('ngOptions', function() { }); }); + beforeEach(module(function($compileProvider, $provide) { + linkLog = []; + + $compileProvider + .directive('customSelect', function() { + return { + restrict: "E", + replace: true, + scope: { + ngModel: '=', + options: '=' + }, + templateUrl: 'select_template.html', + link: function(scope, $element, attributes) { + scope.selectable_options = scope.options; + } + }; + }) + + .directive('oCompileContents', function() { + return { + link: function(scope, element) { + linkLog.push('linkCompileContents'); + $compile(element.contents())(scope); + } + }; + }); + + $provide.decorator('ngOptionsDirective', function($delegate) { + + var origPreLink = $delegate[0].link.pre; + var origPostLink = $delegate[0].link.post; + + $delegate[0].compile = function() { + return { + pre: origPreLink, + post: function() { + linkLog.push('linkNgOptions'); + origPostLink.apply(this, arguments); + } + }; + }; + + return $delegate; + }); + })); + beforeEach(inject(function($rootScope, _$compile_) { scope = $rootScope.$new(); //create a child scope because the root scope can't be $destroy-ed $compile = _$compile_; @@ -2119,6 +2166,35 @@ describe('ngOptions', function() { option = element.find('option').eq(0); expect(option.text()).toBe('A'); }); + + + it('should not throw when a directive compiles the blank option before ngOptions is linked', function() { + expect(function() { + createSelect({ + 'o-compile-contents': '', + 'name': 'select', + 'ng-model': 'value', + 'ng-options': 'item for item in items' + }, true); + }).not.toThrow(); + + expect(linkLog).toEqual(['linkCompileContents', 'linkNgOptions']); + }); + + + it('should not throw with a directive that replaces', inject(function($templateCache, $httpBackend) { + $templateCache.put('select_template.html', ''); + + scope.options = ['a', 'b', 'c', 'd']; + + expect(function() { + element = $compile('')(scope); + scope.$digest(); + }).not.toThrow(); + + dealoc(element); + })); + });