Skip to content
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.

Sign up for GitHub

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

Jump to bottom

scatter & bar hover fixes for #780 #2218

Merged
merged 1 commit into from
Dec 21, 2017
+103 −36
Merged

scatter & bar hover fixes for #780 #2218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 23 additions & 4 deletions src/traces/bar/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,28 +18,35 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
var cd = pointData.cd;
var trace = cd[0].trace;
var t = cd[0].t;
var isClosest = (hovermode === 'closest');

var posVal, sizeVal, posLetter, sizeLetter, dx, dy;

function thisBarMinPos(di) { return di[posLetter] - di.w / 2; }
function thisBarMaxPos(di) { return di[posLetter] + di.w / 2; }

var minPos = (hovermode === 'closest') ?
var minPos = isClosest ?
thisBarMinPos :
function(di) {
/*
* In compare mode, accept a bar if you're on it *or* its group.
* Nearly always it's the group that matters, but in case the bar
* was explicitly set wider than its group we'd better accept the
* whole bar.
*
* use `bardelta` instead of `bargroupwidth` so we accept hover
* in the gap. That way hover doesn't flash on and off as you
* mouse over the plot in compare modes.
* In 'closest' mode though the flashing seems inevitable,
* without far more complex logic
*/
return Math.min(thisBarMinPos(di), di.p - t.bargroupwidth / 2);
return Math.min(thisBarMinPos(di), di.p - t.bardelta / 2);
};

var maxPos = (hovermode === 'closest') ?
var maxPos = isClosest ?
thisBarMaxPos :
function(di) {
return Math.max(thisBarMaxPos(di), di.p + t.bargroupwidth / 2);
return Math.max(thisBarMaxPos(di), di.p + t.bardelta / 2);
};

