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

ngAnimate 1.2 final, inline style and ngStyle are ignored during animations. #4869

Closed
deakster opened this issue Nov 10, 2013 · 13 comments
Closed

Comments

@deakster
Copy link

See plunkr for example: http://plnkr.co/edit/4Ve9tQ8ybLIjcOinOS7E?p=preview . The 70px padding only kicks in after the animation completes.

Basically, during animation any styles that are applied either by an inline style attribute on the element, or using ngStyle, are ignored for the duration of the transition, and then re-applied after the transition completes.

The styles do not need to have anything to do with the animation or the transitioned properties.

This issue was also not present in 1.2RC3

@deakster
Copy link
Author

Ah, just found #4851 which I guess is pretty much the same issue, although here I point out and demonstrate that even styles unrelated to the animation/transition are ignored, and also that it is not just ngStyle, but standard inline style is ignored too.

Last edit: just realised that even styles applied using external javascript or custom angular directives, like element.css(...) are also ignored.

@matsko
Copy link
Contributor

matsko commented Nov 11, 2013

@deakster great find! Can you make a test for this?

@deakster
Copy link
Author

@matsko I have added a test to the PR in the latest commit.

@DrHofman
Copy link

I can confirm deakster PR fixes issue 1251

@matsko
Copy link
Contributor

matsko commented Nov 11, 2013

Tests look good. @deakster could you possibly merge both commits together?

@deakster
Copy link
Author

@matsko Yep, commits merged now.

@deakster
Copy link
Author

@matsko I was just thinking, while this is definitely a worthwhile fix for now, there is also a fundamental issue with the approach of taking a snapshot of the styles prior to animating, and then restoring those styles after animation.

Especially when using ngStyle, it is entirely possible for the set of styles the element should have to change during the time that an ngAnimate animation is progressing. See this plunkr: http://plnkr.co/edit/AD95ZxZzuJsLgTdTXsD5?p=preview , the items after the 5 second fade in transition will have their color set to blue half way through by ngStyle, but then ngAnimate will overwrite that back to the old values once it finishes, leaving the items in an incorrect state.

Perhaps a better approach would be for ngAnimate to track what styles it applies, so that it can remove those selectively, rather just resetting to the old, possibly outdated values?

@matsko
Copy link
Contributor

matsko commented Nov 12, 2013

@deakster that's a great point. I also found a bug that your test brought up with ngAnimate so I'll make that fix first and resume the styling fix afterwards. I guess for now I'll discard your earlier commit. Let me know if you're still interested in doing the next fix.

The styles tracked are: transition-delay, transition-property, transition-duration, animation-delay, animation-duration.

@deakster
Copy link
Author

Yep, I can have a look at that tonight.

deakster pushed a commit to deakster/angular.js that referenced this issue Nov 13, 2013
… also ensure that outdated styles aren't restored on completion. Fixes angular#4869.
deakster pushed a commit to deakster/angular.js that referenced this issue Nov 13, 2013
… also ensure that outdated styles aren't restored on completion. Fixes angular#4869.
@deakster
Copy link
Author

@matsko I have updated my PR with the fix we discussed. So this should now resolve the issue of custom styles being ignored during animations, as well as the issue where animations could set outdated styles on an element on completion.

Tests included to cover both cases, and Travis CI seems to like it finally too!

@wilsonjackson
Copy link

I just ran into the issue of changed styles being overwritten by ngAnimate. Nice fix. Hope to see this make it into master soon.

@matsko
Copy link
Contributor

matsko commented Nov 19, 2013

Once this PR: #5015 is in then I'll rebase your code and merge :)

@matsko
Copy link
Contributor

matsko commented Nov 22, 2013

@deakster please reply to my commit review messages here: matsko@84b498f#commitcomment-4671946

@matsko matsko closed this as completed in c42d0a0 Nov 22, 2013
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.