Correct wrong element targeting on hover in shadow DOM

[dbluhm]

When using plotly inside of a shadow DOM, the calculations used to determine if the pointer has moved off of the graph would use the wrong element to determine the bounding box. This was resulting in a flicker in the hover text as it would find that the mouse pointer had apparently moved off the graph when it actually hadn't and the next mousemove event would immediately redraw the hover text.

This PR uses the Event.composedPath() method of the Event interface to determine the correct element from which to base bounding box calculations.

In my limited testing, I found that occasionally a mousemove event originating from a shadow DOM would get raised that did not have a properly formed composedPath() -- it would return an empty list. I haven't been able to nail down exactly why but for those rare cases, the early return ensures the hover text doesn't flicker.

I'm pretty new to Plotly but I tried to do my research so hopefully this solution isn't too naive. More than happy to work with maintainers to get this to a point where it can be accepted.

Thanks and excellent work on Plotly!

[archmoj]

Thanks very much for the PR.
Is there an open issue related to this problem?
If not, please open an issue including a codepen to illustrate the bug while using the last (not latest) plotly.js version similar to this codepen.

Then you may also reuse that to add a test to lock down the bug here:
https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/hover_label_test.js

[archmoj]

Have you also investigated a possible workaround for IE?

[dbluhm]

I haven't -- I'll do so and update with my findings.

[dbluhm]

After some investigation, I think the best we can do in the case of IE is simply fallback to the original behavior of directly using the event target.

Suggested change

	var target = evt.composedPath && evt.composedPath()[0];
	var target = evt.composedPath && evt.composedPath()[0];
	if(!target) {
	target = evt.target;
	}

[archmoj]

Thanks. Please commit your suggestion.

[archmoj]

Or simply

var target = (evt.composedPath && evt.composedPath()[0]) || evt.target;

@dbluhm in addition to hover (fx), there are some other places e.g. dragelement, shapes and mapbox which evt.target is used. Could you please investigate if there is a issue that similar fix may help?

[archmoj]

ping @dbluhm

[dbluhm]

Sure, I'll take a look! Apologies for the slow response -- working to address this issue just as time arises. Thanks for your patience!

[alexcjohnson]

Thanks @dbluhm - this looks like a great solution! My hunch is that in order to generalize this efficiently to all the places we use event.target it will warrant a function like Lib.getTarget(event), perhaps in lib/dom (and note that then this needs to be exposed in lib/index)

[dbluhm]

I don't believe there is an open issue for this currently; I'll be sure to open one and include a codepen. I'll also work on getting a test for it in hover_label_test.js. Thanks for the prompt feedback!

[dbluhm]

Opened #5277 to track the issue solved by this PR.

[archmoj]

@dbluhm to avoid conflicts namely with mapbox hover please merge upstream/master into this branch before generalizing this logic to few other places.

[dbluhm]

As a brief update, this PR has been moved to my backlog due to some changes in my development priorities associated with an employer change. I'm still hoping to resolve these issues and get this merged but my timeline is less certain now. My primary obstacle at the moment is lack of familiarity with the testing framework (I don't spend too much time in JavaScript) so I've struggled to recreate the conditions needed to exhibit this behavior in tests with my relatively limited time. Still open to feedback and any direction you'd like to give 🙂

[archmoj]

Thanks @dbluhm for the contribution.
💃

Note: I'll work on the remaining items in a following PR.