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

ngRepeat + directive w/ templateUrl + ngIf = printed twice #6006

Closed
elado opened this issue Jan 27, 2014 · 12 comments
Closed

ngRepeat + directive w/ templateUrl + ngIf = printed twice #6006

elado opened this issue Jan 27, 2014 · 12 comments
Assignees
@caitp
Labels
component: $compile component: ngRepeat frequency: moderate needs: investigation type: bug
Milestone
1.2.13

Comments

@elado
Copy link

elado commented Jan 27, 2014

A really weird case of an ng-repeat, a directive with an external template url and ng-if on root element leads to printing twice every directive.

Happens only with this combination.
Doesn't happen with regular template string, or if the ng-if not on the root element or not in an ng-repeat.

Source:

index.html

<!DOCTYPE html>
<html ng-app="app">
  <head>
    <link rel="stylesheet" href="style.css">
    <script src="https://ajax.googleapis.com/ajax/libs/angularjs/1.2.10/angular.min.js"></script>
    <script src="script.js"></script>
  </head>

  <body>
    <div ng-repeat="i in [1,2,3]">
      {{i}}
      <hello></hello>
    </div>
  </body>
</html>

script.js

app = angular.module("app", []);

app.directive('hello', function () {
  return {
    restrict: 'E',
    replace: true,
    templateUrl: "hello.html",
    //template: '<div ng-if="true">Printed once as exptected</div>'
  }
});

hello.html

<div ng-if="true">Printed twice, but should not</div>

Or here:

http://plnkr.co/edit/DmyCkta0G0mjzgMJvchk?p=preview
@caitp
Copy link
Contributor

caitp commented Jan 27, 2014

