Removing -leave classes in CSSTransitionGroup doesn't work well with alternative batching strategies

[gaearon]

Hi,

This is probably by design but I wanted to make sure.

I had a problem with using CSSTransitionGroup together with RAF batching strategy. I'm aware that RAF batching is not officially supported but it works very well for me and solves quite a few problems in my app so I don't want to give it up.

The problem is following: CSSTransitionGroup removes -leave and -leave-active classes just before calling finishCallback(). Is it really necessary to do so?

With batching strategy other than default, this may (and does with RAF) result in -leave and -leave-active classes being removed a tick before element is removed from the DOM. Thus the element flickers.

If I move these calls behind a condition, transition group behaves just fine:

if (animationType !== 'leave') {
  CSSCore.removeClass(node, className);
  CSSCore.removeClass(node, activeClassName);
}

Am I missing something?

Edit: Indeed, I am missing the situation where I fire another enter before leave completes—probably in this case my if won't work. But still I wish there was a way around this.

[gaearon]

For my use case, I solved it by only removing enter classes inside the callback.
I remove leave inside transition handler instead, so they're only removed if we're entering yet again.

In the callback instead of these removeClass calls:

      // We differ from original implementation in that we don't want -leave classes
      // to be removed here if element is to be removed from DOM anyway, as this
      // causes flicker with alternative batching strategies.
      // Instead, we only remove `enter` classes in the end callback.
      // We will remove `leave` classes only if we're about to `enter` again (see below).
      if (animationType === 'enter') {
        CSSCore.removeClass(node, className);
        CSSCore.removeClass(node, activeClassName);
      }

Right before addClass outside the callback:

    // Remove `leave` class if we're about to `enter` again.
    // (See rationale above.)
    if (animationType === 'enter') {
      CSSCore.removeClass(node, this.props.name + '-leave');
      CSSCore.removeClass(node, this.props.name + '-leave-active');
    }

I'm actually using TimeoutTransitionGroup now but since it works fine with this fix, I presume this should fix CSSTransitionGroup behavior as well if someone too bumps into this problem.

(I hope there's a less hackish solution though!)

[sophiebits]

I'm having enough trouble convincing myself that your fix is correct that I don't think I want to take this. If we ever support rAF batching in core, perhaps we will want a fix for this. I'm curious though – what problems does rAF batching solve for you? Does React.addons.batchedUpdates fix the same problems?

[gaearon]

I'm having enough trouble convincing myself that your fix is correct

Can you elaborate a bit so I could learn? My line of thought was that there is no real reason to remove -leave if node is going to removed from DOM anyway. Better remove it from DOM with -leave on it. Is this wrong?

I'm using rAF batching for a few very tight spots in my app where I need to update opacity as user scrolls, and it lags terribly without rAF batching. I used to have complicated userland code for rAF but with rAF batching I was able to just use setState. In my case batchedUpdates won't help because setState happens too often (on scroll) and I want to keep my code simple and still update opacity only when needed (no faster than frames). Does this make sense?

[sophiebits]

The idea sounds good but the code is still tricky, especially for the re-entry case.

That makes sense. Have you looked at all at throttling the scroll events instead? That's the typical way to deal with this. You could do something like this if you still want to do it on rAF boundaries:

function throttleAF(fn) {
  var interval = null;

  function onFrame() {
    interval = null;
    fn();
  }

  return function throttled() {
    if (interval == null) {
      interval = requestAnimationFrame(onFrame);
    }
  };
}

// ...

this.throttledScroll = throttleAF(this.handleScroll)
onScroll={this.throttledScroll}

Basically, rAF batching introduces complexity and makes things harder to reason about (like necessitating your fix here) so we've so far tried to avoid using it.

[gaearon]

I see. Perhaps you're right. When I first wrote that code I wasn't trustring myself to use rAF properly but I think I have a better idea of how to use it now, so I might as well do that (and throttle scrolling). Thanks!

[sophiebits]

No problem. Let me know how it goes!