Fix to work enter animation with CSSTransitionGroup #13

Merged
merged 1 commit into from Feb 9, 2017

Conversation

Projects
None yet
2 participants
@koba04
Contributor

koba04 commented Feb 8, 2017

This PR is to fix #5.

Currently, enter animation with CSSTransitionGroup seems not to work as mentioned in #5.

The following is a reproduce case.

According to the above Chrome timeline images, react-transition-group calls flushClassNameAndNodeQueue before recalculate and repaint.
I think this is the cause of the issue.

So the PR is using setTimeout with TICK = 17, which is same as the original implementation in react-addons-css-transition-group.

After applying the above patch, CSSTransitionGroupChild creates nested duplicate children.
It can fix not to pass children to React.cloneElement

screen shot 2017-02-08 at 19 41 24

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Feb 8, 2017

Collaborator

thanks for the PR! we really want to use requestAnimationFrame here as that is what the original code is emulating (poorly). rather then go back to the old code, which causes other animation issues, I'd like to figure out whats not working and fix the current code thanks!

Collaborator

jquense commented Feb 8, 2017

thanks for the PR! we really want to use requestAnimationFrame here as that is what the original code is emulating (poorly). rather then go back to the old code, which causes other animation issues, I'd like to figure out whats not working and fix the current code thanks!

@koba04

This comment has been minimized.

Show comment
Hide comment
@koba04

koba04 Feb 8, 2017

Contributor

@jquense If you'd like to use rAF, there is a way to force a repaint by accessing a style property.

