Skip to content

Fix hover label positioning for bar trace with set width #1527

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

Merged
merged 6 commits into from
Apr 5, 2017
Merged

Fix hover label positioning for bar trace with set width #1527

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7a4154f
make distance fn accept pt index as arg
etpinard Mar 17, 2017
c84e2a9
generalize bar delta computation
etpinard Mar 17, 2017
591bd9e
add two bar hover cases with set 'width'
etpinard Mar 17, 2017
b34d9ef
Merge branch 'master' into bar-width-hover
alexcjohnson Apr 4, 2017
30bc8c7
broaden bar hover acceptance, and test
alexcjohnson Apr 5, 2017
06097d7
shift new bar hover test for ci
alexcjohnson Apr 5, 2017
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
72 changes: 44 additions & 28 deletions src/traces/bar/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,50 +15,66 @@ var Color = require('../../components/color');


module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
var cd = pointData.cd,
trace = cd[0].trace,
t = cd[0].t,
xa = pointData.xa,
ya = pointData.ya,
barDelta = (hovermode === 'closest') ?
t.barwidth / 2 :
t.bargroupwidth / 2,
barPos;

if(hovermode !== 'closest') barPos = function(di) { return di.p; };
else if(trace.orientation === 'h') barPos = function(di) { return di.y; };
else barPos = function(di) { return di.x; };

var dx, dy;
var cd = pointData.cd;
var trace = cd[0].trace;
var t = cd[0].t;
var xa = pointData.xa;
var ya = pointData.ya;

var posVal, thisBarMinPos, thisBarMaxPos, minPos, maxPos, dx, dy;

var positionFn = function(di) {
return Fx.inbox(minPos(di) - posVal, maxPos(di) - posVal);
};

if(trace.orientation === 'h') {
posVal = yval;
thisBarMinPos = function(di) { return di.y - di.w / 2; };
thisBarMaxPos = function(di) { return di.y + di.w / 2; };
dx = function(di) {
// add a gradient so hovering near the end of a
// bar makes it a little closer match
return Fx.inbox(di.b - xval, di.x - xval) + (di.x - xval) / (di.x - di.b);
};
dy = function(di) {
var centerPos = barPos(di) - yval;
return Fx.inbox(centerPos - barDelta, centerPos + barDelta);
};
dy = positionFn;
}
else {
posVal = xval;
thisBarMinPos = function(di) { return di.x - di.w / 2; };
thisBarMaxPos = function(di) { return di.x + di.w / 2; };
dy = function(di) {
return Fx.inbox(di.b - yval, di.y - yval) + (di.y - yval) / (di.y - di.b);
};
dx = function(di) {
var centerPos = barPos(di) - xval;
return Fx.inbox(centerPos - barDelta, centerPos + barDelta);
};
dx = positionFn;
}

minPos = (hovermode === 'closest') ?
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.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

fixes the "compare" part of #1317 - at least if the bar is wider than our default. I guess if it's narrower and there are other things on the plot, we could be accepting this point when we shouldn't be... maybe I'll look at that case tomorrow and see if I can trigger it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright, the only case I can see where this pops up is an explicitly narrow (width<1) single bar with other things on the plot as well, like:

var data = [
	{type: 'bar', x: [1], y: [2], width: 0.1}, {x:[1.1], y:[1]}
];
var layout = {
	xaxis: {
		range: [-2, 2]
	}
};

Plotly.newPlot(gd, data, layout);

then the hoverable region for the bar is so wide it actually extends past the scatter point. But the scatter point still wins when you'd want it to. So if that's all it is, I don't feel like it's worth addressing this very obscure edge case, it'd be just as likely to cause other problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with ⬆️ . Thanks for documenting it!

return Math.min(thisBarMinPos(di), di.p - t.bargroupwidth / 2);
};

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

var distfn = Fx.getDistanceFunction(hovermode, dx, dy);
Fx.getClosest(cd, distfn, pointData);

