From 7e09adf1ec3f6c1b57f361225dfbe923ae68bde3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Sat, 14 Dec 2013 00:30:48 -0500 Subject: [PATCH] fix($animate): use a scheduled timeout in favor of a fallback property to close transitions With ngAnimate, CSS transitions, that are not properlty triggered, are forceably closed off by appling a fallback property. The fallback property approach works, however, its styling itself may effect CSS inheritance or cause the element to render improperly. Therefore, its best to stick to using a scheduled timeout to run sometime after the highest animation time has passed. Closes #5255 Closes #5241 Closes #5405 --- css/angular.css | 11 --- src/ngAnimate/animate.js | 110 ++++++++++++++------- test/ngAnimate/animateSpec.js | 178 ++++++++++------------------------ 3 files changed, 128 insertions(+), 171 deletions(-) diff --git a/css/angular.css b/css/angular.css index 3e20a999cca0..b88e61e483e2 100644 --- a/css/angular.css +++ b/css/angular.css @@ -9,14 +9,3 @@ ng\:form { display: block; } - -/* The styles below ensure that the CSS transition will ALWAYS - * animate and close. A nasty bug occurs with CSS transitions where - * when the active class isn't set, or if the active class doesn't - * contain any styles to transition to, then, if ngAnimate is used, - * it will appear as if the webpage is broken due to the forever hanging - * animations. The border-spacing (!ie) and zoom (ie) CSS properties are - * used below since they trigger a transition without making the browser - * animate anything and they're both highly underused CSS properties */ -.ng-animate-start { border-spacing:1px 1px; -ms-zoom:1.0001; } -.ng-animate-active { border-spacing:0px 0px; -ms-zoom:1; } diff --git a/src/ngAnimate/animate.js b/src/ngAnimate/animate.js index aeb6e32e9361..097a97ec1dfe 100644 --- a/src/ngAnimate/animate.js +++ b/src/ngAnimate/animate.js @@ -881,27 +881,73 @@ angular.module('ngAnimate', ['ng']) var ANIMATION_ITERATION_COUNT_KEY = 'IterationCount'; var NG_ANIMATE_PARENT_KEY = '$$ngAnimateKey'; var NG_ANIMATE_CSS_DATA_KEY = '$$ngAnimateCSS3Data'; - var NG_ANIMATE_FALLBACK_CLASS_NAME = 'ng-animate-start'; - var NG_ANIMATE_FALLBACK_ACTIVE_CLASS_NAME = 'ng-animate-active'; var ELAPSED_TIME_MAX_DECIMAL_PLACES = 3; + var CLOSING_TIME_BUFFER = 1.5; + var ONE_SECOND = 1000; + var animationCounter = 0; var lookupCache = {}; var parentCounter = 0; + var animationReflowQueue = []; + var animationElementQueue = []; + var animationTimer; + var closingAnimationTime = 0; + var timeOut = false; + function afterReflow(element, callback) { + $timeout.cancel(animationTimer); - var animationReflowQueue = [], animationTimer, timeOut = false; - function afterReflow(callback) { animationReflowQueue.push(callback); - $timeout.cancel(animationTimer); + + var node = extractElementNode(element); + element = angular.element(node); + animationElementQueue.push(element); + + var elementData = element.data(NG_ANIMATE_CSS_DATA_KEY); + closingAnimationTime = Math.max(closingAnimationTime, + (elementData.maxDelay + elementData.maxDuration) * CLOSING_TIME_BUFFER * ONE_SECOND); + + //by placing a counter we can avoid an accidental + //race condition which may close an animation when + //a follow-up animation is midway in its animation + elementData.animationCount = animationCounter; + animationTimer = $timeout(function() { forEach(animationReflowQueue, function(fn) { fn(); }); + + //copy the list of elements so that successive + //animations won't conflict if they're added before + //the closing animation timeout has run + var elementQueueSnapshot = []; + var animationCounterSnapshot = animationCounter; + forEach(animationElementQueue, function(elm) { + elementQueueSnapshot.push(elm); + }); + + $timeout(function() { + closeAllAnimations(elementQueueSnapshot, animationCounterSnapshot); + elementQueueSnapshot = null; + }, closingAnimationTime, false); + animationReflowQueue = []; + animationElementQueue = []; animationTimer = null; lookupCache = {}; + closingAnimationTime = 0; + animationCounter++; }, 10, false); } + function closeAllAnimations(elements, count) { + angular.forEach(elements, function(element) { + var elementData = element.data(NG_ANIMATE_CSS_DATA_KEY); + if(elementData && elementData.animationCount == count) { + (elementData.closeAnimationFn || noop)(); + } + }); + } + function getElementAnimationDetails(element, cacheKey) { var data = cacheKey ? lookupCache[cacheKey] : null; if(!data) { @@ -1007,6 +1053,7 @@ angular.module('ngAnimate', ['ng']) timeout is empty (this would cause a flicker bug normally in the page. There is also no point in performing an animation that only has a delay and no duration */ + var maxDelay = Math.max(timings.transitionDelay, timings.animationDelay); var maxDuration = Math.max(timings.transitionDuration, timings.animationDuration); if(maxDuration === 0) { element.removeClass(className); @@ -1016,13 +1063,9 @@ angular.module('ngAnimate', ['ng']) //temporarily disable the transition so that the enter styles //don't animate twice (this is here to avoid a bug in Chrome/FF). var activeClassName = ''; - if(timings.transitionDuration > 0) { - element.addClass(NG_ANIMATE_FALLBACK_CLASS_NAME); - activeClassName += NG_ANIMATE_FALLBACK_ACTIVE_CLASS_NAME + ' '; - blockTransitions(element); - } else { + timings.transitionDuration > 0 ? + blockTransitions(element) : blockKeyframeAnimations(element); - } forEach(className.split(' '), function(klass, i) { activeClassName += (i > 0 ? ' ' : '') + klass + '-active'; @@ -1032,6 +1075,7 @@ angular.module('ngAnimate', ['ng']) className : className, activeClassName : activeClassName, maxDuration : maxDuration, + maxDelay : maxDelay, classes : className + ' ' + activeClassName, timings : timings, stagger : stagger, @@ -1066,30 +1110,28 @@ angular.module('ngAnimate', ['ng']) } function animateRun(element, className, activeAnimationComplete) { - var data = element.data(NG_ANIMATE_CSS_DATA_KEY); + var elementData = element.data(NG_ANIMATE_CSS_DATA_KEY); var node = extractElementNode(element); - if(node.className.indexOf(className) == -1 || !data) { + if(node.className.indexOf(className) == -1 || !elementData) { activeAnimationComplete(); return; } - var timings = data.timings; - var stagger = data.stagger; - var maxDuration = data.maxDuration; - var activeClassName = data.activeClassName; - var maxDelayTime = Math.max(timings.transitionDelay, timings.animationDelay) * 1000; + var timings = elementData.timings; + var stagger = elementData.stagger; + var maxDuration = elementData.maxDuration; + var activeClassName = elementData.activeClassName; + var maxDelayTime = Math.max(timings.transitionDelay, timings.animationDelay) * ONE_SECOND; var startTime = Date.now(); var css3AnimationEvents = ANIMATIONEND_EVENT + ' ' + TRANSITIONEND_EVENT; - var ii = data.ii; + var ii = elementData.ii; - var applyFallbackStyle, style = '', appliedStyles = []; + var style = '', appliedStyles = []; if(timings.transitionDuration > 0) { var propertyStyle = timings.transitionPropertyStyle; if(propertyStyle.indexOf('all') == -1) { - applyFallbackStyle = true; - var fallbackProperty = $sniffer.msie ? '-ms-zoom' : 'border-spacing'; - style += CSS_PREFIX + 'transition-property: ' + propertyStyle + ', ' + fallbackProperty + '; '; - style += CSS_PREFIX + 'transition-duration: ' + timings.transitionDurationStyle + ', ' + timings.transitionDuration + 's; '; + style += CSS_PREFIX + 'transition-property: ' + propertyStyle + ';'; + style += CSS_PREFIX + 'transition-duration: ' + timings.transitionDurationStyle + 's;'; appliedStyles.push(CSS_PREFIX + 'transition-property'); appliedStyles.push(CSS_PREFIX + 'transition-duration'); } @@ -1098,10 +1140,6 @@ angular.module('ngAnimate', ['ng']) if(ii > 0) { if(stagger.transitionDelay > 0 && stagger.transitionDuration === 0) { var delayStyle = timings.transitionDelayStyle; - if(applyFallbackStyle) { - delayStyle += ', ' + timings.transitionDelay + 's'; - } - style += CSS_PREFIX + 'transition-delay: ' + prepareStaggerDelay(delayStyle, stagger.transitionDelay, ii) + '; '; appliedStyles.push(CSS_PREFIX + 'transition-delay'); @@ -1124,11 +1162,16 @@ angular.module('ngAnimate', ['ng']) element.on(css3AnimationEvents, onAnimationProgress); element.addClass(activeClassName); + elementData.closeAnimationFn = function() { + onEnd(); + activeAnimationComplete(); + }; + return onEnd; // This will automatically be called by $animate so // there is no need to attach this internally to the // timeout done method. - return function onEnd(cancelled) { + function onEnd(cancelled) { element.off(css3AnimationEvents, onAnimationProgress); element.removeClass(activeClassName); animateClose(element, className); @@ -1136,7 +1179,7 @@ angular.module('ngAnimate', ['ng']) for (var i in appliedStyles) { node.style.removeProperty(appliedStyles[i]); } - }; + } function onAnimationProgress(event) { event.stopPropagation(); @@ -1202,7 +1245,7 @@ angular.module('ngAnimate', ['ng']) //data from the element which will not make the 2nd animation //happen in the first place var cancel = preReflowCancellation; - afterReflow(function() { + afterReflow(element, function() { unblockTransitions(element); unblockKeyframeAnimations(element); //once the reflow is complete then we point cancel to @@ -1218,7 +1261,6 @@ angular.module('ngAnimate', ['ng']) function animateClose(element, className) { element.removeClass(className); - element.removeClass(NG_ANIMATE_FALLBACK_CLASS_NAME); element.removeData(NG_ANIMATE_CSS_DATA_KEY); } @@ -1268,7 +1310,7 @@ angular.module('ngAnimate', ['ng']) beforeAddClass : function(element, className, animationCompleted) { var cancellationMethod = animateBefore(element, suffixClasses(className, '-add')); if(cancellationMethod) { - afterReflow(function() { + afterReflow(element, function() { unblockTransitions(element); unblockKeyframeAnimations(element); animationCompleted(); @@ -1285,7 +1327,7 @@ angular.module('ngAnimate', ['ng']) beforeRemoveClass : function(element, className, animationCompleted) { var cancellationMethod = animateBefore(element, suffixClasses(className, '-remove')); if(cancellationMethod) { - afterReflow(function() { + afterReflow(element, function() { unblockTransitions(element); unblockKeyframeAnimations(element); animationCompleted(); diff --git a/test/ngAnimate/animateSpec.js b/test/ngAnimate/animateSpec.js index 01589da19dde..705004d4b934 100644 --- a/test/ngAnimate/animateSpec.js +++ b/test/ngAnimate/animateSpec.js @@ -645,30 +645,6 @@ describe("ngAnimate", function() { expect(element).toBeShown(); })); - it("should fallback to the animation duration if an infinite iteration is provided", - inject(function($animate, $rootScope, $compile, $sniffer, $timeout) { - - var style = '-webkit-animation-duration: 2s;' + - '-webkit-animation-iteration-count: infinite;' + - 'animation-duration: 2s;' + - 'animation-iteration-count: infinite;'; - - ss.addRule('.ng-hide-add', style); - ss.addRule('.ng-hide-remove', style); - - element = $compile(html('
1
'))($rootScope); - - element.addClass('ng-hide'); - expect(element).toBeHidden(); - - $animate.removeClass(element, 'ng-hide'); - if ($sniffer.animations) { - $timeout.flush(); - browserTrigger(element,'animationend', { timeStamp: Date.now() + 2000, elapsedTime: 2 }); - } - expect(element).toBeShown(); - })); - it("should not consider the animation delay is provided", inject(function($animate, $rootScope, $compile, $sniffer, $timeout) { @@ -838,109 +814,10 @@ describe("ngAnimate", function() { expect(elements[2].attr('style')).toMatch(/animation-delay: 1\.2\d*s,\s*2\.2\d*s/); expect(elements[3].attr('style')).toMatch(/animation-delay: 1\.3\d*s,\s*2\.3\d*s/); })); + }); describe("Transitions", function() { - it("should only apply the fallback transition property unless all properties are being animated", - inject(function($compile, $rootScope, $animate, $sniffer, $timeout) { - - if (!$sniffer.animations) return; - - ss.addRule('.all.ng-enter', '-webkit-transition:1s linear all;' + - 'transition:1s linear all'); - - ss.addRule('.one.ng-enter', '-webkit-transition:1s linear color;' + - 'transition:1s linear color'); - - var element = $compile('
')($rootScope); - var child = $compile('
...
')($rootScope); - $rootElement.append(element); - var body = jqLite($document[0].body); - body.append($rootElement); - - $animate.enter(child, element); - $rootScope.$digest(); - $timeout.flush(); - - expect(child.attr('style') || '').not.toContain('transition-property'); - expect(child.hasClass('ng-animate-start')).toBe(true); - expect(child.hasClass('ng-animate-active')).toBe(true); - - browserTrigger(child,'transitionend', { timeStamp: Date.now() + 1000, elapsedTime: 1000 }); - $timeout.flush(); - - expect(child.hasClass('ng-animate')).toBe(false); - expect(child.hasClass('ng-animate-active')).toBe(false); - - child.remove(); - - var child2 = $compile('
...
')($rootScope); - - $animate.enter(child2, element); - $rootScope.$digest(); - $timeout.flush(); - - //IE removes the -ms- prefix when placed on the style - var fallbackProperty = $sniffer.msie ? 'zoom' : 'border-spacing'; - var regExp = new RegExp("transition-property:\\s+color\\s*,\\s*" + fallbackProperty + "\\s*;"); - expect(child2.attr('style') || '').toMatch(regExp); - expect(child2.hasClass('ng-animate')).toBe(true); - expect(child2.hasClass('ng-animate-active')).toBe(true); - - browserTrigger(child2,'transitionend', { timeStamp: Date.now() + 1000, elapsedTime: 1000 }); - $timeout.flush(); - - expect(child2.hasClass('ng-animate')).toBe(false); - expect(child2.hasClass('ng-animate-active')).toBe(false); - })); - - it("should not apply the fallback classes if no animations are going on or if CSS animations are going on", - inject(function($compile, $rootScope, $animate, $sniffer, $timeout) { - - if (!$sniffer.animations) return; - - ss.addRule('.transitions', '-webkit-transition:1s linear all;' + - 'transition:1s linear all'); - - ss.addRule('.keyframes', '-webkit-animation:my_animation 1s;' + - 'animation:my_animation 1s'); - - var element = $compile('
...
')($rootScope); - $rootElement.append(element); - jqLite($document[0].body).append($rootElement); - - $animate.enabled(false); - - $animate.addClass(element, 'klass'); - - expect(element.hasClass('ng-animate-start')).toBe(false); - - element.removeClass('klass'); - - $animate.enabled(true); - - $animate.addClass(element, 'klass'); - - $timeout.flush(); - - expect(element.hasClass('ng-animate-start')).toBe(true); - expect(element.hasClass('ng-animate-active')).toBe(true); - - browserTrigger(element,'transitionend', { timeStamp: Date.now() + 1000, elapsedTime: 1 }); - - expect(element.hasClass('ng-animate-start')).toBe(false); - expect(element.hasClass('ng-animate-active')).toBe(false); - - element.attr('class', 'keyframes'); - - $animate.addClass(element, 'klass2'); - - $timeout.flush(); - - expect(element.hasClass('ng-animate-start')).toBe(false); - expect(element.hasClass('ng-animate-active')).toBe(false); - })); - it("should skip transitions if disabled and run when enabled", inject(function($animate, $rootScope, $compile, $sniffer, $timeout) { @@ -1083,9 +960,9 @@ describe("ngAnimate", function() { } expect(element).toBeShown(); if ($sniffer.transitions) { - expect(element.hasClass('ng-animate-active')).toBe(true); + expect(element.hasClass('ng-hide-remove-active')).toBe(true); browserTrigger(element,'animationend', { timeStamp: Date.now() + 11000, elapsedTime: 11 }); - expect(element.hasClass('ng-animate-active')).toBe(false); + expect(element.hasClass('ng-hide-remove-active')).toBe(false); } })); @@ -1214,6 +1091,55 @@ describe("ngAnimate", function() { expect(elements[2].attr('style')).toMatch(/transition-delay: 2\.2\d*s,\s*4\.2\d*s/); expect(elements[3].attr('style')).toMatch(/transition-delay: 2\.3\d*s,\s*4\.3\d*s/); })); + + it("apply a closing timeout to close all pending transitions", + inject(function($animate, $rootScope, $compile, $sniffer, $timeout) { + + if (!$sniffer.transitions) return; + + ss.addRule('.animated-element', '-webkit-transition:5s linear all;' + + 'transition:5s linear all;'); + + element = $compile(html('
foo
'))($rootScope); + + $animate.addClass(element, 'some-class'); + + $timeout.flush(10); //reflow + expect(element.hasClass('some-class-add-active')).toBe(true); + + $timeout.flush(7500); //closing timeout + expect(element.hasClass('some-class-add-active')).toBe(false); + })); + + it("should not allow the closing animation to close off a successive animation midway", + inject(function($animate, $rootScope, $compile, $sniffer, $timeout) { + + if (!$sniffer.transitions) return; + + ss.addRule('.some-class-add', '-webkit-transition:5s linear all;' + + 'transition:5s linear all;'); + ss.addRule('.some-class-remove', '-webkit-transition:10s linear all;' + + 'transition:10s linear all;'); + + element = $compile(html('
foo
'))($rootScope); + + $animate.addClass(element, 'some-class'); + + $timeout.flush(10); //reflow + expect(element.hasClass('some-class-add-active')).toBe(true); + + $animate.removeClass(element, 'some-class'); + + $timeout.flush(10); //second reflow + + $timeout.flush(7500); //closing timeout for the first animation + expect(element.hasClass('some-class-remove-active')).toBe(true); + + $timeout.flush(15000); //closing timeout for the second animation + expect(element.hasClass('some-class-remove-active')).toBe(false); + + $timeout.verifyNoPendingTasks(); + })); }); it("should apply staggering to both transitions and keyframe animations when used within the same animation",