Skip to content

Hover on fills #673

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 17 commits into from
Jun 22, 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
2 changes: 1 addition & 1 deletion src/lib/coerce.js
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ exports.valObjects = {
propOut.set(dflt);
return;
}
if(opts.extras.indexOf(v) !== -1) {
if((opts.extras || []).indexOf(v) !== -1) {
propOut.set(v);
return;
}
Expand Down
11 changes: 11 additions & 0 deletions src/traces/scatter/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,17 @@ module.exports = {
'then the default is *lines+markers*. Otherwise, *lines*.'
].join(' ')
},
hoveron: {
valType: 'flaglist',
flags: ['points', 'fills'],
role: 'info',
description: [
'Do the hover effects highlight individual points (markers or',
'line points) or do they highlight filled regions?',
'If the fill is *toself* or *tonext* and there are no markers',
'or text, then the default is *fills*, otherwise it is *points*.'
].join(' ')
},
line: {
color: {
valType: 'color',
Expand Down
8 changes: 8 additions & 0 deletions src/traces/scatter/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,11 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
handleTextDefaults(traceIn, traceOut, layout, coerce);
}

var dfltHoverOn = [];

if(subTypes.hasMarkers(traceOut) || subTypes.hasText(traceOut)) {
coerce('marker.maxdisplayed');
dfltHoverOn.push('points');
}

coerce('fill');
Expand All @@ -63,6 +66,11 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
if(!subTypes.hasLines(traceOut)) handleLineShapeDefaults(traceIn, traceOut, coerce);
}

if(traceOut.fill === 'tonext' || traceOut.fill === 'toself') {
dfltHoverOn.push('fills');
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 having second thoughts about making hoveron: 'points+fills' the default for scatter (my mistake).

With hovermode: 'x' by default the 'fills' in hoveron: 'points+fills' is hardly noticeable for most polygons.

Moreover, overriding the layout.hovermode default based on fill would a little too intrusive I think.

While hovermode: 'closest' by default for scatterternary, I'm not sure it warrants a different default than its scatter cousin.

So, I'm leaning towards making hoveron: 'points' the default for scatter and scatterternary regardless of the fill value.

Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ternary is easier here because it doesn't even have a hovermode: 'x'... But I'd also argue that if you have a scatter trace with fill tonext or toself, you're making the statement "I'm enclosing an area" which doesn't make much sense in conjunction with comparing different traces at the same x value. For one, your outline trace is itself generally going to have two (or more) points at the same x value (or 2 crossings of the same x value anyway, on opposite sides of the region) which is a situation that has always confused people ("why do I never see hover data for the second point?")

Not sure if there's an issue about it, but we've talked in the past about using trace info to make a smarter default hovermode choice, and I think this makes that case even stronger. hovermode: 'x' should only really be the default when you have multiple traces all with monotonically increasing (or decreasing) x (or position, like horizontal bars) data. I don't think that would be too intrusive, hovermode is a layout property but it is all about exposing the trace data anyway. If we did that, this concern would take care of itself.

I suppose in the meantime we could put hoveron back to the way it was before as a default: only have hoveron: 'fills' if you have an area fill and no explicit points, otherwise hoveron: 'points', never default to both. But I still think I'd prefer to leave it as is now, because it matches the meaning implied by your trace attributes.

}
coerce('hoveron', dfltHoverOn.join('+') || 'points');

errorBarsSupplyDefaults(traceIn, traceOut, defaultColor, {axis: 'y'});
errorBarsSupplyDefaults(traceIn, traceOut, defaultColor, {axis: 'x', inherit: 'y'});
};
183 changes: 136 additions & 47 deletions src/traces/scatter/hover.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,61 +9,150 @@

'use strict';

var Lib = require('../../lib');
var Fx = require('../../plots/cartesian/graph_interact');
var constants = require('../../plots/cartesian/constants');
var ErrorBars = require('../../components/errorbars');
var getTraceColor = require('./get_trace_color');
var Color = require('../../components/color');


module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
var cd = pointData.cd,
trace = cd[0].trace,
xa = pointData.xa,
ya = pointData.ya,
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
var rad = Math.max(3, di.mrc || 0);
return Math.max(Math.abs(xa.c2p(di.x) - xa.c2p(xval)) - rad, 1 - 3 / rad);
},
dy = function(di) {
var rad = Math.max(3, di.mrc || 0);
return Math.max(Math.abs(ya.c2p(di.y) - ya.c2p(yval)) - rad, 1 - 3 / rad);
},
dxy = function(di) {
var rad = Math.max(3, di.mrc || 0),
dx = Math.abs(xa.c2p(di.x) - xa.c2p(xval)),
dy = Math.abs(ya.c2p(di.y) - ya.c2p(yval));
return Math.max(Math.sqrt(dx * dx + dy * dy) - rad, 1 - 3 / rad);
},
distfn = Fx.getDistanceFunction(hovermode, dx, dy, dxy);

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],
xc = xa.c2p(di.x, true),
yc = ya.c2p(di.y, true),
rad = di.mrc || 1;

pointData.color = getTraceColor(trace, di);

