Permalink
Comparing changes
Open a pull request
- 2 commits
- 10 files changed
- 0 commit comments
- 1 contributor
Commits on Jan 12, 2016
A bug in material has exposed that ngAnimate makes a copy of the provided animation options twice. By making two copies, the same DOM operations are performed during and at the end of the animation. If the CSS classes being added/ removed contain existing transition code, then this will lead to rendering issues. Closes #13722 Closes #13578
Unified
Split
Showing
with
309 additions
and 23 deletions.
- +2 −0 src/AngularPublic.js
- +4 −0 src/ng/animate.js
- +8 −4 src/ng/animateCss.js
- +11 −9 src/ngAnimate/animateCss.js
- +28 −5 src/ngAnimate/animateJs.js
- +82 −4 src/ngMock/angular-mocks.js
- +22 −0 test/ng/animateCssSpec.js
- +22 −0 test/ngAnimate/animateCssSpec.js
- +47 −0 test/ngAnimate/integrationSpec.js
- +83 −1 test/ngMock/angular-mocksSpec.js
| @@ -56,6 +56,7 @@ | ||
| $AnchorScrollProvider, | ||
| $AnimateProvider, | ||
| $CoreAnimateCssProvider, | ||
| $$CoreAnimateJsProvider, | ||
| $$CoreAnimateQueueProvider, | ||
| $$AnimateRunnerFactoryProvider, | ||
| $$AnimateAsyncRunFactoryProvider, | ||
| @@ -217,6 +218,7 @@ function publishExternalAPI(angular) { | ||
| $anchorScroll: $AnchorScrollProvider, | ||
| $animate: $AnimateProvider, | ||
| $animateCss: $CoreAnimateCssProvider, | ||
| $$animateJs: $$CoreAnimateJsProvider, | ||
| $$animateQueue: $$CoreAnimateQueueProvider, | ||
| $$AnimateRunner: $$AnimateRunnerFactoryProvider, | ||
| $$animateAsyncRun: $$AnimateAsyncRunFactoryProvider, | ||
| @@ -53,6 +53,10 @@ function prepareAnimateOptions(options) { | ||
| : {}; | ||
| } | ||
|
|
||
| var $$CoreAnimateJsProvider = function() { | ||
| this.$get = function() {}; | ||
| }; | ||
|
|
||
| // this is prefixed with Core since it conflicts with | ||
| // the animateQueueProvider defined in ngAnimate/animateQueue.js | ||
| var $$CoreAnimateQueueProvider = function() { | ||
| @@ -15,10 +15,14 @@ var $CoreAnimateCssProvider = function() { | ||
| this.$get = ['$$rAF', '$q', '$$AnimateRunner', function($$rAF, $q, $$AnimateRunner) { | ||
|
|
||
| return function(element, initialOptions) { | ||
| // we always make a copy of the options since | ||
| // there should never be any side effects on | ||
| // the input data when running `$animateCss`. | ||
| var options = copy(initialOptions); | ||
| // all of the animation functions should create | ||
| // a copy of the options data, however, if a | ||
| // parent service has already created a copy then | ||
| // we should stick to using that | ||
| var options = initialOptions || {}; | ||
| if (!options.$$prepared) { | ||
| options = copy(options); | ||
| } | ||
|
|
||
| // there is no point in applying the styles since | ||
| // there is no animation that goes on at all in | ||
| @@ -352,9 +352,9 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) { | ||
| var gcsStaggerLookup = createLocalCacheLookup(); | ||
|
|
||
| this.$get = ['$window', '$$jqLite', '$$AnimateRunner', '$timeout', | ||
| '$$forceReflow', '$sniffer', '$$rAFScheduler', '$animate', | ||
| '$$forceReflow', '$sniffer', '$$rAFScheduler', '$$animateQueue', | ||
| function($window, $$jqLite, $$AnimateRunner, $timeout, | ||
| $$forceReflow, $sniffer, $$rAFScheduler, $animate) { | ||
| $$forceReflow, $sniffer, $$rAFScheduler, $$animateQueue) { | ||
|
|
||
| var applyAnimationClasses = applyAnimationClassesFactory($$jqLite); | ||
|
|
||
| @@ -447,21 +447,23 @@ var $AnimateCssProvider = ['$animateProvider', function($animateProvider) { | ||
| } | ||
|
|
||
| return function init(element, initialOptions) { | ||
| // we always make a copy of the options since | ||
| // there should never be any side effects on | ||
| // the input data when running `$animateCss`. | ||
| var options = copy(initialOptions); | ||
| // all of the animation functions should create | ||
| // a copy of the options data, however, if a | ||
| // parent service has already created a copy then | ||
| // we should stick to using that | ||
| var options = initialOptions || {}; | ||
| if (!options.$$prepared) { | ||
| options = prepareAnimationOptions(copy(options)); | ||
| } | ||
|
|
||
| var restoreStyles = {}; | ||
| var node = getDomNode(element); | ||
| if (!node | ||
| || !node.parentNode | ||
| || !$animate.enabled()) { | ||
| || !$$animateQueue.enabled()) { | ||
| return closeAndReturnNoopAnimator(); | ||
| } | ||
|
|
||
| options = prepareAnimationOptions(options); | ||
|
|
||
| var temporaryStyles = []; | ||
| var classes = element.attr('class'); | ||
| var styles = packageStyles(options); | ||
| @@ -11,6 +11,8 @@ var $$AnimateJsProvider = ['$animateProvider', function($animateProvider) { | ||
| var applyAnimationClasses = applyAnimationClassesFactory($$jqLite); | ||
| // $animateJs(element, 'enter'); | ||
| return function(element, event, classes, options) { | ||
| var animationClosed = false; | ||
|
|
||
| // the `classes` argument is optional and if it is not used | ||
| // then the classes will be resolved from the element's className | ||
| // property as well as options.addClass/options.removeClass. | ||
| @@ -63,8 +65,32 @@ var $$AnimateJsProvider = ['$animateProvider', function($animateProvider) { | ||
| applyAnimationClasses(element, options); | ||
| } | ||
|
|
||
| function close() { | ||
| animationClosed = true; | ||
| applyOptions(); | ||
| applyAnimationStyles(element, options); | ||
| } | ||
|
|
||
| var runner; | ||
|
|
||
| return { | ||
| $$willAnimate: true, | ||
| end: function() { | ||
| if (runner) { | ||
| runner.end(); | ||
| } else { | ||
| close(); | ||
| runner = new $$AnimateRunner(); | ||
| runner.complete(true); | ||
| } | ||
| return runner; | ||
| }, | ||
| start: function() { | ||
| if (runner) { | ||
| return runner; | ||
| } | ||
|
|
||
| runner = new $$AnimateRunner(); | ||
| var closeActiveAnimations; | ||
| var chain = []; | ||
|
|
||
| @@ -89,8 +115,7 @@ var $$AnimateJsProvider = ['$animateProvider', function($animateProvider) { | ||
| }); | ||
| } | ||
|
|
||
| var animationClosed = false; | ||
| var runner = new $$AnimateRunner({ | ||
| runner.setHost({ | ||
| end: function() { | ||
| endAnimations(); | ||
| }, | ||
| @@ -103,9 +128,7 @@ var $$AnimateJsProvider = ['$animateProvider', function($animateProvider) { | ||
| return runner; | ||
|
|
||
| function onComplete(success) { | ||
| animationClosed = true; | ||
| applyOptions(); | ||
| applyAnimationStyles(element, options); | ||
| close(success); | ||
| runner.complete(success); | ||
| } | ||
|
|
||
| @@ -751,6 +751,15 @@ angular.mock.TzDate = function(offset, timestamp) { | ||
| angular.mock.TzDate.prototype = Date.prototype; | ||
| /* jshint +W101 */ | ||
|
|
||
|
|
||
| /** | ||
| * @ngdoc service | ||
| * @name $animate | ||
| * | ||
| * @description | ||
| * Mock implementation of the {@link ng.$animate `$animate`} service. Exposes two additional methods | ||
| * for testing animations. | ||
| */ | ||
| angular.mock.animate = angular.module('ngAnimateMock', ['ng']) | ||
|
|
||
| .config(['$provide', function($provide) { | ||
| @@ -783,9 +792,50 @@ angular.mock.animate = angular.module('ngAnimateMock', ['ng']) | ||
| return queueFn; | ||
| }); | ||
|
|
||
| $provide.decorator('$animate', ['$delegate', '$timeout', '$browser', '$$rAF', | ||
| $provide.decorator('$$animateJs', ['$delegate', function($delegate) { | ||
| var runners = []; | ||
|
|
||
| var animateJsConstructor = function() { | ||
| var animator = $delegate.apply($delegate, arguments); | ||
| // If no javascript animation is found, animator is undefined | ||
| if (animator) { | ||
| runners.push(animator); | ||
| } | ||
| return animator; | ||
| }; | ||
|
|
||
| animateJsConstructor.$closeAndFlush = function() { | ||
| runners.forEach(function(runner) { | ||
| runner.end(); | ||
| }); | ||
| runners = []; | ||
| }; | ||
|
|
||
| return animateJsConstructor; | ||
| }]); | ||
|
|
||
| $provide.decorator('$animateCss', ['$delegate', function($delegate) { | ||
| var runners = []; | ||
|
|
||
| var animateCssConstructor = function(element, options) { | ||
| var animator = $delegate(element, options); | ||
| runners.push(animator); | ||
| return animator; | ||
| }; | ||
|
|
||
| animateCssConstructor.$closeAndFlush = function() { | ||
| runners.forEach(function(runner) { | ||
| runner.end(); | ||
| }); | ||
| runners = []; | ||
| }; | ||
|
|
||
| return animateCssConstructor; | ||
| }]); | ||
|
|
||
| $provide.decorator('$animate', ['$delegate', '$timeout', '$browser', '$$rAF', '$animateCss', '$$animateJs', | ||
| '$$forceReflow', '$$animateAsyncRun', '$rootScope', | ||
| function($delegate, $timeout, $browser, $$rAF, | ||
| function($delegate, $timeout, $browser, $$rAF, $animateCss, $$animateJs, | ||
| $$forceReflow, $$animateAsyncRun, $rootScope) { | ||
| var animate = { | ||
| queue: [], | ||
| @@ -797,7 +847,35 @@ angular.mock.animate = angular.module('ngAnimateMock', ['ng']) | ||
| return $$forceReflow.totalReflows; | ||
| }, | ||
| enabled: $delegate.enabled, | ||
| flush: function() { | ||
| /** | ||
| * @ngdoc method | ||
| * @name $animate#closeAndFlush | ||
| * @description | ||
| * | ||
| * This method will close all pending animations (both {@link ngAnimate#javascript-based-animations Javascript} | ||
| * and {@link ngAnimate.$animateCss CSS}) and it will also flush any remaining animation frames and/or callbacks. | ||
| */ | ||
| closeAndFlush: function() { | ||
| // we allow the flush command to swallow the errors | ||
| // because depending on whether CSS or JS animations are | ||
| // used, there may not be a RAF flush. The primary flush | ||
| // at the end of this function must throw an exception | ||
| // because it will track if there were pending animations | ||
| this.flush(true); | ||
| $animateCss.$closeAndFlush(); | ||
| $$animateJs.$closeAndFlush(); | ||
| this.flush(); | ||
| }, | ||
| /** | ||
| * @ngdoc method | ||
| * @name $animate#flush | ||
| * @description | ||
| * | ||
| * This method is used to flush the pending callbacks and animation frames to either start | ||
| * an animation or conclude an animation. Note that this will not actually close an | ||
| * actively running animation (see {@link ngMock.$animate#closeAndFlush `closeAndFlush()`} for that). | ||
| */ | ||
| flush: function(hideErrors) { | ||
| $rootScope.$digest(); | ||
|
|
||
| var doNextRun, somethingFlushed = false; | ||
| @@ -814,7 +892,7 @@ angular.mock.animate = angular.module('ngAnimateMock', ['ng']) | ||
| } | ||
| } while (doNextRun); | ||
|
|
||
| if (!somethingFlushed) { | ||
| if (!somethingFlushed && !hideErrors) { | ||
| throw new Error('No pending animations ready to be closed or flushed'); | ||
| } | ||
|
|
||
| @@ -31,6 +31,28 @@ describe("$animateCss", function() { | ||
| expect(copiedOptions).toEqual(initialOptions); | ||
| })); | ||
|
|
||
| it("should not create a copy of the provided options if they have already been prepared earlier", | ||
| inject(function($animateCss, $$rAF) { | ||
|
|
||
| var options = { | ||
| from: { height: '50px' }, | ||
| to: { width: '50px' }, | ||
| addClass: 'one', | ||
| removeClass: 'two' | ||
| }; | ||
|
|
||
| options.$$prepared = true; | ||
| var runner = $animateCss(element, options).start(); | ||
| runner.end(); | ||
|
|
||
| $$rAF.flush(); | ||
|
|
||
| expect(options.addClass).toBeFalsy(); | ||
| expect(options.removeClass).toBeFalsy(); | ||
| expect(options.to).toBeFalsy(); | ||
| expect(options.from).toBeFalsy(); | ||
| })); | ||
|
|
||
| it("should apply the provided [from] CSS to the element", inject(function($animateCss) { | ||
| $animateCss(element, { from: { height: '50px' }}).start(); | ||
| expect(element.css('height')).toBe('50px'); | ||
| @@ -2036,6 +2036,28 @@ describe("ngAnimate $animateCss", function() { | ||
| expect(copiedOptions).toEqual(initialOptions); | ||
| })); | ||
|
|
||
| it("should not create a copy of the provided options if they have already been prepared earlier", | ||
| inject(function($animate, $animateCss) { | ||
|
|
||
| var options = { | ||
| from: { height: '50px' }, | ||
| to: { width: '50px' }, | ||
| addClass: 'one', | ||
| removeClass: 'two' | ||
| }; | ||
|
|
||
| options.$$prepared = true; | ||
| var runner = $animateCss(element, options).start(); | ||
| runner.end(); | ||
|
|
||
| $animate.flush(); | ||
|
|
||
| expect(options.addClass).toBeFalsy(); | ||
| expect(options.removeClass).toBeFalsy(); | ||
| expect(options.to).toBeFalsy(); | ||
| expect(options.from).toBeFalsy(); | ||
| })); | ||
|
|
||
| describe("[$$skipPreparationClasses]", function() { | ||
| it('should not apply and remove the preparation classes to the element when true', | ||
| inject(function($animateCss) { | ||
| @@ -28,6 +28,27 @@ describe('ngAnimate integration tests', function() { | ||
| describe('CSS animations', function() { | ||
| if (!browserSupportsCssAnimations()) return; | ||
|
|
||
| it("should only create a single copy of the provided animation options", | ||
| inject(function($rootScope, $rootElement, $animate) { | ||
|
|
||
| ss.addRule('.animate-me', 'transition:2s linear all;'); | ||
|
|
||
| var element = jqLite('<div class="animate-me"></div>'); | ||
| html(element); | ||
|
|
||
| var myOptions = {to: { 'color': 'red' }}; | ||
|
|
||
| var spy = spyOn(window, 'copy'); | ||
| expect(spy).not.toHaveBeenCalled(); | ||
|
|
||
| var animation = $animate.leave(element, myOptions); | ||
| $rootScope.$digest(); | ||
| $animate.flush(); | ||
|
|
||
| expect(spy).toHaveBeenCalledOnce(); | ||
| dealoc(element); | ||
| })); | ||
|
|
||
| they('should render an $prop animation', | ||
| ['enter', 'leave', 'move', 'addClass', 'removeClass', 'setClass'], function(event) { | ||
|
|
||
| @@ -390,6 +411,32 @@ describe('ngAnimate integration tests', function() { | ||
| expect(element).not.toHaveClass('hide'); | ||
| })); | ||
|
|
||
| it('should handle ng-if & ng-class with a class that is removed before its add animation has concluded', function() { | ||
| inject(function($animate, $rootScope, $compile, $timeout, $$rAF) { | ||
|
|
||
| ss.addRule('.animate-me', 'transition: all 0.5s;'); | ||
|
|
||
| element = jqLite('<section><div ng-if="true" class="animate-me" ng-class="{' + | ||
| 'red: red,' + | ||
| 'blue: blue' + | ||
| '}"></div></section>'); | ||
|
|
||
| html(element); | ||
| $rootScope.blue = true; | ||
| $rootScope.red = true; | ||
| $compile(element)($rootScope); | ||
| $rootScope.$digest(); | ||
|
|
||
| var child = element.find('div'); | ||
|
|
||
| // Trigger class removal before the add animation has been concluded | ||
| $rootScope.blue = false; | ||
| $animate.closeAndFlush(); | ||
|
|
||
| expect(child).toHaveClass('red'); | ||
| expect(child).not.toHaveClass('blue'); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('JS animations', function() { | ||
Oops, something went wrong.