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

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.registerOption()` method, which is then overridden by
the `ngOptions` directive as a `noop`.

Fixes #11685
Closes #12972
Closes #12968
Closes #13012
  • Loading branch information
@Narretz @petebacondarwin
Narretz authored and petebacondarwin committed Oct 6, 2015
1 parent 256d9a9 commit 7b2ecf4
Show file tree
Hide file tree
Showing 3 changed files with 146 additions and 60 deletions.
19 changes: 13 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,6 @@ 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) {

// if ngModel is not defined, we don't need to do anything
var ngModelCtrl = ctrls[1];
Expand Down Expand Up @@ -451,7 +446,6 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
unknownOption.remove();
};


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

Expand Down Expand Up @@ -729,7 +723,20 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
}

}
}

return {
restrict: 'A',
terminal: true,
require: ['select', '?ngModel'],
link: {
pre: function ngOptionsPreLink(scope, selectElement, attr, ctrls) {
// Deactivate the SelectController.register method to prevent
// option directives from accidentally registering themselves
// (and unwanted $destroy handlers etc.)
ctrls[0].registerOption = 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.registerOption = function(optionScope, 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
optionScope.$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.registerOption(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 @@ -2110,6 +2157,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 7b2ecf4

Please sign in to comment.