From fb8357a87de0995da8080f04c2f086d1c1a31e0d Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Mon, 22 Jan 2018 22:43:49 -0500 Subject: [PATCH] fix #2271 - ghost fill tonext when either trace is emptied out --- src/traces/scatter/link_traces.js | 7 ++--- src/traces/scatter/plot.js | 49 +++++++++++++++++++----------- test/jasmine/tests/scatter_test.js | 44 +++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 21 deletions(-) diff --git a/src/traces/scatter/link_traces.js b/src/traces/scatter/link_traces.js index cbadf7f0e39..d51c8f2c7a8 100644 --- a/src/traces/scatter/link_traces.js +++ b/src/traces/scatter/link_traces.js @@ -9,12 +9,11 @@ 'use strict'; module.exports = function linkTraces(gd, plotinfo, cdscatter) { - var cd, trace; + var trace, i; var prevtrace = null; - for(var i = 0; i < cdscatter.length; ++i) { - cd = cdscatter[i]; - trace = cd[0].trace; + for(i = 0; i < cdscatter.length; ++i) { + trace = cdscatter[i][0].trace; // Note: The check which ensures all cdscatter here are for the same axis and // are either cartesian or scatterternary has been removed. This code assumes diff --git a/src/traces/scatter/plot.js b/src/traces/scatter/plot.js index 8e29717c19e..6d17026691b 100644 --- a/src/traces/scatter/plot.js +++ b/src/traces/scatter/plot.js @@ -323,6 +323,10 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition Drawing.setClipUrl(lineJoin, plotinfo.layerClipId); + function clearFill(selection) { + transition(selection).attr('d', 'M0,0Z'); + } + if(segments.length) { if(ownFillEl3) { if(pt0 && pt1) { @@ -348,30 +352,41 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition } } } - else if(trace.fill.substr(0, 6) === 'tonext' && fullpath && prevRevpath) { - // fill to next: full trace path, plus the previous path reversed - if(trace.fill === 'tonext') { - // tonext: for use by concentric shapes, like manually constructed - // contours, we just add the two paths closed on themselves. - // This makes strange results if one path is *not* entirely - // inside the other, but then that is a strange usage. - transition(tonext).attr('d', fullpath + 'Z' + prevRevpath + 'Z') - .call(Drawing.singleFillStyle); + else if(tonext) { + if(trace.fill.substr(0, 6) === 'tonext' && fullpath && prevRevpath) { + // fill to next: full trace path, plus the previous path reversed + if(trace.fill === 'tonext') { + // tonext: for use by concentric shapes, like manually constructed + // contours, we just add the two paths closed on themselves. + // This makes strange results if one path is *not* entirely + // inside the other, but then that is a strange usage. + transition(tonext).attr('d', fullpath + 'Z' + prevRevpath + 'Z') + .call(Drawing.singleFillStyle); + } + else { + // tonextx/y: for now just connect endpoints with lines. This is + // the correct behavior if the endpoints are at the same value of + // y/x, but if they *aren't*, we should ideally do more complicated + // things depending on whether the new endpoint projects onto the + // existing curve or off the end of it + transition(tonext).attr('d', fullpath + 'L' + prevRevpath.substr(1) + 'Z') + .call(Drawing.singleFillStyle); + } + trace._polygons = trace._polygons.concat(prevPolygons); } else { - // tonextx/y: for now just connect endpoints with lines. This is - // the correct behavior if the endpoints are at the same value of - // y/x, but if they *aren't*, we should ideally do more complicated - // things depending on whether the new endpoint projects onto the - // existing curve or off the end of it - transition(tonext).attr('d', fullpath + 'L' + prevRevpath.substr(1) + 'Z') - .call(Drawing.singleFillStyle); + clearFill(tonext); + trace._polygons = null; } - trace._polygons = trace._polygons.concat(prevPolygons); } trace._prevRevpath = revpath; trace._prevPolygons = thisPolygons; } + else { + if(ownFillEl3) clearFill(ownFillEl3); + else if(tonext) clearFill(tonext); + trace._polygons = trace._prevRevpath = trace._prevPolygons = null; + } function visFilter(d) { diff --git a/test/jasmine/tests/scatter_test.js b/test/jasmine/tests/scatter_test.js index fed15714c31..063b65ea6a4 100644 --- a/test/jasmine/tests/scatter_test.js +++ b/test/jasmine/tests/scatter_test.js @@ -713,6 +713,50 @@ describe('end-to-end scatter tests', function() { expect(fill()).toEqual('rgb(0, 0, 255)'); }).catch(fail).then(done); }); + + it('clears fills tonext when either trace is emptied out', function(done) { + var trace0 = {y: [1, 3, 5, 7]}; + var trace1 = {y: [1, 2, 3, 4], line: {width: 0}, mode: 'lines'}; + var trace2 = {y: [3, 4, 5, 6], fill: 'tonexty', mode: 'none'}; + + function checkFill(visible, msg) { + var fillSelection = d3.select(gd).selectAll('.scatterlayer .js-fill'); + expect(fillSelection.size()).toBe(1, msg); + expect(fillSelection.attr('d')).negateIf(visible).toBe('M0,0Z', msg); + } + + Plotly.newPlot(gd, [trace0, trace1, trace2], {}, {scrollZoom: true}) + .then(function() { + checkFill(true, 'initial'); + + return Plotly.restyle(gd, {y: [[null, null, null, null]]}, [1]); + }) + .then(function() { + checkFill(false, 'null out trace 1'); + + return Plotly.restyle(gd, {y: [[1, 2, 3, 4]]}, [1]); + }) + .then(function() { + checkFill(true, 'restore trace 1'); + + return Plotly.restyle(gd, {y: [[null, null, null, null]]}, [2]); + }) + .then(function() { + checkFill(false, 'null out trace 2'); + + return Plotly.restyle(gd, {y: [[1, 2, 3, 4]]}, [2]); + }) + .then(function() { + checkFill(true, 'restore trace 2'); + + return Plotly.restyle(gd, {y: [[null, null, null, null], [null, null, null, null]]}, [1, 2]); + }) + .then(function() { + checkFill(false, 'null out both traces'); + }) + .catch(fail) + .then(done); + }); }); describe('scatter hoverPoints', function() {