New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSSTransitionGroup buggy due to transition events not firing reliably #1326

Closed
mariusk opened this Issue Mar 29, 2014 · 42 comments

Comments

Projects
None yet
@mariusk

mariusk commented Mar 29, 2014

While trying to convince React to replace DOM nodes in-place (which seems is not supported), I've discovered something that looks like bugs. I've put up a demo on http://jsbin.com/hokahaze/6/edit .

First of all, if a node gets replaced while animation is progressing, it is not removed. In the demo it means additional lines are created. As soon as the animation completes, the node usually goes away.

The second "feature" here is that if you leave that demo running, change to another tab, wait 10 seconds and change back to the tab with the demo running, it will now have accumulated lots of "green items" which are basically stuck (meaning their animation completed, but they were not removed from the dom).

@mariusk

This comment has been minimized.

Show comment
Hide comment
@mariusk

mariusk Mar 29, 2014

FWIW, I've also tried adding the leave transitions as well where I try to stop whatever animation is in progress (tested with "animation: 0;", and repeating the original transition setting with the opaque value set to it's final value), but animation does not seem to stop.

mariusk commented Mar 29, 2014

FWIW, I've also tried adding the leave transitions as well where I try to stop whatever animation is in progress (tested with "animation: 0;", and repeating the original transition setting with the opaque value set to it's final value), but animation does not seem to stop.

@irae

This comment has been minimized.

Show comment
Hide comment
@irae

irae Mar 30, 2014

Contributor

I spent some time debugging that. The part of leaving the tab and cumming back is due to transtionEnd probably not firing during page visibility being false, it is actually something that I believe that CSSTransitionGroup should handle.

Having many items is not actually a bug, it's caused by your transition duration. TransitionGroup low level documentation is explicit about holding on to the element until componentWillEnter callback is fired.

CSSTransitionGroup on the other hand bind both willEnter and willLeave. So in the example above, your transitionEnter takes 1.5 seconds of animation but you fire the mutation block each second, it's expected to overlap, but it is a bit hard to understand.

I am working on a proposal to fix for that, but the current model is also focused on performance. If the blocking behavior is removed or scoped to types of transitions, it may cause a lot of DOM manipulation. So I am experimenting a bit to see if I find a sweet spot in between. I am curious about opinion from someone from the core team.

Contributor

irae commented Mar 30, 2014

I spent some time debugging that. The part of leaving the tab and cumming back is due to transtionEnd probably not firing during page visibility being false, it is actually something that I believe that CSSTransitionGroup should handle.

Having many items is not actually a bug, it's caused by your transition duration. TransitionGroup low level documentation is explicit about holding on to the element until componentWillEnter callback is fired.

CSSTransitionGroup on the other hand bind both willEnter and willLeave. So in the example above, your transitionEnter takes 1.5 seconds of animation but you fire the mutation block each second, it's expected to overlap, but it is a bit hard to understand.

I am working on a proposal to fix for that, but the current model is also focused on performance. If the blocking behavior is removed or scoped to types of transitions, it may cause a lot of DOM manipulation. So I am experimenting a bit to see if I find a sweet spot in between. I am curious about opinion from someone from the core team.

@irae

This comment has been minimized.

Show comment
Hide comment
@irae

irae Mar 30, 2014

Contributor

More details about the tab switching problem: The error of not removing the elements seems to happen only on Firefox and Safari. Chrome was fine for me. The timeout warnings fire on all browsers, but Chrome still triggers the animationEnd event although very delayed.

Contributor

irae commented Mar 30, 2014

More details about the tab switching problem: The error of not removing the elements seems to happen only on Firefox and Safari. Chrome was fine for me. The timeout warnings fire on all browsers, but Chrome still triggers the animationEnd event although very delayed.

@bjyoungblood

This comment has been minimized.

Show comment
Hide comment
@bjyoungblood

bjyoungblood Apr 10, 2014

I'm having a similar problem in Chrome (haven't tested other browsers yet). Not sure if it helps, but I'm able to get the elements to remove by clearing and re-adding the opacity property in the inspector. Doing that to one element causes all of the "stuck" ones to be removed as well.

