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

ngAnimate (v1.2.0rc1) : ngRepeat shows stale items #3613

Closed
damrbaby opened this issue Aug 15, 2013 · 44 comments
Closed

ngAnimate (v1.2.0rc1) : ngRepeat shows stale items #3613

damrbaby opened this issue Aug 15, 2013 · 44 comments

Comments

@damrbaby
Copy link
Contributor

When updating an array that is used for an ngRepeat, the list will momentarily show what appears to be the previous items together with the new items before updating with the new items. This only happens when ngAnimate is a dependency for the app.

Please see http://plnkr.co/edit/fqrI0w1j0HqtV3CTzsFI?p=preview

When you click on the different lists in the above example, you will see how it momentarily shows a combination of the previous and current lists before showing the current one.

Watch carefully, it happens very fast!

This buggy behavior will not occur when you remove ngAnimate.

This bug is serious because it makes ngAnimate virtually unusable for apps that use ngRepeat due to the flashing.

@damrbaby
Copy link
Contributor Author

After further investigation, I have found that the following lines of code are the culprit:

https://github.com/angular/angular.js/blob/master/src/ngAnimate/animate.js#L552
https://github.com/angular/angular.js/blob/master/src/ngAnimate/animate.js#L619

Basically if you remove $timeout from these lines, and just call the functions, the buggy behavior stops.

I think it comes down to ngAnimate is triggering an extra $digest via $timeout even when there is no CSS animation defined on a particular element.

@matsko
Copy link
Contributor

matsko commented Aug 19, 2013

Hey @damrbaby, the 1st $timeout is necessary so that the browser has a change to redraw the page and apply the styles. When we add the class onto the element then it can sniff the transition/keyframe durations and delays from the element. The 2nd one is absolutely required so that it can capture the animation in between.

