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

fix(ngOptions): ignore comment nodes when removing 'selected' attribute #15459

Merged
merged 1 commit into from
Dec 2, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 30 additions & 3 deletions src/ng/directive/ngOptions.js
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,7 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
// option when the viewValue does not match any of the option values.
for (var i = 0, children = selectElement.children(), ii = children.length; i < ii; i++) {
if (children[i].value === '') {
selectCtrl.hasEmptyOption = true;
selectCtrl.emptyOption = children.eq(i);
break;
}
Expand Down Expand Up @@ -556,9 +557,35 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
// compile the element since there might be bindings in it
$compile(selectCtrl.emptyOption)(scope);

// remove the class, which is added automatically because we recompile the element and it
// becomes the compilation root
selectCtrl.emptyOption.removeClass('ng-scope');
if (selectCtrl.emptyOption[0].nodeType === NODE_TYPE_COMMENT) {
// This means the empty option has currently no actual DOM node, probably because
// it has been modified by a transclusion directive.
selectCtrl.hasEmptyOption = false;

// Redefine the registerOption function, which will catch
// options that are added by ngIf etc. (rendering of the node is async because of
// lazy transclusion)
selectCtrl.registerOption = function(optionScope, optionEl) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth just adding this behaviour to the normal registerOption rather than monkeypatching?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Theoretically it already does, because without ngOptions, the option directive adds the empty option (via selectCtrl.registerOption -> selectCtrl.addOption). However, it does more work that we not actually need.
We also noop .registerOption in the ngOptions directive, so that we don't accidentially add options to the selectCtrl. So this fn is only relevant if there's a dynamic empty option.

Maybe we can refactor this later. It feels like there's always room to refactor select and ngOptions ...

if (optionEl.val() === '') {
selectCtrl.hasEmptyOption = true;
selectCtrl.emptyOption = optionEl;
selectCtrl.emptyOption.removeClass('ng-scope');
// This ensures the new empty option is selected if previously no option was selected
ngModelCtrl.$render();

optionEl.on('$destroy', function() {
selectCtrl.hasEmptyOption = false;
selectCtrl.emptyOption = undefined;
});
}
};

} else {
// remove the class, which is added automatically because we recompile the element and it
// becomes the compilation root
selectCtrl.emptyOption.removeClass('ng-scope');
}

}

selectElement.empty();
Expand Down
19 changes: 11 additions & 8 deletions src/ng/directive/select.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,14 @@ var SelectController =
// to create it in <select> and IE barfs otherwise.
self.unknownOption = jqLite(window.document.createElement('option'));

// The empty option is an option with the value '' that te application developer can
// provide inside the select. When the model changes to a value that doesn't match an option,
// it is selected - so if an empty option is provided, no unknown option is generated.
// However, the empty option is not removed when the model matches an option. It is always selectable
// and indicates that a "null" selection has been made.
self.hasEmptyOption = false;
self.emptyOption = undefined;

self.renderUnknownOption = function(val) {
var unknownVal = self.generateUnknownOptionValue(val);
self.unknownOption.val(unknownVal);
Expand All @@ -55,13 +63,6 @@ var SelectController =
if (self.unknownOption.parent()) self.unknownOption.remove();
};

// The empty option is an option with the value '' that te application developer can
// provide inside the select. When the model changes to a value that doesn't match an option,
// it is selected - so if an empty option is provided, no unknown option is generated.
// However, the empty option is not removed when the model matches an option. It is always selectable
// and indicates that a "null" selection has been made.
self.emptyOption = undefined;

self.selectEmptyOption = function() {
if (self.emptyOption) {
$element.val('');
Expand All @@ -70,7 +71,7 @@ var SelectController =
};

self.unselectEmptyOption = function() {
if (self.emptyOption) {
if (self.hasEmptyOption) {
self.emptyOption.removeAttr('selected');
}
};
Expand Down Expand Up @@ -132,6 +133,7 @@ var SelectController =

assertNotHasOwnProperty(value, '"option value"');
if (value === '') {
self.hasEmptyOption = true;
self.emptyOption = element;
}
var count = optionsMap.get(value) || 0;
Expand All @@ -148,6 +150,7 @@ var SelectController =
if (count === 1) {
optionsMap.remove(value);
if (value === '') {
self.hasEmptyOption = false;
self.emptyOption = undefined;
}
} else {
Expand Down
90 changes: 90 additions & 0 deletions test/ng/directive/ngOptionsSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2524,6 +2524,96 @@ describe('ngOptions', function() {
}
);


it('should select the correct option after linking when the ngIf expression is initially falsy', function() {
scope.values = [
{name:'black'},
{name:'white'},
{name:'red'}
];
scope.selected = scope.values[2];

expect(function() {
createSingleSelect('<option ng-if="isBlank" value="">blank</option>');
scope.$apply();
}).not.toThrow();

expect(element.find('option')[2]).toBeMarkedAsSelected();
expect(linkLog).toEqual(['linkNgOptions']);
});


it('should add / remove the "selected" attribute on empty option which has an initially falsy ngIf expression', function() {
scope.values = [
{name:'black'},
{name:'white'},
{name:'red'}
];
scope.selected = scope.values[2];

createSingleSelect('<option ng-if="isBlank" value="">blank</option>');
scope.$apply();

expect(element.find('option')[2]).toBeMarkedAsSelected();

scope.$apply('isBlank = true');
expect(element.find('option')[0].value).toBe('');
expect(element.find('option')[0]).not.toBeMarkedAsSelected();

scope.$apply('selected = null');
expect(element.find('option')[0].value).toBe('');
expect(element.find('option')[0]).toBeMarkedAsSelected();

scope.selected = scope.values[1];
scope.$apply();
expect(element.find('option')[0].value).toBe('');
expect(element.find('option')[0]).not.toBeMarkedAsSelected();
expect(element.find('option')[2]).toBeMarkedAsSelected();
});


it('should add / remove the "selected" attribute on empty option which has an initially truthy ngIf expression when no option is selected', function() {
scope.values = [
{name:'black'},
{name:'white'},
{name:'red'}
];
scope.isBlank = true;

createSingleSelect('<option ng-if="isBlank" value="">blank</option>');
scope.$apply();

expect(element.find('option')[0].value).toBe('');
expect(element.find('option')[0]).toBeMarkedAsSelected();

scope.selected = scope.values[2];
scope.$apply();
expect(element.find('option')[0]).not.toBeMarkedAsSelected();
expect(element.find('option')[3]).toBeMarkedAsSelected();
});


it('should add the "selected" attribute on empty option which has an initially falsy ngIf expression when no option is selected', function() {
scope.values = [
{name:'black'},
{name:'white'},
{name:'red'}
];

createSingleSelect('<option ng-if="isBlank" value="">blank</option>');
scope.$apply();

expect(element.find('option')[0]).not.toBeMarkedAsSelected();

scope.isBlank = true;
scope.$apply();

expect(element.find('option')[0].value).toBe('');
expect(element.find('option')[0]).toBeMarkedAsSelected();
expect(element.find('option')[1]).not.toBeMarkedAsSelected();
});


it('should not throw when a directive compiles the blank option before ngOptions is linked', function() {
expect(function() {
createSelect({
Expand Down