Nested iso transcludes #7499

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
5 participants
Member

petebacondarwin commented May 17, 2014

Fixes #1809

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#7499)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request May 17, 2014

Member

petebacondarwin commented May 17, 2014

Things are never as easy as they seem.

  • First, ngIf and ngRepeat want to create their own child scopes so that they can destroy them later. But they were using their current scope rather than the scope that is bound to $transclude. I updated these to create scopes from the transclusionScope exposed on the $transclude function.
  • Second, ngSwitch doesn't seem to fail for a similar test even though it appears to be creating child scopes based on the isolated scope.
  • Third, ngInclude fails for a similar test, but the fix in this PR doesn't seem to help.

I think that the second and third points may be related to both directives using transclude: 'element' rather than simple transclusion but my brain has gone a bit soft for the rest of today.

src/ng/directive/ngIf.js
@@ -89,7 +89,7 @@ var ngIfDirective = ['$animate', function($animate) {
if (toBoolean(value)) {
if (!childScope) {
- childScope = $scope.$new();
+ childScope = $transclude.transclusionScope.$new();
@IgorMinar

IgorMinar May 19, 2014

Owner

this doesn't look right

@petebacondarwin

petebacondarwin May 19, 2014

Member

We need a way for directives to be able to create a new scope that is the child of the correct scope. $transclude is bound to a scope that may be different to the scope of the directive.

For instance if the ng-if is in a template of a directive that declares isolated scope, but the transclusion comes from outside the isolated scope. In this case we should not be using a child of $scope.

E.g.

If you have a directive, iso, that transcludes and has isolated scope:

<iso><span ng-bind="val"></span></iso>

where the template of iso contains:

<div ng-if="true"><div ng-transclude></div></div>

Then the transcluded content will be a child of $rootScope but the scope of the ng-if is a child of the isolated scope. So you cannot use the scope of ng-if when you want to link, clone and inject the transcluded content.

The $transclude function in ng-if is bound to the correct scope so if we use that directly then all is well, except in the case of ng-if and other directives that want to add and remove copies of the transcluded content, we want to create a new scope that we can destroy later.

I thought about creating new child scopes of the transclude scope if we are cloning the transclude, which would give us the correct scope but we need a way to then reference this scope in order to be able to destroy it later.

@petebacondarwin

petebacondarwin May 19, 2014

Member

OK, I just realised that if you are cloning the transclude then a new scope is defined that is bound correctly and that it also sets up an event handler to destroy the scope when the element gets destroyed so this fixes this problem. We just remove passing in a scope to the $transclude function altogether.

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request May 19, 2014

Member

petebacondarwin commented May 19, 2014

This new set of commits fixes this more cleanly but there are still problems when directives transclude element (or possibly when they async replace their element).

src/ng/directive/ngIf.js
- childScope = $scope.$new();
- $transclude(childScope, function (clone) {
+ if (!block ) {
+ $transclude(function (clone) {
@petebacondarwin

petebacondarwin May 19, 2014

Member

Now we don't pass in a scope but leave $transclude to make a new (correctly bound) scope for us.

-
- if (!block.scope) {
- $transclude(childScope, function(clone) {
+ $transclude(function(clone, scope) {
@petebacondarwin

petebacondarwin May 19, 2014

Member

Similarly we don't need to pass in a scope here now either. The cloneAttachFn kindly provides us with the new scope so we can update it with $index, etc

+ expect(trim(element.text())).toEqual('transcluded content');
+ dealoc(element);
+ });
+ });
@petebacondarwin

petebacondarwin May 19, 2014

Member

This test doesn't fail even without fixing the code. I think I am probably testing the wrong thing here.
Maybe the iso directive needs to go inside the ng-switch-when directive...

+ $httpBackend.flush();
+ expect(trim(element.text())).toEqual('transcluded content');
+ });
+ });
@petebacondarwin

petebacondarwin May 19, 2014

Member

This test correctly fails without a fix but also fails with the fix I thought was correct

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this pull request May 22, 2014

@petebacondarwin Do we have tests/plunkers to prove that 1809 and 7499 are fixed by this?

Also, I don't understand how this fixes the issue we were debugging this morning (vojtajina/angular.js@4d152c8)

Btw, I would do the var renaming in a different commit;-)

I guess you meant this solves the failing tests we had after making vojtajina/angular.js@4d152c8 passing, right? That makes sense...

petebacondarwin added a commit that referenced this pull request May 23, 2014

@btford btford modified the milestones: 1.3.0-beta.11, 1.3.0-beta.10 May 23, 2014

vojtajina added a commit to vojtajina/angular.js that referenced this pull request May 28, 2014

vojtajina added a commit to vojtajina/angular.js that referenced this pull request May 28, 2014

@vojtajina vojtajina referenced this pull request May 29, 2014

Merged

PR 7565 fixed #7610

vojtajina added a commit to vojtajina/angular.js that referenced this pull request May 29, 2014

caitp added a commit to caitp/angular.js that referenced this pull request Jun 13, 2014

petebacondarwin added a commit that referenced this pull request Jun 13, 2014

@malixsys malixsys referenced this pull request in akoenig/angular-deckgrid Aug 19, 2014

Open

ng-if doesn't work inside child element template #44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment