Skip to content

Commit

Permalink
fix(ngInclude): only run anchorScroll after animation is done
Browse files Browse the repository at this point in the history
We need to wait until animations have added the content to the document before
trying to `autoscroll` to anchors that may have been inserted.

Fixes angular#4723
  • Loading branch information
petebacondarwin committed Nov 2, 2013
1 parent 117de8e commit 732ca80
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 7 deletions.
14 changes: 8 additions & 6 deletions src/ng/directive/ngInclude.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,13 @@ var ngIncludeDirective = ['$http', '$templateCache', '$anchorScroll', '$compile'
};

scope.$watch($sce.parseAsResourceUrl(srcExp), function ngIncludeWatchAction(src) {
var afterAnimation = function() {

if (isDefined(autoScrollExp) && (!autoScrollExp || scope.$eval(autoScrollExp))) {
$anchorScroll();
}

};
var thisChangeId = ++changeCounter;

if (src) {
Expand All @@ -192,13 +199,8 @@ var ngIncludeDirective = ['$http', '$templateCache', '$anchorScroll', '$compile'
currentElement = clone;

currentElement.html(response);
$animate.enter(currentElement, null, $element);
$animate.enter(currentElement, null, $element, afterAnimation);
$compile(currentElement.contents())(currentScope);

if (isDefined(autoScrollExp) && (!autoScrollExp || scope.$eval(autoScrollExp))) {
$anchorScroll();
}

currentScope.$emit('$includeContentLoaded');
scope.$eval(onloadExp);

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Nov 5, 2013

I think that this $emit and $eval should stay here at least until we have a usecase where it needs to execute after the animation is done.

I can be convinced otherwise if you have some real world scenarios where this would not work.

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 5, 2013

Author Owner

@IgorMinar: I guess the problem is as they are these "events" are potentially triggered before the content has been added to the DOM, which handlers may be wanting to use.

This comment has been minimized.

Copy link
@mhevery

mhevery Nov 5, 2013

The events should stay put.

  1. AfterAnimation executes without $digest.
  2. presence or absence of animation should not effect the application logic.

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Nov 5, 2013

@petebacondarwin the dom chunk will be in the document by the time the event is fired, it just might not be at it's final position.

Misko's $digest argument convinces me that it should stay where it is now.

});
Expand Down
26 changes: 25 additions & 1 deletion test/ng/directive/ngIncludeSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -322,22 +322,36 @@ describe('ngInclude', function() {
};
}

function spyOnAnimateEnter() {
return function($animate) {
spyOn($animate, 'enter').andCallThrough();
};
}

function runEnterAnimation($animate) {
if ($animate.enter.mostRecentCall) {
$animate.enter.mostRecentCall.args[3]();

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Nov 5, 2013

this is quite gross :)

can you use the mock.animate module instead?

beforeEach(module('mock.animate'));

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 5, 2013

Author Owner

mock.animate brings a whole new set of issue. Naive use causes LEAK errors. We have to then make sure that the elements are attached and dealoc'ed correctly. Plus you still need to call $animate.flush() all over the place.

}
}

function compileAndLink(tpl) {
return function($compile, $rootScope) {
element = $compile(tpl)($rootScope);
};
}

function changeTplAndValueTo(template, value) {
return function($rootScope, $browser) {
return function($rootScope, $browser, $animate) {
$rootScope.$apply(function() {
$rootScope.tpl = template;
$rootScope.value = value;
});
runEnterAnimation($animate);
};
}

beforeEach(module(spyOnAnchorScroll()));
beforeEach(inject(spyOnAnimateEnter()));
beforeEach(inject(
putIntoCache('template.html', 'CONTENT'),
putIntoCache('another.html', 'CONTENT')));
Expand Down Expand Up @@ -374,6 +388,16 @@ describe('ngInclude', function() {
changeTplAndValueTo('template.html', null), function() {
expect(autoScrollSpy).not.toHaveBeenCalled();
}));

it('should only call $anchorScroll after the "enter" animation completes', inject(
compileAndLink('<div><ng:include src="tpl" autoscroll></ng:include></div>'),
function($rootScope, $animate) {
$rootScope.$apply("tpl = 'template.html'");
expect($animate.enter).toHaveBeenCalledOnce();
expect(autoScrollSpy).not.toHaveBeenCalled();
runEnterAnimation($animate);
expect(autoScrollSpy).toHaveBeenCalledOnce();
}));
});
});

Expand Down

2 comments on commit 732ca80

@IgorMinar
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we need a similar fix for ngView?

@mhevery
Copy link

@mhevery mhevery commented on 732ca80 Nov 5, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with igor. Neen same for ngView

Please sign in to comment.