Skip to content

Commit

Permalink
fix(ngOptions): explicitly override selectCtrl default option handling
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Narretz committed Oct 1, 2015
1 parent e51174b commit 45eb52f
Show file tree
Hide file tree
Showing 3 changed files with 105 additions and 10 deletions.
17 changes: 11 additions & 6 deletions src/ng/directive/ngOptions.js
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down Expand Up @@ -448,7 +444,6 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
unknownOption.remove();
};


// Update the controller methods for multiple selectable options
if (!multiple) {

Expand Down Expand Up @@ -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
}
};
}];
20 changes: 17 additions & 3 deletions src/ng/directive/select.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
}];

/**
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -378,7 +393,6 @@ var selectDirective = function() {

}
}
};
};


Expand Down Expand Up @@ -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
Expand Down
78 changes: 77 additions & 1 deletion test/ng/directive/ngOptionsSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

describe('ngOptions', function() {

var scope, formElement, element, $compile;
var scope, formElement, element, $compile, linkLog;

function compile(html) {
formElement = jqLite('<form name="form">' + html + '</form>');
Expand Down Expand Up @@ -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_;
Expand Down Expand Up @@ -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', '<select ng-options="option as option for option in selectable_options"> <option value="">This is a test</option> </select>');

scope.options = ['a', 'b', 'c', 'd'];

expect(function() {
element = $compile('<custom-select ng-model="value" options="options"></custom-select>')(scope);
scope.$digest();
}).not.toThrow();

dealoc(element);
}));

});


Expand Down

0 comments on commit 45eb52f

Please sign in to comment.