From 7a4154fd40ddd2c72554ef459765a14d847d9df2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 17 Mar 2017 17:25:19 -0400 Subject: [PATCH 1/5] make distance fn accept pt index as arg --- src/plots/cartesian/graph_interact.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/plots/cartesian/graph_interact.js b/src/plots/cartesian/graph_interact.js index 85f0130a5e8..5ce23e92459 100644 --- a/src/plots/cartesian/graph_interact.js +++ b/src/plots/cartesian/graph_interact.js @@ -237,9 +237,9 @@ function p2c(axArray, v) { } function quadrature(dx, dy) { - return function(di) { - var x = dx(di), - y = dy(di); + return function(di, i) { + var x = dx(di, i), + y = dy(di, i); return Math.sqrt(x * x + y * y); }; } @@ -661,7 +661,7 @@ fx.getClosest = function(cd, distfn, pointData) { // to create pre-sorted data (by x or y), not sure how to // do this for 'closest' for(var i = 0; i < cd.length; i++) { - var newDistance = distfn(cd[i]); + var newDistance = distfn(cd[i], i); if(newDistance <= pointData.distance) { pointData.index = i; pointData.distance = newDistance; From c84e2a96db41125563f1e5a1010adbf588bfffe6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 17 Mar 2017 17:26:42 -0400 Subject: [PATCH 2/5] generalize bar delta computation - so that traces with set 'width' (array or scalar) use the correct bar span. --- src/traces/bar/hover.js | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/traces/bar/hover.js b/src/traces/bar/hover.js index f65f947461c..6904a7af644 100644 --- a/src/traces/bar/hover.js +++ b/src/traces/bar/hover.js @@ -19,16 +19,17 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) { trace = cd[0].trace, t = cd[0].t, xa = pointData.xa, - ya = pointData.ya, - barDelta = (hovermode === 'closest') ? - t.barwidth / 2 : - t.bargroupwidth / 2, - barPos; + ya = pointData.ya; + var 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 barDelta = (hovermode === 'closest') ? + function(i) { return (t.barwidth[i] || t.barwidth) / 2; } : + function() { return t.bargroupwidth / 2; }; + var dx, dy; if(trace.orientation === 'h') { dx = function(di) { @@ -36,18 +37,20 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) { // 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) { + dy = function(di, i) { var centerPos = barPos(di) - yval; - return Fx.inbox(centerPos - barDelta, centerPos + barDelta); + var delta = barDelta(i); + return Fx.inbox(centerPos - delta, centerPos + delta); }; } else { dy = function(di) { return Fx.inbox(di.b - yval, di.y - yval) + (di.y - yval) / (di.y - di.b); }; - dx = function(di) { + dx = function(di, i) { var centerPos = barPos(di) - xval; - return Fx.inbox(centerPos - barDelta, centerPos + barDelta); + var delta = barDelta(i); + return Fx.inbox(centerPos - delta, centerPos + delta); }; } @@ -58,7 +61,8 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) { 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; @@ -70,16 +74,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(barPos(di) - barDelta(index), true); + pointData.y1 = ya.c2p(barPos(di) + barDelta(index), 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(barPos(di) - barDelta(index), true); + pointData.x1 = xa.c2p(barPos(di) + barDelta(index), true); pointData.xLabelVal = di.p; } From 591bd9e41d5318d1ff8bd152d4899f19658a13bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 17 Mar 2017 17:27:03 -0400 Subject: [PATCH 3/5] add two bar hover cases with set 'width' --- test/jasmine/tests/bar_test.js | 64 ++++++++++++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 2 deletions(-) diff --git a/test/jasmine/tests/bar_test.js b/test/jasmine/tests/bar_test.js index d864a592528..2e7b52314da 100644 --- a/test/jasmine/tests/bar_test.js +++ b/test/jasmine/tests/bar_test.js @@ -1191,9 +1191,9 @@ 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 pt = Bar.hoverPoints(pointData, xval, yval, hovermode)[0]; return { style: [pt.index, pt.color, pt.xLabelVal, pt.yLabelVal], @@ -1274,6 +1274,66 @@ describe('bar hover', function() { }); }); + describe('with special width/offset combinations', function() { + + beforeEach(function() { + gd = createGraphDiv(); + }); + + it('should return correct \'closest\' 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() { + var out = _hover(gd, 0, 0, 'closest'); + + expect(out.style).toEqual([0, 'red', 1, 2]); + assertPos(out.pos, [264, 278, 14, 14]); + }) + .then(done); + }); + + it('should return correct \'closest\' 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]); + }) + .then(function() { + var out = _hover(gd, 164, 0.8, 'closest'); + + expect(out.style).toEqual([1, 'red', 200, 1]); + assertPos(out.pos, [222, 235, 168, 168]); + }) + .then(done); + }); + + }); + }); function mockBarPlot(dataWithoutTraceType, layout) { From 30bc8c712d33ead092b4ae115f7a8bd33758ff43 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 5 Apr 2017 01:02:59 -0400 Subject: [PATCH 4/5] broaden bar hover acceptance, and test --- src/plots/cartesian/graph_interact.js | 8 +-- src/traces/bar/hover.js | 72 ++++++++++++++++----------- src/traces/bar/plot.js | 6 +-- src/traces/bar/set_positions.js | 10 ++-- test/jasmine/tests/bar_test.js | 53 ++++++++++++++++---- 5 files changed, 97 insertions(+), 52 deletions(-) diff --git a/src/plots/cartesian/graph_interact.js b/src/plots/cartesian/graph_interact.js index 5e0c7a3aa3d..f14bb31b5cf 100644 --- a/src/plots/cartesian/graph_interact.js +++ b/src/plots/cartesian/graph_interact.js @@ -237,9 +237,9 @@ function p2c(axArray, v) { } function quadrature(dx, dy) { - return function(di, i) { - var x = dx(di, i), - y = dy(di, i); + return function(di) { + var x = dx(di), + y = dy(di); return Math.sqrt(x * x + y * y); }; } @@ -664,7 +664,7 @@ fx.getClosest = function(cd, distfn, pointData) { // to create pre-sorted data (by x or y), not sure how to // do this for 'closest' for(var i = 0; i < cd.length; i++) { - var newDistance = distfn(cd[i], i); + var newDistance = distfn(cd[i]); if(newDistance <= pointData.distance) { pointData.index = i; pointData.distance = newDistance; diff --git a/src/traces/bar/hover.js b/src/traces/bar/hover.js index 6904a7af644..48c5596bd5e 100644 --- a/src/traces/bar/hover.js +++ b/src/traces/bar/hover.js @@ -15,45 +15,57 @@ 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; - - var 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 barDelta = (hovermode === 'closest') ? - function(i) { return (t.barwidth[i] || t.barwidth) / 2; } : - function() { return t.bargroupwidth / 2; }; - - 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, i) { - var centerPos = barPos(di) - yval; - var delta = barDelta(i); - return Fx.inbox(centerPos - delta, centerPos + delta); - }; + 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, i) { - var centerPos = barPos(di) - xval; - var delta = barDelta(i); - return Fx.inbox(centerPos - delta, centerPos + delta); - }; + 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. + */ + 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); @@ -74,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(index), true); - pointData.y1 = ya.c2p(barPos(di) + barDelta(index), 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(index), true); - pointData.x1 = xa.c2p(barPos(di) + barDelta(index), true); + pointData.x0 = xa.c2p(minPos(di), true); + pointData.x1 = xa.c2p(maxPos(di), true); pointData.xLabelVal = di.p; } diff --git a/src/traces/bar/plot.js b/src/traces/bar/plot.js index ff69c21f01f..8f64743e833 100644 --- a/src/traces/bar/plot.js +++ b/src/traces/bar/plot.js @@ -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) @@ -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, s0 = di.b, s1 = s0 + di.s; diff --git a/src/traces/bar/set_positions.js b/src/traces/bar/set_positions.js index cbed38a0408..3c1405ee98c 100644 --- a/src/traces/bar/set_positions.js +++ b/src/traces/bar/set_positions.js @@ -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); @@ -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); @@ -377,7 +377,7 @@ function applyAttributes(sieve) { } -function setBarCenter(gd, pa, sieve) { +function setBarCenterAndWidth(gd, pa, sieve) { var calcTraces = sieve.traces, pLetter = getAxisLetter(pa); @@ -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; calcBar[pLetter] = calcBar.p + ((poffsetIsArray) ? poffset[j] : poffset) + - ((barwidthIsArray) ? barwidth[j] : barwidth) / 2; + width / 2; } diff --git a/test/jasmine/tests/bar_test.js b/test/jasmine/tests/bar_test.js index 30b00c46ff7..a4111be9816 100644 --- a/test/jasmine/tests/bar_test.js +++ b/test/jasmine/tests/bar_test.js @@ -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'; @@ -1209,7 +1210,10 @@ describe('bar hover', function() { function _hover(gd, xval, yval, hovermode) { var pointData = getPointData(gd); - var pt = Bar.hoverPoints(pointData, xval, yval, hovermode)[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], @@ -1296,7 +1300,7 @@ describe('bar hover', function() { gd = createGraphDiv(); }); - it('should return correct \'closest\' hover data (single bar, trace width)', function(done) { + it('should return correct hover data (single bar, trace width)', function(done) { Plotly.plot(gd, [{ type: 'bar', x: [1], @@ -1307,15 +1311,39 @@ describe('bar hover', function() { xaxis: { range: [-200, 200] } }) .then(function() { - var out = _hover(gd, 0, 0, 'closest'); - - expect(out.style).toEqual([0, 'red', 1, 2]); - assertPos(out.pos, [264, 278, 14, 14]); + // 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 \'closest\' hover data (two bars, array width)', function(done) { + it('should return correct hover data (two bars, array width)', function(done) { Plotly.plot(gd, [{ type: 'bar', x: [1, 200], @@ -1338,13 +1366,18 @@ describe('bar hover', function() { expect(out.style).toEqual([0, 'red', 1, 2]); assertPos(out.pos, [99, 106, 13, 13]); - }) - .then(function() { - var out = _hover(gd, 164, 0.8, 'closest'); + + 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); }); From 06097d7bfa7c0d55861d3ee2b7456f03aa2727fa Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 5 Apr 2017 09:16:57 -0400 Subject: [PATCH 5/5] shift new bar hover test for ci --- test/jasmine/tests/bar_test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/jasmine/tests/bar_test.js b/test/jasmine/tests/bar_test.js index a4111be9816..3092ab3baa6 100644 --- a/test/jasmine/tests/bar_test.js +++ b/test/jasmine/tests/bar_test.js @@ -1375,7 +1375,7 @@ describe('bar hover', function() { out = _hover(gd, 125, 0.8, 'x'); expect(out.style).toEqual([1, 'red', 200, 1]); - assertPos(out.pos, [201, 301, 168, 168]); + assertPos(out.pos, [203, 304, 168, 168]); }) .catch(failTest) .then(done);