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

fix(ngAnimate): correctly animate transcluded clones with templateUrl #15514

Closed
wants to merge 10 commits into from
+230 −119
Closed

fix(ngAnimate): correctly animate transcluded clones with templateUrl #15514

Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3db2092
refactor(ngAnimate): remove unused argument
gkalpak Dec 15, 2016
d5ea497
refactor(ngAnimate): simplify `areAnimationsAllowed()` and remove red…
gkalpak Dec 15, 2016
8c71c02
refactor(ngAnimate): simplify `cleanupEventListeners()` and remove re…
gkalpak Dec 15, 2016
a4d4d3c
refactor(ngAnimate): simplify `closeChildAnimations()` and remove red…
gkalpak Dec 15, 2016
9f55361
refactor(ngAnimate): simplify `clearElementAnimationState()` and remo…
gkalpak Dec 15, 2016
21ca2ee
refactor(ngAnimate): simplify `markElementAnimationState()` and remov…
gkalpak Dec 15, 2016
288351b
refactor(ngAnimate): simplify `findCallbacks()` and remove redundant …
gkalpak Dec 15, 2016
df88676
chore(benchmarks): add basic animation benchmark
gkalpak Dec 17, 2016
d552f11
test(ngAnimate): make expectations more specific
gkalpak Dec 21, 2016
5341108
fix(ngAnimate): correctly animate transcluded clones with `templateUrl`
gkalpak Dec 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 40 additions & 44 deletions src/ngAnimate/animateQueue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ var $$AnimateQueueProvider = ['$animateProvider', /** @this */ function($animate
}
}

