Nested transclusion issues #7565
Conversation
If you have two directives that both expect to receive transcluded content the outer directive works but the inner directive never receives a transclusion function. This only failed if the first transclude directive was not the first directive found in compilation. Handles the regression identified in e994259 Fixes angular#7240 Closes angular#7387
The boundTransclusionFn that is passed in is really the one from the parent node. The change to parentBoundTranscludeFn clarifies this compared to the childBoundTranscludeFn.
|
Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
| expect(function() { | ||
| elm = $compile('<div trans-foo>content</div>')($rootScope); | ||
| }).toThrowMinErr('ngTransclude', 'orphan', | ||
| 'Illegal use of ngTransclude directive in the template! No parent directive that requires a transclusion found. Element: <div class="bar" ng-transclude="">'); |
There was a problem hiding this comment.
What happens if a child of noTransBar template wants a transclude function? seems like you'd want it to not throw in that case
There was a problem hiding this comment.
ng-transclude can be only used in template that belongs to a directive with transclude: true. Thus a child of <div no-trans-bar> could get the transclusion (because it would be inside the template of trans-foo which does transclude). It would however not be used, because no-trans-bar has a template which override the content of <div no-trans-bar>.
…rective If a directive provides a template but is not explicitly requesting transclusion then the compiler should not pass a transclusion function to the directives within the template.
| // jshint bitwise: false | ||
| scope.$odd = !(scope.$even = (index&1) === 0); | ||
| // jshint bitwise: true | ||
| }; |
There was a problem hiding this comment.
@vojtajina - This function was originally just a block of code. It could still be as it is only called once. Should we revert that, since I guess that creating a function in here is a performance cost?
There was a problem hiding this comment.
Yep, inlining might be better.
There was a problem hiding this comment.
There is more important issue however - this is the reason why the google internal tests are failing (it's a regression in Angular).
The ng-repeat state is set AFTER the transclusion is linked. That is wrong. It's pretty scary that we don't have any test for this (all the existing tests assert through binding which eventually contains the value even if the state is set later). This branch shows the problem (failing test) including possible fix (the test is passing): https://github.com/vojtajina/angular.js/tree/pr-7565-ng-repeat-state-set-after-transclusion-issue
|
|
||
| // If there is a previous boundTransclude function and it has a transclusionScope then | ||
| // use this instead of the current scope | ||
| scope = previousBoundTranscludeFn && previousBoundTranscludeFn.transclusionScope || scope; |
There was a problem hiding this comment.
I'm afraid this fix is wrong. See this failing test: https://github.com/vojtajina/angular.js/tree/pr-7565-incorrect-transclusion-scope
I need to investigate this more to figure out some better way how to fix this.
There was a problem hiding this comment.
I pushed a fix into the https://github.com/vojtajina/angular.js/tree/pr-7565-incorrect-transclusion-scope branch.
|
I fixed both problems in https://github.com/vojtajina/angular.js/tree/pr-7565-fixed. |
|
Fixed by #7610 |
This series of commits deals with a number of issues arising from the compiler not passing the correct transclusion functions and scopes down to children.
There are potentially still issues with ng-include and ng-switch when they are combined with isolated scopes.