Here are my proposed solutions:

  1. Change the 1st timeout duration to 0ms (it looks like with 1.2 it doesn't need to have a 1ms delay anymore to trigger the redraw of the page)
  2. Change the 2nd timeout such that if the duration is 0 then no timeout is called (maybe this will prevent the animation from occurring).

Can you possibly try setting the 1ms value to 0 with your code and see if that changes anything?

@damrbaby
Copy link
Contributor Author

@matsko Unfortunately both of your suggestions didn't work. I tried changing the first $timeout for startAnimation to 0ms instead of 1ms, and that doesn't solve the issue (see this plunkr). I'm assuming this is because a $timeout of 0 still is triggering an extra $digest outside of the current execution context.

I also tried the above change in addition to just calling the done() callback without wrapping it in $timeout, with no luck (see this plunkr)

The only solution is to change both of those lines to not use a $timeout at all. See this plunkr which results in the expected behavior. But that won't work if I did add CSS animations to the elements in question.

@matsko
Copy link
Contributor

matsko commented Aug 20, 2013

@damrbaby turns out that both $timeout calls are required, but not to worry, changing them around seems to have helped. Can you take a look at the link and verify if it has the flicker/stale items bug that you mentioned? Here's the link: https://s3.amazonaws.com/angularjs-dev/reflow-bug/example/animate.html

@damrbaby
Copy link
Contributor Author

@matsko the issue remains. If you change the .items class in your example to display: inline-block the flickering will be apparent there as well (in Chrome at least). It appears that the flickering is only apparent when elements are adjacent to each other either by using inline-block or floats.

@deakster
Copy link

Just to add, I am noticing flickering issues absolutely everywhere in my apps which I started to port.

As well as in situations with inline-block and floated adjacent elements, I am also experiencing similar issues in many other contexts.

For example with elements which are absolute positioned, where their position is derived based on dynamic CSS classes (set using ngClass). Whenever the classes on the element change, it appears that momentarily the element resets it's position to 0,0 before adjusting to their correct position. It behaves as if there is a split second when it has no classes, even though that is not the case.

@matsko
Copy link
Contributor

matsko commented Aug 20, 2013

@damrbaby the code is changed again, please try again (do a hard refresh since it's the same URL).
https://s3.amazonaws.com/angularjs-dev/reflow-bug/example/animate.html

And I copied your own demo to use the updated code:
https://s3.amazonaws.com/angularjs-dev/reflow-bug/example/animate2.html

The non-animated code should not touch any $timeout operation now.

@damrbaby
Copy link
Contributor Author

Hey @matsko, I can confirm that the flickering issue is solved for non-animations using the above code! Thanks!

@matsko
Copy link
Contributor

matsko commented Aug 21, 2013

You have no idea how happy I am to hear that @damrbaby!

@damrbaby
Copy link
Contributor Author

Yay! I don't want to ruin the party @matsko, but it appears that there is still a slight performance hit with an ngRepeat that has 100+ items with filters in each item (in my case). Off the cuff I'd say it's taking 30-50% longer to render the list when ngAnimate is a dependency. I actually momentarily see handlebars in chrome's web inspector with ngAnimate when I reassign a new array to ngRepeat, which I don't see when I remove ngAnimate. It's as if just having ngAnimate as a dependency for non-animated items is still causing some additional processing to take place.

@matsko
Copy link
Contributor

matsko commented Aug 23, 2013

@damrbaby we can focus on performance issues with ngRepeat + ngAnimate once this is merged into master.

@jwagner
Copy link

jwagner commented Aug 23, 2013

Wouldn't it be possible to get rid of the timeout by forcing a reflow by accessing a property like offsetWidth? That would probably be a lot faster.

See https://github.com/twbs/bootstrap/blob/f95ab89fb1da85ff0fcb95c43d4fe4af359e302a/js/tab.js#L77 for an example.

@matsko
Copy link
Contributor

matsko commented Aug 23, 2013

@jwagner would this work in cases where the width isn't being transitioned/animated? Say you change the background color?

matsko added a commit to matsko/angular.js that referenced this issue Aug 23, 2013
…imations are used

ngAnimate causes a 1ms flicker on the screen when no CSS animations are present on the element.
The solution is to change $animate to only use $timeouts when a duration is found on the element
before the transition/keyframe animation takes over.

Closes angular#3613
@jwagner
Copy link

jwagner commented Aug 23, 2013

@matsko
Copy link
Contributor

matsko commented Aug 23, 2013

Ouuu nice! Testing it out with ngAnimate...

@matsko matsko closed this as completed in ee2f3d2 Aug 23, 2013
@matsko
Copy link
Contributor

matsko commented Aug 23, 2013

Landed as ee2f3d2

@matsko
Copy link
Contributor

matsko commented Aug 23, 2013

@jwagner can you please make a new PR with your reflow fix? Just a small blurb with what you mentioned here.

@jwagner
Copy link

jwagner commented Aug 25, 2013

I'll have a look at it, but I'm not sure if I'll be able to find my way around the code in a reasonable amount of time. :)

@matsko
Copy link
Contributor

matsko commented Aug 25, 2013

No it doesn't have to have any code in AngularJS. Just copy and paste what you wrote here.

@jwagner
Copy link

jwagner commented Aug 26, 2013

@matsko I'm not quite sure what you mean by 'Just copy and paste what you wrote here.'. The comments?

In any case I think I've found the place where it belongs:
jwagner@5dcd1d3

But it has the side effect of breaking every other test (43 in total) for ng-animate. :(

@matsko
Copy link
Contributor

matsko commented Aug 29, 2013

@jwagner I've been using your prop('clientWidth') approach and so far it works great! I'm porting an older demo repository to use 1.2 just to be sure before I place the fix into master.

@jwagner
Copy link

jwagner commented Aug 31, 2013

@matsko Do you have it in a branch somewhere? I'd be happy to take it on a little test drive.

@matsko
Copy link
Contributor

matsko commented Sep 3, 2013

@jwagner #3801

@matsko
Copy link
Contributor

matsko commented Sep 4, 2013

@jwagner it's in master now.

@matsko
Copy link
Contributor

matsko commented Sep 5, 2013

@damrbaby things may end up being a bit faster after this PR gets merged: #3882 since the $timeout calls are getting removed in favour of transitionend/animationend CSS events (which are fired behind the scenes anyway).

@damrbaby
Copy link
Contributor Author

@matsko sorry to say still having issues with ngAnimate code on master causing noticeably laggy performance just by its inclusion that is especially noticable on slower devices like the iPod Touch 4g

@damrbaby
Copy link
Contributor Author

@matsko digging into it I think the lag is coming from the peformAnimation function, which I noticed is being called 50-100 times per digest in my application (and it does things like regex matching). The number of times it gets called seems directly related to the number of elements in my ngRepeat list. When I comment out peformAnimation the lagginess stops.

Is there a reason why peformAnimation is being called so many times when I don't even have any CSS animations on this particuar page?

@matsko
Copy link
Contributor

matsko commented Sep 23, 2013

@damrbaby the fix is in the works for the other PR that you posted: #4011

jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Sep 25, 2013
…imations are used

ngAnimate causes a 1ms flicker on the screen when no CSS animations are present on the element.
The solution is to change $animate to only use $timeouts when a duration is found on the element
before the transition/keyframe animation takes over.

Closes angular#3613
@cheahkhing
Copy link

I am not sure why this is closed, but i still see this issue on current rc3 latest code? Can anyone kindly show me the fix? To further illustrate, here is my reproduction plunker: http://plnkr.co/edit/lnsxCySFGmUmAnYDqVm3?p=preview

@deakster
Copy link

deakster commented Nov 9, 2013

@cheahkhing The reason you are still seeing the old items still in the list, is because you actually have a 1 second leave transition on the items in your example, so that behaviour is expected. They take 1 second to fully disappear, and in that one second the new items will be appearing. Since you are adding and removing items at the same time, it is expected that both the enter and leave transitions would fire at that time.

This issue was referring to a bug where even without a transition, there would be a momentary overlap of the two sets of items.

jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
…imations are used

ngAnimate causes a 1ms flicker on the screen when no CSS animations are present on the element.
The solution is to change $animate to only use $timeouts when a duration is found on the element
before the transition/keyframe animation takes over.

Closes angular#3613
@mbret
Copy link

mbret commented Mar 8, 2015

I, it seems that I'm still facing this issue with latest angular version. Whenever I import ng-animate in my module I got this problem. Any idea ?

@TheNewSound
Copy link

Same here,
i have an ng-repeat with ng-animate imported.
it updates with a new array of information every 40 seconds and you notice it quickly adds the new info instead of re-rendering it.

@bimal1331
Copy link

Yes, this is serious. I'm also facing this issue with Angular 1.3.5. Ended up removing 'ngAnimate' module from the app.

@charandas
Copy link

@bimal1331 Just add a css class like .repeat-modify to your ng-repeat elements, that you are experiencing the issue with. Then, in your app config block, you can add:

// Let ngAnimate know which elements to not handle
$animateProvider.classNameFilter(/^((?!(repeat-modify)).)*$/)

Then you can continue using ngAnimate in rest of your app.

@bimal1331
Copy link

Thanks @kbdaitch for the suggestion.

@onemenny
Copy link

having the same issue with angular 1.3.15 and bootstrap .form-control class on the repeated items.
@kbdaitch solution seems to solve this, but i'm not sure why animation works by default for repeater :(?

is anyone looking into this?

@buiducanh
Copy link

I have the same issue with Angular 1.3.16 on Foundation for Apps framework. @kbdaitch's solution works for me, and the elements are not being combined.

@privard
Copy link

privard commented Jun 26, 2015

@kbdaitch's solution definitely works. Thanks!

@mbret
Copy link

mbret commented Jun 26, 2015

Any test with angular 1.4 without having to modify something ?

@philBrown
Copy link
Contributor

Seeing this in 1.4.1. The repeated elements have no transitions or keyframe animations.

Will try and get a Plunker example going.

@philBrown
Copy link
Contributor

Plunker example ~ http://plnkr.co/edit/uReg3o5JQ03zrVNN9t8P?p=preview

Putting this together, I noticed that it actually is due to a transition on the item elements however that transition is only on the border property. I feel like this shouldn't effect animations however I see why it does.

Overriding the transition with

transition: none;

fixes the issue.

@lucasloliveira
Copy link

Had the same problem with 1.4.0 and @philBrown suggestion fixed it.

@matsko
Copy link
Contributor

matsko commented Jul 16, 2015

The problem is that .thumbnail (in bootstrap) sets a transition value on it for hover effects. When ngAnimate comes across thumbnail for animation-purposes it thinks that it will trigger itself for the animation.

In addition to the other methods of side-stepping this, another solution is to simply do:

.thumbnail.ng-animate { transition: none; }

You can also place the thumbnail as its own element within the element containing the ng-repeat directive.

Unfortunately there's nothing we can do to detect this due to the nature of specificity. Angular 2.0 will have a better approach to this, but Angular 1.x can't be changed.

@matsko
Copy link
Contributor

matsko commented Jul 16, 2015

I will be locking this issue since anything involving CSS detection may result in a misinterpretation of detected transition and/or keyframe styles.

So if you have a CSS style like:

div { transition:0.5s linear all; }
div:hover { outline:5px solid red; }

Then anytime a div is animated via ngAnimate it will assume that a transition of 0.5s linear all applies to the enter/leave/move or class-based animation. This in turn results in a waiting delay during which the elements may appear on screen when they are not supposed to be.

The unfortunate thing here is that some frameworks like boostrap and gumby spinkle transition values onto elements in a global way.

The solutions to fix this are to:

  1. Place transitions yourself onto elements that you know you want to animate through ngAnimate.
  2. Move elements that already contain a default transition or keyframe to be a child of an element that is managed by Angular
  3. Use $animateProvider.classNameFilter() to set a regex that only allows specific elements with specific classes to be animated.
  4. Use .some-class.ng-animate { transition:none; keyframe:0s none; } to cut out animations on any elements/selectors that contain a global transition and/or keyframe.

If you do happen to have an issue that shows up despite using any of these solutions then please open a new issue.

@angular angular locked and limited conversation to collaborators Jul 16, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.