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 #15456

Merged

Conversation

Narretz
Copy link
Contributor

@Narretz Narretz commented Nov 30, 2016

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
fix (regression)

What is the current behavior? (You can also link to an open issue here)
When an empty option has ngIf on it in an ngOptions select, and the ngIf expression is false, then
an error is thrown, because we try to remove the "selected" attribute from the ngIf comment node.

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

Other information:
The logic is different in master, and the bug should not be happening there. The same bug exists on master

@petebacondarwin
Copy link
Contributor

This should fix #15454

@petebacondarwin
Copy link
Contributor

LGTM

@@ -452,6 +452,7 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
var removeEmptyOption = function() {
if (!providedEmptyOption) {
emptyOption.remove();
// Empty nodes with non-rendered ngIf can be comment nodes
Copy link
Contributor

Choose a reason for hiding this comment

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

it isn't only ngIf that would replace it with a comment, any element transclusion directive, sich as ngSwitch, ngRepeat etc. would also do it. So perhaps the comment should be something along the lines of

// Empty options may be comment nodes if they have been removed by
// transclude directives such as `ngIf`

@petebacondarwin
Copy link
Contributor

@Narretz - Please do land and forward port to 1.6

@Narretz Narretz force-pushed the fix-ngoptions-empty-ngif-rebased branch from fffc646 to 860cb61 Compare December 1, 2016 22:28
@Narretz Narretz merged commit 1d29c91 into angular:v1.5.x-rebased Dec 2, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants