Fixup hover on period positioned points

[archmoj]

Closes #5553.

demo

TODOs:

• add jasmine test
• test traces other than bars

@plotly/plotly_js

[nicolaskruchten]

This does seem like an improvement, yes!

[alexcjohnson]

If I give the scatter trace in your demo a period of its own, we again get multiple labels for that one trace.

https://codepen.io/alexcjohnson/pen/NWpWOdQ?editors=0010

[nicolaskruchten]

I think there's another invariant here that we could maybe implement fairly easily, which should be implicit but let's make explicit:

• there should never be more than one point from the same trace in the same hoverset

no? Seems like a straightforward filter operation?

[alexcjohnson]

Seems like a straightforward filter operation?

Yes, though there may be cases where we lose track of which is the "best" point by the time we're ready to do this filter.

Fixes like this PR may be the best route in the short term, but I think all of this does point to a need eventually to take a step back and reconceptualize our hover/spike picking framework in order to satisfy the invariants @nicolaskruchten has stated, among others that are already assumed, at an architectural level rather than by layering on more tweaks.

[archmoj]

If I give the scatter trace in your demo a period of its own, we again get multiple labels for that one trace.

https://codepen.io/alexcjohnson/pen/NWpWOdQ?editors=0010

Good catch. Fixed by 543698a.

[archmoj]

I think there's another invariant here that we could maybe implement fairly easily, which should be implicit but let's make explicit:

• there should never be more than one point from the same trace in the same hoverset

no? Seems like a straightforward filter operation?

Noting that some trace types produce multiple hover points; so we cannot perform that in general.
But in the case of this particular problem noticed above (i.e. when a period positioned point wins) such an operation is helpful and was missing.
See: 543698a

[nicolaskruchten]

@alexcjohnson can we please get this reviewed and landed, ideally this week? :)

[alexcjohnson]

Clear improvement. There are three funny cases I want to point out, though we may choose not to do anything about them:

(1) the only place I can put the mouse and NOT get a label for any bar is slightly to the left of a first-of-month scatter point:

If I put the mouse to the right of this point I do get a bar hover:

This is presumably because the mouse is not over the range of the bar that matches the winning scatter point - so we include the previous bar in our original set of hovered points but then disqualify it because it's inconsistent with the scatter point.
This happens at all month boundaries in x mode, but if I switch to x unified mode, I see this behavior on the Dec/Jan boundary:

but not on the Jan/Feb boundary, there I always get the Feb bar no matter where my mouse is:

(2) When I zoom in enough, there are gaps between scatter points so that none of them are hovered, and only a bar is shown in the hover info. But if I set xperiod to 5 days on the scatter trace, I would have expected that no matter how much I zoom in there would be a winning scatter point, because I'm always over the period of one of them. That doesn't happen, in either x or x unified hovermode. In both modes there's still a gap where I only see the bar's hover.

(3) Except in a very narrow set of conditions, when no matter which scatter gap my mouse is in, the middle scatter point is included in hover data with the bar. I only see this with an xperiod on the scatter and in x hovermode - not with instant-positioned scatter points and not with x unified, and at least in the codepen given there's a very narrow zoom range where this happens, immediately after gaps in the scatter have opened up but before they get too big. Here my mouse is on about Jan 27:

(This third point may become moot if we choose to address the second point)

[archmoj]

Clear improvement. There are three funny cases I want to point out, though we may choose not to do anything about them:

@alexcjohnson thanks very much for the review.
I suggest we attempt addressing those edge cases in separate PRs to have this improvement as the baseline, also to unblock other development/testing around hover.

[nicolaskruchten]

Thanks for the dive-in Alex! I'll take a close look at them early next week but I don't think any of them break the invariants in #5553 which this PR is meant to enforce, so I think I agree with Mojtaba that if the implementation looks good we should merge this PR.

[alexcjohnson]

Works for me. Looks good! 💃