bjyoungblood commented Apr 10, 2014

I'm having a similar problem in Chrome (haven't tested other browsers yet). Not sure if it helps, but I'm able to get the elements to remove by clearing and re-adding the opacity property in the inspector. Doing that to one element causes all of the "stuck" ones to be removed as well.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits May 22, 2014

Member

Closing as a duplicate of #669.

Member

sophiebits commented May 22, 2014

Closing as a duplicate of #669.

@sophiebits sophiebits closed this May 22, 2014

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits May 23, 2014

Member

Oops, not a dupe of #669.

Member

sophiebits commented May 23, 2014

Oops, not a dupe of #669.

@sophiebits sophiebits reopened this May 23, 2014

@sophiebits sophiebits changed the title from Animation buggy to CSSTransitionGroup animation buggy due to transitionend not firing May 23, 2014

@jessepollak

This comment has been minimized.

Show comment
Hide comment
@jessepollak

jessepollak May 23, 2014

I can verify that this issue also exists in Chrome. Pretty certain it's due to the transitionend event not firing (it's a documented bug in Firefox).

Is there a fix that we could do in CSSTransitionGroup?

jessepollak commented May 23, 2014

I can verify that this issue also exists in Chrome. Pretty certain it's due to the transitionend event not firing (it's a documented bug in Firefox).

Is there a fix that we could do in CSSTransitionGroup?

@syranide

This comment has been minimized.

Show comment
Hide comment
@syranide

syranide May 23, 2014

Contributor

I'm sure you guys are on-top of this, but intuitively, simply listening for tab switch (?) and treating it like transitionend sounds like the simplest approach.

Contributor

syranide commented May 23, 2014

I'm sure you guys are on-top of this, but intuitively, simply listening for tab switch (?) and treating it like transitionend sounds like the simplest approach.

@jessepollak

This comment has been minimized.

Show comment
Hide comment
@jessepollak

jessepollak May 23, 2014

For longer transitions, that could pose a problem (if you switch away from the tab, then back before the transition has ended). On tab away, we could see how long is left on the transition and clear at the end of that if the tab is still backgrounded.

jessepollak commented May 23, 2014

For longer transitions, that could pose a problem (if you switch away from the tab, then back before the transition has ended). On tab away, we could see how long is left on the transition and clear at the end of that if the tab is still backgrounded.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits May 23, 2014

Member

From the small amount of research I've done, it's not trivial to detect the end reliably. Perhaps just waiting the length of the transition duration is the best approach here – though I'm unsure if you run into lots of problems with that being out of sync with the actual animation.

Member

sophiebits commented May 23, 2014

From the small amount of research I've done, it's not trivial to detect the end reliably. Perhaps just waiting the length of the transition duration is the best approach here – though I'm unsure if you run into lots of problems with that being out of sync with the actual animation.

@irae

This comment has been minimized.

Show comment
Hide comment
@irae

irae May 24, 2014

Contributor

Back when I dug into this issue one thing I noted is that the if(_DEV_) timeout always detects the first occurrence of the problem. In production code, there is no timeout detection.

I tried for a while to implement timeouts, but I never got time to do it properly. It would require getting transition time from CSS (or require extra params for the user to supply that), implement timeouts, unbind transitionEnd for a particular transition when timeout fires, and exploring potential performance issues.

Contributor

irae commented May 24, 2014

Back when I dug into this issue one thing I noted is that the if(_DEV_) timeout always detects the first occurrence of the problem. In production code, there is no timeout detection.

I tried for a while to implement timeouts, but I never got time to do it properly. It would require getting transition time from CSS (or require extra params for the user to supply that), implement timeouts, unbind transitionEnd for a particular transition when timeout fires, and exploring potential performance issues.

@benoneal

This comment has been minimized.

Show comment
Hide comment
@benoneal

benoneal Jun 11, 2014

Taking the demo in the OP, http://jsbin.com/hokahaze/6/edit, I can semi-reliably cause this error: "tried to perform an animation without an animationend or transitionend event" to occur more frequently by resizing the browser window while more than one animation is occurring. This is particularly an issue for me since I'm transitioning components on the window.resize event.