diff --git a/src/CSSTransitionGroupChild.js b/src/CSSTransitionGroupChild.js
index d3f5baa..bea150b 100644
--- a/src/CSSTransitionGroupChild.js
+++ b/src/CSSTransitionGroupChild.js
@@ -125,6 +125,11 @@ class CSSTransitionGroupChild extends React.Component {
   flushClassNameAndNodeQueue() {
     if (!this.unmounted) {
       this.classNameAndNodeQueue.forEach((obj) => {
+        // This is for to force a repaint,
+        // which is necessary in order to transition styles when adding a class name.
+        /* eslint-disable no-unused-expressions */
+        obj.node.scrollTop;
+        /* eslint-enable no-unused-expressions */
         addClass(obj.node, obj.className);
       });
     }
@@ -165,6 +170,7 @@ class CSSTransitionGroupChild extends React.Component {
     delete props.appearTimeout;
     delete props.enterTimeout;
     delete props.leaveTimeout;
+    delete props.children;
     return React.cloneElement(React.Children.only(this.props.children), props);
   }
 }

If you prefer this approach, I can update this PR.

Contributor

koba04 commented Feb 8, 2017

@jquense If you'd like to use rAF, there is a way to force a repaint by accessing a style property.

diff --git a/src/CSSTransitionGroupChild.js b/src/CSSTransitionGroupChild.js
index d3f5baa..bea150b 100644
--- a/src/CSSTransitionGroupChild.js
+++ b/src/CSSTransitionGroupChild.js
@@ -125,6 +125,11 @@ class CSSTransitionGroupChild extends React.Component {
   flushClassNameAndNodeQueue() {
     if (!this.unmounted) {
       this.classNameAndNodeQueue.forEach((obj) => {
+        // This is for to force a repaint,
+        // which is necessary in order to transition styles when adding a class name.
+        /* eslint-disable no-unused-expressions */
+        obj.node.scrollTop;
+        /* eslint-enable no-unused-expressions */
         addClass(obj.node, obj.className);
       });
     }
@@ -165,6 +170,7 @@ class CSSTransitionGroupChild extends React.Component {
     delete props.appearTimeout;
     delete props.enterTimeout;
     delete props.leaveTimeout;
+    delete props.children;
     return React.cloneElement(React.Children.only(this.props.children), props);
   }
 }

If you prefer this approach, I can update this PR.

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Feb 8, 2017

Collaborator

hey thanks for looking into it. I'm not really sure why we'd need to trigger a repaint after a RAF honestly. RAF is very specifically for getting a callback on the next repaint, so you should need to trigger one, because teh callback is being called exactly when the browser is about to paint. My guess at what be happening here tho is:

first class is added somewhere between a paint frame and then the RAF callback is called at the end leading to both being added in the same repaint.

If so the best bet would be to trigger a reflow right after the first enter class is added instead of before the active class. I think ideally tho we should add the enter class in a RAF and then trigger the next RAF for the active class...

That may over complicate it all tho

Collaborator

jquense commented Feb 8, 2017

hey thanks for looking into it. I'm not really sure why we'd need to trigger a repaint after a RAF honestly. RAF is very specifically for getting a callback on the next repaint, so you should need to trigger one, because teh callback is being called exactly when the browser is about to paint. My guess at what be happening here tho is:

first class is added somewhere between a paint frame and then the RAF callback is called at the end leading to both being added in the same repaint.

If so the best bet would be to trigger a reflow right after the first enter class is added instead of before the active class. I think ideally tho we should add the enter class in a RAF and then trigger the next RAF for the active class...

That may over complicate it all tho

@koba04

This comment has been minimized.

Show comment
Hide comment
@koba04

koba04 Feb 8, 2017

Contributor

@jquense Thanks!

If so the best bet would be to trigger a reflow right after the first enter class is added instead of before the active class.

I agree with this. The following patch also works fine.

diff --git a/src/CSSTransitionGroupChild.js b/src/CSSTransitionGroupChild.js
index d3f5baa..210c418 100644
--- a/src/CSSTransitionGroupChild.js
+++ b/src/CSSTransitionGroupChild.js
@@ -78,9 +78,10 @@ class CSSTransitionGroupChild extends React.Component {
     let removeListeners;
 
     addClass(node, className);
-
-    // Need to do this to actually trigger a transition.
-    this.queueClassAndNode(activeClassName, node);
+    raf(() => {
+      // Need to do this to actually trigger a transition.
+      this.queueClassAndNode(activeClassName, node);
+    });
 
     // Clean-up the animation after the specified delay
     let finish = (e) => {
@@ -165,6 +166,7 @@ class CSSTransitionGroupChild extends React.Component {
     delete props.appearTimeout;
     delete props.enterTimeout;
     delete props.leaveTimeout;
+    delete props.children;
     return React.cloneElement(React.Children.only(this.props.children), props);
   }
 }

I think rAF calls a callback before the next repaint. So we need to call nested rAFs.

The window.requestAnimationFrame() method tells the browser that you wish to perform an animation and requests that the browser call a specified function to update an animation before the next repaint. The method takes as an argument a callback to be invoked before the repaint.
Note: Your callback routine must itself call requestAnimationFrame() if you want to animate another frame at the next repaint.

Contributor

koba04 commented Feb 8, 2017

@jquense Thanks!

If so the best bet would be to trigger a reflow right after the first enter class is added instead of before the active class.

I agree with this. The following patch also works fine.

diff --git a/src/CSSTransitionGroupChild.js b/src/CSSTransitionGroupChild.js
index d3f5baa..210c418 100644
--- a/src/CSSTransitionGroupChild.js
+++ b/src/CSSTransitionGroupChild.js
@@ -78,9 +78,10 @@ class CSSTransitionGroupChild extends React.Component {
     let removeListeners;
 
     addClass(node, className);
-
-    // Need to do this to actually trigger a transition.
-    this.queueClassAndNode(activeClassName, node);
+    raf(() => {
+      // Need to do this to actually trigger a transition.
+      this.queueClassAndNode(activeClassName, node);
+    });
 
     // Clean-up the animation after the specified delay
     let finish = (e) => {
@@ -165,6 +166,7 @@ class CSSTransitionGroupChild extends React.Component {
     delete props.appearTimeout;
     delete props.enterTimeout;
     delete props.leaveTimeout;
+    delete props.children;
     return React.cloneElement(React.Children.only(this.props.children), props);
   }
 }

I think rAF calls a callback before the next repaint. So we need to call nested rAFs.

The window.requestAnimationFrame() method tells the browser that you wish to perform an animation and requests that the browser call a specified function to update an animation before the next repaint. The method takes as an argument a callback to be invoked before the repaint.
Note: Your callback routine must itself call requestAnimationFrame() if you want to animate another frame at the next repaint.

@jquense

This comment has been minimized.

Show comment
Hide comment
@jquense

jquense Feb 8, 2017

Collaborator

So i've tried this out a few times with a bunch of different approaches noted here. Ultimately I think that your thought: #13 (comment) is actually the simplest and most consistent fix, among the animations i've tried.

I was worried that reflow triggers in the RAF would cause a thrash, but looking at the timeline of it. it seems its actually the least noisy option

Collaborator

jquense commented Feb 8, 2017

So i've tried this out a few times with a bunch of different approaches noted here. Ultimately I think that your thought: #13 (comment) is actually the simplest and most consistent fix, among the animations i've tried.

I was worried that reflow triggers in the RAF would cause a thrash, but looking at the timeline of it. it seems its actually the least noisy option

@koba04

This comment has been minimized.

Show comment
Hide comment
@koba04

koba04 Feb 9, 2017

Contributor

Thank you for your having time! I've updated this to using #13 (comment).

Contributor

koba04 commented Feb 9, 2017

Thank you for your having time! I've updated this to using #13 (comment).

@jquense

jquense approved these changes Feb 9, 2017

@jquense jquense merged commit 2e03f4b into reactjs:master Feb 9, 2017

@koba04

This comment has been minimized.

Show comment
Hide comment
@koba04

koba04 Feb 9, 2017

Contributor

Thanks 🎉

Could you publish this on npm as a new version?
I'd like to use it in my project.

Contributor

koba04 commented Feb 9, 2017

Thanks 🎉

Could you publish this on npm as a new version?
I'd like to use it in my project.

@koba04 koba04 deleted the koba04:fix-enter-animation branch Feb 9, 2017

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