fix(ngAnimate): properly cancel-out previously running class-based an… #13822
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
1 similar comment
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
| @@ -374,11 +373,10 @@ var $$AnimateQueueProvider = ['$animateProvider', function($animateProvider) { | |||
| close(); | |||
| return runner; | |||
| } else { | |||
| mergeAnimationOptions(element, existingAnimation.options, options); | |||
| mergeAnimationDetails(element, existingAnimation.options, options); | |||
There was a problem hiding this comment.
How come we don't need to pass the animation object, but the options (considering all other invocation changed) ?
There was a problem hiding this comment.
Gooood question. Or, better question: why is there no failing test ....
| @@ -25,6 +25,32 @@ describe('ngAnimate integration tests', function() { | |||
| ss.destroy(); | |||
| }); | |||
|
|
|||
| it('should cancel a running and started removeClass animation when a follow-up addClass animation adds the same class', | |||
There was a problem hiding this comment.
what does it mean to be "running and started"?
There was a problem hiding this comment.
Yes, another very good question. Internally, there's a running state, but that can mean two things: first, the animation process waits for the next digest to happen; then it flushes an requestAnimationFrame (for the reflow) to trigger the actual animation start. The previous test didn'T flush the rAF, and so the scenario wasn't created.
Narretz
force-pushed
the
fix-animate-class
branch
from
e25036d
to
c22560c
Compare
…imations Prior to this fix the addition and removal of a CSS class via ngAnimate would cause flicker effects because $animate was unable to keep track of the CSS classes once they were applied to the element. This fix ensures that ngAnimate always keeps a reference to the classes in the currently running animation so that cancelling works accordingly. The commit also adds a test for a previously untested animation merge path. Closes angular#10156 Closes angular#13822
|
@gkalpak I've updated the merge code and added a test for this path. PTAL. |
|
LGTM |
…imations Prior to this fix the addition and removal of a CSS class via ngAnimate would cause flicker effects because $animate was unable to keep track of the CSS classes once they were applied to the element. This fix ensures that ngAnimate always keeps a reference to the classes in the currently running animation so that cancelling works accordingly. The commit also adds a test for a previously untested animation merge path. Closes angular#10156 Closes angular#13822
Narretz
force-pushed
the
fix-animate-class
branch
from
c22560c
to
f43938e
Compare
…imations Prior to this fix the addition and removal of a CSS class via ngAnimate would cause flicker effects because $animate was unable to keep track of the CSS classes once they were applied to the element. This fix ensures that ngAnimate always keeps a reference to the classes in the currently running animation so that cancelling works accordingly. The commit also adds a test for a previously untested animation merge path. Closes #10156 Closes #13822
…imations
Prior to this fix the addition and removal of a CSS class via
ngAnimate would cause flicker effects because $animate was unable
to keep track of the CSS classes once they were applied to the
element. This fix ensures that ngAnimate always keeps a reference
to the classes in the currently running animation so that cancelling
works accordingly.
Closes #10156
Closes #13814
This updated PR adds a new test that ensures the buggy scenario is correctly reproduced. We need to actively start the animation by flushing a requestAnimationFrame, otherwise the addClass / removeClass values are not correct.