pointData.x0 = xc - rad;
pointData.x1 = xc + rad;
pointData.xLabelVal = di.x;

pointData.y0 = yc - rad;
pointData.y1 = yc + rad;
pointData.yLabelVal = di.y;

if(di.tx) pointData.text = di.tx;
else if(trace.text) pointData.text = trace.text;

ErrorBars.hoverInfo(di, trace, pointData);

return [pointData];
xpx = xa.c2p(xval),
ypx = ya.c2p(yval),
pt = [xpx, ypx];

// look for points to hover on first, then take fills only if we
// didn't find a point
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. I'm ok with prioritising points over fills as by default. But, is that true in general?

That is, are there situations where fills should be prioritised over points? If so, maybe we should make hoveron a enumerated with values 'points', 'fills', 'points+fills' (points prioritised) and 'fills+points' (fills prioritised). But oh, that won't scale well if we ever add more hoveron modes.

This is getting way more complicated than I thought. I'm going to rethink about this in the morning. 😑

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think prioritizing fills over points would work well, for two reasons:

  • Some points would become nearly unhoverable, say if they are inside a very sharp angle, or if they're between two polygons (from the same trace) that come close together. If you want to see the point data at all, these are probably some of the most important ones.
  • Weird things would happen when two traces are next to each other... do we prioritize fills over points from both traces, or just over its own points? If the latter, weird things could happen where you see point data from trace A when you're over trace B and vice versa... but if the former, what do we do with trace C which is only points and you definitely want to see its hover data.

So I think we'd better leave it with points always winning, and if you don't like that you turn points off.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some points would become nearly unhoverable, say if they are inside a very sharp angle,

Weird things would happen when two traces are next to each other...

Agreed. The way you have it set up in aec4c95 is fine. Let's go with that.

if(trace.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
var rad = Math.max(3, di.mrc || 0);
return Math.max(Math.abs(xa.c2p(di.x) - xpx) - rad, 1 - 3 / rad);
},
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);
},
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);
},
distfn = Fx.getDistanceFunction(hovermode, dx, dy, dxy);

Fx.getClosest(cd, distfn, pointData);

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

// the closest data point
var di = cd[pointData.index],
xc = xa.c2p(di.x, true),
yc = ya.c2p(di.y, true),
rad = di.mrc || 1;

Lib.extendFlat(pointData, {
color: getTraceColor(trace, di),

x0: xc - rad,
x1: xc + rad,
xLabelVal: di.x,

y0: yc - rad,
y1: yc + rad,
yLabelVal: di.y
});

if(di.tx) pointData.text = di.tx;
else if(trace.text) pointData.text = trace.text;

ErrorBars.hoverInfo(di, trace, pointData);

return [pointData];
}
}

// even if hoveron is 'fills', only use it if we have polygons too
if(trace.hoveron.indexOf('fills') !== -1 && trace._polygons) {
var polygons = trace._polygons,
polygonsIn = [],
inside = false,
xmin = Infinity,
xmax = -Infinity,
ymin = Infinity,
ymax = -Infinity,
i, j, polygon, pts, xCross, x0, x1, y0, y1;

for(i = 0; i < polygons.length; i++) {
polygon = polygons[i];
// TODO: this is not going to work right for curved edges, it will
// act as though they're straight. That's probably going to need
// the elements themselves to capture the events. Worth it?
if(polygon.contains(pt)) {
inside = !inside;
// TODO: need better than just the overall bounding box
polygonsIn.push(polygon);
ymin = Math.min(ymin, polygon.ymin);
ymax = Math.max(ymax, polygon.ymax);
}
}

if(inside) {
// 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.
// Note that this might not be the vertical midpoint of the
// whole trace, if it's disjoint.
var yAvg = (ymin + ymax) / 2;
for(i = 0; i < polygonsIn.length; i++) {
pts = polygonsIn[i].pts;
for(j = 1; j < pts.length; j++) {
y0 = pts[j - 1][1];
y1 = pts[j][1];
if((y0 > yAvg) !== (y1 >= yAvg)) {
x0 = pts[j - 1][0];
x1 = pts[j][0];
xCross = x0 + (x1 - x0) * (yAvg - y0) / (y1 - y0);
xmin = Math.min(xmin, xCross);
xmax = Math.max(xmax, xCross);
}
}
}

// get only fill or line color for the hover color
var color = Color.defaultLine;
if(Color.opacity(trace.fillcolor)) color = trace.fillcolor;
else if(Color.opacity((trace.line || {}).color)) {
color = trace.line.color;
}

Lib.extendFlat(pointData, {
// never let a 2D override 1D type as closest point
distance: constants.MAXDIST + 10,
Copy link
Contributor

Choose a reason for hiding this comment

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

why the + 10 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same as heatmap - other types will return a distance that's no bigger than MAXDIST, so as long as we make this distance more than that, any matching scatter points always win. +10 is arbitrary, but I suppose we could make a priority ladder by making this different from heatmap in case of overlaps? Or leave them identical so trace order is the tiebreaker (though I don't actually know whether the first or last match will get it... I guess for visual consistency it should be the last trace, wouldn't it be clever if I wrote it that way???)

Copy link
Contributor

Choose a reason for hiding this comment

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

Parity with heatmaps is fine for now. Thanks for the clarification. 👍

x0: xmin,
x1: xmax,
y0: yAvg,
y1: yAvg,
color: color
});

delete pointData.index;

if(trace.text && !Array.isArray(trace.text)) {
pointData.text = String(trace.text);
}
else pointData.text = trace.name;
Copy link
Contributor

Choose a reason for hiding this comment

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

I imagine some users would want per-polygon text. But that's outside the scope of our API at the moment.

Maybe @cpsievert 's idea (in #147) of splitting segments of data using nulls isn't such a bad idea ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not even sure theoretically how that API would look. It's easy enough to just make each polygon a trace if they want that.


return [pointData];
}
}
};
18 changes: 17 additions & 1 deletion src/traces/scatter/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ var Lib = require('../../lib');
var Drawing = require('../../components/drawing');
var ErrorBars = require('../../components/errorbars');

