Merge pull request #688 from plotly/fix-ternary-hover-after-zoom

fix ternary hover after zoom/pan events

Author: alexcjohnson

src/plots/ternary/ternary.js

@@ -659,9 +659,9 @@ proto.initInteractions = function() {
         Plotly.relayout(gd, attrs);
     }
 
-    dragElement.init(dragOptions);
-
     // finally, set up hover and click
+    // these event handlers must already be set before dragElement.init
+    // so it can stash them and override them.
     dragger.onmousemove = function(evt) {
         fx.hover(gd, evt, _this.id);
         gd._fullLayout._lasthover = dragger;
@@ -677,6 +677,8 @@ proto.initInteractions = function() {
     dragger.onclick = function(evt) {
         fx.click(gd, evt);
     };
+
+    dragElement.init(dragOptions);
 };
 
 function removeZoombox(gd) {

src/traces/scatter/hover.js

@@ -107,6 +107,11 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
         }
 
         if(inside) {
+            // constrain ymin/max to the visible plot, so the label goes
+            // at the middle of the piece you can see
+            ymin = Math.max(ymin, 0);
+            ymax = Math.min(ymax, ya._length);
+
             // find the overall left-most and right-most points of the
             // polygon(s) we're inside at their combined vertical midpoint.
             // This is where we will draw the hover label.
@@ -128,6 +133,10 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
                 }
             }
 
+            // constrain xmin/max to the visible plot now too
+            xmin = Math.max(xmin, 0);
+            xmax = Math.min(xmax, xa._length);
+
             // get only fill or line color for the hover color
             var color = Color.defaultLine;
             if(Color.opacity(trace.fillcolor)) color = trace.fillcolor;

src/traces/scatterternary/hover.js

@@ -17,12 +17,29 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
     var scatterPointData = scatterHover(pointData, xval, yval, hovermode);
     if(!scatterPointData || scatterPointData[0].index === false) return;
 
+    var newPointData = scatterPointData[0];
+
     // if hovering on a fill, we don't show any point data so the label is
-    // unchanged from what scatter gives us.
-    if(scatterPointData[0].index === undefined) return scatterPointData;
+    // unchanged from what scatter gives us - except that it needs to
+    // be constrained to the trianglular plot area, not just the rectangular
+    // area defined by the synthetic x and y axes
+    // TODO: in some cases the vertical middle of the shape is not within
+    // the triangular viewport at all, so the label can become disconnected
+    // from the shape entirely. But calculating what portion of the shape
+    // is actually visible, as constrained by the diagonal axis lines, is not
+    // so easy and anyway we lost the information we would have needed to do
+    // this inside scatterHover.
+    if(newPointData.index === undefined) {
+        var yFracUp = 1 - (newPointData.y0 / pointData.ya._length),
+            xLen = pointData.xa._length,
+            xMin = xLen * yFracUp / 2,
+            xMax = xLen - xMin;
+        newPointData.x0 = Math.max(Math.min(newPointData.x0, xMax), xMin);
+        newPointData.x1 = Math.max(Math.min(newPointData.x1, xMax), xMin);
+        return scatterPointData;
+    }
 
-    var newPointData = scatterPointData[0],
-        cdi = newPointData.cd[newPointData.index];
+    var cdi = newPointData.cd[newPointData.index];
 
     newPointData.a = cdi.a;
     newPointData.b = cdi.b;

test/jasmine/assets/click.js

@@ -0,0 +1,7 @@
+var mouseEvent = require('./mouse_event');
+
+module.exports = function click(x, y) {
+    mouseEvent('mousemove', x, y);
+    mouseEvent('mousedown', x, y);
+    mouseEvent('mouseup', x, y);
+};

test/jasmine/assets/double_click.js

@@ -0,0 +1,13 @@
+var click = require('./click');
+var DBLCLICKDELAY = require('@src/plots/cartesian/constants').DBLCLICKDELAY;
+
+module.exports = function doubleClick(x, y) {
+    return new Promise(function(resolve) {
+        click(x, y);
+
+        setTimeout(function() {
+            click(x, y);
+            setTimeout(function() { resolve(); }, DBLCLICKDELAY / 2);
+        }, DBLCLICKDELAY / 2);
+    });
+};

test/jasmine/tests/click_test.js

@@ -8,6 +8,12 @@ var mouseEvent = require('../assets/mouse_event');
 var getRectCenter = require('../assets/get_rect_center');
 var customMatchers = require('../assets/custom_matchers');
 
