Merge pull request #2978 from plotly/scatter-layer-order

fix layer ordering of scatter subtypes

Author: alexcjohnson

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

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') {
@@ -412,11 +414,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);
 
@@ -425,7 +426,7 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
         var textFilter = hideFilter;
 
         if(showMarkers || showText) {
-            var showFilter = Lib.identity;
+            var showFilter = identity;
             // if we're stacking, "infer zero" gap mode gets markers in the
             // gap points - because we've inferred a zero there - but other
             // modes (currently "interpolate", later "interrupt" hopefully)
@@ -446,7 +447,7 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
 
         // marker points
 
-        selection = s.selectAll('path.point');
+        selection = points.selectAll('path.point');
 
         join = selection.data(markerFilter, keyFunc);
 
@@ -498,7 +499,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
@@ -537,28 +538,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) {

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,11 +58,13 @@ function styleOnSelect(gd, cd) {
         Drawing.selectedTextStyle(s.selectAll('text'), trace);
     } else {
         stylePoints(s, trace, gd);
+        styleText(s, trace, gd);
     }
 }
 
 module.exports = {
     style: style,
     stylePoints: stylePoints,
+    styleText: styleText,
     styleOnSelect: styleOnSelect
 };

src/traces/scattergeo/style.js

@@ -12,7 +12,9 @@ var d3 = require('d3');
 var Drawing = require('../../components/drawing');
 var Color = require('../../components/color');
 
-var stylePoints = require('../scatter/style').stylePoints;
+var scatterStyle = require('../scatter/style');
+var stylePoints = scatterStyle.stylePoints;
+var styleText = scatterStyle.styleText;
 
 module.exports = function style(gd, calcTrace) {
     if(calcTrace) styleTrace(gd, calcTrace);
@@ -25,6 +27,7 @@ function styleTrace(gd, calcTrace) {
     s.style('opacity', calcTrace[0].trace.opacity);
 
     stylePoints(s, trace, gd);
+    styleText(s, trace, gd);
 
     // this part is incompatible with Drawing.lineGroupStyle
     s.selectAll('path.js-line')

test/jasmine/assets/custom_assertions.js

@@ -312,6 +312,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);
@@ -324,3 +340,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);
+};

test/jasmine/assets/transitions.js

@@ -0,0 +1,35 @@
+'use strict';
+
+/**
+ * Given n states (denoted by their indices 0..n-1) this routine produces
+ * a sequence of indices such that you efficiently execute each transition
+ * from any state to any other state.
+ */
+module.exports = function transitions(n) {
+    var out = [0];
+    var nextStates = [];
+    var i;
+    for(i = 0; i < n; i++) nextStates[i] = (i + 1) % n;
+    var finishedStates = 0;
+    var thisState = 0;
+    var nextState;
+    while(finishedStates < n) {
+        nextState = nextStates[thisState];
+        if(nextState === thisState) {
+            // I don't actually know how to prove that this algorithm works,
+            // but I've never seen it fail for n>1
+            // For prime n it's the same sequence as the one I started with
+            // (n transitions of +1 index, then n transitions +2 etc...)
+            // but this one works for non-prime n as well.
+            throw new Error('your transitions algo failed.');
+        }
+        nextStates[thisState] = (nextStates[thisState] + 1) % n;
+        if(nextStates[thisState] === thisState) finishedStates++;
+        out.push(nextState);
+        thisState = nextState;
+    }
+    if(out.length !== n * (n - 1) + 1) {
+        throw new Error('your transitions algo failed.');
+    }
+    return out;
+};