diff --git a/draftlogs/6865_fix.md b/draftlogs/6865_fix.md new file mode 100644 index 00000000000..10bed88c463 --- /dev/null +++ b/draftlogs/6865_fix.md @@ -0,0 +1 @@ +- Improved hover detection for for scatter plot fill tonext* [[#6865](https://github.com/plotly/plotly.js/pull/6865)], with thanks to @lumip for the contribution! \ No newline at end of file diff --git a/src/traces/scatter/hover.js b/src/traces/scatter/hover.js index e6fb76cdb36..04b0de04e17 100644 --- a/src/traces/scatter/hover.js +++ b/src/traces/scatter/hover.js @@ -119,64 +119,118 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) { } } - // even if hoveron is 'fills', only use it if we have polygons too - if(hoveron.indexOf('fills') !== -1 && trace._polygons) { - var polygons = trace._polygons; + function isHoverPointInFillElement(el) { + // Uses SVGElement.isPointInFill to accurately determine wether + // the hover point / cursor is contained in the fill, taking + // curved or jagged edges into account, which the Polygon-based + // approach does not. + if(!el) { + return false; + } + var svgElement = el.node(); + try { + var domPoint = new DOMPoint(pt[0], pt[1]); + return svgElement.isPointInFill(domPoint); + } catch(TypeError) { + var svgPoint = svgElement.ownerSVGElement.createSVGPoint(); + svgPoint.x = pt[0]; + svgPoint.y = pt[1]; + return svgElement.isPointInFill(svgPoint); + } + } + + function getHoverLabelPosition(polygons) { + // Uses Polygon s to determine the left- and right-most x-coordinates + // of the subshape of the fill that contains the hover point / cursor. + // Doing this with the SVGElement directly is quite tricky, so this falls + // back to the existing relatively simple code, accepting some small inaccuracies + // of label positioning for curved/jagged edges. + var i; var polygonsIn = []; - var inside = false; var xmin = Infinity; var xmax = -Infinity; var ymin = Infinity; var ymax = -Infinity; - - var i, j, polygon, pts, xCross, x0, x1, y0, y1; + var yPos; 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? + var polygon = polygons[i]; + // This is not going to work right for curved or jagged edges, it will + // act as though they're straight. 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) { - // 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); - - // 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]; - if(y1 - y0) { - xCross = x0 + (x1 - x0) * (yAvg - y0) / (y1 - y0); - xmin = Math.min(xmin, xCross); - xmax = Math.max(xmax, xCross); - } + // The above found no polygon that contains the cursor, but we know that + // the cursor must be inside the fill as determined by the SVGElement + // (so we are probably close to a curved/jagged edge...). + if(polygonsIn.length === 0) { + return null; + } + + // 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); + + yPos = (ymin + ymax) / 2; + + // 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 j, pts, xAtYPos, x0, x1, y0, y1; + 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 > yPos) !== (y1 >= yPos)) { + x0 = pts[j - 1][0]; + x1 = pts[j][0]; + if(y1 - y0) { + xAtYPos = x0 + (x1 - x0) * (yPos - y0) / (y1 - y0); + xmin = Math.min(xmin, xAtYPos); + xmax = Math.max(xmax, xAtYPos); } } } + } + + // constrain xmin/max to the visible plot now too + xmin = Math.max(xmin, 0); + xmax = Math.min(xmax, xa._length); + + return { + x0: xmin, + x1: xmax, + y0: yPos, + y1: yPos, + }; + } - // constrain xmin/max to the visible plot now too - xmin = Math.max(xmin, 0); - xmax = Math.min(xmax, xa._length); + // even if hoveron is 'fills', only use it if we have a fill element too + if(hoveron.indexOf('fills') !== -1 && trace._fillElement) { + var inside = isHoverPointInFillElement(trace._fillElement) && !isHoverPointInFillElement(trace._fillExclusionElement); + + if(inside) { + var hoverLabelCoords = getHoverLabelPosition(trace._polygons); + + // getHoverLabelPosition may return null if the cursor / hover point is not contained + // in any of the trace's polygons, which can happen close to curved edges. in that + // case we fall back to displaying the hover label at the cursor position. + if(hoverLabelCoords === null) { + hoverLabelCoords = { + x0: pt[0], + x1: pt[0], + y0: pt[1], + y1: pt[1] + }; + } // get only fill or line color for the hover color var color = Color.defaultLine; @@ -189,10 +243,10 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) { // never let a 2D override 1D type as closest point // also: no spikeDistance, it's not allowed for fills distance: pointData.maxHoverDistance, - x0: xmin, - x1: xmax, - y0: yAvg, - y1: yAvg, + x0: hoverLabelCoords.x0, + x1: hoverLabelCoords.x1, + y0: hoverLabelCoords.y0, + y1: hoverLabelCoords.y1, color: color, hovertemplate: false }); diff --git a/src/traces/scatter/plot.js b/src/traces/scatter/plot.js index eb2e3977943..3ab3481b7b5 100644 --- a/src/traces/scatter/plot.js +++ b/src/traces/scatter/plot.js @@ -143,17 +143,30 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition var ownFillDir = trace.fill.charAt(trace.fill.length - 1); if(ownFillDir !== 'x' && ownFillDir !== 'y') ownFillDir = ''; + var fillAxisIndex, fillAxisZero; + if(ownFillDir === 'y') { + fillAxisIndex = 1; + fillAxisZero = ya.c2p(0, true); + } else if(ownFillDir === 'x') { + fillAxisIndex = 0; + fillAxisZero = xa.c2p(0, true); + } + // store node for tweaking by selectPoints cdscatter[0][plotinfo.isRangePlot ? 'nodeRangePlot3' : 'node3'] = tr; var prevRevpath = ''; var prevPolygons = []; var prevtrace = trace._prevtrace; + var prevFillsegments = null; + var prevFillElement = null; if(prevtrace) { prevRevpath = prevtrace._prevRevpath || ''; tonext = prevtrace._nextFill; - prevPolygons = prevtrace._polygons; + prevPolygons = prevtrace._ownPolygons; + prevFillsegments = prevtrace._fillsegments; + prevFillElement = prevtrace._fillElement; } var thispath; @@ -166,7 +179,15 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition // functions for converting a point array to a path var pathfn, revpathbase, revpathfn; // variables used before and after the data join - var pt0, lastSegment, pt1, thisPolygons; + var pt0, lastSegment, pt1; + + // thisPolygons always contains only the polygons of this trace only + // whereas trace._polygons may be extended to include those of the previous + // trace as well for exclusion during hover detection + var thisPolygons = []; + trace._polygons = []; + + var fillsegments = []; // initialize line join data / method var segments = []; @@ -220,28 +241,47 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition }); // since we already have the pixel segments here, use them to make - // polygons for hover on fill + // polygons for hover on fill; we first merge segments where the fill + // is connected into "fillsegments"; the actual polygon construction + // is deferred to later to distinguish between self and tonext/tozero fills. // TODO: can we skip this if hoveron!=fills? That would mean we // need to redraw when you change hoveron... - thisPolygons = trace._polygons = new Array(segments.length); + fillsegments = new Array(segments.length); + var fillsegmentCount = 0; for(i = 0; i < segments.length; i++) { - trace._polygons[i] = polygonTester(segments[i]); + var curpoints; + var pts = segments[i]; + if(!curpoints || !ownFillDir) { + curpoints = pts.slice(); + fillsegments[fillsegmentCount] = curpoints; + fillsegmentCount++; + } else { + curpoints.push.apply(curpoints, pts); + } } + trace._fillElement = null; + trace._fillExclusionElement = prevFillElement; + + trace._fillsegments = fillsegments.slice(0, fillsegmentCount); + fillsegments = trace._fillsegments; + if(segments.length) { - pt0 = segments[0][0]; + pt0 = segments[0][0].slice(); lastSegment = segments[segments.length - 1]; - pt1 = lastSegment[lastSegment.length - 1]; + pt1 = lastSegment[lastSegment.length - 1].slice(); } makeUpdate = function(isEnter) { return function(pts) { thispath = pathfn(pts); - thisrevpath = revpathfn(pts); + thisrevpath = revpathfn(pts); // side-effect: reverses input + // calculate SVG path over all segments for fills if(!fullpath) { fullpath = thispath; revpath = thisrevpath; } else if(ownFillDir) { + // for fills with fill direction: ignore gaps fullpath += 'L' + thispath.substr(1); revpath = thisrevpath + ('L' + revpath.substr(1)); } else { @@ -249,6 +289,7 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition revpath = thisrevpath + 'Z' + revpath; } + // actual lines get drawn here, with gaps between segments if requested if(subTypes.hasLines(trace)) { var el = d3.select(this); @@ -290,16 +331,58 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition transition(selection).attr('d', 'M0,0Z'); } + // helper functions to create polygons for hoveron fill detection + var makeSelfPolygons = function() { + var polygons = new Array(fillsegments.length); + for(i = 0; i < fillsegments.length; i++) { + polygons[i] = polygonTester(fillsegments[i]); + } + return polygons; + }; + + var makePolygonsToPrevious = function(prevFillsegments) { + var polygons, i; + if(!prevFillsegments || prevFillsegments.length === 0) { + // if there are no fill segments of a previous trace, stretch the + // polygon to the relevant axis + polygons = new Array(fillsegments.length); + for(i = 0; i < fillsegments.length; i++) { + var pt0 = fillsegments[i][0].slice(); + var pt1 = fillsegments[i][fillsegments[i].length - 1].slice(); + + pt0[fillAxisIndex] = pt1[fillAxisIndex] = fillAxisZero; + + var zeropoints = [pt1, pt0]; + var polypoints = zeropoints.concat(fillsegments[i]); + polygons[i] = polygonTester(polypoints); + } + } else { + // if there are more than one previous fill segment, the + // way that fills work is to "self" fill all but the last segments + // of the previous and then fill from the new trace to the last + // segment of the previous. + polygons = new Array(prevFillsegments.length - 1 + fillsegments.length); + for(i = 0; i < prevFillsegments.length - 1; i++) { + polygons[i] = polygonTester(prevFillsegments[i]); + } + + var reversedPrevFillsegment = prevFillsegments[prevFillsegments.length - 1].slice(); + reversedPrevFillsegment.reverse(); + + for(i = 0; i < fillsegments.length; i++) { + polygons[prevFillsegments.length - 1 + i] = polygonTester(fillsegments[i].concat(reversedPrevFillsegment)); + } + } + return polygons; + }; + + // draw fills and create hover detection polygons if(segments.length) { if(ownFillEl3) { ownFillEl3.datum(cdscatter); - if(pt0 && pt1) { + if(pt0 && pt1) { // TODO(2023-12-10): this is always true if segments is not empty (?) if(ownFillDir) { - if(ownFillDir === 'y') { - pt0[1] = pt1[1] = ya.c2p(0, true); - } else if(ownFillDir === 'x') { - pt0[0] = pt1[0] = xa.c2p(0, true); - } + pt0[fillAxisIndex] = pt1[fillAxisIndex] = fillAxisZero; // fill to zero: full trace path, plus extension of // the endpoints to the appropriate axis @@ -308,12 +391,20 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition // animations get a little crazy if the number of points changes. transition(ownFillEl3).attr('d', 'M' + pt1 + 'L' + pt0 + 'L' + fullpath.substr(1)) .call(Drawing.singleFillStyle, gd); + + // create hover polygons that extend to the axis as well. + thisPolygons = makePolygonsToPrevious(null); // polygon to axis } else { // fill to self: just join the path to itself transition(ownFillEl3).attr('d', fullpath + 'Z') .call(Drawing.singleFillStyle, gd); + + // and simply emit hover polygons for each segment + thisPolygons = makeSelfPolygons(); } } + trace._polygons = thisPolygons; + trace._fillElement = ownFillEl3; } else if(tonext) { if(trace.fill.substr(0, 6) === 'tonext' && fullpath && prevRevpath) { // fill to next: full trace path, plus the previous path reversed @@ -324,6 +415,13 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition // inside the other, but then that is a strange usage. transition(tonext).attr('d', fullpath + 'Z' + prevRevpath + 'Z') .call(Drawing.singleFillStyle, gd); + + // and simply emit hover polygons for each segment + thisPolygons = makeSelfPolygons(); + + // we add the polygons of the previous trace which causes hover + // detection to ignore points contained in them. + trace._polygons = thisPolygons.concat(prevPolygons); // this does not modify thisPolygons, on purpose } else { // tonextx/y: for now just connect endpoints with lines. This is // the correct behavior if the endpoints are at the same value of @@ -332,20 +430,26 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition // existing curve or off the end of it transition(tonext).attr('d', fullpath + 'L' + prevRevpath.substr(1) + 'Z') .call(Drawing.singleFillStyle, gd); + + // create hover polygons that extend to the previous trace. + thisPolygons = makePolygonsToPrevious(prevFillsegments); + + // in this case our polygons do not cover that of previous traces, + // so must not include previous trace polygons for hover detection. + trace._polygons = thisPolygons; } - trace._polygons = trace._polygons.concat(prevPolygons); + trace._fillElement = tonext; } else { clearFill(tonext); - trace._polygons = null; } } trace._prevRevpath = revpath; - trace._prevPolygons = thisPolygons; } else { if(ownFillEl3) clearFill(ownFillEl3); else if(tonext) clearFill(tonext); - trace._polygons = trace._prevRevpath = trace._prevPolygons = null; + trace._prevRevpath = null; } + trace._ownPolygons = thisPolygons; function visFilter(d) { diff --git a/test/jasmine/tests/scatter_test.js b/test/jasmine/tests/scatter_test.js index db233454aea..4b191bc2fd2 100644 --- a/test/jasmine/tests/scatter_test.js +++ b/test/jasmine/tests/scatter_test.js @@ -1514,6 +1514,176 @@ describe('scatter hoverPoints', function() { }); }); +describe('scatter hoverFills', function() { + afterEach(destroyGraphDiv); + + function _hover(gd, xval, yval, hovermode, subplotId) { + return gd._fullData.map(function(trace, i) { + var cd = gd.calcdata[i]; + var subplot = gd._fullLayout._plots[subplotId]; + + var out = Scatter.hoverPoints({ + index: false, + distance: 20, + cd: cd, + trace: trace, + xa: subplot.xaxis, + ya: subplot.yaxis + }, xval, yval, hovermode); + + return Array.isArray(out) ? out[0] : null; + }); + } + + it('should correctly detect the fill that is hovered over for self and next fills', function(done) { + var gd = createGraphDiv(); + var mock = Lib.extendDeep({}, require('../../image/mocks/scatter_fill_self_next')); + + var testPoints = [ + [[2, 2.9], [2, 2], [1.1, 2], [5.99, 3.01], [4.6, 3.5]], + [[2, 3.1], [-0.2, 1.1], [5, 2.99], [7, 2], [1.2, 5.1]], + [[6, 5], [7, 6], [8, 5], [7, 5], [6.7, 5.3]] + ]; + + Plotly.newPlot(gd, mock).then(function() { + return Plotly.restyle(gd, 'hoveron', 'fills'); + }) + .then(function() { + for(var i = 0; i < testPoints.length; i++) { + for(var j = 0; j < testPoints[i].length; j++) { + var testCoords = testPoints[i][j]; + var pts = _hover(gd, testCoords[0], testCoords[1], 'x', 'xy'); + expect(pts[i]).toBeTruthy( + 'correct trace not detected ' + testCoords.join(',') + ', should be ' + i + ); + for(var k = 0; k < pts.length; k++) { + var traceId = (i + 1) % pts.length; + expect(pts[traceId]).toBeFalsy( + 'wrong trace detected ' + testCoords.join(',') + '; got ' + + traceId + ' but should be ' + i + ); + } + } + } + }) + .then(done, done.fail); + }); + + it('should correctly detect the fill that is hovered over for tozeroy and tonexty fills', function(done) { + var gd = createGraphDiv(); + var mock = Lib.extendDeep({}, require('../../image/mocks/scatter_fill_corner_cases')); + + var traceOffset = 0; + + var testPoints = [ // all the following points should be in fill region of corresponding tozeroy traces 0-4 + [], // single point has no "fill" when using SVG element containment tests + [[0.1, 0.9], [0.1, 0.8], [1.5, 0.9], [1.5, 1.04], [2, 0.8], [2, 1.09], [3, 0.8]], + [[0.1, 0.75], [0.1, 0.61], [1.01, 0.501], [1.5, 0.8], [1.5, 0.55], [2, 0.74], [2, 0.55], [3, 0.74], [3, 0.51]], + [[0.1, 0.599], [0.1, 0.5], [0.1, 0.3], [0.99, 0.59], [1, 0.49], [1, 0.36], [1.5, 0.26], [2, 0.49], [2, 0.16], [3, 0.49], [3, 0.26]], + [[0.1, 0.25], [0.1, 0.1], [1, 0.34], [1.5, 0.24], [2, 0.14], [3, 0.24], [3, 0.1]], + ]; + + var outsidePoints = [ // all these should not result in a hover detection, for any trace + [1, 1.1], [2, 1.14], [1.5, 1.24], [1.5, 1.06] + ]; + + Plotly.newPlot(gd, mock).then(function() { + return Plotly.restyle(gd, 'hoveron', 'fills'); + }) + .then(function() { + var testCoords, pts; + var i, j, k; + for(i = 0; i < testPoints.length; i++) { + for(j = 0; j < testPoints[i].length; j++) { + testCoords = testPoints[i][j]; + pts = _hover(gd, testCoords[0], testCoords[1], 'x', 'xy'); + expect(pts[traceOffset + i]).toBeTruthy( + 'correct trace not detected ' + testCoords.join(',') + ', should be ' + (traceOffset + i) + ); + + // since all polygons do extend to the zero axis, many points will be detected by the + // correct trace and previous ones, but a point should not be detected as hover points + // by any trace defined later than the correct trace! + // (in actual hover detection, the real _hover takes care of the overlap with previous traces + // so that is not an issue in practice) + for(k = i + 1; k < testPoints.length; k++) { + var traceId = traceOffset + k; + expect(pts[traceId]).toBeFalsy( + 'wrong trace detected ' + testCoords.join(',') + '; got ' + + traceId + ' but should be ' + (traceOffset + i) + ); + } + } + } + + for(j = 0; j < outsidePoints.length; j++) { + testCoords = outsidePoints[j]; + pts = _hover(gd, testCoords[0], testCoords[1], 'x', 'xy'); + for(k = 0; k < testPoints.length; k++) { + expect(pts[i]).toBeFalsy( + 'trace detected for outside point ' + testCoords.join(',') + ', got ' + (traceOffset + k) + ); + } + } + }) + .then(done, done.fail); + }); + + + it('should correctly detect the fill that is hovered over for tonexty fills', function(done) { + var gd = createGraphDiv(); + var mock = Lib.extendDeep({}, require('../../image/mocks/scatter_fill_corner_cases')); + + var traceOffset = 10; + + var testPoints = [ // all the following points should be in fill region of corresponding tonexty traces 10-14 + [], + [[1, 1.1], [1.5, 1.24], [1.5, 1.06], [2, 1.14]], + [[0.1, 0.9], [0.1, 0.8], [1.5, 0.9], [1.5, 1.04], [2, 0.8], [2, 1.09], [3, 0.8]], + [[0.1, 0.75], [0.1, 0.61], [1.01, 0.501], [1.5, 0.8], [1.5, 0.55], [2, 0.74], [2, 0.55], [3, 0.74], [3, 0.51]], + [[0.1, 0.599], [0.1, 0.5], [0.1, 0.3], [0.99, 0.59], [1, 0.49], [1, 0.36], [1.5, 0.26], [2, 0.49], [2, 0.16], [3, 0.49], [3, 0.26]], + ]; + var outsidePoints = [ // all these should not result in a hover detection, for any trace + [0.1, 0.25], [0.1, 0.1], [1, 0.34], [1.5, 0.24], [2, 0.14], [3, 0.24], [3, 0.1], [0.5, 1.15], [2.5, 1.15], + ]; + + Plotly.newPlot(gd, mock).then(function() { + return Plotly.restyle(gd, 'hoveron', 'fills'); + }) + .then(function() { + var testCoords, pts; + var i, j, k; + for(i = 0; i < testPoints.length; i++) { + for(j = 0; j < testPoints[i].length; j++) { + testCoords = testPoints[i][j]; + pts = _hover(gd, testCoords[0], testCoords[1], 'x', 'xy2'); + expect(pts[traceOffset + i]).toBeTruthy( + 'correct trace not detected ' + testCoords.join(',') + ', should be ' + (traceOffset + i) + ); + + for(k = 1; k < testPoints.length; k++) { + var traceId = traceOffset + ((i + k) % testPoints.length); + expect(pts[traceId]).toBeFalsy( + 'wrong trace detected ' + testCoords.join(',') + '; got ' + + traceId + ' but should be ' + (traceOffset + i) + ); + } + } + } + for(j = 0; j < outsidePoints.length; j++) { + testCoords = outsidePoints[j]; + pts = _hover(gd, testCoords[0], testCoords[1], 'x', 'xy2'); + for(k = 0; k < testPoints.length; k++) { + expect(pts[traceOffset + k]).toBeFalsy( + 'trace detected for outside point ' + testCoords.join(',') + ', got ' + (traceOffset + k) + ); + } + } + }) + .then(done, done.fail); + }); +}); + describe('Test Scatter.style', function() { var gd;