From e584f807f921e41f72bd5882fecfbfdb97c61cd2 Mon Sep 17 00:00:00 2001 From: archmoj Date: Fri, 19 Jul 2019 15:11:04 -0400 Subject: [PATCH 1/3] apply Lib.mergeArrayCastPositive in places to ensure having positive sizes --- src/lib/index.js | 2 +- src/traces/funnel/arrays_to_calcdata.js | 14 +++++++------- src/traces/scatter/arrays_to_calcdata.js | 8 ++++---- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/lib/index.js b/src/lib/index.js index da1e75093ba..6fc8f8deceb 100644 --- a/src/lib/index.js +++ b/src/lib/index.js @@ -474,7 +474,7 @@ lib.mergeArray = function(traceAttr, cd, cdAttr, fn) { lib.mergeArrayCastPositive = function(traceAttr, cd, cdAttr) { return lib.mergeArray(traceAttr, cd, cdAttr, function(v) { var w = +v; - return w > 0 ? w : 0; + return isNaN(w) ? NaN : w > 0 ? w : 0; }); }; diff --git a/src/traces/funnel/arrays_to_calcdata.js b/src/traces/funnel/arrays_to_calcdata.js index a722f3dec04..ad40a4ad075 100644 --- a/src/traces/funnel/arrays_to_calcdata.js +++ b/src/traces/funnel/arrays_to_calcdata.js @@ -8,24 +8,24 @@ 'use strict'; -var mergeArray = require('../../lib').mergeArray; +var Lib = require('../../lib'); // arrayOk attributes, merge them into calcdata array module.exports = function arraysToCalcdata(cd, trace) { for(var i = 0; i < cd.length; i++) cd[i].i = i; - mergeArray(trace.text, cd, 'tx'); - mergeArray(trace.hovertext, cd, 'htx'); + Lib.mergeArray(trace.text, cd, 'tx'); + Lib.mergeArray(trace.hovertext, cd, 'htx'); var marker = trace.marker; if(marker) { - mergeArray(marker.opacity, cd, 'mo'); - mergeArray(marker.color, cd, 'mc'); + Lib.mergeArray(marker.opacity, cd, 'mo'); + Lib.mergeArray(marker.color, cd, 'mc'); var markerLine = marker.line; if(markerLine) { - mergeArray(markerLine.color, cd, 'mlc'); - mergeArray(markerLine.width, cd, 'mlw'); + Lib.mergeArray(markerLine.color, cd, 'mlc'); + Lib.mergeArrayCastPositive(markerLine.width, cd, 'mlw'); } } }; diff --git a/src/traces/scatter/arrays_to_calcdata.js b/src/traces/scatter/arrays_to_calcdata.js index ee83c3aac31..8842602402a 100644 --- a/src/traces/scatter/arrays_to_calcdata.js +++ b/src/traces/scatter/arrays_to_calcdata.js @@ -22,22 +22,22 @@ module.exports = function arraysToCalcdata(cd, trace) { Lib.mergeArray(trace.customdata, cd, 'data'); Lib.mergeArray(trace.textposition, cd, 'tp'); if(trace.textfont) { - Lib.mergeArray(trace.textfont.size, cd, 'ts'); + Lib.mergeArrayCastPositive(trace.textfont.size, cd, 'ts'); Lib.mergeArray(trace.textfont.color, cd, 'tc'); Lib.mergeArray(trace.textfont.family, cd, 'tf'); } var marker = trace.marker; if(marker) { - Lib.mergeArray(marker.size, cd, 'ms'); - Lib.mergeArray(marker.opacity, cd, 'mo'); + Lib.mergeArrayCastPositive(marker.size, cd, 'ms'); + Lib.mergeArrayCastPositive(marker.opacity, cd, 'mo'); Lib.mergeArray(marker.symbol, cd, 'mx'); Lib.mergeArray(marker.color, cd, 'mc'); var markerLine = marker.line; if(marker.line) { Lib.mergeArray(markerLine.color, cd, 'mlc'); - Lib.mergeArray(markerLine.width, cd, 'mlw'); + Lib.mergeArrayCastPositive(markerLine.width, cd, 'mlw'); } var markerGradient = marker.gradient; From 39b9840933957640fe26e9a9f9c30e1a5625c782 Mon Sep 17 00:00:00 2001 From: archmoj Date: Tue, 23 Jul 2019 15:54:48 -0400 Subject: [PATCH 2/3] add jasmine tests to guard against negative sizes during calc step --- test/jasmine/tests/bar_test.js | 14 ++++++++ test/jasmine/tests/funnel_test.js | 28 ++++++++++++++++ test/jasmine/tests/scatter_test.js | 52 ++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+) diff --git a/test/jasmine/tests/bar_test.js b/test/jasmine/tests/bar_test.js index 9a108e5644a..16fc23250f1 100644 --- a/test/jasmine/tests/bar_test.js +++ b/test/jasmine/tests/bar_test.js @@ -407,6 +407,20 @@ describe('Bar.calc', function() { assertPointField(cd, 'x', [[1, NaN, NaN, 15]]); assertPointField(cd, 'y', [[1, 2, 10, 30]]); }); + + it('should guard against negative marker.line.width values', function() { + var gd = mockBarPlot([{ + marker: { + line: { + width: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}] + } + }, + y: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] + }], {}); + + var cd = gd.calcdata; + assertPointField(cd, 'mlw', [[2, 1, 0, 0, 0, 1, 0, 0, 0, Infinity, NaN, NaN]]); + }); }); describe('Bar.crossTraceCalc (formerly known as setPositions)', function() { diff --git a/test/jasmine/tests/funnel_test.js b/test/jasmine/tests/funnel_test.js index cb3861833ae..af4898a5979 100644 --- a/test/jasmine/tests/funnel_test.js +++ b/test/jasmine/tests/funnel_test.js @@ -351,6 +351,34 @@ describe('Funnel.calc', function() { assertPointField(cd, 'y', [[1, NaN, NaN, 15]]); assertPointField(cd, 'x', [[0.5, 1, 5, 15]]); }); + + it('should guard against negative marker.line.width values', function() { + var gd = mockFunnelPlot([{ + marker: { + line: { + width: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}] + } + }, + y: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] + }], {}); + + var cd = gd.calcdata; + assertPointField(cd, 'mlw', [[2, 1, 0, 0, 0, 1, 0, 0, 0, Infinity, NaN, NaN]]); + }); + + it('should guard against negative marker.line.width values', function() { + var gd = mockFunnelPlot([{ + marker: { + line: { + width: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}] + } + }, + y: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] + }], {}); + + var cd = gd.calcdata; + assertPointField(cd, 'mlw', [[2, 1, 0, 0, 0, 1, 0, 0, 0, Infinity, NaN, NaN]]); + }); }); describe('Funnel.crossTraceCalc', function() { diff --git a/test/jasmine/tests/scatter_test.js b/test/jasmine/tests/scatter_test.js index 1c24d177a3b..8aab391f5d5 100644 --- a/test/jasmine/tests/scatter_test.js +++ b/test/jasmine/tests/scatter_test.js @@ -3,6 +3,7 @@ var Scatter = require('@src/traces/scatter'); var makeBubbleSizeFn = require('@src/traces/scatter/make_bubble_size_func'); var linePoints = require('@src/traces/scatter/line_points'); var Lib = require('@src/lib'); +var Plots = require('@src/plots/plots'); var Plotly = require('@lib/index'); var createGraphDiv = require('../assets/create_graph_div'); @@ -18,6 +19,8 @@ var assertMultiNodeOrder = customAssertions.assertMultiNodeOrder; var checkEventData = require('../assets/check_event_data'); var constants = require('@src/traces/scatter/constants'); +var supplyAllDefaults = require('../assets/supply_defaults'); + var getOpacity = function(node) { return Number(node.style.opacity); }; var getFillOpacity = function(node) { return Number(node.style['fill-opacity']); }; var getColor = function(node) { return node.style.fill; }; @@ -273,6 +276,55 @@ describe('Test scatter', function() { }); }); + describe('calc', function() { + function assertPointField(calcData, prop, expectation) { + var values = []; + + calcData.forEach(function(calcTrace) { + var vals = calcTrace.map(function(pt) { + return Lib.nestedProperty(pt, prop).get(); + }); + + values.push(vals); + }); + + expect(values).toBeCloseTo2DArray(expectation, undefined, '(field ' + prop + ')'); + } + + it('should guard against negative size values', function() { + var gd = { + data: [{ + type: 'scatter', + mode: 'markers+text', + marker: { + line: { + width: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}] + }, + opacity: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}], + size: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}] + }, + textfont: { + size: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}] + }, + text: ['1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11', '12'], + y: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] + }], + layout: {}, + calcdata: [], + _context: {locale: 'en', locales: {}} + }; + + supplyAllDefaults(gd); + Plots.doCalcdata(gd); + + var cd = gd.calcdata; + assertPointField(cd, 'mlw', [[2, 1, 0, 0, 0, 1, 0, 0, 0, Infinity, NaN, NaN]]); + assertPointField(cd, 'mo', [[2, 1, 0, 0, 0, 1, 0, 0, 0, Infinity, NaN, NaN]]); + assertPointField(cd, 'ms', [[2, 1, 0, 0, 0, 1, 0, 0, 0, Infinity, NaN, NaN]]); + assertPointField(cd, 'ts', [[2, 1, 0, 0, 0, 1, 0, 0, 0, Infinity, NaN, NaN]]); + }); + }); + describe('isBubble', function() { it('should return true when marker.size is an Array', function() { var trace = { From f9db523c7fd838da60607bd9eabf8e84190b97d9 Mon Sep 17 00:00:00 2001 From: archmoj Date: Tue, 23 Jul 2019 16:47:53 -0400 Subject: [PATCH 3/3] cast bad size values to zero instead of NaN - fixup scattergeo jasmine test --- src/lib/index.js | 2 +- test/jasmine/tests/bar_test.js | 6 +++--- test/jasmine/tests/funnel_test.js | 20 +++----------------- test/jasmine/tests/scatter_test.js | 20 ++++++++++---------- test/jasmine/tests/scattergeo_test.js | 2 +- 5 files changed, 18 insertions(+), 32 deletions(-) diff --git a/src/lib/index.js b/src/lib/index.js index 6fc8f8deceb..726cc5e0a30 100644 --- a/src/lib/index.js +++ b/src/lib/index.js @@ -474,7 +474,7 @@ lib.mergeArray = function(traceAttr, cd, cdAttr, fn) { lib.mergeArrayCastPositive = function(traceAttr, cd, cdAttr) { return lib.mergeArray(traceAttr, cd, cdAttr, function(v) { var w = +v; - return isNaN(w) ? NaN : w > 0 ? w : 0; + return !isFinite(w) ? 0 : w > 0 ? w : 0; }); }; diff --git a/test/jasmine/tests/bar_test.js b/test/jasmine/tests/bar_test.js index 16fc23250f1..15526cf35e0 100644 --- a/test/jasmine/tests/bar_test.js +++ b/test/jasmine/tests/bar_test.js @@ -412,14 +412,14 @@ describe('Bar.calc', function() { var gd = mockBarPlot([{ marker: { line: { - width: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}] + width: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}, '12+1', '1e1'] } }, - y: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] + y: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14] }], {}); var cd = gd.calcdata; - assertPointField(cd, 'mlw', [[2, 1, 0, 0, 0, 1, 0, 0, 0, Infinity, NaN, NaN]]); + assertPointField(cd, 'mlw', [[2, 1, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 10]]); }); }); diff --git a/test/jasmine/tests/funnel_test.js b/test/jasmine/tests/funnel_test.js index af4898a5979..6804069adaf 100644 --- a/test/jasmine/tests/funnel_test.js +++ b/test/jasmine/tests/funnel_test.js @@ -356,28 +356,14 @@ describe('Funnel.calc', function() { var gd = mockFunnelPlot([{ marker: { line: { - width: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}] + width: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}, '12+1', '1e1'] } }, - y: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] + y: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14] }], {}); var cd = gd.calcdata; - assertPointField(cd, 'mlw', [[2, 1, 0, 0, 0, 1, 0, 0, 0, Infinity, NaN, NaN]]); - }); - - it('should guard against negative marker.line.width values', function() { - var gd = mockFunnelPlot([{ - marker: { - line: { - width: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}] - } - }, - y: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] - }], {}); - - var cd = gd.calcdata; - assertPointField(cd, 'mlw', [[2, 1, 0, 0, 0, 1, 0, 0, 0, Infinity, NaN, NaN]]); + assertPointField(cd, 'mlw', [[2, 1, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 10]]); }); }); diff --git a/test/jasmine/tests/scatter_test.js b/test/jasmine/tests/scatter_test.js index 8aab391f5d5..d7639333903 100644 --- a/test/jasmine/tests/scatter_test.js +++ b/test/jasmine/tests/scatter_test.js @@ -298,16 +298,16 @@ describe('Test scatter', function() { mode: 'markers+text', marker: { line: { - width: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}] + width: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}, '12+1', '1e1'] }, - opacity: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}], - size: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}] + opacity: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}, '12+1', '1e1'], + size: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}, '12+1', '1e1'] }, textfont: { - size: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}] + size: [2, 1, 0, -1, false, true, null, [], -Infinity, Infinity, NaN, {}, '12+1', '1e1'] }, - text: ['1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11', '12'], - y: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] + text: ['1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11', '12', '13', '14'], + y: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14] }], layout: {}, calcdata: [], @@ -318,10 +318,10 @@ describe('Test scatter', function() { Plots.doCalcdata(gd); var cd = gd.calcdata; - assertPointField(cd, 'mlw', [[2, 1, 0, 0, 0, 1, 0, 0, 0, Infinity, NaN, NaN]]); - assertPointField(cd, 'mo', [[2, 1, 0, 0, 0, 1, 0, 0, 0, Infinity, NaN, NaN]]); - assertPointField(cd, 'ms', [[2, 1, 0, 0, 0, 1, 0, 0, 0, Infinity, NaN, NaN]]); - assertPointField(cd, 'ts', [[2, 1, 0, 0, 0, 1, 0, 0, 0, Infinity, NaN, NaN]]); + assertPointField(cd, 'mlw', [[2, 1, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 10]]); + assertPointField(cd, 'mo', [[2, 1, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 10]]); + assertPointField(cd, 'ms', [[2, 1, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 10]]); + assertPointField(cd, 'ts', [[2, 1, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 10]]); }); }); diff --git a/test/jasmine/tests/scattergeo_test.js b/test/jasmine/tests/scattergeo_test.js index d8b7c659479..718fa6401c4 100644 --- a/test/jasmine/tests/scattergeo_test.js +++ b/test/jasmine/tests/scattergeo_test.js @@ -215,7 +215,7 @@ describe('Test scattergeo calc', function() { expect(calcTrace).toEqual([ { lonlat: [10, 20], mc: 0, ms: 10 }, - { lonlat: [20, 30], mc: null, ms: NaN }, + { lonlat: [20, 30], mc: null, ms: 0 }, { lonlat: [BADNUM, BADNUM], mc: 5, ms: 8 }, { lonlat: [30, 10], mc: 10, ms: 10 } ]);