Skip to content
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

fix: do not ovelap hoverlabels for different tracetypes #6954

Merged
merged 6 commits into from
May 30, 2024
Merged

fix: do not ovelap hoverlabels for different tracetypes #6954

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8d1a49e
fix: do not ovelap hoverlabels for different tracetypes
mbant Apr 10, 2024
74dc83d
chore: draftlog
mbant Apr 10, 2024
6cfc094
fix: var instead of let
mbant Apr 10, 2024
33b28ec
fix: fix eslint mismatch
mbant Apr 10, 2024
4321451
fix: simplify label grouping condition
mbant May 30, 2024
0631d43
Merge branch 'plotly:master' into issue-6917
mbant May 30, 2024
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
1 change: 1 addition & 0 deletions draftlogs/6954_fix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- fix for excessive hoverlabel removal and overlap for plots with both `scatter` and `bar` traces which reported in [#6917](https://github.com/plotly/plotly.js/issues/6917).
2 changes: 1 addition & 1 deletion src/components/fx/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1757,7 +1757,7 @@ function hoverAvoidOverlaps(hoverLabels, rotateLabels, fullLayout, commonLabelBo
topOverlap = p0.pos + p0.dp + p0.size - p1.pos - p1.dp + p1.size;

// Only group points that lie on the same axes
if(topOverlap > 0.01 && (p0.pmin === p1.pmin) && (p0.pmax === p1.pmax)) {
if(topOverlap > 0.01 && p0.crossAxKey === p1.crossAxKey) {
Copy link
Collaborator

@alexcjohnson alexcjohnson May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand this actually - I only see us using crossAxKey as a variable name, not a key in these point objects.

Given that this change fixes the behavior, you've clearly identified that this condition is the one causing this bug, but AFAICT the change is equivalent to just if(topOverlap > 0.01) - and maybe that's right? Maybe the pmin/pmax conditions are no longer useful (which presumably means the comment Only group points that lie on the same axes is also no longer what we want) but that's the question to answer before we move forward with this.

@archmoj I've forgotten the details of this, and anyway it changed recently with the hoversubplots work, but if you can convince yourself that removing this condition entirely is correct in all the cases we care about (in addition to the case being tested here, I'd focus on overlaid subplots with points at the same position on screen but either different x axis, different y axis, or both) then go for it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100% agreed on the condition. It's been a while so not sure why that was there ...

I pushed a commit to simplify the condition (and thus removing the comment) if that helps with testing / convincing; I will have time to pick this up again in the following days so @archmoj let me know if I can help with anything!

// push the new point(s) added to this group out of the way
for(j = g1.length - 1; j >= 0; j--) g1[j].dp += topOverlap;

Expand Down
57 changes: 57 additions & 0 deletions test/jasmine/tests/hover_label_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1668,6 +1668,10 @@ describe('hover info', function() {
return Math.max(0, overlap);
}

function labelCount() {
return d3Select(gd).selectAll('g.hovertext').size();
}

it('centered-aligned, should render labels inside boxes', function(done) {
var trace1 = {
x: ['giraffes'],
Expand Down Expand Up @@ -1787,6 +1791,59 @@ describe('hover info', function() {
})
.then(done, done.fail);
});

it('does not overlap lebels for different trace types', function(done) {
function trace(name, type, delta) {
return {
name: name,
type: type,
y: [0 + delta, 1 + delta, 2 + delta],
x: ['CAT 1', 'CAT 2', 'CAT 3'],
};
}

var scatterName = 'scatter_';
var barName = 'bar_';
var data = [];
var i;
for(i = 0; i < 3; i++) {
data.push(trace(barName + i, 'bar', 0.0));
data.push(trace(scatterName + i, 'scatter', 0.1));
}
var layout = {
width: 600,
height: 400,
hovermode: 'x',
};

Plotly.newPlot(gd, data, layout)
.then(function() {
_hoverNatural(gd, 200, 200);
})
.then(function() {
expect(labelCount()).toBe(6);
})
.then(function() {
var nodes = [];
for(i = 0; i < 3; i++) {
nodes.push(hoverInfoNodes(barName + i).secondaryBox.getBoundingClientRect());
nodes.push(hoverInfoNodes(scatterName + i).secondaryBox.getBoundingClientRect());
}
nodes.sort(function(a, b) { return a.top - b.top; });

for(i = 0; i < 5; i++) {
expect(
calcLineOverlap(
nodes[i].top,
nodes[i].bottom,
nodes[i + 1].top,
nodes[i + 1].bottom
)
).toBeWithin(2, 1);
}
})
.then(done, done.fail);
});
});

describe('constraints info graph viewport', function() {
Expand Down