Fire unhover when dragging plot

[rreusser]

Fixes #5437.

This PR makes a small change to unhover behavior. My premise is that, toward hooking up external HTML/CSS hovers, plotly_hover and plotly_unhover should be enough to manage hover state.

The GIF below shows plotly hover along with HTML/CSS hover. plotly_unhover does not currently fire when you drag the plot, resulting in a stray hover.

The cause is this conditional, which is not passed the event from the onMove handler, hence evt.target is empty.

plotly.js/src/components/dragelement/unhover.js

Line 47 in af9d440

if(evt.target && oldhoverdata) {

plotly.js/src/components/dragelement/index.js

Lines 181 to 184 in af9d440

	if(dx || dy) {
	gd._dragged = true;
	dragElement.unhover(gd);
	}

One solution, without adding additional flags, is to check if the plot is being dragged and fire the event if so. Then, hover/unhover are sufficient to manage the state in this case.

(It could pass the event from onMove in order to accomplish this, but then plotly_beforehover is fired many times while dragging, which would require other flags and fixes to avoid)

/cc @plotly/plotly_js @jonmmease and probably also @nicolaskruchten @alexcjohnson

[rreusser]

Oh, and to be clear, gd._hoverdata is flushed so that with this fix, plotly_unhover is only fired once, when the drag begins.

[archmoj]

Thanks for the PR!
Please tag @plotly/plotly_js, @jonmmease and others in the pull description.

[alexcjohnson]

We do have an event e at that dragElement.unhover(gd) call - can we just pass it in? Consumers of this plotly event may want the base browser event.

[rreusser]

@alexcjohnson Yes, I can pass it in. Then I think I just need to invert the logic and prevent this from firing on every drag move event:

plotly.js/src/components/dragelement/unhover.js

Lines 36 to 40 in 997946b

	if(!evt) evt = {};
	if(evt.target &&
	Events.triggerHandler(gd, 'plotly_beforehover', evt) === false) {
	return;
	}

[rreusser]

Okay, sorry, had to dust this off and double-check it. I think it now passes the event through but uses gd._dragged to avoid plotly_beforehover being called when it doesn't apply. /cc @archmoj

[rreusser]

Debugging nontrivial/non-fluke test failure.

[rreusser]

Update: I rebuilt on circle-ci and am maybe >85% confident that this is not actually a real test failure related to the changes in this PR.

[rreusser]

@archmoj I don't currently have additional things to add to this PR. It looks like it did eventually pass all tests on circle-ci. Just let me know if there's anything else you'd like to have happen on this PR, otherwise I'll put the ball in your court to hit the button and merge, unless you prefer otherwise.

[archmoj]

@archmoj I don't currently have additional things to add to this PR. It looks like it did eventually pass all tests on circle-ci. Just let me know if there's anything else you'd like to have happen on this PR, otherwise I'll put the ball in your court to hit the button and merge, unless you prefer otherwise.

Overall LGTM. Let's me have a second look.
BTW we are missing a test to lock this down.
Could you please also open an issue to illustrate the bug we are fixing here?

[archmoj]

I was wondering if in addition to gd._dragged we should check for gd._editing here?

However the following codepen titled after appears to be correct when editing the title positioned at center.
Before vs After

So I think there is no need for that.

[rreusser]

Good point. I'll check that as well.

[rreusser]

@archmoj Pardon the slow turnaround, but I've added a test and confirmed that this change fixes it! I thought about _editing and, in particular based on your example, I don't immediately see that the behavior should change if editing, no?

[archmoj]

Nicely done.
💃