search

angular / angular.js

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

Race conditions with existing animation runner (replace, templateUrl) #13215

Closed Foxandxss closed 8 years ago

Foxandxss commented 9 years ago

Hello,

I have an issue that I am not really sure if it is something that ngAnimate should fix or it has to be managed in the end applications.

I am using $animate.enter to bring some DOM to the screen and then $animate.leave to delete them (a popup message). Works good. The problem comes when you try to make them leave while they are running. The new ngAnimate (1.4.0+) has a new mechanism to cancel running animations, but it fails under some corner cases.

Check this plunker (Angular 1.4.7) Check this plunker (Angular 1.4.7, no ngAnimate) Check this plunker (Angular 1.3.20, ngAnimate)

The close behavior in there is crappy, but reproduces my issue. My first impression is that even when .leave is called with a proper element, it gets deleted in the mean time so when it reaches ngAnimate, there is nothing to work with and it throws an exception.

This is a poor reproduction of Foxandxss/angular-toastr#136

There is a plunker with the real issue. Also going back to 1.3.20 fixes it.

So, should I somehow prevent this? Or ngAnimate should be able to do this correctly? I want to think about the latter since it used to work.

Thank you.

Narretz commented 8 years ago

If it worked before, it's most likely a regression. Thanks for reporting.

Narretz commented 8 years ago

This is due to a combination of replace and templateUrl on the directive. Replace is also deprecated, you should consider not using it. You could also use the template Property instead of templateUrl.

However, I've noticed another bug: after the leave animation finishes, the element is still in the DOM until some timeout finishes. This happens only if an enter animation is still running.

Foxandxss commented 8 years ago

I have noticed that latter bug indeed.

I am planning a 2.0.0 version of my lib without replace, not sure if that would fix it. About using template instead of templateUrl, not sure if I can make it without making it more problematic for end users. Keep in mind that it is a library and not a end application. As today people are struggling about caching templates before use them (which is problematic if you don't), if I need to make them do anything complicated to change to template.

So I am not asking Angular to workaround this for me. If this is something solved in userland, I will workaround myself, but I think it is still a regression.

Narretz commented 8 years ago

I think we should definitely fix the different behavior of templateUrl vs. template property, but as replace has been deprecated since 1.3?, it just means you shouldn't bet on this being fixed.

Foxandxss commented 8 years ago

You know what is the problem with replace? That once you have it, it is hard to move on without it. In the case of my library, it is pretty tied to that, and if you look to ui-bootstrap, we have lots in there that are not easy to remove.

I will rewrite my whole CSS to remove replace and for ui-bootstrap I will do what is needed for the 1.0.0 release that is close.

Thanks @Narretz

matsko commented 8 years ago

@Foxandxss sorry for this taking a few months to get resolved.

Your best bet is to use template and to include the contents of the toast directly into the directive definition. This avoids a couple of extra digest cycles and the http download of toast.html (which is what causes the race condition in the first place).

We ran into this issue with transclusion as well and there was no proper solution. If your code is still relying on replace:true then maybe what can happen is that ngAnimate inspects the element to see if the element will be replaced and then it skips animations for the to-be-replaced directive element. This way when the replaced value kicks in then there are no animations in the way. Is this something you still need?

Foxandxss commented 8 years ago

Can I use template but still allow custom templates? I will think about that.

About replace, yes, I have to remove it. The CSS is inherited from the library I created the original rewrite and it is made for a concrete html structure that I cannot replicate without replace. So I need to rewrite the CSS which is another history.

samypr100 commented 8 years ago

Any updates on this? I'm really sorry for the bump.

wesleycho commented 8 years ago

This should probably be closed given that replace: true has been marked as deprecated for a while.

We encountered issues in UI Bootstrap as well I believe, but we're in the process of stripping out replace: true (finally!)

gkalpak commented 8 years ago

I agree that since replace: true has been deprecated, this corner-case isn't likely to be fixed. FWIW, it (kind of) works if you defer adding the element to the list of toasts to be removed, until the enter animation has been completed: demo (There is still an error in the console, but it seems to work :disappointed: )