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

fix($animate): ensure that parallel class-based animations are all eventually closed #7767

Closed
wants to merge 1 commit into from

Conversation

matsko
Copy link
Contributor

@matsko matsko commented Jun 10, 2014

When multiple classes are added/removed in parallel then $animate only closes off the
last animation when the fallback timer has expired. Now all animations are closed off.

Fixes #7766

@mary-poppins
Copy link

Mary Poppins needs to take a vacation.

@petebacondarwin
Copy link
Contributor

@matsko - the code and test look good but on the plunker there is a weird slight delay between the loading message disappearing and the actual button disappearing. I don't see why that should be happening. Any thoughts?

@matsko matsko added cla: yes and removed cla: no labels Jun 10, 2014
@btford btford added this to the 1.3.0-beta.12 milestone Jun 10, 2014
@danxshap
Copy link

I'm seeing the delay as well. My expectation would be that since both $scope.buttonVisible and $scope.loading are set to false simultaneously, you shouldn't be able to see the button label change back before the entire button disappears.

@matsko
Copy link
Contributor Author

matsko commented Jun 10, 2014

The delay is there since the ngShow animation commences just after the $timeout is done. The ngShow animation sets the space for the transition since it is applied globally to all animations that share the .smooth-hover-button class. So this works as expected.

@danxshap
Copy link

@matsko are you saying that the transition I have in the .smooth-hover-button class is affecting ngShow? I would think that this wouldn't be the case since I have transition-property: background-color; in that class, so shouldn't the transition only apply to background color changes, which ngShow does not involve?

@matsko
Copy link
Contributor Author

matsko commented Jul 1, 2014

@danxshap this works as expected. ngAnimate is not smart enough to deduce a transition that will take place based on the transition-property since getComputedStyle still returns the transition-duration value when the ngShow hook kicks off.

Notice how cancelling out the transition for ng-hide makes this work: http://plnkr.co/edit/7UhX1HBjnRyfAmOGWRwW?p=preview

…entually closed

When multiple classes are added/removed in parallel then $animate only closes off the
last animation when the fallback timer has expired. Now all animations are closed off.

Fixes angular#7766
@danxshap
Copy link

danxshap commented Jul 1, 2014

Ugh, some Github issue and my comment showed up twice, so I deleted 1 and now they're both gone!

Rewriting...

@matsko I'll admit I don't have an intimate understanding of the ngAnimate source (e.g. how getComputedStyle works), but I will say that in my opinion this would be a "gotcha" for anyone upgrading from 1.2 (note the delay does not happen here: http://plnkr.co/edit/je2xLV9kMfgFxrSFeW8A?p=preview)

From my perspective as an Angular user: I have a button which I've put a CSS transition on so that the background color changes smoothly on hover. Now I simply want to hide the button with Angular.

As you showed in your example, now I'll also have to remember to set the transition-duration property to 0 on my element when the ng-animate class is applied. I guess this isn't that big of a deal now that I'm aware of the issue, but I could definitely see this tripping up quite a few people.

My 2 cents.

@matsko
Copy link
Contributor Author

matsko commented Jul 1, 2014

Yes this is one of those unfortunate side-effects of relying too much on CSS. But sadly getComputedStyle is all we have and there is no way to detect to see if a transition actually kicked off or not without doing a deep copy of every style on getComputedStyle.

I've been searching for an answer for quite some time:
https://stackoverflow.com/questions/22571651/detecting-if-a-transition-takes-off

@danxshap
Copy link

danxshap commented Jul 1, 2014

It would be cool if I could put a class on my element like ng-dont-animate or something and then ngAnimate would just leave it alone. At a high level, sounds like that would be a useful solution :)

@matsko
Copy link
Contributor Author

matsko commented Jul 1, 2014

There's a provider flag called $animateProvider.classNameFilter that does the inverse of that.

@danxshap
Copy link

danxshap commented Jul 1, 2014

Nice, thanks for the tip! If you're not using ngAnimate for that many other elements on the page, that could definitely be a solution here. For anyone who stumbles across this, here's a quick demo (assuming you've checked out the examples above): http://plnkr.co/edit/13WQJe3p4wpHaPGNJRwc?p=preview

@matsko
Copy link
Contributor Author

matsko commented Jul 1, 2014

Very nice. Also (finally) this pull request came out to green. Merging this in now.

@matsko
Copy link
Contributor Author

matsko commented Jul 1, 2014

Merged. Landed as f07af61

@matsko matsko closed this Jul 1, 2014
@matsko
Copy link
Contributor Author

matsko commented Jul 1, 2014

Thank you @danxshap for all the help on finding this bug.

@danxshap
Copy link

danxshap commented Jul 1, 2014

Anytime!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ng-animate CSS class gets stuck on element w/existing CSS transition
7 participants