From 7700e2df096cf50dfdf84841cab7e2d24d2eb96d Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Tue, 12 Jan 2016 16:00:18 +0100 Subject: [PATCH 1/2] fix($animate): correctly handle `$animate.pin()` host elements This commit fixes two bugs: 1) Previously, animate would assume that a found host element was part of the $rootElement (while it's possible that it is also outside the root). 2) Previously, if a parent of the animated element was pinned to a host element, the host would not be checked regarding animations enabled status etc. Closes #13783 --- src/ngAnimate/animateQueue.js | 23 ++++---- test/ngAnimate/animateSpec.js | 106 +++++++++++++++++++++++++++------- 2 files changed, 94 insertions(+), 35 deletions(-) diff --git a/src/ngAnimate/animateQueue.js b/src/ngAnimate/animateQueue.js index d69eae33dee7..9c61c8b2364f 100644 --- a/src/ngAnimate/animateQueue.js +++ b/src/ngAnimate/animateQueue.js @@ -624,25 +624,22 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) { // there is no need to continue traversing at this point if (parentAnimationDetected && animateChildren === false) break; - if (!rootElementDetected) { - // 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); - if (!rootElementDetected) { - parentHost = parentElement.data(NG_ANIMATE_PIN_DATA); - if (parentHost) { - parentElement = parentHost; - rootElementDetected = true; - } - } - } - if (!bodyElementDetected) { // we also need to ensure that the element is or will be apart of the body element // otherwise it is pointless to even issue an animation to be rendered bodyElementDetected = isMatchingElement(parentElement, bodyElement); } + if (!rootElementDetected) { + // If no rootElement is detected, check if the parentElement is pinned to another element + parentHost = parentElement.data(NG_ANIMATE_PIN_DATA); + if (parentHost) { + // The pin target element becomes the next parent element + parentElement = parentHost; + continue; + } + } + parentElement = parentElement.parent(); } diff --git a/test/ngAnimate/animateSpec.js b/test/ngAnimate/animateSpec.js index d281d0abfe0f..bb806ba7a9fe 100644 --- a/test/ngAnimate/animateSpec.js +++ b/test/ngAnimate/animateSpec.js @@ -1483,10 +1483,14 @@ describe("animations", function() { return new $$AnimateRunner(); }; }); + + return function($animate) { + $animate.enabled(true); + }; })); it('should throw if the arguments are not elements', - inject(function($animate, $compile, $document, $rootScope, $rootElement) { + inject(function($animate, $rootElement) { var element = jqLite('
'); @@ -1505,7 +1509,7 @@ describe("animations", function() { they('should animate an element inside a pinned element that is the $prop element', ['same', 'parent', 'grandparent'], function(elementRelation) { - inject(function($animate, $compile, $document, $rootElement, $rootScope) { + inject(function($animate, $document, $rootElement, $rootScope) { var pinElement, animateElement; @@ -1543,34 +1547,92 @@ describe("animations", function() { }); }); - it('should adhere to the disabled state of the hosted parent when an element is pinned', - inject(function($animate, $compile, $document, $rootElement, $rootScope) { + they('should not animate an element when the pinned ($prop) element, is pinned to an element that is not a child of the $rootElement', + ['same', 'parent', 'grandparent'], + function(elementRelation) { + inject(function($animate, $document, $rootElement, $rootScope) { + + var pinElement, animateElement, pinTargetElement = jqLite('
'); - var innerParent = jqLite('
'); - jqLite($document[0].body).append(innerParent); - innerParent.append($rootElement); - var innerChild = jqLite('
'); - $rootElement.append(innerChild); + var innerParent = jqLite('
'); + jqLite($document[0].body).append(innerParent); + innerParent.append($rootElement); - var element = jqLite('
'); - jqLite($document[0].body).append(element); + switch (elementRelation) { + case 'same': + pinElement = jqLite('
'); + break; + case 'parent': + pinElement = jqLite('
'); + break; + case 'grandparent': + pinElement = jqLite('
'); + break; + } - $animate.pin(element, innerChild); + // Append both the pin element and the pinTargetElement outside the app root + jqLite($document[0].body).append(pinElement); + jqLite($document[0].body).append(pinTargetElement); - $animate.enabled(innerChild, false); + animateElement = jqLite($document[0].getElementById('animate')); - $animate.addClass(element, 'blue'); - $rootScope.$digest(); - expect(capturedAnimation).toBeFalsy(); + $animate.addClass(animateElement, 'red'); + $rootScope.$digest(); + expect(capturedAnimation).toBeFalsy(); - $animate.enabled(innerChild, true); + $animate.pin(pinElement, pinTargetElement); - $animate.addClass(element, 'red'); - $rootScope.$digest(); - expect(capturedAnimation).toBeTruthy(); + $animate.addClass(animateElement, 'blue'); + $rootScope.$digest(); + expect(capturedAnimation).toBeFalsy(); - dealoc(element); - })); + dealoc(pinElement); + }); + }); + + they('should adhere to the disabled state of the hosted parent when the $prop element is pinned', + ['same', 'parent', 'grandparent'], + function(elementRelation) { + inject(function($animate, $document, $rootElement, $rootScope) { + + var pinElement, animateElement, pinHostElement = jqLite('
'); + + var innerParent = jqLite('
'); + jqLite($document[0].body).append(innerParent); + innerParent.append($rootElement); + + switch (elementRelation) { + case 'same': + pinElement = jqLite('
'); + break; + case 'parent': + pinElement = jqLite('
'); + break; + case 'grandparent': + pinElement = jqLite('
'); + break; + } + + $rootElement.append(pinHostElement); + jqLite($document[0].body).append(pinElement); + animateElement = jqLite($document[0].getElementById('animate')); + + $animate.pin(pinElement, pinHostElement); + $animate.enabled(pinHostElement, false); + + $animate.addClass(animateElement, 'blue'); + $rootScope.$digest(); + expect(capturedAnimation).toBeFalsy(); + + $animate.enabled(pinHostElement, true); + + $animate.addClass(animateElement, 'red'); + $rootScope.$digest(); + expect(capturedAnimation).toBeTruthy(); + + dealoc(pinElement); + }); + }); }); describe('callbacks', function() { From 683bd92f56990bf1bfeabf619d997716909ebf6b Mon Sep 17 00:00:00 2001 From: Martin Staffa Date: Mon, 18 Jan 2016 15:29:32 +0100 Subject: [PATCH 2/2] perf(ngAnimate): speed up areAnimationsAllowed check This commit speeds up the code that checks if an element can be animated, for the following two cases: The checks will be sped up in cases where the animation is disabled via $animate.enabled(element, false) on any parent element. A minor speed-up is also included for cases where the $rootElement of the app (the bootstrap element) is on the body or lower in the DOM tree. --- src/ngAnimate/animateQueue.js | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/ngAnimate/animateQueue.js b/src/ngAnimate/animateQueue.js index 9c61c8b2364f..07ac1480c6cb 100644 --- a/src/ngAnimate/animateQueue.js +++ b/src/ngAnimate/animateQueue.js @@ -571,6 +571,13 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) { 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'; @@ -604,10 +611,12 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) { if (!parentAnimationDetected) { var parentElementDisabled = disabledElementsLookup.get(parentNode); - // disable animations if the user hasn't explicitly enabled animations on the - // current element if (parentElementDisabled === 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) { elementDisabled = false; } @@ -625,11 +634,17 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) { if (parentAnimationDetected && animateChildren === false) break; if (!bodyElementDetected) { - // we also need to ensure that the element is or will be apart of the body element + // 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); } + if (bodyElementDetected && rootElementDetected) { + // 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 = parentElement.data(NG_ANIMATE_PIN_DATA);