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

fix(select): handle corner case of adding options via a custom directive #13878

Closed
wants to merge 3 commits into from
Closed

fix(select): handle corner case of adding options via a custom directive #13878

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f0e1eb0
fix(select): handle corner case of adding options via a custom directive
gkalpak Jan 28, 2016
359039c
fixup 1
gkalpak Jan 29, 2016
4691a8f
fixup 2
gkalpak Jan 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/ng/directive/select.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ var SelectController =

// Tell the select control that an option, with the given value, has been added
self.addOption = function(value, element) {
// Skip comment nodes, as they only pollute the `optionsMap`
if (element.prop('nodeType') === NODE_TYPE_COMMENT) return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

element[0].nodeType?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my first "reaction", but then I thought: "What if element[0] is undefined ?"
So, instead of element[0] && element[0].nodeType, I went for element.prop(...) (I'm not even sure if the element passed to the linking function can be empty...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it can't. Fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AddOption is a private Api so we guarantee that element will always be jq
wrapped. I'm a bit worried about the overhead of calling prop on every
option.
Am 29.01.2016 13:28 schrieb "Georgios Kalpakas" [email protected]:

In src/ng/directive/select.js
#13878 (comment):

@@ -80,6 +80,9 @@ var SelectController =

// Tell the select control that an option, with the given value, has been added
self.addOption = function(value, element) {

  • // Skip comment nodes, as they only pollute the optionsMap
  • if (element.prop('nodeType') === NODE_TYPE_COMMENT) return;

That was my first "reaction", but then I thought: "What if element[0] is
undefined ?"
So, instead of element[0] && element[0].nodeType, I went for
element.prop(...) (I'm not even sure if the element passed to the linking
function can be empty...)

—
Reply to this email directly or view it on GitHub
https://github.com/angular/angular.js/pull/13878/files#r51255240.


assertNotHasOwnProperty(value, '"option value"');
if (value === '') {
self.emptyOption = element;
Expand Down Expand Up @@ -452,7 +455,6 @@ var optionDirective = ['$interpolate', function($interpolate) {
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 interpolateValueFn = $interpolate(attr.value, true);
Expand All @@ -466,7 +468,6 @@ var optionDirective = ['$interpolate', function($interpolate) {
}

return function(scope, element, attr) {

// This is an optimization over using ^^ since we don't want to have to search
// all the way to the root of the DOM for every single option element
var selectCtrlName = '$selectController',
Expand Down
29 changes: 27 additions & 2 deletions test/ng/directive/selectSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,17 @@ describe('select', function() {
}
};
});

$compileProvider.directive('myOptions', function() {
return {
scope: {myOptions: '='},
replace: true,
template:
'<option value="{{ option.value }}" ng-repeat="option in myOptions">' +
'{{ options.label }}' +
'</option>'
};
});
}));

beforeEach(inject(function($rootScope, _$compile_) {
Expand Down Expand Up @@ -313,7 +324,7 @@ describe('select', function() {
});


it('should cope with a dynamic empty option added to a static empty option', function() {
it('should cope with a dynamic empty option added to a static empty option', function() {
scope.dynamicOptions = [];
scope.robot = 'x';
compile('<select ng-model="robot">' +
Expand All @@ -340,7 +351,7 @@ describe('select', function() {
scope.dynamicOptions = [];
scope.$digest();
expect(element).toEqualSelect(['']);
});
});

it('should select the empty option when model is undefined', function() {
compile('<select ng-model="robot">' +
Expand Down Expand Up @@ -611,6 +622,20 @@ describe('select', function() {

});


it('should not break when adding options via a directive with `replace: true` '
+ 'and a structural directive in its template',
function() {
scope.options = [
{value: '1', label: 'Option 1'},
{value: '2', label: 'Option 2'},
{value: '3', label: 'Option 3'}
];
compile('<select ng-model="mySelect"><option my-options="options"></option></select>');

expect(element).toEqualSelect([unknownValue()], '1', '2', '3');
}
);
});


Expand Down