fix(ngOptions): ignore comment nodes when removing 'selected' attribute #15459
Conversation
| if (self.emptyOption) { | ||
| // Empty options may be comment nodes if they have been removed by | ||
| // transclude directives such as `ngIf` | ||
| if (self.emptyOption && self.emptyOption[0].nodeType === NODE_TYPE_ELEMENT) { |
There was a problem hiding this comment.
Isn't there always going to be a comment there, even when ngIf is true?
Shouldn't we look for the first non-comment element in self.emptyOption?
There was a problem hiding this comment.
That's right. That means the whole adding / removing "selected" attribute is currently unsound when ngIf / a transclusion directive is used.
and it's not as easy as looking for the first non-comment element, because self.emptyOption is sometimes only the comment
|
I've updated the PR with an actual solution. Basically, we now require that emptyOption is always an actual option element. If the emptyOption is a comment after compilation, we use a modified version of the registerOption function on the select controller to add the empty option. |
| // 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) { |
There was a problem hiding this comment.
Is it worth just adding this behaviour to the normal registerOption rather than monkeypatching?
There was a problem hiding this comment.
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 ...
When the "empty option" element contains a transclusion directive, the result of the compilation always includes a comment node. Since we are adding / removing the "selected" attribute on the empty option, we need to make sure it's an actual element. To solve this, we take advantage of the fact the each option element has an option directive that tries to register the option with the selectController. With ngOptions, this registerOption function is normally noop'd since it's not possible to add dynamic options. Now if the result of the empty option compilation is a comment, we re-define the function so that it catches empty options when they are actually linked / rendered. Related angular#15454 Closes angular#15459
Narretz
force-pushed
the
fix-ngoptions-empty-ngif-master
branch
from
3159d01 to
d173dbb
Compare
When the "empty option" element contains a transclusion directive, the result of the compilation always includes a comment node. Since we are adding / removing the "selected" attribute on the empty option, we need to make sure it's an actual element. To solve this, we take advantage of the fact the each option element has an option directive that tries to register the option with the selectController. With ngOptions, this registerOption function is normally noop'd since it's not possible to add dynamic options. Now if the result of the empty option compilation is a comment, we re-define the function so that it catches empty options when they are actually linked / rendered. Closes angular#15454 Closes angular#15459
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
[ ] Docs have been added / updated (for bug fixes / features)Other information: