Skip to content

fix #2598 - sloppy click on cartesian zoom #2649

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 22, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/dragelement/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
14 changes: 8 additions & 6 deletions src/plots/cartesian/dragbox.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {};

Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one is in zoomPrep


zb = makeZoombox(zoomlayer, lum, xs, ys, path0);

Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this one move in zoomMove


updateZoombox(zb, corners, box, path0, dimmed, lum);
dimmed = true;
}
Expand Down Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, dragTail no longer takes arguments after 5b6a1fa

}, REDRAWDELAY);

e.preventDefault();
Expand Down
8 changes: 8 additions & 0 deletions test/jasmine/assets/click.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 || {});
Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, slop is only used for testing? This is fine, but perhaps we could add a comment about this in the jsDoc above so that we don't feel like 🔪 down the road.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure -> c56f223

if(!callContext) mouseEvent('mouseup', x, y, upOpts);
if(!rightClick) mouseEvent('click', x, y, upOpts);
};
13 changes: 11 additions & 2 deletions test/jasmine/tests/click_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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) {
Expand Down
5 changes: 2 additions & 3 deletions test/jasmine/tests/dragelement_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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() {
Expand Down