benoneal commented Jun 11, 2014

Taking the demo in the OP, http://jsbin.com/hokahaze/6/edit, I can semi-reliably cause this error: "tried to perform an animation without an animationend or transitionend event" to occur more frequently by resizing the browser window while more than one animation is occurring. This is particularly an issue for me since I'm transitioning components on the window.resize event.

@victor-homyakov

This comment has been minimized.

Show comment
Hide comment
@victor-homyakov

victor-homyakov Jun 17, 2014

Contributor

http://jsbin.com/hokahaze/6

  • the first problem (no more than 2-3-1 lines) easily reproduces in Firefox 30 and Chrome 35
  • the second (adding green lines while in background tab) reproduces in IE10, Firefox 30, Chrome 35
Contributor

victor-homyakov commented Jun 17, 2014

http://jsbin.com/hokahaze/6

  • the first problem (no more than 2-3-1 lines) easily reproduces in Firefox 30 and Chrome 35
  • the second (adding green lines while in background tab) reproduces in IE10, Firefox 30, Chrome 35
@victor-homyakov

This comment has been minimized.

Show comment
Hide comment
@victor-homyakov

victor-homyakov Jun 18, 2014

Contributor

Bootstrap had similar problems (broken transitions, transitions stopped in background tab etc.). They were solved via triggering transitionend event manually if it wasn't fired by the browser: https://github.com/twbs/bootstrap/blob/master/js/carousel.js#L153
https://github.com/twbs/bootstrap/blob/master/js/collapse.js#L79

Contributor

victor-homyakov commented Jun 18, 2014

Bootstrap had similar problems (broken transitions, transitions stopped in background tab etc.). They were solved via triggering transitionend event manually if it wasn't fired by the browser: https://github.com/twbs/bootstrap/blob/master/js/carousel.js#L153
https://github.com/twbs/bootstrap/blob/master/js/collapse.js#L79

@chenglou chenglou added the addons label Aug 14, 2014

@mickm

This comment has been minimized.

Show comment
Hide comment
@mickm

mickm Oct 16, 2014

In case it helps anyone: Khan Academy has a drop-in replacement that seems to solve the problem for some use-cases: https://github.com/Khan/react-components/blob/master/js/timeout-transition-group.jsx

mickm commented Oct 16, 2014

In case it helps anyone: Khan Academy has a drop-in replacement that seems to solve the problem for some use-cases: https://github.com/Khan/react-components/blob/master/js/timeout-transition-group.jsx

@ninjabiscuit

This comment has been minimized.

Show comment
Hide comment
@ninjabiscuit

ninjabiscuit Oct 28, 2014

Has anyone looked any further into this problem? Or is there a suggested workaround?

ninjabiscuit commented Oct 28, 2014

Has anyone looked any further into this problem? Or is there a suggested workaround?

@jiyinyiyong

This comment has been minimized.

Show comment
Hide comment
@jiyinyiyong

jiyinyiyong Jan 26, 2015

Is this issue going to be fixed now?

jiyinyiyong commented Jan 26, 2015

Is this issue going to be fixed now?

@bradens

This comment has been minimized.

Show comment
Hide comment
@bradens

bradens Jan 26, 2015

@ninjabiscuit the suggested workaround is the one in https://github.com/Khan/react-components/blob/master/js/timeout-transition-group.jsx which just relies on a timeout, rather than assuming the browser will fire the event. Until the browsers fire the event properly, this will probably be the only thing we can do.

bradens commented Jan 26, 2015

@ninjabiscuit the suggested workaround is the one in https://github.com/Khan/react-components/blob/master/js/timeout-transition-group.jsx which just relies on a timeout, rather than assuming the browser will fire the event. Until the browsers fire the event properly, this will probably be the only thing we can do.

@Flaise

This comment has been minimized.

Show comment
Hide comment
@Flaise

Flaise Jan 26, 2015

What @bradens suggested is what I'm doing. It works alright but it's less maintainable than a solution that doesn't require extra parameters.

Flaise commented Jan 26, 2015

