Selection re-ordering for scatter traces with `ids` #1709

etpinard · 2017-05-19T22:01:50Z

fixes #1706 (an attempt at least)

- this reorders the object-constant nodes (i.e. for traces w/ ids) correctly 🎉 - 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!

etpinard · 2017-05-19T22:03:20Z

FYI: more info on selection.order() here

etpinard · 2017-05-19T22:13:16Z

src/traces/scatter/plot.js

@@ -483,6 +485,8 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
                });
            });

+        join.order();


🆘 IMPORTANT 🆘

This commit breaks scatter/plot.js for traces with non-numeric (x,y) positions.

That is due to Drawing.translatePoint which is called on the enter and merge selections. When points with non-numeric (x,y) are detected, Drawing.translatePoint removes them. Unfortunately, this makes the join selections out-of-sync with the DOM and leads to 'TypeError: Cannot read property 'textContent' of null exception thrown by d3.

So, I have two questions for the d3-heads looking at this PR:

Is there a way to re-select the join and call order() on that selection?

Or should we make our updates more d3-esque by filtering data items with non-numeric position before binding them to the selections?

~~How about join.filter(onlyNumeric).order()?~~

I'm not sure I understand how makePoints works.

Shouldn't these lines:

enter.call(Drawing.pointStyle, trace) .call(Drawing.translatePoints, xa, ya, trace);

be:

join.call(Drawing.pointStyle, trace) .call(Drawing.translatePoints, xa, ya, trace);

Otherwise DOM and data won't be in sync. Here's a codepen showing a similar issue.

... 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

etpinard · 2017-05-23T15:57:21Z

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)


@n-riesco thanks very much 🎉

Your solution appears to work:

My only concern now is how this interacts with animations. I think the reason why translatePoints and pointStyle are called on enter selection and merge selection has to do with this block below where entering nodes have their opacity transitioned from 0 to 1. I suspect this commit will break this behavior (although the tests are passing) has the entering aren't positioned and styled properly before opacity transition cc @rreusser . It would be nice to write a test case here 😏

I'm thinking instead, we could keep those enter.call(), make Drawing.translatePoints not (and maybe never) remove nodes and make the merge selection .each block below (which comes after the .order()) handle the cases where nodes need to be removed.

Thoughts?

I think the point you make about animations is important.
And the solution you propose (i.e. remove nodes only after we're done with the enter selection) is more robust.
👍 from me.

@etpinard ~~after thinking a little bit about it. I'm still think that translatePoints and pointStyle should be applied to the merge selection to keep in sync DOM and trace.~~

~~I haven't tested it, but .order should take care of my concern.~~

I'm still think that translatePoints and pointStyle should be applied to the merge

That's always been the case here and here.

- 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

etpinard · 2017-05-23T18:03:49Z

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) {


This test is failing after c8c3197

and fixed in commit 9c35675

Before 9c35675

After:

etpinard · 2017-05-23T18:05:43Z

src/components/drawing/index.js

@@ -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) {


To my surprise, Drawing.translatePoint is called only in scatter/plot.js and box/plot.js:

Note that this patch does not affect the box/plot.js logic where points with non-numeric positions are filtered out of the data bind.

etpinard · 2017-05-23T18:18:29Z

Tagging this as reviewable, pinging @alexcjohnson (as this is a pretty important PR) and putting this under the 1.28.0 milestone (as the changes here are too heavy for a patch release IMHO).

Thanks again @n-riesco for your help!

alexcjohnson · 2017-05-23T18:35:20Z

src/traces/scatter/plot.js

@@ -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();


why do we need join.order() twice?

one for marker nodes, one for text nodes

ah right - didn't notice join got reassigned between 👍

alexcjohnson · 2017-05-23T18:53:19Z

Looks great, nice work! 💃

etpinard added 2 commits May 19, 2017 17:55

add failing test cases showcasing issue #1706

cbcec26

etpinard added status: has TODOs type: bug labels May 19, 2017

etpinard commented May 19, 2017

View reviewed changes

rm Drawing.pointStyle and Drawing.translatePoints on marker enter sel

c8c3197

... 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

etpinard commented May 23, 2017

View reviewed changes

etpinard added 2 commits May 23, 2017 14:00

add test case for nodes add/remove on redraw:false animations

5fa1e97

do not remove nodes in Drawing.translatePoints

9c35675

- 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

etpinard commented May 23, 2017

View reviewed changes

etpinard added status: reviewable and removed status: has TODOs labels May 23, 2017

etpinard added this to the 1.28.0 milestone May 23, 2017

alexcjohnson reviewed May 23, 2017

View reviewed changes

alexcjohnson mentioned this pull request May 23, 2017

Data ids interfere with transforms #1706

Closed

etpinard merged commit 13c1237 into master May 23, 2017

etpinard deleted the scatter-id-selection-order branch May 23, 2017 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Selection re-ordering for scatter traces with `ids` #1709

Selection re-ordering for scatter traces with `ids` #1709

etpinard commented May 19, 2017

etpinard commented May 19, 2017

etpinard May 19, 2017

n-riesco May 22, 2017 •

edited

Loading

etpinard May 23, 2017 •

edited

Loading

n-riesco May 23, 2017

n-riesco May 23, 2017 •

edited

Loading

etpinard May 23, 2017

etpinard May 23, 2017

etpinard May 23, 2017

etpinard May 23, 2017 •

edited

Loading

etpinard commented May 23, 2017

alexcjohnson May 23, 2017

etpinard May 23, 2017

alexcjohnson May 23, 2017

alexcjohnson commented May 23, 2017

Selection re-ordering for scatter traces with ids #1709

Selection re-ordering for scatter traces with ids #1709

Conversation

etpinard commented May 19, 2017

etpinard commented May 19, 2017

Choose a reason for hiding this comment

n-riesco May 22, 2017 • edited Loading

Choose a reason for hiding this comment

etpinard May 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-riesco May 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard May 23, 2017 • edited Loading

Choose a reason for hiding this comment

etpinard commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented May 23, 2017

Selection re-ordering for scatter traces with `ids` #1709

Selection re-ordering for scatter traces with `ids` #1709

n-riesco May 22, 2017 •

edited

Loading

etpinard May 23, 2017 •

edited

Loading

n-riesco May 23, 2017 •

edited

Loading

etpinard May 23, 2017 •

edited

Loading