Merge pull request #3811 from plotly/better-did-margin-change-check

Better did-margin-change check
plotly · Apr 29, 2019 · c820abb · c820abb
2 parents 29f94b6 + d5595d7
commit c820abb
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 8 deletions.
diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js
@@ -198,7 +198,7 @@ function plot(gd, data, layout, config) {
      * start async-friendly code - now we're actually drawing things
      */
 
-    var oldmargins = JSON.stringify(fullLayout._size);
+    var oldMargins = Lib.extendFlat({}, fullLayout._size);
 
     // draw framework first so that margin-pushing
     // components can position themselves correctly
@@ -311,7 +311,7 @@ function plot(gd, data, layout, config) {
 
     // in case the margins changed, draw margin pushers again
     function marginPushersAgain() {
-        if(JSON.stringify(fullLayout._size) === oldmargins) return;
+        if(!Plots.didMarginChange(oldMargins, fullLayout._size)) return;
 
         return Lib.syncOrAsync([
             marginPushers,

diff --git a/src/plots/plots.js b/src/plots/plots.js
@@ -1802,7 +1802,9 @@ plots.autoMargin = function(gd, id, o) {
             pushMarginIds[id] = 1;
         }
 
-        if(!fullLayout._replotting) plots.doAutoMargin(gd);
+        if(!fullLayout._replotting) {
+            plots.doAutoMargin(gd);
+        }
     }
 };
 
@@ -1812,8 +1814,8 @@ plots.doAutoMargin = function(gd) {
     initMargins(fullLayout);
 
     var gs = fullLayout._size;
-    var oldmargins = JSON.stringify(gs);
     var margin = fullLayout.margin;
+    var oldMargins = Lib.extendFlat({}, gs);
 
     // adjust margins for outside components
     // fullLayout.margin is the requested margin,
@@ -1892,10 +1894,7 @@ plots.doAutoMargin = function(gd) {
     gs.h = Math.round(height) - gs.t - gs.b;
 
     // if things changed and we're not already redrawing, trigger a redraw
-    if(!fullLayout._replotting &&
-        oldmargins !== '{}' &&
-        oldmargins !== JSON.stringify(fullLayout._size)
-    ) {
+    if(!fullLayout._replotting && plots.didMarginChange(oldMargins, gs)) {
         if('_redrawFromAutoMarginCount' in fullLayout) {
             fullLayout._redrawFromAutoMarginCount++;
         } else {
@@ -1905,6 +1904,22 @@ plots.doAutoMargin = function(gd) {
     }
 };
 
+var marginKeys = ['l', 'r', 't', 'b', 'p', 'w', 'h'];
+
+plots.didMarginChange = function(margin0, margin1) {
+    for(var i = 0; i < marginKeys.length; i++) {
+        var k = marginKeys[i];
+        var m0 = margin0[k];
+        var m1 = margin1[k];
+        // use 1px tolerance in case we old/new differ only
+        // by rounding errors, which can lead to infinite loops
+        if(!isNumeric(m0) || Math.abs(m1 - m0) > 1) {
+            return true;
+        }
+    }
+    return false;
+};
+
 /**
  * JSONify the graph data and layout
  *

diff --git a/test/jasmine/tests/plots_test.js b/test/jasmine/tests/plots_test.js
@@ -1,6 +1,7 @@
 var Plotly = require('@lib/index');
 var Plots = require('@src/plots/plots');
 var Lib = require('@src/lib');
+var Registry = require('@src/registry');
 
 var d3 = require('d3');
 var createGraphDiv = require('../assets/create_graph_div');
@@ -936,6 +937,74 @@ describe('Test Plots', function() {
             .then(done);
         });
     });
+
+    describe('Test Plots.doAutoMargin', function() {
+        afterEach(destroyGraphDiv);
+
+        it('should trigger a replot when necessary', function(done) {
+            var gd = createGraphDiv();
+            var r0;
+            var w0;
+
+            function _assert(msg, exp) {
+                var fullLayout = gd._fullLayout;
+
+                expect(fullLayout._size.r).toBe(exp.r);
+                expect(fullLayout._size.w).toBe(exp.w);
+
+                expect(Registry.call).toHaveBeenCalledTimes(exp.plotCallCnt);
+                Registry.call.calls.reset();
+            }
+
+            Plotly.newPlot(gd, [{
+                y: [1, 2, 1],
+                name: 'A trace name long enough to push the right margin'
+            }], {
+                showlegend: true
+            })
+            .then(function() {
+                r0 = gd._fullLayout._size.r;
+                w0 = gd._fullLayout._size.w;
+                spyOn(Registry, 'call');
+            })
+            .then(function() {
+                return Plots.doAutoMargin(gd);
+            })
+            .then(function() {
+                _assert('after doAutoMargin() with identical margins', {
+                    r: r0,
+                    w: w0,
+                    plotCallCnt: 0
+                });
+            })
+            .then(function() {
+                gd._fullLayout._pushmargin.legend.r.size += 2;
+                return Plots.doAutoMargin(gd);
+            })
+            .then(function() {
+                _assert('after doAutoMargin() with bigger margins', {
+                    r: r0 + 2,
+                    w: w0 - 2,
+                    plotCallCnt: 1
+                });
+            })
+            .then(function() {
+                gd._fullLayout._pushmargin.legend.r.size += 1;
+                return Plots.doAutoMargin(gd);
+            })
+            .then(function() {
+                // see https://github.com/plotly/plotly.js/issues/3561#issuecomment-485953778
+                // for more info
+                _assert('after doAutoMargin() with bigger margins under tolerance', {
+                    r: r0 + 3,
+                    w: w0 - 3,
+                    plotCallCnt: 0
+                });
+            })
+            .catch(failTest)
+            .then(done);
+        });
+    });
 });
 
 describe('grids', function() {