Merge pull request #1108 from plotly/fix-layout-animation

Fix race condition in animation resolution

Author: rreusser

src/plot_api/plot_api.js

@@ -2214,6 +2214,20 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
         }
     }
 
+    // Execute a callback after the wrapper function has been called n times.
+    // This is used to defer the resolution until a transition has resovled *and*
+    // the frame has completed. If it's not done this way, then we get a race
+    // condition in which the animation might resolve before a transition is complete
+    // or vice versa.
+    function callbackOnNthTime(cb, n) {
+        var cnt = 0;
+        return function() {
+            if(cb && ++cnt === n) {
+                return cb();
+            }
+        };
+    }
+
     return new Promise(function(resolve, reject) {
         function discardExistingFrames() {
             if(trans._frameQueue.length === 0) {
@@ -2264,7 +2278,7 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
                     // loop (which may exist as a result of a *different* .animate call)
                     // still resolves or rejecdts this .animate call's promise. once it's
                     // complete.
-                    nextFrame.onComplete = resolve;
+                    nextFrame.onComplete = callbackOnNthTime(resolve, 2);
                     nextFrame.onInterrupt = reject;
                 }
 
@@ -2302,7 +2316,6 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
                 // Execute the callback and unset it to ensure it doesn't
                 // accidentally get called twice
                 trans._currentFrame.onComplete();
-                trans._currentFrame.onComplete = null;
             }
 
             var newFrame = trans._currentFrame = trans._frameQueue.shift();
@@ -2322,7 +2335,12 @@ Plotly.animate = function(gd, frameOrGroupNameOrFrameList, animationOpts) {
                     helpers.coerceTraceIndices(gd, newFrame.frame.traces),
                     newFrame.frameOpts,
                     newFrame.transitionOpts
-                );
+                ).then(function() {
+                    if(newFrame.onComplete) {
+                        newFrame.onComplete();
+                    }
+
+                });
 
                 gd.emit('plotly_animatingframe', {
                     name: newFrame.name,

src/plots/plots.js

@@ -1104,8 +1104,16 @@ plots.purge = function(gd) {
     // remove modebar
     if(fullLayout._modeBar) fullLayout._modeBar.destroy();
 
-    if(gd._transitionData && gd._transitionData._animationRaf) {
-        window.cancelAnimationFrame(gd._transitionData._animationRaf);
+    if(gd._transitionData) {
+        // Ensure any dangling callbacks are simply dropped if the plot is purged.
+        // This is more or less only actually important for testing.
+        if(gd._transitionData._interruptCallbacks) {
+            gd._transitionData._interruptCallbacks.length = 0;
+        }
+
+        if(gd._transitionData._animationRaf) {
+            window.cancelAnimationFrame(gd._transitionData._animationRaf);
+        }
     }
 
     // data and layout
@@ -1714,6 +1722,9 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)
     var aborted = false;
 
     function executeTransitions() {
+
+        gd.emit('plotly_transitioning', []);
+
         return new Promise(function(resolve) {
             // This flag is used to disabled things like autorange:
             gd._transitioning = true;
@@ -1798,7 +1809,9 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)
     }
 
     function completeTransition(callback) {
-        // Fail-safe against purged plot:
+        // This a simple workaround for tests which purge the graph before animations
+        // have completed. That's not a very common case, so this is the simplest
+        // fix.
         if(!gd._transitionData) return;
 
         flushCallbacks(gd._transitionData._interruptCallbacks);
@@ -1848,13 +1861,13 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)
 
     var seq = [plots.previousPromises, interruptPreviousTransitions, prepareTransitions, executeTransitions];
 
-
     var transitionStarting = Lib.syncOrAsync(seq, gd);
 
-    if(!transitionStarting || !transitionStarting.then) transitionStarting = Promise.resolve();
+    if(!transitionStarting || !transitionStarting.then) {
+        transitionStarting = Promise.resolve();
+    }
 
     return transitionStarting.then(function() {
-        gd.emit('plotly_transitioning', []);
         return gd;
     });
 };

test/jasmine/tests/animate_test.js

@@ -591,32 +591,84 @@ describe('Test animate API', function() {
 describe('Animate API details', function() {
     'use strict';
 
-    var gd, mockCopy;
+    var gd;
+    var dur = 30;
+    var mockCopy;
 
     beforeEach(function(done) {
         gd = createGraphDiv();
-
         mockCopy = Lib.extendDeep({}, mock);
-
-        Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(function() {
-            return Plotly.addFrames(gd, mockCopy.frames);
-        }).then(done);
+        Plotly.plot(gd, mockCopy.data, mockCopy.layout).then(done);
     });
 
     afterEach(function() {
         Plotly.purge(gd);
         destroyGraphDiv();
     });
 
-    it('null frames should not break everything', function(done) {
-        gd._transitionData._frames.push(null);
+    it('redraws after a layout animation', function(done) {
+        var redraws = 0;
+        gd.on('plotly_redraw', function() {redraws++;});
 
-        Plotly.animate(gd, null, {
-            frame: {duration: 0},
-            transition: {duration: 0}
+        Plotly.animate(gd,
+            {layout: {'xaxis.range': [0, 1]}},
+            {frame: {redraw: true, duration: dur}, transition: {duration: dur}}
+        ).then(function() {
+            expect(redraws).toBe(1);
         }).catch(fail).then(done);
     });
 
+    it('forces a relayout after layout animations', function(done) {
+        var relayouts = 0;
+        var restyles = 0;
+        var redraws = 0;
+        gd.on('plotly_relayout', function() {relayouts++;});
+        gd.on('plotly_restyle', function() {restyles++;});
+        gd.on('plotly_redraw', function() {redraws++;});
+
+        Plotly.animate(gd,
+            {layout: {'xaxis.range': [0, 1]}},
+            {frame: {redraw: false, duration: dur}, transition: {duration: dur}}
+        ).then(function() {
+            expect(relayouts).toBe(1);
+            expect(restyles).toBe(0);
+            expect(redraws).toBe(0);
+        }).catch(fail).then(done);
+    });
+
+    it('triggers plotly_animated after a single layout animation', function(done) {
+        var animateds = 0;
+        gd.on('plotly_animated', function() {animateds++;});
+
+        Plotly.animate(gd, [
+            {layout: {'xaxis.range': [0, 1]}},
+        ], {frame: {redraw: false, duration: dur}, transition: {duration: dur}}
+        ).then(function() {
+            // Wait a bit just to be sure:
+            setTimeout(function() {
+                expect(animateds).toBe(1);
+                done();
+            }, dur);
+        });
+    });
+
+    it('triggers plotly_animated after a multi-step layout animation', function(done) {
+        var animateds = 0;
+        gd.on('plotly_animated', function() {animateds++;});
+
+        Plotly.animate(gd, [
+            {layout: {'xaxis.range': [0, 1]}},
+            {layout: {'xaxis.range': [2, 4]}},
+        ], {frame: {redraw: false, duration: dur}, transition: {duration: dur}}
+        ).then(function() {
+            // Wait a bit just to be sure:
+            setTimeout(function() {
+                expect(animateds).toBe(1);
+                done();
+            }, dur);
+        });
+    });
+
     it('does not fail if strings are not used', function(done) {
         Plotly.addFrames(gd, [{name: 8, data: [{x: [8, 7, 6]}]}]).then(function() {
             // Verify it was added as a string name:
@@ -634,12 +686,23 @@ describe('Animate API details', function() {
         var cnt = 0;
         gd.on('plotly_animatingframe', function() {cnt++;});
 
-        Plotly.animate(gd, ['frame0', null, undefined], {transition: {duration: 0}, frame: {duration: 0}}).then(function() {
+        Plotly.addFrames(gd, mockCopy.frames).then(function() {
+            return Plotly.animate(gd, ['frame0', null, undefined], {transition: {duration: 0}, frame: {duration: 0}});
+        }).then(function() {
             // Check only one animating was fired:
             expect(cnt).toEqual(1);
 
             // Check unused frames did not affect the current frame:
             expect(gd._fullLayout._currentFrame).toEqual('frame0');
         }).catch(fail).then(done);
     });
+
+    it('null frames should not break everything', function(done) {
+        gd._transitionData._frames.push(null);
+
+        Plotly.animate(gd, null, {
+            frame: {duration: 0},
+            transition: {duration: 0}
+        }).catch(fail).then(done);
+    });
 });