From 4aca3157a5bd290137f7c23c412101efb80cac31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 31 Oct 2018 15:32:50 -0400 Subject: [PATCH 1/7] lint in scattergl.plot --- src/traces/scattergl/index.js | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/src/traces/scattergl/index.js b/src/traces/scattergl/index.js index 8831fdb6bdb..8604853dd90 100644 --- a/src/traces/scattergl/index.js +++ b/src/traces/scattergl/index.js @@ -395,7 +395,8 @@ function plot(gd, subplot, cdata) { scene.line2d.update(scene.lineOptions); scene.lineOptions = scene.lineOptions.map(function(lineOptions) { if(lineOptions && lineOptions.positions) { - var pos = [], srcPos = lineOptions.positions; + var pos = []; + var srcPos = lineOptions.positions; var firstptdef = 0; while(isNaN(srcPos[firstptdef]) || isNaN(srcPos[firstptdef + 1])) { @@ -403,7 +404,7 @@ function plot(gd, subplot, cdata) { } var lastptdef = srcPos.length - 2; while(isNaN(srcPos[lastptdef]) || isNaN(srcPos[lastptdef + 1])) { - lastptdef += -2; + lastptdef -= 2; } pos = pos.concat(srcPos.slice(firstptdef, lastptdef + 2)); lineOptions.positions = pos; @@ -437,36 +438,38 @@ function plot(gd, subplot, cdata) { if(trace._nexttrace) fillData.push(i + 1); if(fillData.length) scene.fillOrder[i] = fillData; - var pos = [], srcPos = (lineOptions && lineOptions.positions) || stash.positions; + var pos = []; + var srcPos = (lineOptions && lineOptions.positions) || stash.positions; + var firstptdef, lastptdef; if(trace.fill === 'tozeroy') { - var firstpdef = 0; while(isNaN(srcPos[firstpdef + 1])) { - firstpdef += 2; + firstptdef = 0; + firstptdef += 2; } - var lastpdef = srcPos.length - 2; while(isNaN(srcPos[lastpdef + 1])) { - lastpdef += -2; + lastptdef = srcPos.length - 2; + lastptdef -= 2; } - if(srcPos[firstpdef + 1] !== 0) { - pos = [ srcPos[firstpdef], 0 ]; + if(srcPos[firstptdef + 1] !== 0) { + pos = [srcPos[firstptdef], 0]; } - pos = pos.concat(srcPos.slice(firstpdef, lastpdef + 2)); - if(srcPos[lastpdef + 1] !== 0) { - pos = pos.concat([ srcPos[lastpdef], 0 ]); + pos = pos.concat(srcPos.slice(firstptdef, lastptdef + 2)); + if(srcPos[lastptdef + 1] !== 0) { + pos = pos.concat([srcPos[lastptdef], 0]); } } else if(trace.fill === 'tozerox') { - var firstptdef = 0; while(isNaN(srcPos[firstptdef])) { + firstptdef = 0; firstptdef += 2; } - var lastptdef = srcPos.length - 2; while(isNaN(srcPos[lastptdef])) { - lastptdef += -2; + lastptdef = srcPos.length - 2; + lastptdef -= 2; } if(srcPos[firstptdef] !== 0) { - pos = [ 0, srcPos[firstptdef + 1] ]; + pos = [0, srcPos[firstptdef + 1]]; } pos = pos.concat(srcPos.slice(firstptdef, lastptdef + 2)); if(srcPos[lastptdef] !== 0) { From 0ce79bcaaa3cdc3ea1e6001371d0f5f461a6e0fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 31 Oct 2018 15:34:09 -0400 Subject: [PATCH 2/7] guard against infinite loops ... in scattergl positions cleanup. --- src/traces/scattergl/index.js | 12 +-- test/jasmine/tests/gl2d_plot_interact_test.js | 73 +++++++++++++++++++ 2 files changed, 79 insertions(+), 6 deletions(-) diff --git a/src/traces/scattergl/index.js b/src/traces/scattergl/index.js index 8604853dd90..fadfaf96932 100644 --- a/src/traces/scattergl/index.js +++ b/src/traces/scattergl/index.js @@ -399,11 +399,11 @@ function plot(gd, subplot, cdata) { var srcPos = lineOptions.positions; var firstptdef = 0; - while(isNaN(srcPos[firstptdef]) || isNaN(srcPos[firstptdef + 1])) { + while(firstptdef < srcPos.length && (isNaN(srcPos[firstptdef]) || isNaN(srcPos[firstptdef + 1]))) { firstptdef += 2; } var lastptdef = srcPos.length - 2; - while(isNaN(srcPos[lastptdef]) || isNaN(srcPos[lastptdef + 1])) { + while(lastptdef > -1 && (isNaN(srcPos[lastptdef]) || isNaN(srcPos[lastptdef + 1]))) { lastptdef -= 2; } pos = pos.concat(srcPos.slice(firstptdef, lastptdef + 2)); @@ -443,12 +443,12 @@ function plot(gd, subplot, cdata) { var firstptdef, lastptdef; if(trace.fill === 'tozeroy') { - while(isNaN(srcPos[firstpdef + 1])) { firstptdef = 0; + while(firstptdef < srcPos.length && isNaN(srcPos[firstptdef + 1])) { firstptdef += 2; } - while(isNaN(srcPos[lastpdef + 1])) { lastptdef = srcPos.length - 2; + while(lastptdef > -1 && isNaN(srcPos[lastptdef + 1])) { lastptdef -= 2; } if(srcPos[firstptdef + 1] !== 0) { @@ -460,12 +460,12 @@ function plot(gd, subplot, cdata) { } } else if(trace.fill === 'tozerox') { - while(isNaN(srcPos[firstptdef])) { firstptdef = 0; + while(firstptdef < srcPos.length && isNaN(srcPos[firstptdef])) { firstptdef += 2; } - while(isNaN(srcPos[lastptdef])) { lastptdef = srcPos.length - 2; + while(lastptdef > -1 && isNaN(srcPos[lastptdef])) { lastptdef -= 2; } if(srcPos[firstptdef] !== 0) { diff --git a/test/jasmine/tests/gl2d_plot_interact_test.js b/test/jasmine/tests/gl2d_plot_interact_test.js index e80d9606352..342b7cbc280 100644 --- a/test/jasmine/tests/gl2d_plot_interact_test.js +++ b/test/jasmine/tests/gl2d_plot_interact_test.js @@ -1186,6 +1186,79 @@ describe('Test gl2d plots', function() { .catch(failTest) .then(done); }); + + it('@gl should not cause infinite loops when coordinate arrays start/end with NaN', function(done) { + function _assertPositions(msg, cont, exp) { + var pos = gd._fullLayout._plots.xy._scene[cont] + .map(function(opt) { return opt.positions; }); + expect(pos).toBeCloseTo2DArray(exp, 2, msg); + } + + Plotly.plot(gd, [{ + type: 'scattergl', + mode: 'lines', + x: [1, 2, 3], + y: [null, null, null] + }, { + type: 'scattergl', + mode: 'lines', + x: [1, 2, 3], + y: [1, 2, null] + }, { + type: 'scattergl', + mode: 'lines', + x: [null, 2, 3], + y: [1, 2, 3] + }, { + type: 'scattergl', + mode: 'lines', + x: [null, null, null], + y: [1, 2, 3] + }, { + }]) + .then(function() { + _assertPositions('base', 'lineOptions', [ + [], + [1, 1, 2, 2], + [2, 2, 3, 3], + [] + ]); + + return Plotly.restyle(gd, 'fill', 'tozerox'); + }) + .then(function() { + _assertPositions('tozerox', 'lineOptions', [ + [], + [1, 1, 2, 2], + [2, 2, 3, 3], + [] + ]); + _assertPositions('tozerox', 'fillOptions', [ + [0, undefined, 0, undefined], + [0, 1, 1, 1, 2, 2, 0, 2], + [0, 2, 2, 2, 3, 3, 0, 3], + [0, undefined, 0, undefined] + ]); + + return Plotly.restyle(gd, 'fill', 'tozeroy'); + }) + .then(function() { + _assertPositions('tozeroy', 'lineOptions', [ + [], + [1, 1, 2, 2], + [2, 2, 3, 3], + [] + ]); + _assertPositions('tozeroy', 'fillOptions', [ + [undefined, 0, undefined, 0], + [1, 0, 1, 1, 2, 2, 2, 0], + [2, 0, 2, 2, 3, 3, 3, 0], + [undefined, 0, undefined, 0] + ]); + }) + .catch(failTest) + .then(done); + }); }); describe('Test scattergl autorange:', function() { From 8f627dcacc40a031a7da1516c4e438dc5f795618 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 31 Oct 2018 15:45:48 -0400 Subject: [PATCH 3/7] add flaky to "responsive after update" test --- test/jasmine/tests/config_test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/jasmine/tests/config_test.js b/test/jasmine/tests/config_test.js index dd6ce559ca0..0950fde7e13 100644 --- a/test/jasmine/tests/config_test.js +++ b/test/jasmine/tests/config_test.js @@ -615,7 +615,7 @@ describe('config argument', function() { .then(done); }); - it('should still be responsive if the plot is edited', function(done) { + it('@flaky should still be responsive if the plot is edited', function(done) { fillParent(1, 1); Plotly.plot(gd, data, {}, {responsive: true}) .then(function() {return Plotly.restyle(gd, 'y[0]', data[0].y[0] + 2);}) From dfdb1f4e99e6e94709041556f161053031230b3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 31 Oct 2018 16:15:46 -0400 Subject: [PATCH 4/7] (arrg) make the whole responsive suite flaky --- test/jasmine/tests/config_test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/jasmine/tests/config_test.js b/test/jasmine/tests/config_test.js index 0950fde7e13..0598da0f549 100644 --- a/test/jasmine/tests/config_test.js +++ b/test/jasmine/tests/config_test.js @@ -548,7 +548,7 @@ describe('config argument', function() { }); }); - describe('responsive figure', function() { + describe('@flaky responsive figure', function() { var gd, data = [{x: [1, 2, 3, 4], y: [5, 10, 2, 8]}]; var width = 960, height = 800; @@ -615,7 +615,7 @@ describe('config argument', function() { .then(done); }); - it('@flaky should still be responsive if the plot is edited', function(done) { + it('should still be responsive if the plot is edited', function(done) { fillParent(1, 1); Plotly.plot(gd, data, {}, {responsive: true}) .then(function() {return Plotly.restyle(gd, 'y[0]', data[0].y[0] + 2);}) From 5441ef9fc6365b8ebb4b21ee5bae2837a720d418 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 31 Oct 2018 16:20:02 -0400 Subject: [PATCH 5/7] simplify line position cleanup --- src/traces/scattergl/index.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/traces/scattergl/index.js b/src/traces/scattergl/index.js index fadfaf96932..f0297537433 100644 --- a/src/traces/scattergl/index.js +++ b/src/traces/scattergl/index.js @@ -395,7 +395,6 @@ function plot(gd, subplot, cdata) { scene.line2d.update(scene.lineOptions); scene.lineOptions = scene.lineOptions.map(function(lineOptions) { if(lineOptions && lineOptions.positions) { - var pos = []; var srcPos = lineOptions.positions; var firstptdef = 0; @@ -406,8 +405,7 @@ function plot(gd, subplot, cdata) { while(lastptdef > -1 && (isNaN(srcPos[lastptdef]) || isNaN(srcPos[lastptdef + 1]))) { lastptdef -= 2; } - pos = pos.concat(srcPos.slice(firstptdef, lastptdef + 2)); - lineOptions.positions = pos; + lineOptions.positions = srcPos.slice(firstptdef, lastptdef + 2); } return lineOptions; }); From 2c8a29049579ccce9097bd280766a4bb7f109ee0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 31 Oct 2018 16:20:57 -0400 Subject: [PATCH 6/7] aj-proof downward line position loop --- src/traces/scattergl/index.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/traces/scattergl/index.js b/src/traces/scattergl/index.js index f0297537433..63a0a03dcbf 100644 --- a/src/traces/scattergl/index.js +++ b/src/traces/scattergl/index.js @@ -402,7 +402,7 @@ function plot(gd, subplot, cdata) { firstptdef += 2; } var lastptdef = srcPos.length - 2; - while(lastptdef > -1 && (isNaN(srcPos[lastptdef]) || isNaN(srcPos[lastptdef + 1]))) { + while(lastptdef > firstptdef && (isNaN(srcPos[lastptdef]) || isNaN(srcPos[lastptdef + 1]))) { lastptdef -= 2; } lineOptions.positions = srcPos.slice(firstptdef, lastptdef + 2); @@ -446,7 +446,7 @@ function plot(gd, subplot, cdata) { firstptdef += 2; } lastptdef = srcPos.length - 2; - while(lastptdef > -1 && isNaN(srcPos[lastptdef + 1])) { + while(lastptdef > firstptdef && isNaN(srcPos[lastptdef + 1])) { lastptdef -= 2; } if(srcPos[firstptdef + 1] !== 0) { @@ -463,7 +463,7 @@ function plot(gd, subplot, cdata) { firstptdef += 2; } lastptdef = srcPos.length - 2; - while(lastptdef > -1 && isNaN(srcPos[lastptdef])) { + while(lastptdef > firstptdef && isNaN(srcPos[lastptdef])) { lastptdef -= 2; } if(srcPos[firstptdef] !== 0) { From c19e727628c286d688b6b918089bef44b9f09496 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Wed, 31 Oct 2018 16:34:59 -0400 Subject: [PATCH 7/7] mv flaky tags from 'describe' -> 'it' --- test/jasmine/tests/config_test.js | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/test/jasmine/tests/config_test.js b/test/jasmine/tests/config_test.js index 0598da0f549..7054a849f88 100644 --- a/test/jasmine/tests/config_test.js +++ b/test/jasmine/tests/config_test.js @@ -548,7 +548,7 @@ describe('config argument', function() { }); }); - describe('@flaky responsive figure', function() { + describe('responsive figure', function() { var gd, data = [{x: [1, 2, 3, 4], y: [5, 10, 2, 8]}]; var width = 960, height = 800; @@ -608,14 +608,14 @@ describe('config argument', function() { gd = parent.childNodes[0]; } - it('should resize when the viewport width/height changes', function(done) { + it('@flaky should resize when the viewport width/height changes', function(done) { fillParent(1, 1); Plotly.plot(gd, data, {}, {responsive: true}) .then(testResponsive) .then(done); }); - it('should still be responsive if the plot is edited', function(done) { + it('@flaky should still be responsive if the plot is edited', function(done) { fillParent(1, 1); Plotly.plot(gd, data, {}, {responsive: true}) .then(function() {return Plotly.restyle(gd, 'y[0]', data[0].y[0] + 2);}) @@ -623,7 +623,7 @@ describe('config argument', function() { .then(done); }); - it('should still be responsive if the plot is purged and replotted', function(done) { + it('@flaky should still be responsive if the plot is purged and replotted', function(done) { fillParent(1, 1); Plotly.plot(gd, data, {}, {responsive: true}) .then(function() {return Plotly.newPlot(gd, data, {}, {responsive: true});}) @@ -631,7 +631,7 @@ describe('config argument', function() { .then(done); }); - it('should only have one resize handler when plotted more than once', function(done) { + it('@flaky should only have one resize handler when plotted more than once', function(done) { fillParent(1, 1); var cntWindowResize = 0; window.addEventListener('resize', function() {cntWindowResize++;}); @@ -650,7 +650,7 @@ describe('config argument', function() { .then(done); }); - it('should become responsive if configured as such via Plotly.react', function(done) { + it('@flaky should become responsive if configured as such via Plotly.react', function(done) { fillParent(1, 1); Plotly.plot(gd, data, {}, {responsive: false}) .then(function() {return Plotly.react(gd, data, {}, {responsive: true});}) @@ -658,7 +658,7 @@ describe('config argument', function() { .then(done); }); - it('should stop being responsive if configured as such via Plotly.react', function(done) { + it('@flaky should stop being responsive if configured as such via Plotly.react', function(done) { fillParent(1, 1); Plotly.plot(gd, data, {}, {responsive: true}) // Check initial size @@ -676,7 +676,7 @@ describe('config argument', function() { }); // Testing fancier CSS layouts - it('should resize horizontally in a flexbox when responsive: true', function(done) { + it('@flaky should resize horizontally in a flexbox when responsive: true', function(done) { parent.style.display = 'flex'; parent.style.flexDirection = 'row'; fillParent(1, 2, function() { @@ -688,7 +688,7 @@ describe('config argument', function() { .then(done); }); - it('should resize vertically in a flexbox when responsive: true', function(done) { + it('@flaky should resize vertically in a flexbox when responsive: true', function(done) { parent.style.display = 'flex'; parent.style.flexDirection = 'column'; fillParent(2, 1, function() { @@ -700,7 +700,7 @@ describe('config argument', function() { .then(done); }); - it('should resize in both direction in a grid when responsive: true', function(done) { + it('@flaky should resize in both direction in a grid when responsive: true', function(done) { var numCols = 2, numRows = 2; parent.style.display = 'grid'; parent.style.gridTemplateColumns = 'repeat(' + numCols + ', 1fr)'; @@ -712,7 +712,7 @@ describe('config argument', function() { .then(done); }); - it('should provide a fixed non-zero width/height when autosize/responsive: true and container\' size is zero', function(done) { + it('@flaky 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; @@ -740,7 +740,7 @@ describe('config argument', function() { // 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) { + it('@flaky 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';