Skip to content

Commit 3ff6348

Browse files
gkalpakgkalpak
committed
fix($animateCss): avoid flicker caused by temporary classes
Currently, this only affects `ngHide`/`ngShow` animations (but the problem was more general and could theoretically affect other animations in the future). Adding the temporary classes immediately, while forcing a reflow before starting the animation, could cause flickering. This commit fixes the issue, by not applying the temporary classes until the animation actually starts. Since `tempClasses` was an undocumented, private feature (surrently only used for `ngHide`/`ngShow`), it will not affect custom animations. Alternative approach to angular#15463. Fixes angular#14015 Closes angular#15463
1 parent 7a667c7 commit 3ff6348

File tree

7 files changed

+163
-15
lines changed

7 files changed

+163
-15
lines changed

‎src/ngAnimate/animateCss.js

+5
Original file line numberDiff line numberDiff line change
@@ -835,6 +835,11 @@ var $AnimateCssProvider = ['$animateProvider', /** @this */ function($animatePro
835835
return;
836836
}
837837

838+
if (options.tempClasses) {
839+
$$jqLite.addClass(element, options.tempClasses);
840+
options.tempClasses = null;
841+
}
842+
838843
// even though we only pause keyframe animations here the pause flag
839844
// will still happen when transitions are used. Only the transition will
840845
// not be paused since that is not possible. If the animation ends when

‎src/ngAnimate/animateJs.js

+5
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,11 @@ var $$AnimateJsProvider = ['$animateProvider', /** @this */ function($animatePro
9090
return runner;
9191
}
9292

93+
if (options.tempClasses) {
94+
$$jqLite.addClass(element, options.tempClasses);
95+
options.tempClasses = null;
96+
}
97+
9398
runner = new $$AnimateRunner();
9499
var closeActiveAnimations;
95100
var chain = [];

‎src/ngAnimate/animation.js

+2-7
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,6 @@ var $$AnimationProvider = ['$animateProvider', /** @this */ function($animatePro
134134
var tempClasses = options.tempClasses;
135135
if (tempClasses) {
136136
classes += ' ' + tempClasses;
137-
options.tempClasses = null;
138137
}
139138

140139
var prepareClassName;
@@ -185,9 +184,8 @@ var $$AnimationProvider = ['$animateProvider', /** @this */ function($animatePro
185184
toBeSortedAnimations.push({
186185
domNode: getDomNode(animationEntry.from ? animationEntry.from.element : animationEntry.element),
187186
fn: function triggerAnimationStart() {
188-
// it's important that we apply the `ng-animate` CSS class and the
189-
// temporary classes before we do any driver invoking since these
190-
// CSS classes may be required for proper CSS detection.
187+
// It is important that we apply the `ng-animate` CSS class before we do any driver
188+
// invoking since these CSS classes may be required for proper CSS detection.
191189
animationEntry.beforeStart();
192190

193191
var startAnimationFn, closeFn = animationEntry.close;
@@ -360,9 +358,6 @@ var $$AnimationProvider = ['$animateProvider', /** @this */ function($animatePro
360358

361359
function beforeStart() {
362360
element.addClass(NG_ANIMATE_CLASSNAME);
363-
if (tempClasses) {
364-
$$jqLite.addClass(element, tempClasses);
365-
}
366361
if (prepareClassName) {
367362
$$jqLite.removeClass(element, prepareClassName);
368363
prepareClassName = null;

‎test/ngAnimate/animateCssSpec.js

+20
Original file line numberDiff line numberDiff line change
@@ -1641,6 +1641,26 @@ describe('ngAnimate $animateCss', function() {
16411641
expect(element).toHaveClass('super-active');
16421642
}));
16431643

1644+
it('should apply `tempClasses` when starting the animation',
1645+
inject(function($animateCss, $document, $rootElement) {
1646+
1647+
var element = angular.element('<div></div>');
1648+
$rootElement.append(element);
1649+
$document.find('body').append($rootElement);
1650+
1651+
$animateCss(element, {
1652+
event: 'super',
1653+
duration: 1000,
1654+
to: fakeStyle,
1655+
tempClasses: 'foo'
1656+
}).start();
1657+
1658+
expect(element).not.toHaveClass('foo');
1659+
1660+
triggerAnimationStartFrame();
1661+
expect(element).toHaveClass('foo');
1662+
}));
1663+
16441664
describe('structural animations', function() {
16451665
they('should decorate the element with the ng-$prop CSS class',
16461666
['enter', 'leave', 'move'], function(event) {

‎test/ngAnimate/animateJsSpec.js

+18-1
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ describe('ngAnimate $$animateJs', function() {
193193
});
194194
});
195195

196-
it('should always run the provided animation in atleast one RAF frame if defined', function() {
196+
it('should always run the provided animation in at least one RAF frame if defined', function() {
197197
var before, after, endCalled;
198198
module(function($animateProvider) {
199199
$animateProvider.register('.the-end', function() {
@@ -325,6 +325,23 @@ describe('ngAnimate $$animateJs', function() {
325325
});
326326
});
327327

328+
it('should apply `tempClasses` when starting the animation', function() {
329+
module(function($animateProvider) {
330+
$animateProvider.register('.foo-element', function() {
331+
return {foo: noop};
332+
});
333+
});
334+
inject(function($$animateJs, $animate, $rootScope) {
335+
var element = jqLite('<div class="foo-element"></div>');
336+
var animator = $$animateJs(element, 'foo', 'foo-element', {tempClasses: 'temp'});
337+
338+
expect(element).not.toHaveClass('temp');
339+
340+
animator.start();
341+
expect(element).toHaveClass('temp');
342+
});
343+
});
344+
328345
describe('events', function() {
329346
var animations, runAnimation, element, log;
330347
beforeEach(module(function($animateProvider) {

‎test/ngAnimate/animationSpec.js

+48-7
Original file line numberDiff line numberDiff line change
@@ -859,13 +859,14 @@ describe('$$animation', function() {
859859
element = jqLite('<div></div>');
860860
parent = jqLite('<div></div>');
861861

862-
return function($$AnimateRunner, $rootElement, $document) {
862+
return function($$AnimateRunner, $$jqLite, $rootElement, $document) {
863863
jqLite($document[0].body).append($rootElement);
864864
$rootElement.append(parent);
865865

866-
mockedDriverFn = function(element, method, options, domOperation) {
866+
mockedDriverFn = function(animationEntry) {
867867
return {
868868
start: function() {
869+
$$jqLite.addClass(animationEntry.element, animationEntry.options.tempClasses);
869870
runner = new $$AnimateRunner();
870871
return runner;
871872
}
@@ -874,7 +875,7 @@ describe('$$animation', function() {
874875
};
875876
}));
876877

877-
it('should temporarily assign the provided CSS class for the duration of the animation',
878+
it('should temporarily assign the provided CSS classes for the duration of the animation',
878879
inject(function($rootScope, $$animation) {
879880

880881
parent.append(element);
@@ -894,6 +895,26 @@ describe('$$animation', function() {
894895
expect(element).not.toHaveClass('fudge');
895896
}));
896897

898+
it('should remove the temporary CSS classes if the animation gets cancelled',
899+
inject(function($rootScope, $$animation) {
900+
901+
parent.append(element);
902+
903+
$$animation(element, 'enter', {
904+
tempClasses: 'temporary fudge'
905+
});
906+
$rootScope.$digest();
907+
908+
expect(element).toHaveClass('temporary');
909+
expect(element).toHaveClass('fudge');
910+
911+
runner.cancel();
912+
$rootScope.$digest();
913+
914+
expect(element).not.toHaveClass('temporary');
915+
expect(element).not.toHaveClass('fudge');
916+
}));
917+
897918
it('should add and remove the ng-animate CSS class when the animation is active',
898919
inject(function($$animation, $rootScope) {
899920

@@ -910,11 +931,9 @@ describe('$$animation', function() {
910931
}));
911932

912933

913-
it('should apply the `ng-animate` and temporary CSS classes before the driver is invoked', function() {
934+
it('should apply the `ng-animate` CSS class before the driver is invoked', function() {
914935
var capturedElementClasses;
915936

916-
parent.append(element);
917-
918937
module(function($provide) {
919938
$provide.factory('mockedTestDriver', function() {
920939
return function(details) {
@@ -932,7 +951,29 @@ describe('$$animation', function() {
932951
$rootScope.$digest();
933952

934953
expect(capturedElementClasses).toMatch(/\bng-animate\b/);
935-
expect(capturedElementClasses).toMatch(/\btemp-class-name\b/);
954+
});
955+
});
956+
957+
it('should not apply `tempClasses` before the driver is invoked', function() {
958+
var capturedElementClasses;
959+
960+
module(function($provide) {
961+
$provide.factory('mockedTestDriver', function($$jqLite) {
962+
return function(details) {
963+
capturedElementClasses = details.element.attr('class');
964+
};
965+
});
966+
});
967+
968+
inject(function($$animation, $animate, $rootScope) {
969+
parent.append(element);
970+
971+
$$animation(element, 'enter', {
972+
tempClasses: 'temp-class-name'
973+
});
974+
$rootScope.$digest();
975+
976+
expect(capturedElementClasses).not.toMatch(/\btemp-class-name\b/);
936977
});
937978
});
938979

‎test/ngAnimate/integrationSpec.js

+65
Original file line numberDiff line numberDiff line change
@@ -544,6 +544,71 @@ describe('ngAnimate integration tests', function() {
544544
expect(child).not.toHaveClass('blue');
545545
});
546546
});
547+
548+
it('should apply a temporary class only for the actual duration of the animation', function() {
549+
var elementClassList = [];
550+
var hasTempClass = {
551+
beforeCssAnimationStart: null,
552+
afterCssAnimationStart: null,
553+
afterCssAnimationTriggered: null,
554+
afterCssAnimationFinished: null
555+
};
556+
557+
module(function($provide) {
558+
$provide.decorator('$animateCss', function($delegate) {
559+
var decorated = function(element) {
560+
var animator = $delegate.apply(null, arguments);
561+
var startFn = animator.start;
562+
563+
animator.start = function() {
564+
hasTempClass.beforeCssAnimationStart = element.hasClass('temp-class');
565+
var runner = startFn.apply(animator, arguments);
566+
hasTempClass.afterCssAnimationStart = element.hasClass('temp-class');
567+
return runner;
568+
};
569+
570+
return animator;
571+
};
572+
573+
return decorated;
574+
});
575+
});
576+
577+
inject(function($animate, $compile, $rootScope, $rootElement, $$rAF) {
578+
element = jqLite('<div class="animate-me"></div>');
579+
$compile(element)($rootScope);
580+
581+
var className = 'klass';
582+
var parent = jqLite('<div></div>');
583+
html(parent);
584+
585+
parent.append(element);
586+
587+
ss.addRule('.animate-me', 'transition: all 2s;');
588+
589+
var runner = $animate.addClass(element, className, {
590+
tempClasses: 'temp-class'
591+
});
592+
593+
$rootScope.$digest();
594+
$animate.flush();
595+
596+
hasTempClass.afterCssAnimationTriggered = element.hasClass('temp-class');
597+
598+
browserTrigger(element, 'transitionend', {timeStamp: Date.now(), elapsedTime: 2});
599+
$rootScope.$digest();
600+
$animate.flush();
601+
602+
hasTempClass.afterCssAnimationFinished = element.hasClass('temp-class');
603+
604+
expect(hasTempClass).toEqual({
605+
beforeCssAnimationStart: false,
606+
afterCssAnimationStart: false,
607+
afterCssAnimationTriggered: true,
608+
afterCssAnimationFinished: false
609+
});
610+
});
611+
});
547612
});
548613

549614
describe('JS animations', function() {

0 commit comments

Comments
 (0)