Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngAnimate): $timeout without invokeApply #12282

Closed

Conversation

startswithaj
Copy link
Contributor

Currently when using ngAnimate in-conjunction
with ng-repeat a $rootScope.$digest happens
per element animated.
This change calls $timeout with the invokeApply
parameter set to false which stops ngAnimate
from invoking its changes inside an $apply block.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). 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.

@startswithaj
Copy link
Contributor Author

resolves #12281

I have signed the cla

This change calls $timeout with the invokeApply
parameter set to false which stops ngAnimate
from invoking its changes inside an $apply block
@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Jul 7, 2015
@wesleycho
Copy link
Contributor

This looks like a good change to me, but probably needs another pair of eyes to be sure.

@simon-lang
Copy link

+1 thanks

@startswithaj
Copy link
Contributor Author

There are plunkrs that demo this bug on #12281

@ntr-808
Copy link

ntr-808 commented Jul 9, 2015

solid change +1

@startswithaj
Copy link
Contributor Author

@wesleycho the $timeout definitely shouldn't be forcing an apply but the original code doesn't make total sense to me. So I agree it needs another set of eyes.

The $timeout queues up onAnimationExpired() to run after a given time period but then says in the code comments the below.

        function onAnimationExpired() {
          // although an expired animation is a failed animation, getting to
          // this outcome is very easy if the CSS code screws up. Therefore we
          // should still continue normally as if the animation completed correctly.
          close();
        }

A reference to the $timeout isn't stored and cancelled if the animation is successful in onAnimationProgress(). So the $timedout function onAnimationExpired() runs for every element regardless of where its failed or not. So it goes
onAnimationExpired ->
close() ->
if (animationClosed || (animationCompleted && animationPaused)) return;

And then because of no invokeApply false passed to the timeout it does a full apply/digest cycle per element.

If we do end up in the close() block and indeed the animation hasn't finished and we need to clean up the element for whatever reason this may necessitate a digest() (but im not sure). I put in the code below "// IF WE GET HERE MAYBE DIGEST ON THE SCOPE OF THE ELEMENT" where this should probably happen.

function close(rejected) { // jshint ignore:line
        // if the promise has been called already then we shouldn't close
        // the animation again
        if (animationClosed || (animationCompleted && animationPaused)) return;
        animationClosed = true;
        animationPaused = false;

        $$jqLite.removeClass(element, setupClasses);
        $$jqLite.removeClass(element, activeClasses);

        blockKeyframeAnimations(node, false);
        blockTransitions(node, false);

        forEach(temporaryStyles, function(entry) {
          // There is only one way to remove inline style properties entirely from elements.
          // By using `removeProperty` this works, but we need to convert camel-cased CSS
          // styles down to hyphenated values.
          node.style[entry[0]] = '';
        });

        applyAnimationClasses(element, options);
        applyAnimationStyles(element, options);

        // the reason why we have this option is to allow a synchronous closing callback
        // that is fired as SOON as the animation ends (when the CSS is removed) or if
        // the animation never takes off at all. A good example is a leave animation since
        // the element must be removed just after the animation is over or else the element
        // will appear on screen for one animation frame causing an overbearing flicker.
        if (options.onDone) {
          // IF WE GET HERE MAYBE DIGEST ON THE SCOPE OF THE ELEMENT
          options.onDone();
        }

        // if the preparation function fails then the promise is not setup
        if (runner) {
          runner.complete(!rejected);
        }
      }


@matsko
Copy link
Contributor

matsko commented Jul 16, 2015

This is great. Thank you for finding this.

@matsko matsko closed this in 7db5f36 Jul 16, 2015
@matsko
Copy link
Contributor

matsko commented Jul 16, 2015

Merged in now to master. Thanks again for finding this.

@startswithaj startswithaj deleted the animage-timeout-fix branch July 28, 2015 00:08
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
This change calls $timeout with the invokeApply
parameter set to false which stops ngAnimate
from invoking its changes inside an $apply block

Closes angular#12281
Closes angular#12282
ggershoni pushed a commit to ggershoni/angular.js that referenced this pull request Sep 29, 2015
This change calls $timeout with the invokeApply
parameter set to false which stops ngAnimate
from invoking its changes inside an $apply block

Closes angular#12281
Closes angular#12282
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants