fix layer ordering of scatter subtypes

alexcjohnson · alexcjohnson · commit aeef7f724ea7 · 2018-09-06T18:49:03.000-04:00
diff --git a/src/components/legend/style.js b/src/components/legend/style.js
@@ -208,7 +208,9 @@ module.exports = function style(s, gd) {
 
         var pts = ptgroup.selectAll('path.scatterpts')
             .data(showMarkers ? dMod : []);
-        pts.enter().append('path').classed('scatterpts', true)
+        // make sure marker is on the bottom, in case it enters after text
+        pts.enter().insert('path', ':first-child')
+            .classed('scatterpts', true)
             .attr('transform', 'translate(20,0)');
         pts.exit().remove();
         pts.call(Drawing.pointStyle, tMod, gd);
diff --git a/src/traces/scatter/plot.js b/src/traces/scatter/plot.js
@@ -13,6 +13,8 @@ var d3 = require('d3');
 
 var Registry = require('../../registry');
 var Lib = require('../../lib');
+var ensureSingle = Lib.ensureSingle;
+var identity = Lib.identity;
 var Drawing = require('../../components/drawing');
 
 var subTypes = require('./subtypes');
@@ -43,7 +45,7 @@ module.exports = function plot(gd, plotinfo, cdscatter, scatterLayer, transition
     // the z-order of fill layers is correct.
     linkTraces(gd, plotinfo, cdscatter);
 
-    createFills(gd, scatterLayer, plotinfo);
+    createFills(gd, join, plotinfo);
 
     // Sort the traces, once created, so that the ordering is preserved even when traces
     // are shown and hidden. This is needed since we're not just wiping everything out
@@ -52,10 +54,10 @@ module.exports = function plot(gd, plotinfo, cdscatter, scatterLayer, transition
         uids[cdscatter[i][0].trace.uid] = i;
     }
 
-    scatterLayer.selectAll('g.trace').sort(function(a, b) {
+    join.sort(function(a, b) {
         var idx1 = uids[a[0].trace.uid];
         var idx2 = uids[b[0].trace.uid];
-        return idx1 > idx2 ? 1 : -1;
+        return idx1 - idx2;
     });
 
     if(hasTransition) {
@@ -97,51 +99,45 @@ module.exports = function plot(gd, plotinfo, cdscatter, scatterLayer, transition
     scatterLayer.selectAll('path:not([d])').remove();
 };
 
-function createFills(gd, scatterLayer, plotinfo) {
-    var trace;
+function createFills(gd, traceJoin, plotinfo) {
+    traceJoin.each(function(d) {
+        var fills = ensureSingle(d3.select(this), 'g', 'fills');
+        Drawing.setClipUrl(fills, plotinfo.layerClipId);
 
-    scatterLayer.selectAll('g.trace').each(function(d) {
-        var tr = d3.select(this);
-
-        // Loop only over the traces being redrawn:
-        trace = d[0].trace;
+        var trace = d[0].trace;
 
-        // make the fill-to-next path now for the NEXT trace, so it shows
-        // behind both lines.
+        var fillData = [];
+        if(trace.fill && (trace.fill.substr(0, 6) === 'tozero' || trace.fill === 'toself' ||
+                (trace.fill.substr(0, 2) === 'to' && !trace._prevtrace))
+        ) {
+            fillData = ['_ownFill'];
+        }
         if(trace._nexttrace) {
-            trace._nextFill = tr.select('.js-fill.js-tonext');
-            if(!trace._nextFill.size()) {
+            // make the fill-to-next path now for the NEXT trace, so it shows
+            // behind both lines.
+            fillData.push('_nextFill');
+        }
 
-                // If there is an existing tozero fill, we must insert this *after* that fill:
-                var loc = ':first-child';
-                if(tr.select('.js-fill.js-tozero').size()) {
-                    loc += ' + *';
-                }
+        var fillJoin = fills.selectAll('g')
+            .data(fillData, identity);
 
-                trace._nextFill = tr.insert('path', loc).attr('class', 'js-fill js-tonext');
-            }
-        } else {
-            tr.selectAll('.js-fill.js-tonext').remove();
-            trace._nextFill = null;
-        }
+        fillJoin.enter().append('g');
 
-        if(trace.fill && (trace.fill.substr(0, 6) === 'tozero' || trace.fill === 'toself' ||
-                (trace.fill.substr(0, 2) === 'to' && !trace._prevtrace))) {
-            trace._ownFill = tr.select('.js-fill.js-tozero');
-            if(!trace._ownFill.size()) {
-                trace._ownFill = tr.insert('path', ':first-child').attr('class', 'js-fill js-tozero');
-            }
-        } else {
-            tr.selectAll('.js-fill.js-tozero').remove();
-            trace._ownFill = null;
-        }
+        fillJoin.exit()
+            .each(function(d) { trace[d] = null; })
+            .remove();
 
-        tr.selectAll('.js-fill').call(Drawing.setClipUrl, plotinfo.layerClipId);
+        fillJoin.order().each(function(d) {
+            // make a path element inside the fill group, just so
+            // we can give it its own data later on and the group can
+            // keep its simple '_*Fill' data
+            trace[d] = ensureSingle(d3.select(this), 'path', 'js-fill');
+        });
     });
 }
 
 function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transitionOpts) {
-    var join, i;
+    var i;
 
     // Since this has been reorganized and we're executing this on individual traces,
     // we need to pass it the full list of cdscatter as well as this trace's index (idx)
@@ -157,12 +153,17 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
     var xa = plotinfo.xaxis,
         ya = plotinfo.yaxis;
 
-    var trace = cdscatter[0].trace,
-        line = trace.line,
-        tr = d3.select(element);
+    var trace = cdscatter[0].trace;
+    var line = trace.line;
+    var tr = d3.select(element);
+
+    var errorBarGroup = ensureSingle(tr, 'g', 'errorbars');
+    var lines = ensureSingle(tr, 'g', 'lines');
+    var points = ensureSingle(tr, 'g', 'points');
+    var text = ensureSingle(tr, 'g', 'text');
 
     // error bars are at the bottom
-    Registry.getComponentMethod('errorbars', 'plot')(tr, plotinfo, transitionOpts);
+    Registry.getComponentMethod('errorbars', 'plot')(errorBarGroup, plotinfo, transitionOpts);
 
     if(trace.visible !== true) return;
 
@@ -303,7 +304,7 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
         };
     }
 
-    var lineJoin = tr.selectAll('.js-line').data(segments);
+    var lineJoin = lines.selectAll('.js-line').data(segments);
 
     transition(lineJoin.exit())
         .style('opacity', 0)
@@ -325,6 +326,7 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
 
     if(segments.length) {
         if(ownFillEl3) {
+            ownFillEl3.datum(cdscatter);
             if(pt0 && pt1) {
                 if(ownFillDir) {
                     if(ownFillDir === 'y') {
@@ -404,11 +406,10 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
         return false;
     }
 
-    function makePoints(d) {
+    function makePoints(points, text, cdscatter) {
         var join, selection, hasNode;
 
-        var trace = d[0].trace;
-        var s = d3.select(this);
+        var trace = cdscatter[0].trace;
         var showMarkers = subTypes.hasMarkers(trace);
         var showText = subTypes.hasText(trace);
 
@@ -417,16 +418,16 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
         var textFilter = hideFilter;
 
         if(showMarkers) {
-            markerFilter = (trace.marker.maxdisplayed || trace._needsCull) ? visFilter : Lib.identity;
+            markerFilter = (trace.marker.maxdisplayed || trace._needsCull) ? visFilter : identity;
         }
 
         if(showText) {
-            textFilter = (trace.marker.maxdisplayed || trace._needsCull) ? visFilter : Lib.identity;
+            textFilter = (trace.marker.maxdisplayed || trace._needsCull) ? visFilter : identity;
         }
 
         // marker points
 
-        selection = s.selectAll('path.point');
+        selection = points.selectAll('path.point');
 
         join = selection.data(markerFilter, keyFunc);
 
@@ -478,7 +479,7 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
         }
 
         // text points
-        selection = s.selectAll('g');
+        selection = text.selectAll('g');
         join = selection.data(textFilter, keyFunc);
 
         // each text needs to go in its own 'g' in case
@@ -517,28 +518,16 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
         join.exit().remove();
     }
 
-    // NB: selectAll is evaluated on instantiation:
-    var pointSelection = tr.selectAll('.points');
-
-    // Join with new data
-    join = pointSelection.data([cdscatter]);
-
-    // Transition existing, but don't defer this to an async .transition since
-    // there's no timing involved:
-    pointSelection.each(makePoints);
-
-    join.enter().append('g')
-        .classed('points', true)
-        .each(makePoints);
-
-    join.exit().remove();
+    points.datum(cdscatter);
+    text.datum(cdscatter);
+    makePoints(points, text, cdscatter);
 
     // lastly, clip points groups of `cliponaxis !== false` traces
     // on `plotinfo._hasClipOnAxisFalse === true` subplots
-    join.each(function(d) {
-        var hasClipOnAxisFalse = d[0].trace.cliponaxis === false;
-        Drawing.setClipUrl(d3.select(this), hasClipOnAxisFalse ? null : plotinfo.layerClipId);
-    });
+    var hasClipOnAxisFalse = trace.cliponaxis === false;
+    var clipUrl = hasClipOnAxisFalse ? null : plotinfo.layerClipId;
+    Drawing.setClipUrl(points, clipUrl);
+    Drawing.setClipUrl(text, clipUrl);
 }
 
 function selectMarkers(gd, idx, plotinfo, cdscatter, cdscatterAll) {
diff --git a/src/traces/scatter/style.js b/src/traces/scatter/style.js
@@ -26,6 +26,12 @@ function style(gd, cd) {
         stylePoints(sel, trace, gd);
     });
 
+    s.selectAll('g.text').each(function(d) {
+        var sel = d3.select(this);
+        var trace = d.trace || d[0].trace;
+        styleText(sel, trace, gd);
+    });
+
     s.selectAll('g.trace path.js-line')
         .call(Drawing.lineGroupStyle);
 
@@ -37,6 +43,9 @@ function style(gd, cd) {
 
 function stylePoints(sel, trace, gd) {
     Drawing.pointStyle(sel.selectAll('path.point'), trace, gd);
+}
+
+function styleText(sel, trace, gd) {
     Drawing.textPointStyle(sel.selectAll('text'), trace, gd);
 }
 
@@ -49,6 +58,7 @@ function styleOnSelect(gd, cd) {
         Drawing.selectedTextStyle(s.selectAll('text'), trace);
     } else {
         stylePoints(s, trace, gd);
+        styleText(s, trace, gd);
     }
 }
 
diff --git a/test/jasmine/assets/custom_assertions.js b/test/jasmine/assets/custom_assertions.js
@@ -298,6 +298,22 @@ exports.assertNodeOrder = function(selectorBehind, selectorInFront, msg) {
     }
 };
 
+/**
+ * Ordering test for any number of nodes - calls assertNodeOrder n-1 times.
+ * Note that we only take the first matching node for each selector, and it's
+ * not necessary that the nodes be siblings or at the same level of nesting.
+ *
+ * @param {Array[string]} selectorArray: css selectors in the order they should
+ *     appear in the document, from back to front.
+ * @param {string} msg: context for debugging
+ */
+exports.assertMultiNodeOrder = function(selectorArray, msg) {
+    for(var i = 0; i < selectorArray.length - 1; i++) {
+        var msgi = (msg ? msg + ' - ' : '') + 'entries ' + i + ' and ' + (i + 1);
+        exports.assertNodeOrder(selectorArray[i], selectorArray[i + 1], msgi);
+    }
+};
+
 function getParents(node) {
     var parent = node.parentNode;
     if(parent) return getParents(parent).concat(node);
@@ -310,3 +326,9 @@ function collectionToArray(collection) {
     for(var i = 0; i < len; i++) a[i] = collection[i];
     return a;
 }
+
+exports.assertD3Data = function(selection, expectedData) {
+    var data = [];
+    selection.each(function(d) { data.push(d); });
+    expect(data).toEqual(expectedData);
+};
diff --git a/test/jasmine/tests/cartesian_test.js b/test/jasmine/tests/cartesian_test.js
@@ -7,6 +7,7 @@ var Drawing = require('@src/components/drawing');
 var createGraphDiv = require('../assets/create_graph_div');
 var destroyGraphDiv = require('../assets/destroy_graph_div');
 var failTest = require('../assets/fail_test');
+var assertD3Data = require('../assets/custom_assertions').assertD3Data;
 
 describe('restyle', function() {
     describe('scatter traces', function() {
@@ -21,37 +22,35 @@ describe('restyle', function() {
         it('reuses SVG fills', function(done) {
             var fills, firstToZero, secondToZero, firstToNext, secondToNext;
             var mock = Lib.extendDeep({}, require('@mocks/basic_area.json'));
+            function getFills() {
+                return d3.selectAll('g.trace.scatter .fills>g');
+            }
 
             Plotly.plot(gd, mock.data, mock.layout).then(function() {
-                // Assert there are two fills:
-                fills = d3.selectAll('g.trace.scatter .js-fill')[0];
+                fills = getFills();
 
-                // First is tozero, second is tonext:
-                expect(d3.selectAll('g.trace.scatter .js-fill').size()).toEqual(2);
-                expect(fills[0]).toBeClassed(['js-fill', 'js-tozero']);
-                expect(fills[1]).toBeClassed(['js-fill', 'js-tonext']);
+                // Assert there are two fills, first is tozero, second is tonext
+                assertD3Data(fills, ['_ownFill', '_nextFill']);
+
+                firstToZero = fills[0][0];
+                firstToNext = fills[0][1];
 
-                firstToZero = fills[0];
-                firstToNext = fills[1];
-            }).then(function() {
                 return Plotly.restyle(gd, {visible: [false]}, [1]);
             }).then(function() {
+                fills = getFills();
                 // Trace 1 hidden leaves only trace zero's tozero fill:
-                expect(d3.selectAll('g.trace.scatter .js-fill').size()).toEqual(1);
-                expect(fills[0]).toBeClassed(['js-fill', 'js-tozero']);
-            }).then(function() {
+                assertD3Data(fills, ['_ownFill']);
+
                 return Plotly.restyle(gd, {visible: [true]}, [1]);
             }).then(function() {
-                // Reshow means two fills again AND order is preserved:
-                fills = d3.selectAll('g.trace.scatter .js-fill')[0];
+                fills = getFills();
 
+                // Reshow means two fills again AND order is preserved
                 // First is tozero, second is tonext:
-                expect(d3.selectAll('g.trace.scatter .js-fill').size()).toEqual(2);
-                expect(fills[0]).toBeClassed(['js-fill', 'js-tozero']);
-                expect(fills[1]).toBeClassed(['js-fill', 'js-tonext']);
+                assertD3Data(fills, ['_ownFill', '_nextFill']);
 
-                secondToZero = fills[0];
-                secondToNext = fills[1];
+                secondToZero = fills[0][0];
+                secondToNext = fills[0][1];
 
                 // The identity of the first is retained:
                 expect(firstToZero).toBe(secondToZero);
@@ -61,8 +60,7 @@ describe('restyle', function() {
 
                 return Plotly.restyle(gd, 'visible', false);
             }).then(function() {
-                expect(d3.selectAll('g.trace.scatter').size()).toEqual(0);
-
+                expect(d3.selectAll('g.trace.scatter').size()).toBe(0);
             })
             .catch(failTest)
             .then(done);
diff --git a/test/jasmine/tests/scatter_test.js b/test/jasmine/tests/scatter_test.js
diff --git a/test/jasmine/tests/select_test.js b/test/jasmine/tests/select_test.js
