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

@@ -497,15 +497,8 @@ plots.supplyDefaults = function(gd, opts) {
         if(uids[uid] === 'old') delete tracePreGUI[uid];
     }
 
-    // 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) {
@@ -1686,7 +1679,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 = {};
 }
@@ -1709,8 +1715,6 @@ function setupAutoMargin(fullLayout) {
 plots.autoMargin = function(gd, id, o) {
     var fullLayout = gd._fullLayout;
 
-    setupAutoMargin(fullLayout);
-
     var pushMargin = fullLayout._pushmargin;
     var pushMarginIds = fullLayout._pushmarginIds;
 
@@ -1754,18 +1758,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;
 
@@ -1835,7 +1840,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

@@ -298,6 +298,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_react_test.js

@@ -468,6 +468,51 @@ describe('@noCIdep Plotly.react', 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);