Skip to content

fix ternary hover after zoom/pan events #688

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 5 commits into from
Jun 27, 2016
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
6 changes: 4 additions & 2 deletions src/plots/ternary/ternary.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -677,6 +677,8 @@ proto.initInteractions = function() {
dragger.onclick = function(evt) {
fx.click(gd, evt);
};

dragElement.init(dragOptions);
};

function removeZoombox(gd) {
Expand Down
9 changes: 9 additions & 0 deletions src/traces/scatter/hover.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So if you have, for example, a shape that goes way off the top of the plot, the label will still be centered on the visible portion rather than pinned to the top (and because of how this happens, the x position of the label could also have ended up screwy previously)

Copy link
Contributor

Choose a reason for hiding this comment

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

Great. That sounds good.

Now that all the scatter ternary hover label testing infrastructure is in place, would you mind adding a test case for this?


// 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.
Expand All @@ -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;
Expand Down
25 changes: 21 additions & 4 deletions src/traces/scatterternary/hover.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This TODO is fairly uncommon, but when it happens you can see things like this:
screen shot 2016-06-25 at 1 01 15 am
where the label is not touching the shape at all. Because it's rare, doesn't actually stop the user from getting the information, and the fix would be really hairy, I propose to ignore it 🙈

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm ok with ignoring this. 👍

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;
Expand Down
7 changes: 7 additions & 0 deletions test/jasmine/assets/click.js
Original file line number Diff line number Diff line change
@@ -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);
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@etpinard per my commit message... somehow eslint run through sublime cannot sort out the config files within the jasmine directory, even though as far as I can tell it's all correct. In any other directory things look fine. Have you seen this? If I change test/jasmine/.eslintrc from "extends": "../../.eslintrc" to "extends": "../.eslintrc" it no longer complains, though that's clearly not what you want as it'll think node is allowed.

I suppose we could make that change and also explicitly set env.node=false, that seems to work for me, but it's strange that this would be necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. No, I've never seen that one before.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking it may be worth moving all our .eslintrc in a config/ at the root of the repo instead of having them scattered around (and hidden in) the directory tree.

We could move test_syntax.js to tasks/.

Having

test/
  |
  - image/
  - jasmine/
  - test_syntax.js
  - .eslintrc  (which is only relevant for test_syntax.js)

isn't great.

13 changes: 13 additions & 0 deletions test/jasmine/assets/double_click.js
Original file line number Diff line number Diff line change
@@ -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);
});
};
26 changes: 6 additions & 20 deletions test/jasmine/tests/click_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

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

Fantastic. Thanks so much!



describe('Test click interactions:', function() {
var mock = require('@mocks/14.json');
Expand All @@ -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);
Expand Down
20 changes: 19 additions & 1 deletion test/jasmine/tests/hover_label_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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],
Expand All @@ -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);
});
});
27 changes: 5 additions & 22 deletions test/jasmine/tests/select_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -104,7 +87,7 @@ describe('select box and lasso', function() {

drag([[x0, y0], [x1, y1]]);

doubleClick(x2, y2, done);
doubleClick(x2, y2).then(done);
});
});

Expand Down Expand Up @@ -153,7 +136,7 @@ describe('select box and lasso', function() {

drag([[x0, y0], [x1, y1]]);

doubleClick(x2, y2, done);
doubleClick(x2, y2).then(done);
Copy link
Contributor

Choose a reason for hiding this comment

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

much better. 🎉

});
});

Expand Down Expand Up @@ -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();
});
Expand Down Expand Up @@ -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();
});
Expand Down
20 changes: 2 additions & 18 deletions test/jasmine/tests/ternary_test.js
Original file line number Diff line number Diff line change
@@ -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');


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

Expand Down