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

Correct wrong element targeting on hover in shadow DOM #5256

Merged
merged 3 commits into from
Jan 5, 2021
Merged

Correct wrong element targeting on hover in shadow DOM #5256

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1dfc11b
fix: correct wrong element targeting on hover
dbluhm Nov 6, 2020
b523565
fix: eslint errors, add comment
dbluhm Nov 6, 2020
9ee5002
fix: hover event target IE fallback
dbluhm Nov 15, 2020
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
8 changes: 7 additions & 1 deletion src/components/fx/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,13 @@ function _hover(gd, evt, subplot, noHoverEvent) {
return;
}

var dbb = evt.target.getBoundingClientRect();
// Discover event target, traversing open shadow roots.
var target = evt.composedPath && evt.composedPath()[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you also investigated a possible workaround for IE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Please commit your suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

ping @dbluhm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

if(!target) {
// Fallback for browsers not supporting composedPath
target = evt.target;
}
var dbb = target.getBoundingClientRect();

xpx = evt.clientX - dbb.left;
ypx = evt.clientY - dbb.top;
Expand Down