From 9f7a8e70534962e4bea13559c03766a5430eb582 Mon Sep 17 00:00:00 2001 From: Antoine Roy-Gobeil Date: Wed, 10 Oct 2018 11:41:46 -0400 Subject: [PATCH 1/2] improve autosize in edge case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - provide fixed non-zero width/height when autosize:true and container's size is zero - when setting container's widht/height, ignore autosize:true if width/height is explicitly specified by the user. - 🔒 down in test that explicit width/height win over autosize:true --- src/plot_api/plot_api.js | 7 +++--- src/plot_api/subroutines.js | 4 ++-- test/jasmine/tests/config_test.js | 38 +++++++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/src/plot_api/plot_api.js b/src/plot_api/plot_api.js index 609cf6cb3b2..a6875712fd5 100644 --- a/src/plot_api/plot_api.js +++ b/src/plot_api/plot_api.js @@ -488,6 +488,10 @@ function setPlotContext(gd, config) { if(context.setBackground === 'transparent' || typeof context.setBackground !== 'function') { context.setBackground = setBackground; } + + // Check if gd has a specified widht/height to begin with + context._hasZeroHeight = context._hasZeroHeight || gd.clientHeight === 0; + context._hasZeroWidth = context._hasZeroWidth || gd.clientWidth === 0; } function plotPolar(gd, data, layout) { @@ -3248,9 +3252,6 @@ function makePlotFramework(gd) { var gd3 = d3.select(gd); var fullLayout = gd._fullLayout; - // Check if gd has a specified height - fullLayout._hasZeroHeight = gd.clientHeight === 0; - // Plot container fullLayout._container = gd3.selectAll('.plot-container').data([0]); fullLayout._container.enter().insert('div', ':first-child') diff --git a/src/plot_api/subroutines.js b/src/plot_api/subroutines.js index 364762ba229..89521a227b1 100644 --- a/src/plot_api/subroutines.js +++ b/src/plot_api/subroutines.js @@ -54,8 +54,8 @@ function lsInner(gd) { var i, subplot, plotinfo, xa, ya; fullLayout._paperdiv.style({ - width: (fullLayout.autosize) ? '100%' : fullLayout.width + 'px', - height: (fullLayout.autosize && !fullLayout._hasZeroHeight) ? '100%' : fullLayout.height + 'px' + width: (fullLayout.autosize && !gd._context._hasZeroWidth && !gd.layout.width) ? '100%' : fullLayout.width + 'px', + height: (fullLayout.autosize && !gd._context._hasZeroHeight && !gd.layout.height) ? '100%' : fullLayout.height + 'px' }) .selectAll('.main-svg') .call(Drawing.setSize, fullLayout.width, fullLayout.height); diff --git a/test/jasmine/tests/config_test.js b/test/jasmine/tests/config_test.js index 44bc64bb07a..a7dc8a9dcab 100644 --- a/test/jasmine/tests/config_test.js +++ b/test/jasmine/tests/config_test.js @@ -174,6 +174,44 @@ describe('config argument', function() { .then(done); }); + it('should provide a fixed non-zero width/height when autosize: true and container\' size is zero', function(done) { + gd.style.display = 'inline-block'; + Plotly.plot(gd, data, {autosize: true}) + .then(function() { + checkLayoutSize(700, 450); + expect(gd.clientWidth).toBe(700); + expect(gd.clientHeight).toBe(450); + }) + .then(function() { + return Plotly.newPlot(gd, data, {autosize: true}); + }) + // It is important to test newPlot to make sure an initially zero size container + // is still considered to have zero size after a plot is drawn into it. + .then(function() { + checkLayoutSize(700, 450); + expect(gd.clientWidth).toBe(700); + expect(gd.clientHeight).toBe(450); + }) + .catch(failTest) + .then(done); + }); + + // The following test is to guarantee we're not breaking the existing behaviour which may not be ideal. + // In a future version, one may prefer autosize:true winning over an explicit width/height when embedded in a webpage. + it('should use the explicitly provided width/height even if autosize:true', function(done) { + gd.style.width = '1000px'; + gd.style.height = '500px'; + Plotly.plot(gd, data, {autosize: true, width: 1200, height: 700}) + .then(function() { + expect(gd.clientWidth).toBe(1000); + expect(gd.clientHeight).toBe(500); + // The plot should overflow the container! + checkLayoutSize(1200, 700); + }) + .catch(failTest) + .then(done); + }); + it('should respect attribute autosizable: false', function(done) { var autosize = false; var config = { From af4a2cefe10de4eb855f31c9503cb87e68458a90 Mon Sep 17 00:00:00 2001 From: Antoine Roy-Gobeil Date: Wed, 10 Oct 2018 13:47:36 -0400 Subject: [PATCH 2/2] only set container's size to 100% when responsive:true This change will prevent any existing applications from breaking. Anyway setting the container's size to 100% was only useful for responsiveness in modern CSS layouts. --- src/plot_api/plot_config.js | 5 +- src/plot_api/subroutines.js | 4 +- test/jasmine/tests/config_test.js | 83 +++++++++++++++++-------------- 3 files changed, 51 insertions(+), 41 deletions(-) diff --git a/src/plot_api/plot_config.js b/src/plot_api/plot_config.js index 574b618e0a7..a3751322ec5 100644 --- a/src/plot_api/plot_config.js +++ b/src/plot_api/plot_config.js @@ -57,7 +57,10 @@ module.exports = { */ autosizable: false, - // responsive: determines whether to change the layout size when window is resized + /* + * responsive: determines whether to change the layout size when window is resized. + * In v2, this option will be removed and will always be true. + */ responsive: false, // set the length of the undo/redo queue diff --git a/src/plot_api/subroutines.js b/src/plot_api/subroutines.js index 89521a227b1..07445cd3816 100644 --- a/src/plot_api/subroutines.js +++ b/src/plot_api/subroutines.js @@ -54,8 +54,8 @@ function lsInner(gd) { var i, subplot, plotinfo, xa, ya; fullLayout._paperdiv.style({ - width: (fullLayout.autosize && !gd._context._hasZeroWidth && !gd.layout.width) ? '100%' : fullLayout.width + 'px', - height: (fullLayout.autosize && !gd._context._hasZeroHeight && !gd.layout.height) ? '100%' : fullLayout.height + 'px' + width: (gd._context.responsive && fullLayout.autosize && !gd._context._hasZeroWidth && !gd.layout.width) ? '100%' : fullLayout.width + 'px', + height: (gd._context.responsive && fullLayout.autosize && !gd._context._hasZeroHeight && !gd.layout.height) ? '100%' : fullLayout.height + 'px' }) .selectAll('.main-svg') .call(Drawing.setSize, fullLayout.width, fullLayout.height); diff --git a/test/jasmine/tests/config_test.js b/test/jasmine/tests/config_test.js index a7dc8a9dcab..e85451e6a5d 100644 --- a/test/jasmine/tests/config_test.js +++ b/test/jasmine/tests/config_test.js @@ -174,44 +174,6 @@ describe('config argument', function() { .then(done); }); - it('should provide a fixed non-zero width/height when autosize: true and container\' size is zero', function(done) { - gd.style.display = 'inline-block'; - Plotly.plot(gd, data, {autosize: true}) - .then(function() { - checkLayoutSize(700, 450); - expect(gd.clientWidth).toBe(700); - expect(gd.clientHeight).toBe(450); - }) - .then(function() { - return Plotly.newPlot(gd, data, {autosize: true}); - }) - // It is important to test newPlot to make sure an initially zero size container - // is still considered to have zero size after a plot is drawn into it. - .then(function() { - checkLayoutSize(700, 450); - expect(gd.clientWidth).toBe(700); - expect(gd.clientHeight).toBe(450); - }) - .catch(failTest) - .then(done); - }); - - // The following test is to guarantee we're not breaking the existing behaviour which may not be ideal. - // In a future version, one may prefer autosize:true winning over an explicit width/height when embedded in a webpage. - it('should use the explicitly provided width/height even if autosize:true', function(done) { - gd.style.width = '1000px'; - gd.style.height = '500px'; - Plotly.plot(gd, data, {autosize: true, width: 1200, height: 700}) - .then(function() { - expect(gd.clientWidth).toBe(1000); - expect(gd.clientHeight).toBe(500); - // The plot should overflow the container! - checkLayoutSize(1200, 700); - }) - .catch(failTest) - .then(done); - }); - it('should respect attribute autosizable: false', function(done) { var autosize = false; var config = { @@ -743,5 +705,50 @@ describe('config argument', function() { .then(testResponsive) .then(done); }); + + it('should provide a fixed non-zero width/height when autosize/responsive: true and container\' size is zero', function(done) { + fillParent(1, 1, function() { + this.style.display = 'inline-block'; + this.style.width = null; + this.style.height = null; + }); + Plotly.plot(gd, data, {autosize: true}, {responsive: true}) + .then(function() { + checkLayoutSize(700, 450); + expect(gd.clientWidth).toBe(700); + expect(gd.clientHeight).toBe(450); + }) + .then(function() { + return Plotly.newPlot(gd, data, {autosize: true}, {responsive: true}); + }) + // It is important to test newPlot to make sure an initially zero size container + // is still considered to have zero size after a plot is drawn into it. + .then(function() { + checkLayoutSize(700, 450); + expect(gd.clientWidth).toBe(700); + expect(gd.clientHeight).toBe(450); + }) + .catch(failTest) + .then(done); + }); + + // The following test is to guarantee we're not breaking the existing (although maybe not ideal) behaviour. + // In a future version, one may prefer responsive/autosize:true winning over an explicit width/height when embedded in a webpage. + it('should use the explicitly provided width/height even if autosize/responsive:true', function(done) { + fillParent(1, 1, function() { + this.style.width = '1000px'; + this.style.height = '500px'; + }); + + Plotly.plot(gd, data, {autosize: true, width: 1200, height: 700}, {responsive: true}) + .then(function() { + expect(gd.clientWidth).toBe(1000); + expect(gd.clientHeight).toBe(500); + // The plot should overflow the container! + checkLayoutSize(1200, 700); + }) + .catch(failTest) + .then(done); + }); }); });