refactor($animate): use CSS3 transition/animation events instead of $timeouts to track ongoing animations #3882
Conversation
|
|
||
| //but some still use vendor-prefixed styles | ||
| var vendorAnimationProp = $sniffer.vendorPrefix + 'Animation'; | ||
| var vendorTransitionProp = $sniffer.vendorPrefix + 'Transition'; | ||
| var vendorAnimationEvent = $sniffer.vendorPrefix + 'AnimationEnd'; | ||
| var vendorTransitionEvent = $sniffer.vendorPrefix + 'TransitionEnd'; |
There was a problem hiding this comment.
I think it's a little more complicated than this. WebkitTransitionEnd is not the correct name. The vendor prefixed events seem to be: webkitTransitionEnd, oTransitionEnd, otransitionend, and MSTransitionEnd.
There was a problem hiding this comment.
MSTransitionEnd doesn't seem to be in use. This makes sense since IE10 doesn't require a -ms- prefix. http://msdn.microsoft.com/en-us/library/ie/hh673535(v=vs.85).aspx#transitions_dom_events
There was a problem hiding this comment.
A simple .toLowerCase() should do it then. That's what I did for https://github.com/mgcrea/angular-touch-nav
There was a problem hiding this comment.
Regarding setting styles on the element, Firefox uses uppercase for it's Moz prefix. Webkit can use both upper and lowercase. Older versions of opera use the uppercase OTransition while new versions omit the O prefix. I'm testing further with the events now...
There was a problem hiding this comment.
@matsko Opera 12.1x, i.e. the oldest Presto Opera doesn't need a prefix, Firefox has long dropped it as well. I think only the WebKit prefix is needed.
There was a problem hiding this comment.
Webkit can be both so I think the code is fine. Maybe I should run a test through browser stack just to be sure.
There was a problem hiding this comment.
Looks like webkit has to be lowercase for this particular event as well as transition end.
There was a problem hiding this comment.
Even more reasons to just special-case WebKit here and get rid of the rest.
There was a problem hiding this comment.
Moz was taken out in FF16. I doubt anyone is using that version since it's almost 10 versions back on the current version of FF.
|
This PR needs to work with this issue first: #3874 |
|
|
||
| //but some still use vendor-prefixed styles | ||
| var vendorAnimationProp = $sniffer.vendorPrefix + 'Animation'; | ||
| var vendorTransitionProp = $sniffer.vendorPrefix + 'Transition'; | ||
| var vendorAnimationEvent = $sniffer.vendorPrefix.toLowerCase() + 'AnimationEnd'; | ||
| var vendorTransitionEvent = $sniffer.vendorPrefix.toLowerCase() + 'TransitionEnd'; |
There was a problem hiding this comment.
Why not just use transitionendEvent variable defined as follows:
var transitionendEvent = window.ontransitionend !== undefined ?
'transitionend' :
window.onwebkittransitionend !== undefined ? 'webkitTransitionEnd' : undefined;similarly with animationend? All the detection would be in one place, it would eliminate further duplication.
There was a problem hiding this comment.
@matsko This is a good point. Can we simplify this in future PR?
There was a problem hiding this comment.
@mhevery Sure, if that's easier for you. :)
There was a problem hiding this comment.
@mhevery By "we" did you mean that sb has plans to do that or did you ask me to create a PR with a change? :)
…timeouts to track ongoing animations Closes angular#3629 Closes angular#3874
|
Seems like the tests fail on this one: http://ci.angularjs.org/view/Misko's/job/angular.js-misko/846/console |
Closes #3629
Closes #3874