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

fix(ngOptions) skip authentic empty options when looking for options #13033

Closed
wants to merge 1 commit into from
Closed

fix(ngOptions) skip authentic empty options when looking for options #13033

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 11 additions & 2 deletions src/ng/directive/ngOptions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
}
}

var providedEmptyOption = !!emptyOption;
var providedEmptyOption = emptyOption;

var unknownOption = jqLite(optionTemplate.cloneNode(false));
unknownOption.val('?');
Expand Down Expand Up @@ -618,10 +618,19 @@ var ngOptionsDirective = ['$compile', '$parse', function($compile, $parse) {
var emptyOption_ = emptyOption && emptyOption[0];
var unknownOption_ = unknownOption && unknownOption[0];

// If the compiled empty option was replaced by a comment because
// it had an "element" transclusion directive on it (such as ngIf)
// then compare against the pre-compiled empty element instead
if (emptyOption && emptyOption_.nodeType === NODE_TYPE_COMMENT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can actually remove this whole block then, can't we? (the tests pass without it)

Copy link
Contributor

Choose a reason for hiding this comment

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

Because of providedEmptyOption = emptyOption, providedEmptyOption is also the compiled result. But the added checks below do the trick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess so. As long as there is not some crazy replace directive on the option :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Even that works in its simplest form, I just tested it. :) This case is probably very rare anyway, because having an option that replaces itself with an option takes this whole issue to strange new places!

ShallI remove the block and commit?

Copy link
Member

Choose a reason for hiding this comment

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

If this block is removed, don't forget to also undo the change on line 411.

emptyOption_ = providedEmptyOption[0];
}

if (emptyOption_ || unknownOption_) {
while (current &&
(current === emptyOption_ ||
current === unknownOption_)) {
current === unknownOption_ ||
current.nodeType === NODE_TYPE_COMMENT ||
current.value === '')) {
current = current.nextSibling;
}
}
Expand Down
46 changes: 46 additions & 0 deletions test/ng/directive/ngOptionsSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2144,6 +2144,52 @@ describe('ngOptions', function() {
});


it('should be possible to use ngIf in the blank option', function() {
var option;
createSingleSelect('<option ng-if="isBlank" value="">blank</option>');

scope.$apply(function() {
scope.values = [{name: 'A'}];
scope.isBlank = true;
});

expect(element.find('option').length).toBe(2);
option = element.find('option').eq(0);
expect(option.val()).toBe('');
expect(option.text()).toBe('blank');

scope.$apply(function() {
scope.isBlank = false;
});

expect(element.find('option').length).toBe(1);
option = element.find('option').eq(0);
expect(option.text()).toBe('A');
});


it('should be possible to use ngIf in the blank option when values are available upon linking',
function() {
var options;

scope.values = [{name: 'A'}];
createSingleSelect('<option ng-if="isBlank" value="">blank</option>');

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

options = element.find('option');
expect(options.length).toBe(2);
expect(options.eq(0).val()).toBe('');
expect(options.eq(0).text()).toBe('blank');

scope.$apply('isBlank = false');

options = element.find('option');
expect(options.length).toBe(1);
expect(options.eq(0).text()).toBe('A');
}
);

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