fix(ngAnimate): remove event prepare class when skipping animation #16677

swuttich · 2018-08-28T22:42:40Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
It can happen that an element keeps the ng-enter-prepare css class (maybe for other events as well) when animation is skipped. This can result for example in unwanted invisible elements in your DOM in case prepare phase is using display:none or opacity:0 to avoid blink during animation.

What is the new behavior (if this is a feature change)?
Prepare class is removed when skipping animation.

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:

googlebot · 2018-08-28T22:43:37Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

googlebot · 2018-08-28T22:43:42Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

swuttich · 2018-08-28T22:53:11Z

CLA signed.

googlebot · 2018-08-28T22:53:13Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2018-08-28T22:53:13Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2018-08-28T23:06:09Z

CLAs look good, thanks!

googlebot · 2018-08-28T23:06:10Z

CLAs look good, thanks!

…imation has no duration Background: ngAnimate writes helper classes to DOM elements to see if animations are defined on them. If many elements have the same definition, and the same parent, we can cache the definition and skip the application of the helper classes altogether. This helps particularly with large ngRepeat collections. Closes #14165 Closes #14166 Closes #16613

Narretz · 2018-08-29T16:59:56Z

Thanks for the PR! This looks good at first glance. However, we also need a test for this behavior. Writing tests for ngAnimate isn't the easiest thing, unfortunately. But if you want to give it a shot, here are some tests regarding preparation.

angular.js/test/ngAnimate/integrationSpec.js

Lines 365 to 469 in a553735

    
           it('should add the preparation class for an enter animation before a parent class-based animation is applied', function() { 
        
             module('ngAnimateMock'); 
        
             inject(function($animate, $compile, $rootScope, $rootElement, $document) { 
        
               element = jqLite( 
        
                 '<div ng-class="{parent:exp}">' + 
        
                   '<div ng-if="exp">' + 
        
                   '</div>' + 
        
                 '</div>' 
        
               ); 
        
               ss.addRule('.ng-enter', 'transition:2s linear all;'); 
        
               ss.addRule('.parent-add', 'transition:5s linear all;'); 
        
               $rootElement.append(element); 
        
               jqLite($document[0].body).append($rootElement); 
        
               $compile(element)($rootScope); 
        
               $rootScope.exp = true; 
        
               $rootScope.$digest(); 
        
               var parent = element; 
        
               var child = element.find('div'); 
        
               expect(parent).not.toHaveClass('parent'); 
        
               expect(parent).toHaveClass('parent-add'); 
        
               expect(child).not.toHaveClass('ng-enter'); 
        
               expect(child).toHaveClass('ng-enter-prepare'); 
        
               $animate.flush(); 
        
               expect(parent).toHaveClass('parent parent-add parent-add-active'); 
        
               expect(child).toHaveClass('ng-enter ng-enter-active'); 
        
               expect(child).not.toHaveClass('ng-enter-prepare'); 
        
             }); 
        
           }); 
        
           it('should avoid adding the ng-enter-prepare method to a parent structural animation that contains child animations', function() { 
        
             module('ngAnimateMock'); 
        
             inject(function($animate, $compile, $rootScope, $rootElement, $document, $$rAF) { 
        
               element = jqLite( 
        
                 '<div ng-animate-children="true">' + 
        
                   '<div ng-if="parent" class="parent">' + 
        
                     '<div ng-if="child" class="child">' + 
        
                       '<div ng-class="{something:true}"></div>' + 
        
                     '</div>' + 
        
                   '</div>' + 
        
                 '</div>' 
        
               ); 
        
               ss.addRule('.ng-enter', 'transition:2s linear all;'); 
        
               $rootElement.append(element); 
        
               jqLite($document[0].body).append($rootElement); 
        
               $compile(element)($rootScope); 
        
               $rootScope.parent = true; 
        
               $rootScope.child = true; 
        
               $rootScope.$digest(); 
        
               var parent = jqLite(element[0].querySelector('.parent')); 
        
               var child = jqLite(element[0].querySelector('.child')); 
        
               expect(parent).not.toHaveClass('ng-enter-prepare'); 
        
               expect(child).toHaveClass('ng-enter-prepare'); 
        
               $$rAF.flush(); 
        
               expect(parent).not.toHaveClass('ng-enter-prepare'); 
        
               expect(child).not.toHaveClass('ng-enter-prepare'); 
        
             }); 
        
           }); 
        
           it('should add the preparation class for an enter animation before a parent class-based animation is applied', function() { 
        
             module('ngAnimateMock'); 
        
             inject(function($animate, $compile, $rootScope, $rootElement, $document) { 
        
               element = jqLite( 
        
                 '<div ng-class="{parent:exp}">' + 
        
                   '<div ng-if="exp">' + 
        
                   '</div>' + 
        
                 '</div>' 
        
               ); 
        
               ss.addRule('.ng-enter', 'transition:2s linear all;'); 
        
               ss.addRule('.parent-add', 'transition:5s linear all;'); 
        
               $rootElement.append(element); 
        
               jqLite($document[0].body).append($rootElement); 
        
               $compile(element)($rootScope); 
        
               $rootScope.exp = true; 
        
               $rootScope.$digest(); 
        
               var parent = element; 
        
               var child = element.find('div'); 
        
               expect(parent).not.toHaveClass('parent'); 
        
               expect(parent).toHaveClass('parent-add'); 
        
               expect(child).not.toHaveClass('ng-enter'); 
        
               expect(child).toHaveClass('ng-enter-prepare'); 
        
               $animate.flush(); 
        
               expect(parent).toHaveClass('parent parent-add parent-add-active'); 
        
               expect(child).toHaveClass('ng-enter ng-enter-active'); 
        
               expect(child).not.toHaveClass('ng-enter-prepare'); 
        
             }); 
        
           });

