fix #3255 - get automargin calls out of supplyDefaults

Author: alexcjohnson

src/components/rangeslider/draw.js

@@ -445,6 +445,11 @@ function drawRangePlot(rangeSlider, gd, axisOpts, opts) {
         var xa = mockFigure._fullLayout.xaxis;
         var ya = mockFigure._fullLayout[oppAxisName];
 
+        xa.clearCalc();
+        xa.setScale();
+        ya.clearCalc();
+        ya.setScale();
+
         var plotinfo = {
             id: id,
             plotgroup: plotgroup,

src/plots/plots.js

@@ -479,15 +479,8 @@ plots.supplyDefaults = function(gd, opts) {
     // relink functions and _ attributes to promote consistency between plots
     relinkPrivateKeys(newFullLayout, oldFullLayout);
 
-    // TODO may return a promise
-    plots.doAutoMargin(gd);
-
-    // set scale after auto margin routine
-    var axList = axisIDs.list(gd);
-    for(i = 0; i < axList.length; i++) {
-        var ax = axList[i];
-        ax.setScale();
-    }
+    // set up containers for margin calculations
+    initMargins(newFullLayout);
 
     // update object references in calcdata
     if(!skipUpdateCalc && oldCalcdata.length === newFullData.length) {
@@ -1651,7 +1644,20 @@ plots.allowAutoMargin = function(gd, id) {
     gd._fullLayout._pushmarginIds[id] = 1;
 };
 
-function setupAutoMargin(fullLayout) {
+function initMargins(fullLayout) {
+    var margin = fullLayout.margin;
+
+    if(!fullLayout._size) {
+        var gs = fullLayout._size = {
+            l: Math.round(margin.l),
+            r: Math.round(margin.r),
+            t: Math.round(margin.t),
+            b: Math.round(margin.b),
+            p: Math.round(margin.pad)
+        };
+        gs.w = Math.round(fullLayout.width) - gs.l - gs.r;
+        gs.h = Math.round(fullLayout.height) - gs.t - gs.b;
+    }
     if(!fullLayout._pushmargin) fullLayout._pushmargin = {};
     if(!fullLayout._pushmarginIds) fullLayout._pushmarginIds = {};
 }
@@ -1674,8 +1680,6 @@ function setupAutoMargin(fullLayout) {
 plots.autoMargin = function(gd, id, o) {
     var fullLayout = gd._fullLayout;
 
-    setupAutoMargin(fullLayout);
-
     var pushMargin = fullLayout._pushmargin;
     var pushMarginIds = fullLayout._pushmarginIds;
 
@@ -1719,18 +1723,19 @@ plots.autoMargin = function(gd, id, o) {
 plots.doAutoMargin = function(gd) {
     var fullLayout = gd._fullLayout;
     if(!fullLayout._size) fullLayout._size = {};
-    setupAutoMargin(fullLayout);
+    initMargins(fullLayout);
 
-    var gs = fullLayout._size,
-        oldmargins = JSON.stringify(gs);
+    var gs = fullLayout._size;
+    var oldmargins = JSON.stringify(gs);
+    var margin = fullLayout.margin;
 
     // adjust margins for outside components
     // fullLayout.margin is the requested margin,
     // fullLayout._size has margins and plotsize after adjustment
-    var ml = Math.max(fullLayout.margin.l || 0, 0);
-    var mr = Math.max(fullLayout.margin.r || 0, 0);
-    var mt = Math.max(fullLayout.margin.t || 0, 0);
-    var mb = Math.max(fullLayout.margin.b || 0, 0);
+    var ml = margin.l;
+    var mr = margin.r;
+    var mt = margin.t;
+    var mb = margin.b;
     var pushMargin = fullLayout._pushmargin;
     var pushMarginIds = fullLayout._pushmarginIds;
 
@@ -1800,7 +1805,7 @@ plots.doAutoMargin = function(gd) {
     gs.r = Math.round(mr);
     gs.t = Math.round(mt);
     gs.b = Math.round(mb);
-    gs.p = Math.round(fullLayout.margin.pad);
+    gs.p = Math.round(margin.pad);
     gs.w = Math.round(fullLayout.width) - gs.l - gs.r;
     gs.h = Math.round(fullLayout.height) - gs.t - gs.b;
 

test/jasmine/tests/heatmap_test.js

@@ -303,6 +303,11 @@ describe('heatmap calc', function() {
 
         fullTrace._extremes = {};
 
+        // clearCalc used to be (oddly enough) part of supplyDefaults.
+        // Now it's in doCalcData, which we don't include in this partial pathway.
+        fullLayout.xaxis.clearCalc();
+        fullLayout.yaxis.clearCalc();
+
         var out = Heatmap.calc(gd, fullTrace)[0];
         out._xcategories = fullLayout.xaxis._categories;
         out._ycategories = fullLayout.yaxis._categories;

test/jasmine/tests/plot_api_test.js

@@ -3087,6 +3087,51 @@ describe('Test plot api', function() {
             .then(done);
         });
 
+        it('can change from scatter to category scatterpolar and back', function(done) {
+            function scatter() {
+                return {
+                    data: [{x: ['a', 'b'], y: [1, 2]}],
+                    layout: {width: 400, height: 400, margin: {r: 80, t: 20}}
+                };
+            }
+
+            function scatterpolar() {
+                return {
+                    // the bug https://github.com/plotly/plotly.js/issues/3255
+                    // required all of this to change:
+                    // - type -> scatterpolar
+                    // - category theta
+                    // - margins changed
+                    data: [{type: 'scatterpolar', r: [1, 2, 3], theta: ['a', 'b', 'c']}],
+                    layout: {width: 400, height: 400, margin: {r: 80, t: 50}}
+                };
+            }
+
+            function countTraces(scatterTraces, polarTraces) {
+                expect(document.querySelectorAll('.scatter').length)
+                    .toBe(scatterTraces + polarTraces);
+                expect(document.querySelectorAll('.xy .scatter').length)
+                    .toBe(scatterTraces);
+                expect(document.querySelectorAll('.polar .scatter').length)
+                    .toBe(polarTraces);
+            }
+
+            Plotly.newPlot(gd, scatter())
+            .then(function() {
+                countTraces(1, 0);
+                return Plotly.react(gd, scatterpolar());
+            })
+            .then(function() {
+                countTraces(0, 1);
+                return Plotly.react(gd, scatter());
+            })
+            .then(function() {
+                countTraces(1, 0);
+            })
+            .catch(failTest)
+            .then(done);
+        });
+
         it('can change data in candlesticks multiple times', function(done) {
             // test that we've fixed the original issue in
             // https://github.com/plotly/plotly.js/issues/2510

test/jasmine/tests/plots_test.js

@@ -83,9 +83,9 @@ describe('Test Plots', function() {
             expect(gd._fullLayout.someFunc).toBe(oldFullLayout.someFunc);
 
             expect(gd._fullLayout.xaxis.c2p)
-                .not.toBe(oldFullLayout.xaxis.c2p, '(set during ax.setScale');
+                .not.toBe(oldFullLayout.xaxis.c2p, '(set during setConvert)');
             expect(gd._fullLayout.yaxis._m)
-                .not.toBe(oldFullLayout.yaxis._m, '(set during ax.setScale');
+                .toBe(oldFullLayout.yaxis._m, '(we don\'t run ax.setScale here)');
         });
 
         it('should include the correct reference to user data', function() {

test/jasmine/tests/splom_test.js

@@ -820,11 +820,11 @@ describe('Test splom interactions:', function() {
 
         function _assert(msg, exp) {
             var splomScenes = gd._fullLayout._splomScenes;
-            var ids = Object.keys(splomScenes);
+            var ids = gd._fullData.map(function(trace) { return trace.uid; });
 
             for(var i = 0; i < 3; i++) {
                 var drawFn = splomScenes[ids[i]].draw;
-                expect(drawFn).toHaveBeenCalledTimes(exp[i], msg + ' - trace ' + i);
+                expect(drawFn.calls.count()).toBe(exp[i], msg + ' - trace ' + i);
                 drawFn.calls.reset();
             }
         }
@@ -869,7 +869,7 @@ describe('Test splom interactions:', function() {
 
         methods.forEach(function(m) { spyOn(Plots, m).and.callThrough(); });
 
-        function assetsFnCall(msg, exp) {
+        function assertFnCall(msg, exp) {
             methods.forEach(function(m) {
                 expect(Plots[m]).toHaveBeenCalledTimes(exp[m], msg);
                 Plots[m].calls.reset();
@@ -879,7 +879,7 @@ describe('Test splom interactions:', function() {
         spyOn(Lib, 'log');
 
         Plotly.plot(gd, fig).then(function() {
-            assetsFnCall('base', {
+            assertFnCall('base', {
                 cleanPlot: 1,       // called once from inside Plots.supplyDefaults
                 supplyDefaults: 1,
                 doCalcdata: 1
@@ -892,9 +892,9 @@ describe('Test splom interactions:', function() {
             return Plotly.relayout(gd, {width: 4810, height: 3656});
         })
         .then(function() {
-            assetsFnCall('after', {
-                cleanPlot: 4,       // 3 three from supplyDefaults, once in drawFramework
-                supplyDefaults: 3,  // 1 from relayout, 1 from automargin, 1 in drawFramework
+            assertFnCall('after', {
+                cleanPlot: 3,       // 2 from supplyDefaults, once in drawFramework
+                supplyDefaults: 2,  // 1 from relayout, 1 in drawFramework
                 doCalcdata: 1       // once in drawFramework
             });
             assertDims('after', 4810, 3656);