+// cartesian click events events use the hover data
+// from the mousemove events and then simulate
+// a click event on mouseup
+var click = require('../assets/click');
+var doubleClick = require('../assets/double_click');
+
 
 describe('Test click interactions:', function() {
     var mock = require('@mocks/14.json');
@@ -31,26 +37,6 @@ describe('Test click interactions:', function() {
 
     afterEach(destroyGraphDiv);
 
-    // cartesian click events events use the hover data
-    // from the mousemove events and then simulate
-    // a click event on mouseup
-    function click(x, y) {
-        mouseEvent('mousemove', x, y);
-        mouseEvent('mousedown', x, y);
-        mouseEvent('mouseup', x, y);
-    }
-
-    function doubleClick(x, y) {
-        return new Promise(function(resolve) {
-            click(x, y);
-
-            setTimeout(function() {
-                click(x, y);
-                setTimeout(function() { resolve(); }, DBLCLICKDELAY / 2);
-            }, DBLCLICKDELAY / 2);
-        });
-    }
-
     function drag(fromX, fromY, toX, toY, delay) {
         return new Promise(function(resolve) {
             mouseEvent('mousemove', fromX, fromY);

test/jasmine/tests/hover_label_test.js

@@ -8,6 +8,7 @@ var Lib = require('@src/lib');
 var createGraphDiv = require('../assets/create_graph_div');
 var destroyGraphDiv = require('../assets/destroy_graph_div');
 var mouseEvent = require('../assets/mouse_event');
+var doubleClick = require('../assets/double_click');
 
 describe('hover info', function() {
     'use strict';
@@ -693,8 +694,9 @@ describe('hover on fill', function() {
 
     it('should work for scatterternary too', function(done) {
         var mock = Lib.extendDeep({}, require('@mocks/ternary_fill.json'));
+        var gd = createGraphDiv();
 
-        Plotly.plot(createGraphDiv(), mock.data, mock.layout).then(function() {
+        Plotly.plot(gd, mock.data, mock.layout).then(function() {
             // hover over a point when that's closest, even if you're over
             // a fill, because by default we have hoveron='points+fills'
             return assertLabelsCorrect([237, 150], [240.0, 144],
@@ -706,6 +708,22 @@ describe('hover on fill', function() {
             return assertLabelsCorrect([237, 218], [266.75, 265], 'trace 1');
         }).then(function() {
             return assertLabelsCorrect([237, 251], [247.7, 254], 'trace 0');
+        }).then(function() {
+            // zoom in to test clipping of large out-of-viewport shapes
+            return Plotly.relayout(gd, {
+                'ternary.aaxis.min': 0.5,
+                'ternary.baxis.min': 0.25
+            });
+        }).then(function() {
+            // this particular one has a hover label disconnected from the shape itself
+            // so if we ever fix this, the test will have to be fixed too.
+            return assertLabelsCorrect([295, 218], [275.1, 166], 'trace 2');
+        }).then(function() {
+            // trigger an autoscale redraw, which goes through dragElement
+            return doubleClick(237, 251);
+        }).then(function() {
+            // then make sure we can still select a *different* item afterward
+            return assertLabelsCorrect([237, 218], [266.75, 265], 'trace 1');
         }).then(done);
     });
 });

test/jasmine/tests/select_test.js

@@ -2,7 +2,7 @@ var d3 = require('d3');
 
 var Plotly = require('@lib/index');
 var Lib = require('@src/lib');
-var DBLCLICKDELAY = require('@src/plots/cartesian/constants').DBLCLICKDELAY;
+var doubleClick = require('../assets/double_click');
 
 var createGraphDiv = require('../assets/create_graph_div');
 var destroyGraphDiv = require('../assets/destroy_graph_div');
@@ -35,23 +35,6 @@ describe('select box and lasso', function() {
         mouseEvent('mouseup', path[len - 1][0], path[len - 1][1]);
     }
 
-    // cartesian click events events use the hover data
-    // from the mousemove events and then simulate
-    // a click event on mouseup
-    function click(x, y) {
-        mouseEvent('mousemove', x, y);
-        mouseEvent('mousedown', x, y);
-        mouseEvent('mouseup', x, y);
-    }
-
-    function doubleClick(x, y, cb) {
-        click(x, y);
-        setTimeout(function() {
-            click(x, y);
-            cb();
-        }, DBLCLICKDELAY / 2);
-    }
-
     function assertRange(actual, expected) {
         var PRECISION = 4;
 
@@ -104,7 +87,7 @@ describe('select box and lasso', function() {
 
             drag([[x0, y0], [x1, y1]]);
 
-            doubleClick(x2, y2, done);
+            doubleClick(x2, y2).then(done);
         });
     });
 
@@ -153,7 +136,7 @@ describe('select box and lasso', function() {
 
             drag([[x0, y0], [x1, y1]]);
 
-            doubleClick(x2, y2, done);
+            doubleClick(x2, y2).then(done);
         });
     });
 
@@ -225,7 +208,7 @@ describe('select box and lasso', function() {
                 y: [0.10209191961595454, 24.512223978291406]
             }, 'with the correct selected range');
 
-            doubleClick(250, 200, function() {
+            doubleClick(250, 200).then(function() {
                 expect(doubleClickData).toBe(null, 'with the correct deselect data');
                 done();
             });
@@ -283,7 +266,7 @@ describe('select box and lasso', function() {
                 y: 2.75
             }], 'with the correct selected points');
 
-            doubleClick(250, 200, function() {
+            doubleClick(250, 200).then(function() {
                 expect(doubleClickData).toBe(null, 'with the correct deselect data');
                 done();
             });

test/jasmine/tests/ternary_test.js

@@ -1,13 +1,14 @@
 var Plotly = require('@lib');
 var Lib = require('@src/lib');
-var DBLCLICKDELAY = require('@src/plots/cartesian/constants').DBLCLICKDELAY;
 
 var supplyLayoutDefaults = require('@src/plots/ternary/layout/defaults');
 
 var d3 = require('d3');
 var createGraphDiv = require('../assets/create_graph_div');
 var destroyGraphDiv = require('../assets/destroy_graph_div');
 var mouseEvent = require('../assets/mouse_event');
+var click = require('../assets/click');
+var doubleClick = require('../assets/double_click');
 var customMatchers = require('../assets/custom_matchers');
 
 
@@ -238,23 +239,6 @@ describe('ternary plots', function() {
         return d3.select('.hoverlayer').selectAll('g');
     }
 
-    function click(x, y) {
-        mouseEvent('mousemove', x, y);
-        mouseEvent('mousedown', x, y);
-        mouseEvent('mouseup', x, y);
-    }
-
-    function doubleClick(x, y) {
-        return new Promise(function(resolve) {
-            click(x, y);
-
-            setTimeout(function() {
-                click(x, y);
-                resolve();
-            }, DBLCLICKDELAY / 2);
-        });
-    }
-
     function drag(path) {
         var len = path.length;