angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.8k stars 27.48k forks source link

ng-scope class is set on root DOM element linked with public linking function (returned by $compile()) even if directive does not create new scope #1633

Open danilsomsikov opened 11 years ago

danilsomsikov commented 11 years ago

See http://jsfiddle.net/danilsomsikov/aEpkh/

Original fiddle with transclusion function http://jsfiddle.net/danilsomsikov/mMWyU/4/

pkozlowski-opensource commented 11 years ago

@danilsomsikov this works as designed. In fact a directive can create 2 sibling scopes as explained in http://docs.angularjs.org/guide/directive (check the "Understanding Transclusion and Scopes" section).

BTW: there was a nice article published just few days back that also touches on problem of transclusion: http://blog.omkarpatil.com/2012/11/transclude-in-angularjs.html

petebacondarwin commented 11 years ago

Is this not because the transclusion itself creates a new scope?

On 30 November 2012 15:48, Danil Somsikov notifications@github.com wrote:

See http://jsfiddle.net/danilsomsikov/mMWyU/4/

— Reply to this email directly or view it on GitHubhttps://github.com/angular/angular.js/issues/1633.

danilsomsikov commented 11 years ago

But as you can see in fiddle there's only one scope created, however there're two elements having css class ng-scope.

pkozlowski-opensource commented 11 years ago

@danilsomsikov oh, sorry, didn't notice that both scopes got the same ID, I was sure that we are talking about this case: http://jsfiddle.net/GqqmS/4/

Anyway, I saw already an issue opened for those spans: https://github.com/angular/angular.js/issues/1293 There was a thread about is well: https://groups.google.com/forum/?fromgroups=#!msg/angular/2rPL2lqdCho/QU37wXUPyxoJ

Comments from Misko were:

You can thank microsoft for the spans. :-) So in IE we have this issue that we can't attach data to text content. We solve this by wrapping the text in spans. And that is what you see. Simply get rid of whitespace in your markup and spans will disappear with it. This is a bug, so you should file it, although I am not sure we can fix it without breaking IE.

On a related note, i think maybe you should do it differently. Try using the compile function to manually change the DOM structure. The template property is a short hand for compile and it looks like you have reached the end of what it can do.

So I guess it is the best to comment on your case in https://github.com/angular/angular.js/issues/1293 as this one seems to be duplicate of https://github.com/angular/angular.js/issues/1293.

danilsomsikov commented 11 years ago

Spans are not the case as well. I don't know why did I put it to the sample code. The problems persist without any spans. See http://jsfiddle.net/danilsomsikov/mMWyU/5/ Directive element has ng-scope class on it, which it should not have.

pkozlowski-opensource commented 11 years ago

@danilsomsikov Danil, thnx for providing more info, this starts to look interesting now. So this is not a new transcusion scope nor additional spans and I can't explain why this scope class is added without digging more into the code.

Re-opening this one, probably people more familiar with $compile should look into it.

danilsomsikov commented 11 years ago

The class is added at this line.

petebacondarwin commented 11 years ago

ng-scope is also added in applyDirectivesToNode() on line 558: https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L558

danilsomsikov commented 11 years ago

Yes, however it is not the case in the issue I report.

petebacondarwin commented 11 years ago

Yes you are right!

So at https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L585 you can see that the transclude function is created by a call to compile(). It is this call to compile that applies the ng-scope.

Now I guess the assumption is that if you are transcluding then you are going to create a new child scope for the transclusion, i.e. scope.$new(...). You are not doing this. You are just passing the directive's original scope into the transclusion function.

But then you are not really getting much use out of transclude in this case - you could just let the compiler pass down to the child elements itself.

I guess that you real use case is doing something more clever. But if it is then you should probably be creating a new scope and then being responsible for destroying it later, if necessary.

petebacondarwin commented 11 years ago

http://plnkr.co/edit/k6qR7KCw4GJ1X2EA375w?p=preview

danilsomsikov commented 11 years ago

It turns that public linking function (the one returned by $compile(...)) is setting ng-scope CSS class and $scope data even if element does not create new scope.

btford commented 11 years ago

As part of our effort to clean out old issues, this issue is being automatically closed since it has been inactivite for over two months.

Please try the newest versions of Angular (1.0.8 and 1.2.0-rc.1), and if the issue persists, comment below so we can discuss it.

Thanks!

danilsomsikov commented 11 years ago

The issue hasn't been fixed in either version. See http://jsfiddle.net/aEpkh/3/ for 1.2.0-rc.1 and http://jsfiddle.net/aEpkh/2/ for 1.0.8

tb01923 commented 11 years ago

I think I am experiencing a similar issue Fiddle: http://jsfiddle.net/U5UxX/

petebacondarwin commented 11 years ago

Even slimmer version of example against "snapshot": http://jsfiddle.net/zPqPR/

petebacondarwin commented 11 years ago

The problem is that $compile isn't to know whether you passed in a new scope or reused one from elsewhere. It currently assumes that one would never call it with a reused scope. @danilsomsikov - What is your use case for not creating a new child scope when calling $compile?

danilsomsikov commented 11 years ago

If $compile doesn't know what kind of scope it gets, why would it set css class? I don't have particular use case, I found it when I was reading source code to study how compilation works.