Weird! I'll take a look at this... It doesn't seem to be a regression (without bisecting, but it's present in every release since ngIf was introduced). So, thanks for catching this!

@caitp
Copy link
Contributor

caitp commented Jan 27, 2014

I'm having trouble reproducing this exactly without ngRepeat (the way it works for you with a synchronous template, but not with an asynchronous template). It's very peculiar. I think it might be sort-of related to how transclude: "element" works, but maybe some of the other folks are aware of it already and might have more of an idea. I'll dig into this more tomorrow if I'm not busy with other stuff.

@elado
Copy link
Author

elado commented Jan 27, 2014

I should add that replace: true is also to blame.

@caitp
Copy link
Contributor

caitp commented Jan 27, 2014

Yeah, I just mean reproducing this in isolation from ngRepeat is a bit tricky... But I don't think it's really an issue with ngRepeat or ngIf, it's a bug with the compiler.

...OK, working reproduction without any builtins except ngIf http://plnkr.co/edit/diLpqXPHrDAjTzOVGH1L?p=preview

... The reason seems to be that the ngIfDirective's element is not the comment node which it should be expecting. Hm

@ghost ghost assigned IgorMinar Jan 28, 2014
@IgorMinar
Copy link
Contributor

yeah, this looks like a valid issue in the compiler.

@caitp can you please look into this? I can help if needed.

@ghost ghost assigned caitp Jan 28, 2014
@caitp
Copy link
Contributor

caitp commented Jan 28, 2014

Yeah, absolutely

@caitp
Copy link
Contributor

caitp commented Jan 28, 2014

It seems that applyDirectivesToNode() gets called more times than it should be for asynchronous directives with replace: true, it gets called once with the "compiled" template, and then again when the replace template is merged into the compiled template.

For non-asynchronous replace directives, applyDirectivesToNode() is only called once on the compiled template, and not on the merged template.

I don't know if that's necessarily the issue, but it strikes me as an inconsistency worth investigating :> I'm not sure what the right criteria would be to tell it not to call applyDirectivesToNode(), though, or what to replace the afterTemplateChildLinkFn object with.

@caitp
Copy link
Contributor

caitp commented Jan 28, 2014

hmmm I'm wrong, I don't think that's the issue with this. still poking through it.

@caitp
Copy link
Contributor

caitp commented Jan 30, 2014

So, this is a tricky one to debug, I've been trying to generate a call graph by hand just to get a better overview of the difference between synchronous vs asynchronous behaviour.

http://pygments.org/demo/223495/ << this is a diff, with good.txt being synchronous behaviour, and bad.txt being asynchronous behaviour. Of course I"m not logging every possible thing that's happening (having good static analysis tools in javascript would be really nice!)

So, from the diff it's fairly easy to see what's happening, the child link functions would be called with the comment node in a synchronous mode, but are instead being called with a merged template of element nodes. I feel like changing this would probably break a lot of stuff, but it's worth a shot

@tbosch tbosch modified the milestones: 1.2.12, 1.2.11 Feb 3, 2014
caitp added a commit to caitp/angular.js that referenced this issue Feb 3, 2014
@caitp
fix($compile): link async+replace element transclusion directives wit…
2ae678f 
…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
@caitp caitp mentioned this issue Feb 3, 2014
Closed
caitp added a commit to caitp/angular.js that referenced this issue Feb 3, 2014
@caitp
fix($compile): link async+replace element transclusion directives wit…
b2b9ef1 
…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
caitp added a commit to caitp/angular.js that referenced this issue Feb 3, 2014
@caitp
fix($compile): link async+replace element transclusion directives wit…
daa4059 
…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
caitp added a commit to caitp/angular.js that referenced this issue Feb 3, 2014
@caitp
fix($compile): link async+replace element transclusion directives wit…
48fa08b 
…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
@caitp caitp mentioned this issue Feb 4, 2014
Closed
caitp added a commit to caitp/angular.js that referenced this issue Feb 4, 2014
@caitp
fix($compile): link async+replace element transclusion directives wit…
0d36cf9 
…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
caitp added a commit to caitp/angular.js that referenced this issue Feb 7, 2014
@caitp
fix($compile): link async+replace element transclusion directives wit…
4fc523d 
…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
caitp added a commit to caitp/angular.js that referenced this issue Feb 7, 2014
@caitp
fix($compile): link async+replace element transclusion directives wit…
2e892b4 
…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
@tbosch tbosch modified the milestones: 1.2.13, 1.2.12 Feb 7, 2014
@caitp caitp closed this as completed in e7338d3 Feb 10, 2014
@caitp
Copy link
Contributor

caitp commented Feb 10, 2014

Holy cow, lots of mentions to this issue, sorry about that ;)

Anyways, this is fixed, it would be super helpful if you could help verify the solution just to make sure it doesn't break anything else. I know you have other things to do, but you know.

khepin pushed a commit to khepin/angular.js that referenced this issue Feb 19, 2014
@caitp
fix($compile): ensure element transclusion directives are linked with…
ea11fb2 
… 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
@joeltello joeltello mentioned this issue Sep 2, 2014
Closed
@Foxandxss Foxandxss mentioned this issue Sep 2, 2014
Closed
@cindyloo
Copy link

I'm seeing this using a third-party directive - the angularjs multiselect. anything I should do? I did hack it to use only the template vs the templateurl and it did stop the repetition but kind of broke the widget...

@thatkookooguy
Copy link

I still see some problems with this scenario. I have this when using angular multiselect, just like @cindyloo .

http://plnkr.co/edit/0KpEwgW8P2p00UfTYH6e?p=preview

Also, when I tried the plunkr with an updated version of angular, I get a different bug:

the output is

1
2
3
Printed twice, but should not
Printed twice, but should not
Printed twice, but should not

but the expected output should be:

1
Printed twice, but should not
2
Printed twice, but should not
3
Printed twice, but should not

@thatkookooguy thatkookooguy mentioned this issue Mar 27, 2016
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@caitp caitp

Labels
component: $compile component: ngRepeat frequency: moderate needs: investigation type: bug
Projects
None yet
Milestone
1.2.13
6 participants
@elado @IgorMinar @cindyloo @tbosch @caitp @thatkookooguy