// skip the rest (for this trace) if we didn't find a close point
if(pointData.index === false) return;

// the closest data point
var di = cd[pointData.index],
var index = pointData.index,
di = cd[index],
mc = di.mcc || trace.marker.color,
mlc = di.mlcc || trace.marker.line.color,
mlw = di.mlw || trace.marker.line.width;
Expand All @@ -70,16 +86,16 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
pointData.x0 = pointData.x1 = xa.c2p(di.x, true);
pointData.xLabelVal = size;

pointData.y0 = ya.c2p(barPos(di) - barDelta, true);
pointData.y1 = ya.c2p(barPos(di) + barDelta, true);
pointData.y0 = ya.c2p(minPos(di), true);
pointData.y1 = ya.c2p(maxPos(di), true);
pointData.yLabelVal = di.p;
}
else {
pointData.y0 = pointData.y1 = ya.c2p(di.y, true);
pointData.yLabelVal = size;

pointData.x0 = xa.c2p(barPos(di) - barDelta, true);
pointData.x1 = xa.c2p(barPos(di) + barDelta, true);
pointData.x0 = xa.c2p(minPos(di), true);
pointData.x1 = xa.c2p(maxPos(di), true);
pointData.xLabelVal = di.p;
}

Expand Down
6 changes: 2 additions & 4 deletions src/traces/bar/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,7 @@ module.exports = function plot(gd, plotinfo, cdbar) {
var t = d[0].t,
trace = d[0].trace,
poffset = t.poffset,
poffsetIsArray = Array.isArray(poffset),
barwidth = t.barwidth,
barwidthIsArray = Array.isArray(barwidth);
poffsetIsArray = Array.isArray(poffset);

d3.select(this).selectAll('g.point')
.data(Lib.identity)
Expand All @@ -61,7 +59,7 @@ module.exports = function plot(gd, plotinfo, cdbar) {
// log values go off-screen by plotwidth
// so you see them continue if you drag the plot
var p0 = di.p + ((poffsetIsArray) ? poffset[i] : poffset),
p1 = p0 + ((barwidthIsArray) ? barwidth[i] : barwidth),
p1 = p0 + di.w,
Copy link
Collaborator

Choose a reason for hiding this comment

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

a little follow-on benefit of adding di.w

s0 = di.b,
s1 = s0 + di.s;

Expand Down
10 changes: 6 additions & 4 deletions src/traces/bar/set_positions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ function setOffsetAndWidth(gd, pa, sieve) {
applyAttributes(sieve);

// store the bar center in each calcdata item
setBarCenter(gd, pa, sieve);
setBarCenterAndWidth(gd, pa, sieve);

// update position axes
updatePositionAxis(gd, pa, sieve);
Expand Down Expand Up @@ -286,7 +286,7 @@ function setOffsetAndWidthInGroupMode(gd, pa, sieve) {
applyAttributes(sieve);

// store the bar center in each calcdata item
setBarCenter(gd, pa, sieve);
setBarCenterAndWidth(gd, pa, sieve);

// update position axes
updatePositionAxis(gd, pa, sieve, overlap);
Expand Down Expand Up @@ -377,7 +377,7 @@ function applyAttributes(sieve) {
}


function setBarCenter(gd, pa, sieve) {
function setBarCenterAndWidth(gd, pa, sieve) {
var calcTraces = sieve.traces,
pLetter = getAxisLetter(pa);

Expand All @@ -392,9 +392,11 @@ function setBarCenter(gd, pa, sieve) {
for(var j = 0; j < calcTrace.length; j++) {
var calcBar = calcTrace[j];

// store the actual bar width and position, for use by hover
var width = calcBar.w = (barwidthIsArray) ? barwidth[j] : barwidth;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect we could clean up the hover routine quite a bit more if we stopped using .x and .y in calcdata... then we'd only have to switch at the beginning of the routine "are xval and yval position and size or vice versa" but I wasn't sure what the consequences of that would be beyond hover, just adding one more value was safe and easy.

calcBar[pLetter] = calcBar.p +
((poffsetIsArray) ? poffset[j] : poffset) +
((barwidthIsArray) ? barwidth[j] : barwidth) / 2;
width / 2;


}
Expand Down
97 changes: 95 additions & 2 deletions test/jasmine/tests/bar_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ var Axes = PlotlyInternal.Axes;
var createGraphDiv = require('../assets/create_graph_div');
var destroyGraphDiv = require('../assets/destroy_graph_div');
var customMatchers = require('../assets/custom_matchers');
var failTest = require('../assets/fail_test');

describe('Bar.supplyDefaults', function() {
'use strict';
Expand Down Expand Up @@ -1207,9 +1208,12 @@ describe('bar hover', function() {
};
}

function _hover(gd, xval, yval, closest) {
function _hover(gd, xval, yval, hovermode) {
var pointData = getPointData(gd);
var pt = Bar.hoverPoints(pointData, xval, yval, closest)[0];
var pts = Bar.hoverPoints(pointData, xval, yval, hovermode);
if(!pts) return false;

var pt = pts[0];

return {
style: [pt.index, pt.color, pt.xLabelVal, pt.yLabelVal],
Expand Down Expand Up @@ -1290,6 +1294,95 @@ describe('bar hover', function() {
});
});

describe('with special width/offset combinations', function() {

beforeEach(function() {
gd = createGraphDiv();
});

it('should return correct hover data (single bar, trace width)', function(done) {
Plotly.plot(gd, [{
type: 'bar',
x: [1],
y: [2],
width: 10,
marker: { color: 'red' }
}], {
xaxis: { range: [-200, 200] }
})
.then(function() {
// all these x, y, hovermode should give the same (the only!) hover label
[
[0, 0, 'closest'],
[-3.9, 1, 'closest'],
[5.9, 1.9, 'closest'],
[-3.9, -10, 'x'],
[5.9, 19, 'x']
].forEach(function(hoverSpec) {
var out = _hover(gd, hoverSpec[0], hoverSpec[1], hoverSpec[2]);

expect(out.style).toEqual([0, 'red', 1, 2], hoverSpec);
assertPos(out.pos, [264, 278, 14, 14], hoverSpec);
});

// then a few that are off the edge so yield nothing
[
[1, -0.1, 'closest'],
[1, 2.1, 'closest'],
[-4.1, 1, 'closest'],
[6.1, 1, 'closest'],
[-4.1, 1, 'x'],
[6.1, 1, 'x']
].forEach(function(hoverSpec) {
var out = _hover(gd, hoverSpec[0], hoverSpec[1], hoverSpec[2]);

expect(out).toBe(false, hoverSpec);
});
})
.catch(failTest)
.then(done);
});

it('should return correct hover data (two bars, array width)', function(done) {
Plotly.plot(gd, [{
type: 'bar',
x: [1, 200],
y: [2, 1],
width: [10, 20],
marker: { color: 'red' }
}, {
type: 'bar',
x: [1, 200],
y: [1, 2],
width: [20, 10],
marker: { color: 'green' }
}], {
xaxis: { range: [-200, 300] },
width: 500,
height: 500
})
.then(function() {
var out = _hover(gd, -36, 1.5, 'closest');

expect(out.style).toEqual([0, 'red', 1, 2]);
assertPos(out.pos, [99, 106, 13, 13]);

out = _hover(gd, 164, 0.8, 'closest');

expect(out.style).toEqual([1, 'red', 200, 1]);
assertPos(out.pos, [222, 235, 168, 168]);

out = _hover(gd, 125, 0.8, 'x');

expect(out.style).toEqual([1, 'red', 200, 1]);
assertPos(out.pos, [201, 301, 168, 168]);
})
.catch(failTest)
.then(done);
});

});

});

function mockBarPlot(dataWithoutTraceType, layout) {
Expand Down