From cbcec26705c7d0761538816d2c99c3f7887f6c72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 19 May 2017 17:55:25 -0400 Subject: [PATCH 1/5] add failing test cases showcasing issue #1706 --- test/jasmine/tests/scatter_test.js | 102 +++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/test/jasmine/tests/scatter_test.js b/test/jasmine/tests/scatter_test.js index 5c9aa1161eb..4db595f6fcf 100644 --- a/test/jasmine/tests/scatter_test.js +++ b/test/jasmine/tests/scatter_test.js @@ -437,6 +437,108 @@ describe('end-to-end scatter tests', function() { .catch(fail) .then(done); }); + + function _assertNodes(ptStyle, txContent) { + var pts = d3.selectAll('.point'); + var txs = d3.selectAll('.textpoint'); + + expect(pts.size()).toEqual(ptStyle.length); + expect(txs.size()).toEqual(txContent.length); + + pts.each(function(_, i) { + expect(d3.select(this).style('fill')).toEqual(ptStyle[i], 'pt ' + i); + }); + + txs.each(function(_, i) { + expect(d3.select(this).select('text').text()).toEqual(txContent[i], 'tx ' + i); + }); + } + + it('should reorder point and text nodes even when linked to ids (shuffle case)', function(done) { + Plotly.plot(gd, [{ + x: [150, 350, 650], + y: [100, 300, 600], + text: ['apple', 'banana', 'clementine'], + ids: ['A', 'B', 'C'], + mode: 'markers+text', + marker: { + color: ['rgb(255, 0, 0)', 'rgb(0, 255, 0)', 'rgb(0, 0, 255)'] + }, + transforms: [{ + type: 'sort', + enabled: false, + target: [0, 1, 0] + }] + }]) + .then(function() { + _assertNodes( + ['rgb(255, 0, 0)', 'rgb(0, 255, 0)', 'rgb(0, 0, 255)'], + ['apple', 'banana', 'clementine'] + ); + + return Plotly.restyle(gd, 'transforms[0].enabled', true); + }) + .then(function() { + _assertNodes( + ['rgb(255, 0, 0)', 'rgb(0, 0, 255)', 'rgb(0, 255, 0)'], + ['apple', 'clementine', 'banana'] + ); + + return Plotly.restyle(gd, 'transforms[0].enabled', false); + }) + .then(function() { + _assertNodes( + ['rgb(255, 0, 0)', 'rgb(0, 255, 0)', 'rgb(0, 0, 255)'], + ['apple', 'banana', 'clementine'] + ); + }) + .catch(fail) + .then(done); + }); + + it('should reorder point and text nodes even when linked to ids (add/remove case)', function(done) { + Plotly.plot(gd, [{ + x: [150, 350, null, 600], + y: [100, 300, null, 700], + text: ['apple', 'banana', null, 'clementine'], + ids: ['A', 'B', null, 'C'], + mode: 'markers+text', + marker: { + color: ['rgb(255, 0, 0)', 'rgb(0, 255, 0)', null, 'rgb(0, 0, 255)'] + }, + transforms: [{ + type: 'filter', + enabled: false, + target: [1, 0, 0, 1], + operation: '=', + value: 1 + }] + }]) + .then(function() { + _assertNodes( + ['rgb(255, 0, 0)', 'rgb(0, 255, 0)', 'rgb(0, 0, 255)'], + ['apple', 'banana', 'clementine'] + ); + + return Plotly.restyle(gd, 'transforms[0].enabled', true); + }) + .then(function() { + _assertNodes( + ['rgb(255, 0, 0)', 'rgb(0, 0, 255)'], + ['apple', 'clementine'] + ); + + return Plotly.restyle(gd, 'transforms[0].enabled', false); + }) + .then(function() { + _assertNodes( + ['rgb(255, 0, 0)', 'rgb(0, 255, 0)', 'rgb(0, 0, 255)'], + ['apple', 'banana', 'clementine'] + ); + }) + .catch(fail) + .then(done); + }); }); describe('scatter hoverPoints', function() { From 83234b55594f0cd7336613d883804eaa408ac38b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 19 May 2017 17:59:52 -0400 Subject: [PATCH 2/5] [breaking] add `join.order()` calls during marker and text node updates - this reorders the object-constant nodes (i.e. for traces w/ ids) correctly :tada: - BUT, this also brakes the update pattern for traces with non-numeric (x,y), as the .remove call in Drawing.translatePoint throws the join selections out-of-sync, leading an exception! --- src/traces/scatter/plot.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/traces/scatter/plot.js b/src/traces/scatter/plot.js index 22ddcfd8a66..f1cfc51eaec 100644 --- a/src/traces/scatter/plot.js +++ b/src/traces/scatter/plot.js @@ -444,6 +444,8 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition } }); + join.order(); + if(hasTransition) { join.exit().transition() .style('opacity', 0) @@ -483,6 +485,8 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition }); }); + join.order(); + join.exit().remove(); } From c8c319789413c8c90be1c6874bffe009d2da56a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 23 May 2017 11:46:15 -0400 Subject: [PATCH 3/5] rm Drawing.pointStyle and Drawing.translatePoints on marker enter sel ... and order nodes on merge selection before styling/translating/removing-non-numeric item so that the order call acts on a merge selection in-sync with the DOM --- src/traces/scatter/plot.js | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/traces/scatter/plot.js b/src/traces/scatter/plot.js index f1cfc51eaec..eafddc3e772 100644 --- a/src/traces/scatter/plot.js +++ b/src/traces/scatter/plot.js @@ -419,9 +419,6 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition var enter = join.enter().append('path') .classed('point', true); - enter.call(Drawing.pointStyle, trace) - .call(Drawing.translatePoints, xa, ya, trace); - if(hasTransition) { enter.style('opacity', 0).transition() .style('opacity', 1); @@ -430,6 +427,8 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition var markerScale = showMarkers && Drawing.tryColorscale(trace.marker, ''); var lineScale = showMarkers && Drawing.tryColorscale(trace.marker, 'line'); + join.order(); + join.each(function(d) { var el = d3.select(this); var sel = transition(el); @@ -444,8 +443,6 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition } }); - join.order(); - if(hasTransition) { join.exit().transition() .style('opacity', 0) @@ -462,6 +459,8 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition // it gets converted to mathjax join.enter().append('g').classed('textpoint', true).append('text'); + join.order(); + join.each(function(d) { var g = d3.select(this); var sel = transition(g.select('text')); @@ -485,8 +484,6 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition }); }); - join.order(); - join.exit().remove(); } From 5fa1e97b98b7bd76753b2565dd5b874f138b6e70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 23 May 2017 14:00:22 -0400 Subject: [PATCH 4/5] add test case for nodes add/remove on redraw:false animations --- test/jasmine/tests/scatter_test.js | 36 ++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/test/jasmine/tests/scatter_test.js b/test/jasmine/tests/scatter_test.js index 4db595f6fcf..fd309f2f2dc 100644 --- a/test/jasmine/tests/scatter_test.js +++ b/test/jasmine/tests/scatter_test.js @@ -539,6 +539,42 @@ describe('end-to-end scatter tests', function() { .catch(fail) .then(done); }); + + it('should smoothly add/remove nodes tags with *ids* during animations', function(done) { + Plotly.plot(gd, { + data: [{ + mode: 'markers+text', + y: [1, 2, 1], + text: ['apple', 'banana', 'clementine'], + ids: ['A', 'B', 'C'], + marker: { color: ['red', 'green', 'blue'] } + }], + frames: [{ + data: [{ + y: [2, 1, 2], + text: ['apple', 'banana', 'dragon fruit'], + ids: ['A', 'C', 'D'], + marker: { color: ['red', 'blue', 'yellow'] } + }] + }] + }) + .then(function() { + _assertNodes( + ['rgb(255, 0, 0)', 'rgb(0, 128, 0)', 'rgb(0, 0, 255)'], + ['apple', 'banana', 'clementine'] + ); + + return Plotly.animate(gd, null, {frame: {redraw: false}}); + }) + .then(function() { + _assertNodes( + ['rgb(255, 0, 0)', 'rgb(0, 0, 255)', 'rgb(255, 255, 0)'], + ['apple', 'banana', 'dragon fruit'] + ); + }) + .catch(fail) + .then(done); + }); }); describe('scatter hoverPoints', function() { From 9c356755c3e85c15ac533ee5e2368f89db8333d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Tue, 23 May 2017 14:02:05 -0400 Subject: [PATCH 5/5] do not remove nodes in Drawing.translatePoints - handle that in scatter/plot.js - call Drawing.pointStyle and Drawing.translatePoints on enter selection to ensure that added point are drawn in with a smooth transition --- src/components/drawing/index.js | 5 ++--- src/traces/scatter/plot.js | 8 +++++++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/components/drawing/index.js b/src/components/drawing/index.js index 57abb7c5aa2..093f4ffe04a 100644 --- a/src/components/drawing/index.js +++ b/src/components/drawing/index.js @@ -47,7 +47,7 @@ drawing.setRect = function(s, x, y, w, h) { s.call(drawing.setPosition, x, y).call(drawing.setSize, w, h); }; -/** Translate / remove node +/** Translate node * * @param {object} d : calcdata point item * @param {sel} sel : d3 selction of node to translate @@ -56,7 +56,7 @@ drawing.setRect = function(s, x, y, w, h) { * * @return {boolean} : * true if selection got translated - * false if selection got removed + * false if selection could not get translated */ drawing.translatePoint = function(d, sel, xa, ya) { // put xp and yp into d if pixel scaling is already done @@ -71,7 +71,6 @@ drawing.translatePoint = function(d, sel, xa, ya) { sel.attr('transform', 'translate(' + x + ',' + y + ')'); } } else { - sel.remove(); return false; } diff --git a/src/traces/scatter/plot.js b/src/traces/scatter/plot.js index eafddc3e772..9175c40f068 100644 --- a/src/traces/scatter/plot.js +++ b/src/traces/scatter/plot.js @@ -420,7 +420,11 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition .classed('point', true); if(hasTransition) { - enter.style('opacity', 0).transition() + enter + .call(Drawing.pointStyle, trace) + .call(Drawing.translatePoints, xa, ya, trace) + .style('opacity', 0) + .transition() .style('opacity', 1); } @@ -440,6 +444,8 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition if(trace.customdata) { el.classed('plotly-customdata', d.data !== null && d.data !== undefined); } + } else { + sel.remove(); } });