From 407bcf29fe956b7be4cae96fe1a458db96bc1524 Mon Sep 17 00:00:00 2001 From: Lukas Prediger Date: Sat, 9 Dec 2023 16:36:09 +0200 Subject: [PATCH 1/6] Added tests for hoveron fills to scatter jasmine test suite. The tests rely on scatter mocks `scatter_fill_self_next` and `scatter_fill_corner_cases` and test for a number of points that are currently not correctly detected when hovered over for tonexty and tozeroy fills. --- test/jasmine/tests/scatter_test.js | 170 +++++++++++++++++++++++++++++ 1 file changed, 170 insertions(+) diff --git a/test/jasmine/tests/scatter_test.js b/test/jasmine/tests/scatter_test.js index db233454aea..b8cebe2efb6 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 + [[1.5, 1.24], [1.5, 1.06]], // single point has "fill" along line to zero + [[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], + ]; + + 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; From 52bf7c8edacdbea13ed6fe9c717a3922afecf4e9 Mon Sep 17 00:00:00 2001 From: Lukas Prediger Date: Wed, 6 Dec 2023 18:22:15 +0200 Subject: [PATCH 2/6] Fix for hover behavior on scatter fills tonextx/y. Previous code would not correctly construct the polygons used for detection whether the cursor is within the fill region of a trace. Some of the previously created tests fail with this as the hover points in question cannot currently be correctly detected using the detection polygons. --- src/traces/scatter/hover.js | 2 +- src/traces/scatter/plot.js | 132 +++++++++++++++++++++++++++++++----- 2 files changed, 115 insertions(+), 19 deletions(-) diff --git a/src/traces/scatter/hover.js b/src/traces/scatter/hover.js index e6fb76cdb36..02d2ba0ad94 100644 --- a/src/traces/scatter/hover.js +++ b/src/traces/scatter/hover.js @@ -120,7 +120,7 @@ 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) { + if(hoveron.indexOf('fills') !== -1 && trace._polygons && trace._polygons.length > 0) { var polygons = trace._polygons; var polygonsIn = []; var inside = false; diff --git a/src/traces/scatter/plot.js b/src/traces/scatter/plot.js index eb2e3977943..eb4c721cd76 100644 --- a/src/traces/scatter/plot.js +++ b/src/traces/scatter/plot.js @@ -143,17 +143,28 @@ 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; if(prevtrace) { prevRevpath = prevtrace._prevRevpath || ''; tonext = prevtrace._nextFill; - prevPolygons = prevtrace._polygons; + prevPolygons = prevtrace._ownPolygons; + prevFillsegments = prevtrace._fillsegments; } var thispath; @@ -166,7 +177,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 +239,43 @@ 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._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 +283,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 +325,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 +385,19 @@ 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; } 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 +408,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 +423,25 @@ 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); } 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) { From 5ba718fa4e9621836babddaa063295ac779b2d28 Mon Sep 17 00:00:00 2001 From: Lukas Prediger Date: Tue, 16 Jan 2024 18:12:26 +0200 Subject: [PATCH 3/6] Using SVGElement.isPointInFill instead of custom polygons for hover tests in scatter plots. However, SVGElement does not allow for an easy way to determine the positioning of the hover label, so the polygons are still in use for that. --- src/traces/scatter/hover.js | 139 ++++++++++++++++++++--------- src/traces/scatter/plot.js | 10 ++- test/jasmine/tests/scatter_test.js | 4 +- 3 files changed, 106 insertions(+), 47 deletions(-) diff --git a/src/traces/scatter/hover.js b/src/traces/scatter/hover.js index 02d2ba0ad94..8bec67ab8e5 100644 --- a/src/traces/scatter/hover.js +++ b/src/traces/scatter/hover.js @@ -119,64 +119,115 @@ 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 && trace._polygons.length > 0) { - 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 yminAll = Infinity; + var ymaxAll = -Infinity; + 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. + yminAll = Math.min(yminAll, polygon.ymin); + ymaxAll = Math.max(ymaxAll, polygon.ymax); 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...). In this case + // as a crude approximation, simply consider all polygons for determination + // of the hover label position. + // TODO: This might cause some jumpiness of the label close to edges... + if(polygonsIn.length === 0) { + polygonsIn = polygons; + ymin = yminAll; + ymax = ymaxAll; + } + + // 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); // get only fill or line color for the hover color var color = Color.defaultLine; @@ -189,10 +240,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 eb4c721cd76..3ab3481b7b5 100644 --- a/src/traces/scatter/plot.js +++ b/src/traces/scatter/plot.js @@ -159,12 +159,14 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition var prevPolygons = []; var prevtrace = trace._prevtrace; var prevFillsegments = null; + var prevFillElement = null; if(prevtrace) { prevRevpath = prevtrace._prevRevpath || ''; tonext = prevtrace._nextFill; prevPolygons = prevtrace._ownPolygons; prevFillsegments = prevtrace._fillsegments; + prevFillElement = prevtrace._fillElement; } var thispath; @@ -257,6 +259,10 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition curpoints.push.apply(curpoints, pts); } } + + trace._fillElement = null; + trace._fillExclusionElement = prevFillElement; + trace._fillsegments = fillsegments.slice(0, fillsegmentCount); fillsegments = trace._fillsegments; @@ -398,6 +404,7 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition } } 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 @@ -409,7 +416,7 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition transition(tonext).attr('d', fullpath + 'Z' + prevRevpath + 'Z') .call(Drawing.singleFillStyle, gd); - // and simply emit hover polygons for each segment + // and simply emit hover polygons for each segment thisPolygons = makeSelfPolygons(); // we add the polygons of the previous trace which causes hover @@ -431,6 +438,7 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition // so must not include previous trace polygons for hover detection. trace._polygons = thisPolygons; } + trace._fillElement = tonext; } else { clearFill(tonext); } diff --git a/test/jasmine/tests/scatter_test.js b/test/jasmine/tests/scatter_test.js index b8cebe2efb6..4b191bc2fd2 100644 --- a/test/jasmine/tests/scatter_test.js +++ b/test/jasmine/tests/scatter_test.js @@ -1576,7 +1576,7 @@ describe('scatter hoverFills', function() { var traceOffset = 0; var testPoints = [ // all the following points should be in fill region of corresponding tozeroy traces 0-4 - [[1.5, 1.24], [1.5, 1.06]], // single point has "fill" along line to zero + [], // 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]], @@ -1584,7 +1584,7 @@ describe('scatter hoverFills', function() { ]; var outsidePoints = [ // all these should not result in a hover detection, for any trace - [1, 1.1], [2, 1.14], + [1, 1.1], [2, 1.14], [1.5, 1.24], [1.5, 1.06] ]; Plotly.newPlot(gd, mock).then(function() { From 6f8ca1e59962fc1b6510aa00cc62b9460b7bacd6 Mon Sep 17 00:00:00 2001 From: Lukas Prediger Date: Tue, 23 Jan 2024 20:03:14 +0200 Subject: [PATCH 4/6] Display hover label at cursor position as fallback for scatter traces. For curved edges of fills there is a chance that the hover detection polygons miss a point and the correct hover label position cannot be determined. Previous code fell back to positioning the label at the largest edge of any polygon of the trace of their combined vertical midpoint, with undetermined behaviour if no polygon overlapped the midpoint. This change instead falls back to simply displaying the label at the current cursor position, which simplifies code and results in less confusing label placement. --- src/traces/scatter/hover.js | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/traces/scatter/hover.js b/src/traces/scatter/hover.js index 8bec67ab8e5..04b0de04e17 100644 --- a/src/traces/scatter/hover.js +++ b/src/traces/scatter/hover.js @@ -151,16 +151,12 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) { var xmax = -Infinity; var ymin = Infinity; var ymax = -Infinity; - var yminAll = Infinity; - var ymaxAll = -Infinity; var yPos; for(i = 0; i < polygons.length; i++) { var polygon = polygons[i]; // This is not going to work right for curved or jagged edges, it will // act as though they're straight. - yminAll = Math.min(yminAll, polygon.ymin); - ymaxAll = Math.max(ymaxAll, polygon.ymax); if(polygon.contains(pt)) { polygonsIn.push(polygon); ymin = Math.min(ymin, polygon.ymin); @@ -170,14 +166,9 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) { // 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...). In this case - // as a crude approximation, simply consider all polygons for determination - // of the hover label position. - // TODO: This might cause some jumpiness of the label close to edges... + // (so we are probably close to a curved/jagged edge...). if(polygonsIn.length === 0) { - polygonsIn = polygons; - ymin = yminAll; - ymax = ymaxAll; + return null; } // constrain ymin/max to the visible plot, so the label goes @@ -229,6 +220,18 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) { 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; if(Color.opacity(trace.fillcolor)) color = trace.fillcolor; From 889b6c6e761e29635c20554ddb859a5739e89d11 Mon Sep 17 00:00:00 2001 From: Lukas Prediger Date: Tue, 23 Jan 2024 20:35:37 +0200 Subject: [PATCH 5/6] Adding draftlog markdown file for "Improved hover detection for for scatter plot fill tonext* #6865" --- draftlogs/6865_fix.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 draftlogs/6865_fix.md diff --git a/draftlogs/6865_fix.md b/draftlogs/6865_fix.md new file mode 100644 index 00000000000..26ad467bdfb --- /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)] \ No newline at end of file From af0af9ca124deae46e1ca895385e95909b77931d Mon Sep 17 00:00:00 2001 From: Lukas Prediger Date: Sat, 27 Jan 2024 13:53:33 +0200 Subject: [PATCH 6/6] Mention contributor in draftlogs/6865_fix.md Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com> --- draftlogs/6865_fix.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/draftlogs/6865_fix.md b/draftlogs/6865_fix.md index 26ad467bdfb..10bed88c463 100644 --- a/draftlogs/6865_fix.md +++ b/draftlogs/6865_fix.md @@ -1 +1 @@ -- Improved hover detection for for scatter plot fill tonext* [[#6865](https://github.com/plotly/plotly.js/pull/6865)] \ No newline at end of file +- 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