What @bradens suggested is what I'm doing. It works alright but it's less maintainable than a solution that doesn't require extra parameters.

@artursapek

This comment has been minimized.

Show comment
Hide comment
@artursapek

artursapek Feb 16, 2015

I think there should be a warning about this in the docs. It's a pretty big flaw and caught me off guard.

I would do it myself and submit a pull request but I'm not sure what the convention is for warning about bugs in the React docs. Does it belong as a "Note" (markdown block quote) or is there a convention for more severe warnings? I can't find any examples.

artursapek commented Feb 16, 2015

I think there should be a warning about this in the docs. It's a pretty big flaw and caught me off guard.

I would do it myself and submit a pull request but I'm not sure what the convention is for warning about bugs in the React docs. Does it belong as a "Note" (markdown block quote) or is there a convention for more severe warnings? I can't find any examples.

djrodgerspryor added a commit to djrodgerspryor/react that referenced this issue Aug 7, 2015

Clean-up animations after no-event timeout
As discussed in issue 1326
(facebook#1326) transitionend events are
unreliable; they may not fire because the element is no longer painted,
the browser tab is no longer focused or for a range of other reasons.
This is particularly harmful during leave transitions since the leaving
element will be permanently stuck in the DOM (and possibly visible).
There is already a 5 second timeout in dev mode to warn about the
missing event, this commit uses that timeout to clean-up the animation
(in both dev and non-dev modes).  This is hardly an ideal solution, but
it's much better than the status-quo of failed transition elements
hanging-around indefinitely.

The `if (__DEV__)` wrapper around the `warning` call has been removed
since `warning` itself has such a wrapper.
@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Aug 12, 2015

Member

Going to go with #4561 I think.

Member

sophiebits commented Aug 12, 2015

Going to go with #4561 I think.

djrodgerspryor added a commit to djrodgerspryor/react that referenced this issue Aug 20, 2015

ReactCSSTransitionGroup timeouts
As discussed in issue 1326
(facebook#1326) transitionend events are
unreliable; they may not fire because the element is no longer painted,
the browser tab is no longer focused or for a range of other reasons.
This is particularly harmful during leave transitions since the leaving
element will be permanently stuck in the DOM (and possibly visible).

The ReactCSSTransitionGroup now requires timeouts to be passed in
explicitly for each type of animation. Omitting the timeout duration
for a transition now triggers a PropTypes warning with a link to the
updated documentation.
@oozaa

This comment has been minimized.

Show comment
Hide comment
@oozaa

oozaa Aug 27, 2015

I have similar problem with TransitionGroup(not CSS, using callbacks) and wondering is that the same one and I can hope for fix in 0.14.

At start my TransitionGroup is empty, then component re-renders and adds 5 components to it, triggers componentWillEnter,componentDidEnter - all good.
After some action my component re-renders and puts 0 components to TransitionGroup, triggers 5 times componentWillLeave(on detach I do not perform any transition, calling to callback straight away) and detaches ONLY ONE component! If I rerender again with 0 components it detaches again ONLY ONE.
If in componentWillLeave i use setTimeout((),100) all works how it should be.

oozaa commented Aug 27, 2015

I have similar problem with TransitionGroup(not CSS, using callbacks) and wondering is that the same one and I can hope for fix in 0.14.

At start my TransitionGroup is empty, then component re-renders and adds 5 components to it, triggers componentWillEnter,componentDidEnter - all good.
After some action my component re-renders and puts 0 components to TransitionGroup, triggers 5 times componentWillLeave(on detach I do not perform any transition, calling to callback straight away) and detaches ONLY ONE component! If I rerender again with 0 components it detaches again ONLY ONE.
If in componentWillLeave i use setTimeout((),100) all works how it should be.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Aug 27, 2015

Member

@oozaa That was a separate issue which should also be fixed.

Member

sophiebits commented Aug 27, 2015

@oozaa That was a separate issue which should also be fixed.

@jedwards1211

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Aug 28, 2015

Contributor

@spicyj do you have a link to the issue/fix? Does it work properly even when components are removed before done entering or added back before done leaving?

Contributor

jedwards1211 commented Aug 28, 2015

@spicyj do you have a link to the issue/fix? Does it work properly even when components are removed before done entering or added back before done leaving?

@jedwards1211

This comment has been minimized.

Show comment
Hide comment
@jedwards1211

jedwards1211 Aug 28, 2015

Contributor

@oozaa if nothing else works for you feel free to try out my robust-transition-group

Contributor

jedwards1211 commented Aug 28, 2015

@oozaa if nothing else works for you feel free to try out my robust-transition-group

@dragoscd

This comment has been minimized.

Show comment
Hide comment
@dragoscd

dragoscd Aug 31, 2015

I used focus and blur on window to disable the animation while the window is not focused.

onWindowBlur: function() {
clearInterval(this.sliderInterval);
},

onWindowFocus: function() {
this.activateAutoSlide();
}

dragoscd commented Aug 31, 2015

I used focus and blur on window to disable the animation while the window is not focused.

onWindowBlur: function() {
clearInterval(this.sliderInterval);
},

onWindowFocus: function() {
this.activateAutoSlide();
}

jkytomak added a commit to Opetushallitus/valtionavustus that referenced this issue Sep 1, 2015

Use TimeoutTransitionGroup instead of React.addons.CSSTransitionGroup
Because tarnsitions where failing at least with phantomjs.
See: facebook/react#1326
@flipace

This comment has been minimized.

Show comment
Hide comment
@flipace

flipace Sep 10, 2015

You might also want to checkout VelocityTransitionGroup: https://github.com/tkafka/react-VelocityTransitionGroup - it uses Velocity.js instead of CSS transitions though.

(I made a minor addition to this component in my fork which allows you to specify if the transitiontype should stay the same after it has been set once and another transitionName is set later: https://github.com/flipace/react-VelocityTransitionGroup)

It seems like an adapted version of this is used on http://web.whatsapp.com too.

flipace commented Sep 10, 2015

You might also want to checkout VelocityTransitionGroup: https://github.com/tkafka/react-VelocityTransitionGroup - it uses Velocity.js instead of CSS transitions though.

(I made a minor addition to this component in my fork which allows you to specify if the transitiontype should stay the same after it has been set once and another transitionName is set later: https://github.com/flipace/react-VelocityTransitionGroup)

It seems like an adapted version of this is used on http://web.whatsapp.com too.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Sep 10, 2015

Member

In 0.14 RC you're asked to specify durations, so this is fixed.

Member

sophiebits commented Sep 10, 2015

In 0.14 RC you're asked to specify durations, so this is fixed.

@AvraamMavridis

This comment has been minimized.

Show comment
Hide comment
@AvraamMavridis

AvraamMavridis Oct 2, 2015

Is this issue solved? I have it in 0.13.3

AvraamMavridis commented Oct 2, 2015

Is this issue solved? I have it in 0.13.3

@flipace

This comment has been minimized.

Show comment
Hide comment
@flipace

flipace Oct 2, 2015

@AvraamMavridis As @spicyj wrote - it'll be fixed in 0.14 by asking you for durations. In 0.13.3 it's still an issue.

flipace commented Oct 2, 2015

@AvraamMavridis As @spicyj wrote - it'll be fixed in 0.14 by asking you for durations. In 0.13.3 it's still an issue.

@chriswalkr

This comment has been minimized.

Show comment
Hide comment
@chriswalkr

chriswalkr Mar 10, 2016

Is this issue solved for ReactTransitionGroup? It doesn't require passing in timeouts. I'm getting an animation bug which is resolved using ReactCSSTransitionGroup but exists when using ReactCSSTransitionGroup.

chriswalkr commented Mar 10, 2016

Is this issue solved for ReactTransitionGroup? It doesn't require passing in timeouts. I'm getting an animation bug which is resolved using ReactCSSTransitionGroup but exists when using ReactCSSTransitionGroup.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Mar 10, 2016

Member

ReactTransitionGroup itself shouldn't have any issues since it doesn't have anything to do with transition events.

Member

sophiebits commented Mar 10, 2016

ReactTransitionGroup itself shouldn't have any issues since it doesn't have anything to do with transition events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment