Merge pull request #2649 from plotly/sloppy-click

fix #2598 - sloppy click on cartesian zoom

Author: alexcjohnson

src/components/dragelement/index.js

@@ -217,7 +217,7 @@ dragElement.init = function init(options) {
         }
 
         if(gd._dragged) {
-            if(options.doneFn) options.doneFn(e);
+            if(options.doneFn) options.doneFn();
         }
         else {
             if(options.clickFn) options.clickFn(numClicks, initialEvent);

src/plots/cartesian/dragbox.js

@@ -252,6 +252,9 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
         zb,
         corners;
 
+    // zoom takes over minDrag, so it also has to take over gd._dragged
+    var zoomDragged;
+
     // collected changes to be made to the plot by relayout at the end
     var updates = {};
 
@@ -266,6 +269,7 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
         path0 = 'M0,0H' + pw + 'V' + ph + 'H0V0';
         dimmed = false;
         zoomMode = 'xy';
+        zoomDragged = false;
 
         zb = makeZoombox(zoomlayer, lum, xs, ys, path0);
 
@@ -341,6 +345,9 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
         box.w = box.r - box.l;
         box.h = box.b - box.t;
 
+        if(zoomMode) zoomDragged = true;
+        gd._dragged = zoomDragged;
+
         updateZoombox(zb, corners, box, path0, dimmed, lum);
         dimmed = true;
     }
@@ -458,12 +465,7 @@ function makeDragBox(gd, plotinfo, x, y, w, h, ns, ew) {
         // no more scrolling is coming
         redrawTimer = setTimeout(function() {
             scrollViewBox = [0, 0, pw, ph];
-
-            var zoomMode;
-            if(isSubplotConstrained) zoomMode = 'xy';
-            else zoomMode = (ew ? 'x' : '') + (ns ? 'y' : '');
-
-            dragTail(zoomMode);
+            dragTail();
         }, REDRAWDELAY);
 
         e.preventDefault();

test/jasmine/assets/click.js

@@ -21,6 +21,8 @@ var Lib = require('../../../src/lib');
  *  @param {bool} cancelContext: act as though `preventDefault` was called during a `contextmenu`
  *    handler, which stops native contextmenu and therefore allows mouseup events to be fired.
  *    Only relevant if button=2 or ctrlKey=true.
+ * @param {array(2)} opts.slop - shift [x, y] between mousedown and mouseup.
+ *    Just for testing purposes, to simulate a sloppy click
  */
 module.exports = function click(x, y, optsIn) {
     var opts = Lib.extendFlat({}, optsIn || {});
@@ -43,10 +45,16 @@ module.exports = function click(x, y, optsIn) {
 
     var downOpts = Lib.extendFlat({buttons: buttons}, opts);
     var upOpts = opts;
+    var slop = opts.slop || [];
+    var slopX = slop[0] || 0;
+    var slopY = slop[1] || 0;
+    var slopOpts = Lib.extendFlat({}, downOpts);
+    delete slopOpts.button;
 
     mouseEvent('mousemove', x, y, moveOpts);
     mouseEvent('mousedown', x, y, downOpts);
     if(callContext) mouseEvent('contextmenu', x, y, downOpts);
+    if(slopX || slopY) mouseEvent('mousemove', x + slopX, y + slopY, slopOpts);
     if(!callContext) mouseEvent('mouseup', x, y, upOpts);
     if(!rightClick) mouseEvent('click', x, y, upOpts);
 };

test/jasmine/tests/click_test.js

@@ -117,8 +117,7 @@ describe('Test click interactions:', function() {
             expect(contextPassthroughs).toBe(0);
         });
 
-        it('should contain the correct fields', function() {
-            click(pointPos[0], pointPos[1]);
+        function checkPointData() {
             expect(futureData.points.length).toEqual(1);
             expect(clickPassthroughs).toBe(2);
             expect(contextPassthroughs).toBe(0);
@@ -136,6 +135,16 @@ describe('Test click interactions:', function() {
             var evt = futureData.event;
             expect(evt.clientX).toEqual(pointPos[0]);
             expect(evt.clientY).toEqual(pointPos[1]);
+        }
+
+        it('should contain the correct fields', function() {
+            click(pointPos[0], pointPos[1]);
+            checkPointData();
+        });
+
+        it('should work with a sloppy click (shift < minDrag before mouseup)', function() {
+            click(pointPos[0], pointPos[1], {slop: [4, 4]});
+            checkPointData();
         });
 
         it('works with fixedrange axes', function(done) {

test/jasmine/tests/dragelement_test.js

@@ -81,7 +81,7 @@ describe('dragElement', function() {
         expect(args[1]).toEqual(10);
     });
 
-    it('should pass the event to doneFn on mouseup after mousemove', function() {
+    it('does not pass the event to doneFn on mouseup after mousemove', function() {
         var args = [];
         var options = {
             element: this.element,
@@ -99,8 +99,7 @@ describe('dragElement', function() {
         mouseEvent('mousemove', this.x + 10, this.y + 10);
         mouseEvent('mouseup', this.x, this.y);
 
-        expect(args.length).toBe(1);
-        expect(args[0].type).toBe('mouseup');
+        expect(args.length).toBe(0);
     });
 
     it('should pass numClicks and event to clickFn on mouseup after no/small mousemove', function() {