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

fix($compile): link async+replace element transclusion directives with comment element #6101

Closed
wants to merge 1 commit into from

Conversation

caitp
Copy link
Contributor

@caitp caitp commented Feb 3, 2014

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.

replaceWith(linkRootElement, jqLite(beforeTemplateLinkNode), linkNode);

// Copy in CSS classes from original node
safeAddClass(jqLite(linkNode), oldClasses);
if (isDefined(oldClasses)) {
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 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') {
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 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.

@rodyhaddad
Copy link
Contributor

Shouldn't you port hasElementTranscludeDirective from the previousCompileContext, similar to other variables in the function?

hasElementTranscludeDirective seems to have two roles:

  • 1. preventing controllers from being added to the .data of comment nodes (jQuery doesn't allow it)
  • 2. something related to the controller array that gets passed to a directive's link function

@caitp
Copy link
Contributor Author

caitp commented Feb 6, 2014

@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
@IgorMinar
Copy link
Contributor

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

@caitp
Copy link
Contributor Author

caitp commented Feb 7, 2014

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.

@tbosch tbosch modified the milestones: 1.2.13, 1.2.12 Feb 7, 2014
@caitp
Copy link
Contributor Author

caitp commented Feb 10, 2014

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

@caitp caitp closed this in e7338d3 Feb 10, 2014
khepin pushed a commit to khepin/angular.js that referenced this pull request Feb 19, 2014
… 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
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.

ngRepeat + directive w/ templateUrl + ngIf = printed twice
4 participants