Avoid overlap of point and axis hover labels

[dagroe]

This is an attempt at resolving issue #3973

When hover info extends below the lower edge of the plot it may overlap the hover axis info, as illustrated in this codepen: https://codepen.io/dagroe/pen/BaPmNXr

The goal of this MR is to move the hover info up to always be above the lower edge and thus above the axis hover info.

[dagroe]

Before I fix the tests, @archmoj could you have a look if this is a possible solution? 😄

[archmoj]

Thanks for the PR.
Could you fix the error in the codepen please?

[dagroe]

Sorry about that. I fixed it.

[archmoj]

Thanks and here is a codepen showing the proposed behavior.
Looks good to my eyes.
@alexcjohnson what do you think?

[dagroe]

I updated my branch to cover the cases of hovermode: 'y' and different sides.

I looked into the failing tests. This fix results in less space for hover labels, thus in many cases fewer labels are displayed, which makes the test fail.
To me this seems to be a larger caveat, especially since in some cases there is no overlap and the entire space could be used, e.g. here:

So my question is if that is a sacrifice we are willing to make or whether we should look into different approaches, e.g. actually checking for overlap with the axis label or moving the axis label to the side.

[alexcjohnson]

That sacrifice is fine IMO - a common label as short as 1 like in that image is itself an edge case, in most cases there will be overlap. But it looks like all of those labels got pushed too far up, the lowest one could go right on the axis line.

[dagroe]

I have adapted the approach to check for overlap of the hover label's background rect with the axis label. With that all existing tests pass.

I have added one test to test the overlap functionality.

But it looks like all of those labels got pushed too far up, the lowest one could go right on the axis line.

It seems the algorithm layouts all labels and then only removes those that overlap the boundaries. It marks points as deleted while iterating but does not remove those points from the positioning process. I'd rather not touch this as part of this PR.

[dagroe]

Unfortunately I cannot reproduce the failing test locally.

[dagroe]

@archmoj Could you help me with the one failing test? Locally the test passes fine and I don't see an overlap of labels or why there would be one. Other than this test, this PR is ready from my side.

[archmoj]

@archmoj Could you help me with the one failing test? Locally the test passes fine and I don't see an overlap of labels or why there would be one. Other than this test, this PR is ready from my side.

The timezone test is to ensure we produce correct hover in different timezones:

plotly.js/.circleci/config.yml

Lines 52 to 68 in 70c49ce

	timezone-jasmine:
	docker:
	# need '-browsers' version to test in real (xvfb-wrapped) browsers
	- image: cimg/node:16.17.1-browsers
	working_directory: ~/plotly.js
	steps:
	- browser-tools/install-browser-tools: &browser-versions
	chrome-version: 93.0.4577.63
	install-firefox: false
	install-geckodriver: false
	- attach_workspace:
	at: ~/
	- run:
	name: Run hover_label test in UTC timezone
	environment:
	TZ: "UTC"
	command: date && npm run test-jasmine hover_label

Could you switch the timezone or a use docker to debug?

[dagroe]

Could you switch the timezone or a use docker to debug?

I can try. The same test also fails in no-gl-jasmine, so I thought timezone is not the issue.

[archmoj]

Could you switch the timezone or a use docker to debug?

I can try. The same test also fails in no-gl-jasmine, so I thought timezone is not the issue.

You are right. It's also no-gl-jasmine. It passes on my machine too.
So let's add noCI to the test i.e.

it('@noCI centered-aligned, should stack nicely upon each other', function(done) {

[dagroe]

Ok. I'll give docker a try on Monday. If it also passes there I'll add the @noCI.

[dagroe]

I got a docker container up based on CI config. The test also failed there.

I had to dig a bit but ultimately the positions and sizes on CI had minor differences to the local run. In one case this led to an overlap of less than 1 pixel between one hover label and axis label while the other labels didn't overlap. Thus the algorithm produced different layouts on CI and local runs.

I changed the code to ignore overlaps of less than 1 pixel.

So the approach is not perfect and we can still have some overlap in edge cases, e.g. when not all hover labels are stacked on top of each other, but I assume that is an edge case that we can live with.

[archmoj]

Thanks @dagroe. Now it looks very good to my eyes.
@alexcjohnson over to you 🙏

[dagroe]

@alexcjohnson would appreciate your review to hopefully see this in a release soon 😄

[alexcjohnson]

@dagroe nice touch continuing to allow the labels to show when the common label is very short ✨

I do notice a problem though, that doesn't seem to be present before this PR: sometimes labels in the middle now overlap each other:

Here's the graph in that screenshot:

Plotly.react(
    gd,
    [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20].map(v=>({x:[100,200,300],y:[v,v+1,v+2]})),
    {width:500,height:400,hovermode:'x'}
)

If all the y values are the same (as in your screenshots above) this problem doesn't occur.

Plotly.react(
    gd,
    [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20].map(v=>({x:[100,200,300],y:[1,2,3]})),
    {width:500,height:400,hovermode:'x'}
)

[alexcjohnson]

looks like in that particular case, the 2-digit labels slide behind the 1-digit labels 🤔

[dagroe]

Good catch @alexcjohnson. Seems I was actually using the wrong rect behind text2 instead of the actual path behind text. This caused different x position for 1-digit and 2-digit labels which resulted in two pointgroups instead of one. Apparently there is no overlap prevention between different pointgroups but I assume when they are built correctly then there should be no overlap.

I fixed it and added a test for the example you provided.

No idea what is happening with CI. @archmoj can you help with that?

[alexcjohnson]

hmph that's a weird CI error, but I get a similar failure when I try to pull your changes locally, looks like something went wrong at the git level 🙄

> git pull dagroe issue-3973-overlap-hover
remote: fatal: bad tree object 59144c5f100ab5bde6fbcd473e71f3a75fa060ec
remote: aborting due to possible repository corruption on the remote side.
fatal: protocol error: bad pack header

Usually we don't like to do this because it makes re-reviews harder, but maybe you could try to rebase locally, then force push? Just so git goes through the motions again and hopefully avoids whatever it broke here?

[dagroe]

That worked, thanks.

One test I added fails. Seems the intersection takes the entire axis-label width into account instead only the actual path behind the label. I just looked at the calculation to create the path, but this will require a bit of time to make sure it's correct in all cases, so I'll do it on Monday.

[alexcjohnson]

Behavior looks good to me now - I tried a few more cases like several separated groups that get pushed together:

Plotly.react(
    gd,
    [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20].map(v0=>{
        const v = v0 > 9 ? v0 + 100 : v0; return {x:[100,200,300],y:[v,v+1,v+2]}
    }),
    {width:500,height:400,hovermode:'x',margin:{t:0}}
)

Seems like the logic is solid now. Once the tests are passing I think this is good to go! 💃

[dagroe]

I implemented the changes mentioned above. The test that failed passes now as expected but one test marked as flaky fails. It passes locally. Maybe you could re-run that one pipeline if that makes sense?

[archmoj]

I implemented the changes mentioned above. The test that failed passes now as expected but one test marked as flaky fails. It passes locally. Maybe you could re-run that one pipeline if that makes sense?

All passed.
If possible, please create draftlog/6442_fix.md file as described here: https://github.com/plotly/plotly.js/blob/master/draftlogs/README.md
Thank you!

[dagroe]

All passed. If possible, please create draftlog/6442_fix.md file as described here: https://github.com/plotly/plotly.js/blob/master/draftlogs/README.md Thank you!

👍 Done

[aberres]

@alexcjohnson @archmoj Is this ready to be merged?