var polygonTester = require('../../lib/polygon').tester;

var subTypes = require('./subtypes');
var arraysToCalcdata = require('./arrays_to_calcdata');
var linePoints = require('./line_points');
Expand All @@ -41,6 +43,7 @@ module.exports = function plot(gd, plotinfo, cdscatter) {

// BUILD LINES AND FILLS
var prevpath = '',
prevPolygons = [],
ownFillEl3, ownFillDir, tonext, nexttonext;

scattertraces.each(function(d) {
Expand Down Expand Up @@ -125,12 +128,23 @@ module.exports = function plot(gd, plotinfo, cdscatter) {
linear: line.shape === 'linear'
});

// since we already have the pixel segments here, use them to make
// polygons for hover on fill
// TODO: can we skip this if hoveron!=fills? That would mean we
// need to redraw when you change hoveron...
var thisPolygons = trace._polygons = new Array(segments.length),
i;

for(i = 0; i < segments.length; i++) {
trace._polygons[i] = polygonTester(segments[i]);
}

if(segments.length) {
var pt0 = segments[0][0],
lastSegment = segments[segments.length - 1],
pt1 = lastSegment[lastSegment.length - 1];

for(var i = 0; i < segments.length; i++) {
for(i = 0; i < segments.length; i++) {
var pts = segments[i];
thispath = pathfn(pts);
thisrevpath = revpathfn(pts);
Expand Down Expand Up @@ -185,8 +199,10 @@ module.exports = function plot(gd, plotinfo, cdscatter) {
// existing curve or off the end of it
tonext.attr('d', fullpath + 'L' + prevpath.substr(1) + 'Z');
}
trace._polygons = trace._polygons.concat(prevPolygons);
}
prevpath = revpath;
prevPolygons = thisPolygons;
}
});

Expand Down
1 change: 1 addition & 0 deletions src/traces/scatterternary/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ module.exports = {
hoverinfo: extendFlat({}, plotAttrs.hoverinfo, {
flags: ['a', 'b', 'c', 'text', 'name']
}),
hoveron: scatterAttrs.hoveron,
_nestedModules: {
'marker.colorbar': 'Colorbar'
}
Expand Down
8 changes: 8 additions & 0 deletions src/traces/scatterternary/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,11 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
handleTextDefaults(traceIn, traceOut, layout, coerce);
}

var dfltHoverOn = [];

if(subTypes.hasMarkers(traceOut) || subTypes.hasText(traceOut)) {
coerce('marker.maxdisplayed');
dfltHoverOn.push('points');
}

coerce('fill');
Expand All @@ -92,4 +95,9 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
}

coerce('hoverinfo', (layout._dataLength === 1) ? 'a+b+c+text' : undefined);

if(traceOut.fill === 'tonext' || traceOut.fill === 'toself') {
dfltHoverOn.push('fills');
}
coerce('hoveron', dfltHoverOn.join('+') || 'points');
};
4 changes: 4 additions & 0 deletions src/traces/scatterternary/hover.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
var scatterPointData = scatterHover(pointData, xval, yval, hovermode);
if(!scatterPointData || scatterPointData[0].index === false) return;

// 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;

var newPointData = scatterPointData[0],
cdi = newPointData.cd[newPointData.index];

Expand Down
6 changes: 6 additions & 0 deletions test/jasmine/assets/create_graph_div.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,11 @@ module.exports = function createGraphDiv() {
var gd = document.createElement('div');
gd.id = 'graph';
document.body.appendChild(gd);

// force the graph to be at position 0,0 no matter what
gd.style.position = 'fixed';
gd.style.left = 0;
gd.style.top = 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Different browsers seem to disagree on the default body margin. Before this gd had its origin at something like 6,6 or 8,8, now it's a consistent 0,0, as long as you make it with createGraphDiv(). I had to change a lot of mouse positions after that, but at least now they really correspond to pixels on the plot (so if you know the exact plot layout you can calculate them) and behave consistently across the different test runners.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is fantastic. Thanks!

Copy link
Contributor

@etpinard etpinard Jun 21, 2016

Choose a reason for hiding this comment

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

and thanks for fixing all the tests below.


return gd;
};
Loading