fix($compile): link async+replace element transclusion directives with comment element #6101
Conversation
| replaceWith(linkRootElement, jqLite(beforeTemplateLinkNode), linkNode); | ||
|
|
||
| // Copy in CSS classes from original node | ||
| safeAddClass(jqLite(linkNode), oldClasses); | ||
| if (isDefined(oldClasses)) { |
There was a problem hiding this comment.
I just realized that oldClasses could be undefined if the linkNode is a comment, that's why this is changed.
| @@ -1685,6 +1685,12 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { | |||
| } | |||
| directives = templateDirectives.concat(directives); | |||
| mergeTemplateAttributes(tAttrs, tempTemplateAttrs); | |||
| for (var i=0, ii=directives.length; i<ii; ++i) { | |||
| if (directives[i].transclude === 'element') { | |||
There was a problem hiding this comment.
I don't expect iterating over this list again to have a huge performance impact, but it's theoretically possible... Also, I only really want to set replaceTransclude to true if it's in the root of the template... I don't know if that's possible.
|
Shouldn't you port
|
|
@rodyhaddad to my knowledge, the previousCompileContext would refer to the compile context from the parent node, so it should not be necessary. Otherwise we probably would have been doing that to begin with. |
…h comment element Previously, element transclusion directives would not receive a comment node in their link functions when they were the root of an asynchronous replace template. This would cause duplicate elements to appear in an ng-if expression within ng-repeat. Closes angular#6006
|
just for the record I think that @rodyhaddad is right. the previousCompileContext is used in cases when we need to stop compilation and resume it later on the same node. it's heavily used when we do node replacement. And also in async directives where we resume compilation after template arrives. the previousCompileContext is a change I introduced fairly late in the 1.2 sprint back then I did feel that there is potential to have more variables preserved via the context but didn't want to mess with the compiler much. honestly that code is the scariest code I've ever worked with. we need to redo it from scratch, but that's not going to happen until v2 |
|
I agree =) I have made the change though and tests are passing locally, so I guess it hasn't hurt to use the stored value, as far as I can tell. |
|
Since review comments have been addressed and I've been asked to merge this today, I'm going to do that... However I worry that it might need to be reverted. If any problems come up with other apps, please revert and notify me so that I can try to fix them |
… comment element
This corrects a complicated compiler issue, described in detail below:
Previously, if an element transclusion directive contained an asynchronous directive whose template
contained another element transclusion directive, the inner element transclusion directive would be
linked with the element, rather than the expected comment node.
An example manifestation of this bug would look like so:
```html
<div ng-repeat="i in [1,2,3,4,5]">
<div my-directive>
</div>
</div>
```
`my-directive` would be a replace directive, and its template would contain another element
transclusion directive, like so:
```html
<div ng-if="true">{{i}}</div>
```
ngIf would be linked with this template content, rather than the comment node, and the template element
would be attached to the DOM, rather than the comment. As a result, this caused ng-if to duplicate the
template when its expression evaluated to true.
Closes angular#6006
Closes angular#6101
Previously, element transclusion directives would not receive a comment node
in their link functions when they were the root of an asynchronous replace
template. This would cause duplicate elements to appear in an ng-if expression
within ng-repeat.
Closes #6006
WARNING: This patch needs some serious dog-fooding, and very careful review.
@IgorMinar, please take a look when you have some time for this.