function positionFn(di) {
Expand Down Expand Up @@ -79,6 +86,18 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
// skip the rest (for this trace) if we didn't find a close point
if(pointData.index === false) return;

// if we get here and we're not in 'closest' mode, push min/max pos back
// onto the group - even though that means occasionally the mouse will be
// over the hover label.
if(!isClosest) {
minPos = function(di) {
return Math.min(thisBarMinPos(di), di.p - t.bargroupwidth / 2);
};
maxPos = function(di) {
return Math.max(thisBarMaxPos(di), di.p + t.bargroupwidth / 2);
};
}

// the closest data point
var index = pointData.index;
var di = cd[index];
Expand Down
2 changes: 2 additions & 0 deletions src/traces/bar/set_positions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ function setOffsetAndWidth(gd, pa, sieve) {
t.barwidth = barWidth;
t.poffset = offsetFromCenter;
t.bargroupwidth = barGroupWidth;
t.bardelta = minDiff;
}

// stack bars that only differ by rounding
Expand Down Expand Up @@ -277,6 +278,7 @@ function setOffsetAndWidthInGroupMode(gd, pa, sieve) {
t.barwidth = barWidth;
t.poffset = offsetFromCenter;
t.bargroupwidth = barGroupWidth;
t.bardelta = minDiff;
}

// stack bars that only differ by rounding
Expand Down
45 changes: 27 additions & 18 deletions src/traces/scatter/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,35 +18,44 @@ var fillHoverText = require('./fill_hover_text');
var MAXDIST = Fx.constants.MAXDIST;

module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
var cd = pointData.cd,
trace = cd[0].trace,
xa = pointData.xa,
ya = pointData.ya,
xpx = xa.c2p(xval),
ypx = ya.c2p(yval),
pt = [xpx, ypx],
hoveron = trace.hoveron || '';
var cd = pointData.cd;
var trace = cd[0].trace;
var xa = pointData.xa;
var ya = pointData.ya;
var xpx = xa.c2p(xval);
var ypx = ya.c2p(yval);
var pt = [xpx, ypx];
var hoveron = trace.hoveron || '';
var minRad = (trace.mode.indexOf('markers') !== -1) ? 3 : 0.5;

// look for points to hover on first, then take fills only if we
// didn't find a point
if(hoveron.indexOf('points') !== -1) {
var dx = function(di) {
// scatter points: d.mrc is the calculated marker radius
// adjust the distance so if you're inside the marker it
// always will show up regardless of point size, but
// prioritize smaller points
// dx and dy are used in compare modes - here we want to always
// prioritize the closest data point, at least as long as markers are
// the same size or nonexistent, but still try to prioritize small markers too.
var rad = Math.max(3, di.mrc || 0);
return Math.max(Math.abs(xa.c2p(di.x) - xpx) - rad, 1 - 3 / rad);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I copied this piece of logic elsewhere e.g. in scattergeo/hover.js. Should we update it there too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

scattergeo should be fine, as it only uses the closest form (ie real quadrature distance, not just x or just y). I suppose the minRad change is still relevant there, to better handle line-only traces, but that's it.

var kink = 1 - 1 / rad;
var dxRaw = Math.abs(xa.c2p(di.x) - xpx);
var d = (dxRaw < rad) ? (kink * dxRaw / rad) : (dxRaw - rad + kink);
return d;
},
dy = function(di) {
var rad = Math.max(3, di.mrc || 0);
return Math.max(Math.abs(ya.c2p(di.y) - ypx) - rad, 1 - 3 / rad);
var kink = 1 - 1 / rad;
var dyRaw = Math.abs(ya.c2p(di.y) - ypx);
return (dyRaw < rad) ? (kink * dyRaw / rad) : (dyRaw - rad + kink);
},
dxy = function(di) {
var rad = Math.max(3, di.mrc || 0),
dx = xa.c2p(di.x) - xpx,
dy = ya.c2p(di.y) - ypx;
return Math.max(Math.sqrt(dx * dx + dy * dy) - rad, 1 - 3 / rad);
// scatter points: d.mrc is the calculated marker radius
// adjust the distance so if you're inside the marker it
// always will show up regardless of point size, but
// prioritize smaller points
var rad = Math.max(minRad, di.mrc || 0);
var dx = xa.c2p(di.x) - xpx;
var dy = ya.c2p(di.y) - ypx;
return Math.max(Math.sqrt(dx * dx + dy * dy) - rad, 1 - minRad / rad);
},
distfn = Fx.getDistanceFunction(hovermode, dx, dy, dxy);

Expand Down
65 changes: 51 additions & 14 deletions test/jasmine/tests/hover_label_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1108,6 +1108,43 @@ describe('hover info on stacked subplots', function() {
});


describe('hover on many lines+bars', function() {
'use strict';

afterEach(destroyGraphDiv);

it('shows hover info for both traces', function(done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice test 🔒

// see https://github.com/plotly/plotly.js/issues/780
var values = new Array(1000);
var values2 = new Array(values.length);
for(var i = 0; i < values.length; i++) {
values[i] = i;
values2[i] = i * 2;
}

var gd = createGraphDiv();
Plotly.newPlot(gd, [
{y: values2},
{y: values, type: 'bar'}
], {
width: 400,
height: 400,
margin: {l: 100, r: 100, t: 100, b: 100}
})
.then(function() {
Lib.clearThrottle();
mouseEvent('mousemove', 200, 100);
Lib.clearThrottle();

expect(d3.select(gd).selectAll('g.hovertext').size()).toBe(2);
expect(d3.select(gd).selectAll('g.axistext').size()).toBe(1);
})
.catch(fail)
.then(done);
});
});


describe('hover info on overlaid subplots', function() {
'use strict';

Expand Down Expand Up @@ -1274,7 +1311,7 @@ describe('hover updates', function() {

afterEach(destroyGraphDiv);

function assertLabelsCorrect(mousePos, labelPos, labelText) {
function assertLabelsCorrect(mousePos, labelPos, labelText, msg) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great. So none of the current tests needed to be updated, is that correct?

return new Promise(function(resolve) {
if(mousePos) {
mouseEvent('mousemove', mousePos[0], mousePos[1]);
Expand All @@ -1283,14 +1320,14 @@ describe('hover updates', function() {
setTimeout(function() {
var hoverText = d3.selectAll('g.hovertext');
if(labelPos) {
expect(hoverText.size()).toEqual(1);
expect(hoverText.text()).toEqual(labelText);
expect(hoverText.size()).toBe(1, msg);
expect(hoverText.text()).toBe(labelText, msg);

var transformParts = hoverText.attr('transform').split('(');
expect(transformParts[0]).toEqual('translate');
expect(transformParts[0]).toBe('translate', msg);
var transformCoords = transformParts[1].split(')')[0].split(',');
expect(+transformCoords[0]).toBeCloseTo(labelPos[0], -1, labelText + ':x');
expect(+transformCoords[1]).toBeCloseTo(labelPos[1], -1, labelText + ':y');
expect(+transformCoords[0]).toBeCloseTo(labelPos[0], -1, labelText + ':x ' + msg);
expect(+transformCoords[1]).toBeCloseTo(labelPos[1], -1, labelText + ':y ' + msg);
} else {
expect(hoverText.size()).toEqual(0);
}
Expand Down Expand Up @@ -1318,7 +1355,7 @@ describe('hover updates', function() {
var gd = createGraphDiv();
Plotly.plot(gd, mock).then(function() {
// The label text gets concatenated together when queried. Such is life.
return assertLabelsCorrect([100, 100], [103, 100], 'trace 00.5');
return assertLabelsCorrect([100, 100], [103, 100], 'trace 00.5', 'animation/update 0');
}).then(function() {
return Plotly.animate(gd, [{
data: [{x: [0], y: [0]}, {x: [0.5], y: [0.5]}],
Expand All @@ -1327,25 +1364,25 @@ describe('hover updates', function() {
}).then(function() {
// No mouse event this time. Just change the data and check the label.
// Ditto on concatenation. This is "trace 1" + "0.5"
return assertLabelsCorrect(null, [103, 100], 'trace 10.5');
return assertLabelsCorrect(null, [103, 100], 'trace 10.5', 'animation/update 1');
}).then(function() {
// Restyle to move the point out of the window:
return Plotly.relayout(gd, {'xaxis.range': [2, 3]});
}).then(function() {
// Assert label removed:
return assertLabelsCorrect(null, null);
return assertLabelsCorrect(null, null, null, 'animation/update 2');
}).then(function() {
// Move back to the original xaxis range:
return Plotly.relayout(gd, {'xaxis.range': [0, 1]});
}).then(function() {
// Assert label restored:
return assertLabelsCorrect(null, [103, 100], 'trace 10.5');
return assertLabelsCorrect(null, [103, 100], 'trace 10.5', 'animation/update 3');
}).catch(fail).then(done);
});

it('should not trigger infinite loop of plotly_unhover events', function(done) {
var gd = createGraphDiv();
var colors0 = ['#00000', '#00000', '#00000', '#00000', '#00000', '#00000', '#00000'];
var colors0 = ['#000000', '#000000', '#000000', '#000000', '#000000', '#000000', '#000000'];

function unhover() {
return new Promise(function(resolve) {
Expand All @@ -1365,7 +1402,7 @@ describe('hover updates', function() {
y: [1, 2, 3, 2, 3, 4, 3],
marker: {
size: 16,
colors: colors0.slice()
color: colors0.slice()
}
}])
.then(function() {
Expand All @@ -1383,14 +1420,14 @@ describe('hover updates', function() {
Plotly.restyle(gd, 'marker.color', [colors0.slice()]);
});

return assertLabelsCorrect([351, 251], [358, 272], '2');
return assertLabelsCorrect([351, 251], [358, 272], '2', 'events 0');
})
.then(unhover)
.then(function() {
expect(hoverCnt).toEqual(1);
expect(unHoverCnt).toEqual(1);

return assertLabelsCorrect([400, 200], [435, 198], '3');
return assertLabelsCorrect([420, 100], [435, 198], '3', 'events 1');
Copy link
Contributor

Choose a reason for hiding this comment

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

Ha. Found one. Why did this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

x=400 is a little too far away using the new compare logic. I don't know exactly where the center of the marker is (that's why I've moved to more explicitly-sized-and-ranged plots for tests like this, so we can easily reason about the pixel positions of different points), probably the test would have worked just increasing to 405 or something but I just moved the mouse to a point I knew would be in range.

})
.then(unhover)
.then(function() {
Expand Down