It'll also help if you can tell me how exactly the animation is skipped in your case.

swuttich · 2018-08-30T10:12:42Z

src/ngAnimate/animation.js

@@ -187,7 +187,7 @@ var $$AnimationProvider = ['$animateProvider', /** @this */ function($animatePro
          var element = animationEntry.from ? animationEntry.from.element : animationEntry.element;
          var extraClasses = options.addClass;
          extraClasses = (extraClasses ? (extraClasses + ' ') : '') + NG_ANIMATE_CLASSNAME;
-          var cacheKey = $$animateCache.cacheKey(node, event, extraClasses, options.removeClass);
+          var cacheKey = $$animateCache.cacheKey(node, animationEntry.event, extraClasses, options.removeClass);


I'm still unable to isolate the steps which lead to the issue. While trying to find out I managed to get why it leaves on the containsCachedAnimationWithoutDuration() check. I can see that there is a leave and an enter animation getting triggered. Both are in the animation queue, but when code goes through the animations it creates the cache key based on the first event (leave in this case). When it later comes to the actual execution of the animation it first handles the leave, containsCachedAnimationWithoutDuration() returns false as this key is not in the cache yet and then adds it to cache later (with !valid as the transition-duration is null). Then later the same happens for the enter animation, but due to the same cache key as the leave animation containsCachedAnimationWithoutDuration() now finds the existing non valid entry and returns true and the enter is skipped.

Any idea how the duration can be null?

So I managed now to reproduce it

http://plnkr.co/edit/6CsoTul8LBIGBcHfioYJ
(hitting action, when text goes red it means we are into the situation).

So you need a directive and changing ng-switch and changing ng-class as far as I see. No idea how to put that into a test.

Thanks, I'll take a look. I take it the custom directive is necessary? replace: true always complicates things.

Any chance that one can trigger the animation events for an element in the tests? Might be simpler than rebuilding the complex structure.

There's animationSpec.js, which tests this behavior somewhat in isolation:

angular.js/test/ngAnimate/animationSpec.js

Lines 551 to 571 in a553735

they('should remove the preparation class before the $prop-animation starts',

['enter', 'leave', 'move'], function(animationType) {

inject(function($$animation, $rootScope, $$rAF) {

var expectedClassName = 'ng-' + animationType + '-prepare';

var child = jqLite('<div></div>');

element.append(child);

$$animation(element, animationType);

$$animation(child, animationType);

$rootScope.$digest();

expect(element).not.toHaveClass(expectedClassName);

expect(child).toHaveClass(expectedClassName);

$$rAF.flush();

expect(element).not.toHaveClass(expectedClassName);

expect(child).not.toHaveClass(expectedClassName);

});

});

But since the cacheKey is created in animateCss, I'm not sure it's possible to recreate the case here.
Maybe we'll have to build a full integration test including the custom directive first.

I managed to trim down your plnkr to what I think is the minimum to reproduce the problem: http://plnkr.co/edit/m5CaegjmdtpR1pUX7zZ8?p=preview I'll try to put this into a test, then it should be easier to see what happens when. Interestingly, the ngClass directive is necessary for the bug to appear.

Any idea how the duration can be null?

ngAnimate is reading possible animation styles from the element during animation. So if you have an element / directive that could technically be animated, it will look on the classes to find animations. If none are found (basically the default), the animation duration is null. Or did you mean null as in null and not 0?

I've opened a PR #16681 that simplifies the fix and adds tests

…tions Closes angular#16677

…tions Closes #16681 Closes #16677

…tions Closes angular#16681 Closes angular#16677

swuttich changed the base branch from master to v1.7.x August 28, 2018 22:43

googlebot added the cla: no label Aug 28, 2018

swuttich force-pushed the missing_removal_prepare_class branch from 51cb9a7 to feb01ef Compare August 28, 2018 23:06

googlebot added cla: yes and removed cla: no labels Aug 28, 2018

swuttich changed the base branch from v1.7.x to master August 29, 2018 07:38

swuttich force-pushed the missing_removal_prepare_class branch from feb01ef to 5a022bf Compare August 29, 2018 07:38

Narretz added component: ngAnimate type: bug severity: regression labels Aug 29, 2018

Narretz added this to the 1.7.x milestone Aug 29, 2018

Narretz added the needs: test label Aug 29, 2018

fix(ngAnimate): remove event prepare class when skipping animation

0761de2

swuttich force-pushed the missing_removal_prepare_class branch from 5a022bf to 0761de2 Compare August 30, 2018 09:59

swuttich commented Aug 30, 2018

View reviewed changes

Narretz added a commit to Narretz/angular.js that referenced this pull request Sep 3, 2018

fix(ngAnimate): remove prepare classes with multiple structural anima…

c6b718a

…tions Closes angular#16677

Narretz mentioned this pull request Sep 3, 2018

fix(ngAnimate): remove prepare classes with multiple structural anima… #16681

Merged

3 tasks

Narretz closed this in #16681 Sep 6, 2018

Narretz added a commit that referenced this pull request Sep 6, 2018

fix(ngAnimate): remove prepare classes with multiple structural anima…

44cc823

…tions Closes #16681 Closes #16677

mgol pushed a commit to mgol/angular.js that referenced this pull request Sep 6, 2018

fix(ngAnimate): remove prepare classes with multiple structural anima…

3105b2c

…tions Closes angular#16681 Closes angular#16677

dependencies bot mentioned this pull request Sep 7, 2018

Update 7 dependencies from npm JetBrains/ring-ui#399

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ngAnimate): remove event prepare class when skipping animation #16677

fix(ngAnimate): remove event prepare class when skipping animation #16677

swuttich commented Aug 28, 2018

googlebot commented Aug 28, 2018

googlebot commented Aug 28, 2018

swuttich commented Aug 28, 2018

googlebot commented Aug 28, 2018

googlebot commented Aug 28, 2018

googlebot commented Aug 28, 2018

googlebot commented Aug 28, 2018

Narretz commented Aug 29, 2018

swuttich Aug 30, 2018

swuttich Aug 30, 2018

Narretz Aug 30, 2018

swuttich Aug 31, 2018

Narretz Aug 31, 2018

Narretz Sep 3, 2018

Narretz Sep 3, 2018

	they('should remove the preparation class before the $prop-animation starts',
	['enter', 'leave', 'move'], function(animationType) {
	inject(function($$animation, $rootScope, $$rAF) {
	var expectedClassName = 'ng-' + animationType + '-prepare';

	var child = jqLite('<div></div>');
	element.append(child);

	$$animation(element, animationType);
	$$animation(child, animationType);
	$rootScope.$digest();

	expect(element).not.toHaveClass(expectedClassName);
	expect(child).toHaveClass(expectedClassName);

	$$rAF.flush();

	expect(element).not.toHaveClass(expectedClassName);
	expect(child).not.toHaveClass(expectedClassName);
	});
	});

fix(ngAnimate): remove event prepare class when skipping animation #16677

fix(ngAnimate): remove event prepare class when skipping animation #16677

Conversation

swuttich commented Aug 28, 2018

googlebot commented Aug 28, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Aug 28, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

swuttich commented Aug 28, 2018

googlebot commented Aug 28, 2018

googlebot commented Aug 28, 2018

googlebot commented Aug 28, 2018

googlebot commented Aug 28, 2018

Narretz commented Aug 29, 2018

swuttich Aug 30, 2018

Choose a reason for hiding this comment

swuttich Aug 30, 2018

Choose a reason for hiding this comment

Narretz Aug 30, 2018

Choose a reason for hiding this comment

swuttich Aug 31, 2018

Choose a reason for hiding this comment

Narretz Aug 31, 2018

Choose a reason for hiding this comment

Narretz Sep 3, 2018

Choose a reason for hiding this comment

Narretz Sep 3, 2018

Choose a reason for hiding this comment