function isAllowed(ruleType, element, currentAnimation, previousAnimation) {
function isAllowed(ruleType, currentAnimation, previousAnimation) {
return rules[ruleType].some(function(fn) {
return fn(element, currentAnimation, previousAnimation);
return fn(currentAnimation, previousAnimation);
});
}

Expand All @@ -48,40 +48,40 @@ var $$AnimateQueueProvider = ['$animateProvider', /** @this */ function($animate
return and ? a && b : a || b;
}

rules.join.push(function(element, newAnimation, currentAnimation) {
rules.join.push(function(newAnimation, currentAnimation) {
// if the new animation is class-based then we can just tack that on
return !newAnimation.structural && hasAnimationClasses(newAnimation);
});

rules.skip.push(function(element, newAnimation, currentAnimation) {
rules.skip.push(function(newAnimation, currentAnimation) {
// there is no need to animate anything if no classes are being added and
// there is no structural animation that will be triggered
return !newAnimation.structural && !hasAnimationClasses(newAnimation);
});

rules.skip.push(function(element, newAnimation, currentAnimation) {
rules.skip.push(function(newAnimation, currentAnimation) {
// why should we trigger a new structural animation if the element will
// be removed from the DOM anyway?
return currentAnimation.event === 'leave' && newAnimation.structural;
});

rules.skip.push(function(element, newAnimation, currentAnimation) {
rules.skip.push(function(newAnimation, currentAnimation) {
// if there is an ongoing current animation then don't even bother running the class-based animation
return currentAnimation.structural && currentAnimation.state === RUNNING_STATE && !newAnimation.structural;
});

rules.cancel.push(function(element, newAnimation, currentAnimation) {
rules.cancel.push(function(newAnimation, currentAnimation) {
// there can never be two structural animations running at the same time
return currentAnimation.structural && newAnimation.structural;
});

rules.cancel.push(function(element, newAnimation, currentAnimation) {
rules.cancel.push(function(newAnimation, currentAnimation) {
// if the previous animation is already running, but the new animation will
// be triggered, but the new animation is structural
return currentAnimation.state === RUNNING_STATE && newAnimation.structural;
});

rules.cancel.push(function(element, newAnimation, currentAnimation) {
rules.cancel.push(function(newAnimation, currentAnimation) {
// cancel the animation if classes added / removed in both animation cancel each other out,
// but only if the current animation isn't structural

Expand Down Expand Up @@ -381,7 +381,7 @@ var $$AnimateQueueProvider = ['$animateProvider', /** @this */ function($animate
// there is no point in traversing the same collection of parent ancestors if a followup
// animation will be run on the same element that already did all that checking work
if (!skipAnimations && (!hasExistingAnimation || existingAnimation.state !== PRE_DIGEST_STATE)) {
skipAnimations = !areAnimationsAllowed(element, parent, event);
skipAnimations = !areAnimationsAllowed(node, getDomNode(parent), event);
}

if (skipAnimations) {
Expand All @@ -408,7 +408,7 @@ var $$AnimateQueueProvider = ['$animateProvider', /** @this */ function($animate
};

if (hasExistingAnimation) {
var skipAnimationFlag = isAllowed('skip', element, newAnimation, existingAnimation);
var skipAnimationFlag = isAllowed('skip', newAnimation, existingAnimation);
if (skipAnimationFlag) {
if (existingAnimation.state === RUNNING_STATE) {
close();
Expand All @@ -418,7 +418,7 @@ var $$AnimateQueueProvider = ['$animateProvider', /** @this */ function($animate
return existingAnimation.runner;
}
}
var cancelAnimationFlag = isAllowed('cancel', element, newAnimation, existingAnimation);
var cancelAnimationFlag = isAllowed('cancel', newAnimation, existingAnimation);
if (cancelAnimationFlag) {
if (existingAnimation.state === RUNNING_STATE) {
// this will end the animation right away and it is safe
Expand All @@ -440,7 +440,7 @@ var $$AnimateQueueProvider = ['$animateProvider', /** @this */ function($animate
// a joined animation means that this animation will take over the existing one
// so an example would involve a leave animation taking over an enter. Then when
// the postDigest kicks in the enter will be ignored.
var joinAnimationFlag = isAllowed('join', element, newAnimation, existingAnimation);
var joinAnimationFlag = isAllowed('join', newAnimation, existingAnimation);
if (joinAnimationFlag) {
if (existingAnimation.state === RUNNING_STATE) {
normalizeAnimationDetails(element, newAnimation);
Expand Down Expand Up @@ -610,65 +610,61 @@ var $$AnimateQueueProvider = ['$animateProvider', /** @this */ function($animate
activeAnimationsLookup.remove(node);
}

function isMatchingElement(nodeOrElmA, nodeOrElmB) {
return getDomNode(nodeOrElmA) === getDomNode(nodeOrElmB);
}

/**
* This fn returns false if any of the following is true:
* a) animations on any parent element are disabled, and animations on the element aren't explicitly allowed
* b) a parent element has an ongoing structural animation, and animateChildren is false
* c) the element is not a child of the body
* d) the element is not a child of the $rootElement
*/
function areAnimationsAllowed(element, parentElement, event) {
var bodyElement = jqLite($document[0].body);
var bodyElementDetected = isMatchingElement(element, bodyElement) || element[0].nodeName === 'HTML';
var rootElementDetected = isMatchingElement(element, $rootElement);
function areAnimationsAllowed(node, parentNode, event) {
var bodyNode = $document[0].body;
var rootNode = getDomNode($rootElement);

var bodyNodeDetected = (node === bodyNode) || node.nodeName === 'HTML';
var rootNodeDetected = (node === rootNode);
Copy link
Member

Choose a reason for hiding this comment

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

Why the parens? (here & in other places)

Copy link
Member Author

Choose a reason for hiding this comment

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

A couple of reasons (at least):

  1. I find them more readable. I think it is a remnant from my early Java days (and this ancient document).
  2. It is also visually closer to isMatchingElement(...) 😛

(Hey, I changed the trailing dots, I am not removing the parens.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we need an eslint rule for that :D But I prefer the parens too

Copy link
Member

Choose a reason for hiding this comment

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

I like not having parents here but as long as ESLint will shout at me if I forget them I'd be satisfied as well. ;)

var parentAnimationDetected = false;
var elementDisabled = disabledElementsLookup.get(node);
var animateChildren;
var elementDisabled = disabledElementsLookup.get(getDomNode(element));

var parentHost = jqLite.data(element[0], NG_ANIMATE_PIN_DATA);
var parentHost = jqLite.data(node, NG_ANIMATE_PIN_DATA);
if (parentHost) {
parentElement = parentHost;
parentNode = getDomNode(parentHost);
}

parentElement = getDomNode(parentElement);

while (parentElement) {
if (!rootElementDetected) {
while (parentNode) {
if (!rootNodeDetected) {
// angular doesn't want to attempt to animate elements outside of the application
// therefore we need to ensure that the rootElement is an ancestor of the current element
rootElementDetected = isMatchingElement(parentElement, $rootElement);
rootNodeDetected = (parentNode === rootNode);
}

if (parentElement.nodeType !== ELEMENT_NODE) {
if (parentNode.nodeType !== ELEMENT_NODE) {
// no point in inspecting the #document element
break;
}

var details = activeAnimationsLookup.get(parentElement) || {};
var details = activeAnimationsLookup.get(parentNode) || {};
// either an enter, leave or move animation will commence
// therefore we can't allow any animations to take place
// but if a parent animation is class-based then that's ok
if (!parentAnimationDetected) {
var parentElementDisabled = disabledElementsLookup.get(parentElement);
var parentNodeDisabled = disabledElementsLookup.get(parentNode);

if (parentElementDisabled === true && elementDisabled !== false) {
if (parentNodeDisabled === true && elementDisabled !== false) {
// disable animations if the user hasn't explicitly enabled animations on the
// current element
elementDisabled = true;
// element is disabled via parent element, no need to check anything else
break;
} else if (parentElementDisabled === false) {
} else if (parentNodeDisabled === false) {
elementDisabled = false;
}
parentAnimationDetected = details.structural;
}

if (isUndefined(animateChildren) || animateChildren === true) {
var value = jqLite.data(parentElement, NG_ANIMATE_CHILDREN_DATA);
var value = jqLite.data(parentNode, NG_ANIMATE_CHILDREN_DATA);
if (isDefined(value)) {
animateChildren = value;
}
Expand All @@ -677,33 +673,33 @@ var $$AnimateQueueProvider = ['$animateProvider', /** @this */ function($animate
// there is no need to continue traversing at this point
if (parentAnimationDetected && animateChildren === false) break;

if (!bodyElementDetected) {
if (!bodyNodeDetected) {
// we also need to ensure that the element is or will be a part of the body element
// otherwise it is pointless to even issue an animation to be rendered
bodyElementDetected = isMatchingElement(parentElement, bodyElement);
bodyNodeDetected = (parentNode === bodyNode);
}

if (bodyElementDetected && rootElementDetected) {
if (bodyNodeDetected && rootNodeDetected) {
// If both body and root have been found, any other checks are pointless,
// as no animation data should live outside the application
break;
}

if (!rootElementDetected) {
// If no rootElement is detected, check if the parentElement is pinned to another element
parentHost = jqLite.data(parentElement, NG_ANIMATE_PIN_DATA);
if (!rootNodeDetected) {
// If `rootNode` is not detected, check if `parentNode` is pinned to another element
parentHost = jqLite.data(parentNode, NG_ANIMATE_PIN_DATA);
if (parentHost) {
// The pin target element becomes the next parent element
parentElement = getDomNode(parentHost);
parentNode = getDomNode(parentHost);
continue;
}
}

parentElement = parentElement.parentNode;
parentNode = parentNode.parentNode;
}

var allowAnimation = (!parentAnimationDetected || animateChildren) && elementDisabled !== true;
return allowAnimation && rootElementDetected && bodyElementDetected;
return allowAnimation && rootNodeDetected && bodyNodeDetected;
}

function markElementAnimationState(element, state, details) {
Expand Down