Skip to content

Commit

Permalink
fix(ngOptions): override select option registration
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
register options on the selectCtrl since this may cause errors during the
ngOptions lifecycle.

This can happen in the following cases:

- there is a blank option below the select element, an ngModel
directive, an ngOptions directive and some other directive on the select
element, which compiles the children of the select
(i.e. the option elements) before ngOptions is has finished linking.

- there is a blank option below the select element, an ngModel
directive, an ngOptions directive and another directive, which uses
templateUrl and replace:true.

What happens is:
- the option directive is compiled and adds an element `$destroy` listener
that will call `ngModel.$render` when the option element is removed.
- when `ngOptions` processes the option, it removes the element, and
triggers the `$destroy` listener on the option.
- the registered `$destroy` listener calls `$render` on `ngModel`.
- $render calls `selectCtrl.writeValue()`, which accesses the `options`
object in the `ngOptions` directive.
- Since `ngOptions` has not yet completed linking the `options` has not
yet been defined and we get an error.

This fix moves the registration code for the `option` directive into the
`SelectController`, which can then be easily overridden by the `ngOptions`
directive as a `noop`.

Fixes angular#11685
Closes angular#12972
Closes angular#12968
  • Loading branch information
@Narretz @petebacondarwin
Narretz authored and petebacondarwin committed Oct 5, 2015
1 parent 51a27c0 commit 254586d
Show file tree
Hide file tree
Showing 3 changed files with 144 additions and 60 deletions.
17 changes: 11 additions & 6 deletions src/ng/directive/ngOptions.js
View file
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].register = noop;
},
post: ngOptionsPostLink
}
};
}];
109 changes: 56 additions & 53 deletions src/ng/directive/select.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,15 @@

var noopNgModelController = { $setViewValue: noop, $render: noop };

function chromeHack(optionElement) {
// Workaround for https://code.google.com/p/chromium/issues/detail?id=381459
// Adding an <option selected="selected"> element to a <select required="required"> should
// automatically select the new element
if (optionElement[0].hasAttribute('selected')) {
optionElement[0].selected = true;
}
}

/**
* @ngdoc type
* @name select.SelectController
Expand Down Expand Up @@ -77,6 +86,8 @@ var SelectController =
}
var count = optionsMap.get(value) || 0;
optionsMap.put(value, count + 1);
self.ngModelCtrl.$render();
chromeHack(element);
};

// Tell the select control that an option, with the given value, has been removed
Expand All @@ -98,6 +109,39 @@ var SelectController =
self.hasOption = function(value) {
return !!optionsMap.get(value);
};


self.register = function($scope, $optionElement, $optionAttrs, interpolateValueFn, interpolateTextFn) {

if (interpolateValueFn) {
// The value attribute is interpolated
var oldVal;
$optionAttrs.$observe('value', function valueAttributeObserveAction(newVal) {
if (isDefined(oldVal)) {
self.removeOption(oldVal);
}
oldVal = newVal;
self.addOption(newVal, $optionElement);
});
} else if (interpolateTextFn) {
// The text content is interpolated
$scope.$watch(interpolateTextFn, function interpolateWatchAction(newVal, oldVal) {
$optionAttrs.$set('value', newVal);
if (oldVal !== newVal) {
self.removeOption(oldVal);
}
self.addOption(newVal, $optionElement);
});
} else {
// The value attribute is static
self.addOption($optionAttrs.value, $optionElement);
}

$optionElement.on('$destroy', function() {
self.removeOption($optionAttrs.value);
self.ngModelCtrl.$render();
});
};
}];

/**
Expand Down Expand Up @@ -308,7 +352,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,37 +428,26 @@ var selectDirective = function() {

}
}
};
};


// The option directive is purely designed to communicate the existence (or lack of)
// of dynamically created (and destroyed) option elements to their containing select
// directive via its controller.
var optionDirective = ['$interpolate', function($interpolate) {

function chromeHack(optionElement) {
// Workaround for https://code.google.com/p/chromium/issues/detail?id=381459
// Adding an <option selected="selected"> element to a <select required="required"> should
// automatically select the new element
if (optionElement[0].hasAttribute('selected')) {
optionElement[0].selected = true;
}
}

return {
restrict: 'E',
priority: 100,
compile: function(element, attr) {

if (isDefined(attr.value)) {
// If the value attribute is defined, check if it contains an interpolation
var valueInterpolated = $interpolate(attr.value, true);
var interpolateValueFn = $interpolate(attr.value, true);
} else {
// If the value attribute is not defined then we fall back to the
// text content of the option element, which may be interpolated
var interpolateFn = $interpolate(element.text(), true);
if (!interpolateFn) {
var interpolateTextFn = $interpolate(element.text(), true);
if (!interpolateTextFn) {
attr.$set('value', element.text());
}
}
Expand All @@ -422,44 +461,8 @@ var optionDirective = ['$interpolate', function($interpolate) {
selectCtrl = parent.data(selectCtrlName) ||
parent.parent().data(selectCtrlName); // in case we are in optgroup

function addOption(optionValue) {
selectCtrl.addOption(optionValue, element);
selectCtrl.ngModelCtrl.$render();
chromeHack(element);
}

// Only update trigger option updates if this is an option within a `select`
// that also has `ngModel` attached
if (selectCtrl && selectCtrl.ngModelCtrl) {

if (valueInterpolated) {
// The value attribute is interpolated
var oldVal;
attr.$observe('value', function valueAttributeObserveAction(newVal) {
if (isDefined(oldVal)) {
selectCtrl.removeOption(oldVal);
}
oldVal = newVal;
addOption(newVal);
});
} else if (interpolateFn) {
// The text content is interpolated
scope.$watch(interpolateFn, function interpolateWatchAction(newVal, oldVal) {
attr.$set('value', newVal);
if (oldVal !== newVal) {
selectCtrl.removeOption(oldVal);
}
addOption(newVal);
});
} else {
// The value attribute is static
addOption(attr.value);
}

element.on('$destroy', function() {
selectCtrl.removeOption(attr.value);
selectCtrl.ngModelCtrl.$render();
});
if (selectCtrl) {
selectCtrl.register(scope, element, attr, interpolateValueFn, interpolateTextFn);
}
};
}
Expand Down
78 changes: 77 additions & 1 deletion test/ng/directive/ngOptionsSpec.js
View file
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 254586d

Please sign in to comment.