angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.8k stars 27.48k forks source link

$animate - feature request - proceed animation just for special marked nodes #5283

Closed ludinov closed 10 years ago

ludinov commented 10 years ago

In our app we use huge amount of ng-if/switch/repeat/class ... but only couple of them really should support animation (in terms of ngAnimate). But for each element with these directives anguar run lot of unnecessary checks, style lookups (that often trigger reflow) and so on...

Maybe it makes sense to run extended animation only for elements with ng-animate attribute. Or at least have some directive (or attribute check, or rule/method for $animate service) that disable animation for certain elements.

If this functionality about kind of partial limitations already exist (or some common technique) please let us know (in docs).

For mobile apps it can save some ticks and increase performance at no costs...

tbosch commented 10 years ago

@matsko Could you answer the question here?

matsko commented 10 years ago

@ludinov this issue has appeared before and we put a lot of work to fine tune the performance of ngAnimate. Infact, we boosted the speed by 5000% between RC2 and RC3.

As browsers get better and better this issue will slowly go away, but yes mobile browsers are still quite slow. Unfortunately we can't add anything into the AngularJS core which allows you to disable animations within the template since animations should be exclusively defined in CSS/JS and only referenced in HTML.

But if you really want to do this then you can just make your own directive and disable the animations within the compile function.

myModule.directive('skipNgAnimate', function($animate) {
  return {
    restrict : 'E',
    compile : function(element) {
      $animate.enabled(false, element);
    }
  };
});

This way you can set it as a CSS class on your element.

<div ng-include="tpl" class="skip-ng-animate"></div>
<div ng-if="exp" class="skip-ng-animate"></div>
<div class="skip-ng-animate">
  <div ng-repeat="exp"></div>
</div>

The $animate.enabled(false, element) operation will disable animations for child elements (not the element that it was defined on). So if you really want to disable it for the element that it is on then you can place it on the directive before (or just modify the directive code).

ludinov commented 10 years ago

@matsko Thank you very mush for your reply and once again for amazing job you do for animate module. Just couple off-topic notes: . possible typo in #L799, #L802

      function cancelAnimations(animations) {
        var isCancelledFlag = true;
        forEach(animations, function(animation) {
          if(!animations.beforeComplete) {
            (animation.beforeEnd || noop)(isCancelledFlag);
          }
          if(!animations.afterComplete) {
            (animation.afterEnd || noop)(isCancelledFlag);
          }
        });
      }

animation[s] instead of animation

. #L786 - node.querySelectorAll('.' + NG_ANIMATE_CLASS_NAME).

Probably node.getElementsByClassName(NG_ANIMATE_CLASS_NAME) - should work bit faster and less buggy (http://ejohn.org/blog/thoughts-on-queryselectorall/)

. Animation still cause some "dom nodes phantom leak" (https://github.com/angular/angular.js/issues/4864), that more likely is debugger/chrome bug, but it is hard to find the real application problems when you see these "phantoms". So I'm still trying to find the glitch line in the animate.js :)

deakster commented 10 years ago

I feel that in most cases where this is a concern, including @ludinov's where he mentions "only couple of them really should support animation", the inverse of a skipNgAnimate would be the most useful, and result in the least amount of markup noise/complexity.

Basically, being able to disable animation globally, but being able to turn it on selectively in specific areas.

cburgdorf commented 10 years ago

Yep, I feel the same. I think the css based animations are really great but why not just moving all the work behind a ng-animatable attribute. So only when a DOM node has this attribute then the additional work to check for such css classes would be done. In addition there could be a ng-children-animatable attribute or something like that to also include all the child DOM nodes. This could be placed directly on the html node if one wants to have the current behaviour where each node is being checked for animations.

matsko commented 10 years ago

The entire purpose of ngAnimate is to move all animation code into CSS or into a separate JS file containing only JS animations with the least amount of coupling possible to the HTML template. Having a CSS class present on element is all that you need to trigger the animation, but also having another attribute to enable/disable animations brings in a more strict coupling. It would need to be something custom in your own code (like I provided above) or a better caching system inside of $animate to make this perform better.

deakster commented 10 years ago

Yea, I think the default behaviour is fine as is, i.e. animations work automatically across the board, using CSS only. In most cases, this is what you would want and expect.

There definitely are situations where this isn't ideal (larger apps / mobile / performance sensitive apps) where an "explicit" animation mode would be desirable.

Assuming you need this control, then I think a per-element "opt-in" mechanism would be more useful in most cases than a per-element "opt-out". Opting out every single ngRepeat, ngShow, ngClass etc, except for the few things that you actually do want animated, would add far more coupling and noise than having the few things that you do want animated opt-in.

Even conceptually the way I would approach the problem then is "Right, what features do I actually want animated", rather than "Right, what are all the things I do not want animated". The stuff I don't want animated is literally everything, except for the things I specifically created animations for in my CSS/JS.

I don't mind if this functionality is outside of angular-animate, but is it possible? I just assumed when/if I need to do that, I can just disable animation globally, and selectively turn it on for the handful of important elements that benefit the most from it.

matsko commented 10 years ago

@deakster great point, but at the moment if you disable globally then they're all disabled and you cannot enable items within. Although I do agree this should be changed. Could you open up another issue pointing this out?

deakster commented 10 years ago

@matsko Yep, see #5357