NgAnimate - Transition flickers on it's beginning when using NgClass and ng-animate class

[FRSgit]

I'm submitting a ...

• [x] bug report
• [x] feature request
• [ ] other

Current behavior: When changing class with NgClass directive transition style seems to be overriden by something on animation start, so animation doesn't work when easing in, only when easing out (only when using ng-animate class to contain animation styling).

Expected / new behavior: Transition style should be taken into account also when animation begins.

Minimal reproduction of the problem with instructions: latest snapshot - not working

1.6.9 (current stable version) - not working

1.4.0-rc.0 - not working

1.4.0-beta.6 - working

AngularJS version: Regression happened in 1.4.0-rc.0 and still exist (tested in latest snapshot - see above).

Browser: Firefox / Chrome / Opera / Edge (tested on Windows 10, but I believe it doesn't matter).

Anything else: Something broke when refactoring code between 1.4.0-rc.0 and 1.4.0-beta.6. I tried to trace this issue but too many things was moved in the code for me to follow.

[gkalpak]

I believe the current behavior is intentional (although ngAnimate is pretty complex and it is hard to tell for sure :grin:). You can achieve what you want by setting the styles on the animationClass-add/remove-active classes: updated plnkr

I am going to close this, since changing the behavior would be a breaking change (which not an option atm).

[FRSgit]

I didn't posted my real question, because I thought it's not necessary since it's a clear bug in the library. But, if needed here is plunker showing something more similar to my actual use case and problem.

Basically I have a situation when, using ngClass, class on my element is being frequently changed (let's say on each user click, like in example above) and I need to react (with animation) on each class change with fadeOut/fadeIn (I don't care/know what class is being added/removed - there are over a dozen of different classes which can be there).

I guess that's an exact scenario when ng-animate class could be used, but, unfortunately, it isn't working.

As I tested a little bit more I found out everything is working when !important is added to transition declaration like here. But, since using importants is an antipattern and can lead to future problems (app I develope is rather big & will be maintained also in the future for a few years) it cannot be considered a solution.

About this change being an breaking change:

• it's not a breaking change, but a fix of a regression which happened 2 minor versions ago,
• if we consider what's now as a "correct" behaviour then, still, somebody didn't thinked of that change as a breaking when it was introduced in 1.4.0, so neither should we.

Can anybody at least quickly introduce me to the library's code so I can try to fix this bug?

[gkalpak]

it's a clear bug in the library

As mentioned in my previous comment, I believe the current behavior is intentional; not a bug. The idea is that you can specify "base" styles (using .ng-animate in the selector) that will be applied only for the duration of the animation (not while the element is not being animated) and will be applied immediately before starting the animation (without being themselves subject to animation). Essentially, this sets your element up for the animation to begin. (Also see here.)

I understand this will not be desirable in certain cases, but this is how it is implemented (and changing the implementation will break many usecases that depend on the current behavior).

I don't care/know what class is being added/removed - there are over a dozen of different classes which can be there.

Indeed it is not ideal, but still possible to define styles for all these classes. (There are other solutions available - keep reading :smiley:)

I guess that's an exact scenario when ng-animate class could be used, but, unfortunately, it isn't working.

No, it is not (see above) :grin:

As I tested a little bit more I found out everything is working when !important is added to transition declaration.

This is because we use style.transitionDelay to "disabled" animations while adding the ng-animate and other preparation classes (whose styles should not be animated). (See there.)

It's not a breaking change, but a fix of a regression which happened 2 minor versions ago. If we consider what's now as a "correct" behaviour then, still, somebody didn't think of that change as breaking when it was introduced in 1.4.0, so neither should we.

Every coin has two sides :smiley: In any case, this change was intentional (as indicated by the comments in the source code linked above). There was a big ngAnimate rewrite that landed in 1.4.0-rc.0 (with several breaking changes). It was almost a rewrite from scratch and it was really hard to keep track of all the changes. I guess we missed this one 😞 (You are more than welcome to contribute docs/changelog improvements.)

Can anybody at least quickly introduce me to the library's code so I can try to fix this bug?

@FRSgit, meet $animate, $animateCss, $animateRunner, and the rest of the crew. ngAnimate, meet @FRSgit. Have fun and be nice to each other.

Kidding aside, this is not something we intend to change at this point :grin:

As mentioned above, an easier/shorter fix (if you don't want to explicitly add a selector for each added class), would be to target ngClass animations via a [class*="-add"][class*="-add-active"] selector (which should be good enough, even if not perfect):

Updated plnkr

Sorry for the long post :flushed: Hopefully there is some value in there for you ☮️

[FRSgit]

Hey @gkalpak, thanks for long response, it was very useful to say at least!

I have only one question, which don't have to be answered, but after searching the internet and checking out performance time by myself I cannot answer it by myself and want just to know (litte curious nerd here):

I don't see the reason why transition-delay is added in the first place - do you know what was the case which provided any unintended behaviour and because of which this feature was added?

As I checked in the console, there is no difference in time of code evaluation when adding/removing class with/without transition style set (at least no visible/measurable when adding classes on multiple elements at once). I guess somebody wanted to avoid any incoming transitions set on ngAnimate's initial classes (so I suppose on ng-animate, ng-animate-shim, etc), but I cannot see any particular reason for that.

On the other hand, if somebody wants to have a transition there what's the problem with letting him to? As you see, in my scenario there is no good option to go using CSS (having class checking on -add, and -add-active could lead to efficiency problems since I rely on ngClass animations in lots of places and in my case having every class listed for transition in scss is just unmaintainable).

[FRSgit]

Okay, just FYI @gkalpak, I finally got it working as expected with somehow-clear-solution - see plunker. Sorry, I completely forgot about ngAnimateSwap directive.

I found out a one thing there - if you attach transition to ng-animate (as it is working for ngIf animations - example from docs) the animation doesn't fire for ng-enter (plunker). Would this one be considered as a bug then?

Also, I still have the question - (which was elaborated above) what is the reason for this transition-delay added before adding ng-animate class?

And as a last I want to improve docs a bit, so ngAnimateSwap would be little more visible. First of all I think it would be good to add ngAnimateSwap to the directives supporting animations table. Secondly I would like to create separated paragraph about this use case on ngAnimate's module docs page, e.g. as Animating any scope's variable change. What do you think?

Thanks for your time & assistance!

[gkalpak]

I don't see the reason why transition-delay is added in the first place - do you know what was the case which provided any unintended behaviour and because of which this feature was added?

The negative delay is essentially disabling any animations/transitions that may apply to the element. This is because we want the styles associated with the .ng-animate class (if any) to be applied immediately, without animating.

As you see, in my scenario there is no good option to go using CSS (having class checking on -add, and -add-active could lead to efficiency problems.

I am not sure this is true (e.g. if you use another class to differentiate the ngClass transitions that need to be animated). E.g. .staticClass[class*="-add"][class*="-add-active"]. But I am not that familiar with the inner workings of CSS selector matching. TBH, I was thinking it would be noce if we added ng-class/ng-class-active classes (similar to ng-enter/ng-enter-active etc) for all ngClass related operations. This way, it would be much easier to target the animating elements whose animation was triggered by ngClass.

I finally got it working as expected with somehow-clear-solution [...] using the ngAnimateSwap directive.

If this fits your usecase, that's great. One thing to keep in mind though, is that ngAnimateSwap is a structural directive, which means that it creates a new instance of the element (including any directives it may have) and links it to a new scope every time you swap. In some cases, this might not be desirable (e.g. for performance reasons, or when wishing to retain internal state on the original element instance).

If you attach transition to .ng-animate [...] the animation doesn't fire for .ng-enter. Would this one be considered as a bug then?

From a brief look, it does look unexpected. Could be a bug. I need to investigate more.

And as a last I want to improve docs a bit, so ngAnimateSwap would be little more visible.

That would be awesome. You are more than welcome to do so :smiley: Your suggestions sound reasonable. Feel free to open a PR and we can discuss the details there.

[gkalpak]

If you attach transition to .ng-animate [...] the animation doesn't fire for .ng-enter. Would this one be considered as a bug then?

Fortunately, it turns out everything works as intended 😌 It is a styling issue. Essentially, by applying opacity: 0 to .ng-enter, it also affects, .ng-enter.ng-enter-active (which is not what you want). You should let opacity be 0 for .ng-enter, but not for .ng-enter.ng-enter-active. One way to do it:

.staticClass.ng-leave-active,
.staticClass.ng-enter:not(.ng-enter-active) {
  opacity: 0;
}

(Updated plnkr)

[FRSgit]

The negative delay is essentially disabling any animations/transitions that may apply to the element. This is because we want the styles associated with the .ng-animate class (if any) to be applied immediately, without animating.

Well, that doesn't answer to question why at all, but I understand it's approach you choose (unfortunately breaking stuff from versions 1.4.0-) and want to stick with it just to have compatibility since 1.4.0.

I am not sure this is true (e.g. if you use another class to differentiate the ngClass transitions that need to be animated). E.g. .staticClass[class="-add"][class="-add-active"]. But I am not that familiar with the inner workings of CSS selector matching.

Well, I'm not 100% sure about exact browser's implementations as well. Selectors by default are read from last part to first, so in this case every element with class ending with -add or -add-active needs to be checked for staticClass as well (and if have any parent classes in selector these will need to be checked as well). BTW. even plnkr shows warning on line with .staticClass[class*="-add"][class*="-add-active"] 😄

TBH, I was thinking it would be noce if we added ng-class/ng-class-active classes (similar to ng-enter/ng-enter-active etc) for all ngClass related operations. This way, it would be much easier to target the animating elements whose animation was triggered by ngClass.

+1 from me on that!

Fortunately, it turns out everything works as intended 😌 It is a styling issue. Essentially, by applying opacity: 0 to .ng-enter, it also affects, .ng-enter.ng-enter-active (which is not what you want). You should let opacity be 0 for .ng-enter, but not for .ng-enter.ng-enter-active.

That would be awesome. You are more than welcome to do so 😃 Your suggestions sound reasonable. Feel free to open a PR and we can discuss the details there.

Sure, look for me in pull requests - probably something will land till Friday 😃

[gkalpak]

Well, that doesn't answer to question why at all.

Sorry, I misunderstood the question. The reason why we want to apply the styles associated with .ng-animate immediately (i.e. with animation/transition) is that .ng-animate is applied for the duration of the animation and it might define styles that you want to be applied to an animated element (without being animated themselves).

From my previous comment:

The idea is that you can specify "base" styles (using .ng-animate in the selector) that will be applied only for the duration of the animation (not while the element is not being animated) and will be applied immediately before starting the animation (without being themselves subject to animation). Essentially, this sets your element up for the animation to begin.

For example, you might have an element with transition: all 1s that you want to slide in/out. And you also want your animating element to have a special transform: scale(2). But you don't want to animate from scale(1) to scale(2) while sliding in/out. You want it to be immediately grown 2x when the animation is about to begin, then you animate it in/out, then it returns back to scale(1) (without animation). .ng-animate is the place to define such styles.

[FRSgit]

@gkalpak Docs have been updated in two pull requests from me. I allowed myself to quote you directly in new section in ngAnimate module, hope you don't mind 😄

TBH, I was thinking it would be noce if we added ng-class/ng-class-active classes (similar to ng-enter/ng-enter-active etc) for all ngClass related operations. This way, it would be much easier to target the animating elements whose animation was triggered by ngClass. Do you know if anyone did any work to bring this functionality? If not, would it be okay if I try to do so? Should something like this be done on ngClass directive level, or somewhere more globally, like animate/animateCss services?

[gkalpak]

Awesome, I'll take a look asap.

Do you know if anyone did any work to bring this functionality? If not, would it be okay if I try to do so?

You are welcome to take it on, but be warned that it won't be an easy undertaking :smiley:

Should something like this be done on ngClass directive level, or somewhere more globally, like animate/animateCss services?

Good question. I haven't thought everything through, but here are some off-the-top-of-my-head thoughts:

• Should this only affect ngClass or also class="{{ interlocated-class }}"? This needs to be taken into account in the implementation.
• Should this apply to all class-adding/removing animations? (I strongly lean towards "no".)
• If this should only affect ngClass (and not interpolated class), then the change needs to "start" inside the ngClass directive.
• If this should also affect interpolated class, then the change needs to "start" inside Attributes' $addClass/$removeClass/$updateClassaddAttrInterpolateDirective().
• In any case, since the classes should be integrated with ngAnimate, the main implementation should be inside ngAnimate (possibly its addClass/removeClass methods).

[gkalpak]

cc @Narretz (who is more experienced in ngAnimate than me)

[Narretz]

Interesting. I haven't read everything, but as far as I can see @gkalpak has covered the implementation considerations. One questions. Why

Should this apply to all class-adding/removing animations? (I strongly lean towards "no".)

Why shouldn't it apply to all class based animations?

[gkalpak]

I think it will get too noisy. Besides, most animated classes are specific and easily targeted (e.g. ng-dirty/pristine/valid/invalid/active/inactive/hide etc). The ones that can have any value are those added/removed by ngClass/interpolated class attribute.

[FRSgit]

most animated classes are specific and easily targeted (e.g. ng-dirty/pristine/valid/invalid/active/inactive/hide etc). The ones that can have any value are those added/removed by ngClass/interpolated class attribute.

Actually I can think of use cases when you want to animate on every change of other "animatable directives" than ngClass (e.g. every ngModel's state change).

I think it will get too noisy.

I agree with that. 2 more classes on every animation of every directive is not a small amount. However, we have to remember that there is the solution which allows to limit "noise" made by ngAnimate module (BTW which is "out-of-the-box" quite big now) - I look at you customFilter and classNameFilter - and probably most of production applications are already using it. When you have limited ngAnimate's scope to small bunch of elements - then this noise wouldn't be noticable (performance-wise).

Also - there would be need to create new event name for this type of classes - maybe change or active? (I'm talking here about 2nd parameter of customFilter method).

[FRSgit]

Ping Any other arguments for/against?

[Narretz]

I'd be more in favor of a general solution, i.e. apply the classes for every $animate.addClass etc. action. It's hard to say how big the perf impact is without an implementation, and since we are closing feature dev at the end of June the latest, there's really only a window of about 2 weeks here to implement and test this. If you want to go ahead with a basic implementation, we can take a look at it, but won't guarantee it'll actually make it.