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.

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

Fix #4963 - SVG <use> swallows click events #5720

Merged
merged 1 commit into from Feb 17, 2016

Conversation

Projects
None yet
7 participants
@edmellum
Copy link
Contributor

commented Dec 23, 2015

Info about the issue in #4963

This fix uses the corresponding <use> element as the target of a click handler instead of the <svg> it refers to, thereby normalizing the behavior of IE, Edge and iOS to the behavior of Chrome. Without this fix there is no way to handle click events on <use> elements in the aforementioned browsers as their target becomes the referred to <svg> which may be far outside of the attached click handler causing it to be dropped.

@wnr

This comment has been minimized.

Copy link

commented Feb 12, 2016

This is a bug that we are experiencing in our team as well. I have verified that your solution works. I hope that this will be merged asap.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2016

Looks legit to me, cc @spicyj for sanity check.

@@ -20,6 +20,12 @@
*/
function getEventTarget(nativeEvent) {
var target = nativeEvent.target || nativeEvent.srcElement || window;

// Normalize SVG <use> element events #4963
if (target.correspondingUseElement && !target.nodeType) {

This comment has been minimized.

Copy link
@sophiebits

sophiebits Feb 12, 2016

Collaborator

What is target such that it doesn't have a nodeType?

This comment has been minimized.

Copy link
@wnr

wnr Feb 14, 2016

It seems to me like the !target.nodeType doesn't to anything really. If the target has a correspondingUseElement property, the target must be an instance of SVGElementInstance and lives in the shadow DOM. Such elements do not have a nodeType property, and therefore it will always be falsy. Perhaps the check is present for some weird browser bugs? @edmellum

https://www.w3.org/TR/SVG/single-page.html#struct-__svg__SVGElementInstance__correspondingUseElement

This comment has been minimized.

Copy link
@edmellum

edmellum Feb 17, 2016

Author Contributor

I've looked over and retested everything. !target.nodeType is a relic from my exploration during early implementation that only partially solved the issues on IE, whereas correspondingUseElement properly solves it. I'll remove it and rebase onto the current commit so there's no noise to merge.

@edmellum edmellum force-pushed the edmellum:master branch from 39b62a9 to 2fdaba4 Feb 17, 2016

@edmellum

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2016

As mentioned in the line notes I've fixed the !target.nodeType issue, looked over and retested everything.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Feb 17, 2016

@edmellum updated the pull request.

jimfb added a commit that referenced this pull request Feb 17, 2016

Merge pull request #5720 from edmellum/master
Fix #4963 - SVG <use> swallows click events

@jimfb jimfb merged commit c9e0fc7 into facebook:master Feb 17, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jimfb

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2016

Thanks @edmellum!

@wnr

This comment has been minimized.

Copy link

commented Feb 17, 2016

👍

@ghost

This comment has been minimized.

Copy link

commented Feb 17, 2016

I don't understand this crap
On Feb 12, 2016 11:36 AM, "Lucas Wiener" notifications@github.com wrote:

This is a bug that we are experiencing in our team as well. I have
verified that your solution works. I hope that this will be merged asap.


Reply to this email directly or view it on GitHub
#5720 (comment).

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2016

@shastings86 care to elaborate? Also, please watch your attitude; at first glance, your comment appears to be very negative and not constructive - which is not the type of community we want to foster.

@ghost

This comment has been minimized.

Copy link

commented Feb 18, 2016

I can't read it

On February 17, 2016, at 12:37 PM, Jim notifications@github.com wrote:

@shastings86 care to elaborate? Also, please watch your attitude; at first glance, your comment appears to be very negative and not constructive - which is not the type of community we want to foster.


Reply to this email directly or view it on GitHub.

@justo

This comment has been minimized.

Copy link

commented Feb 21, 2017

Thank you for this fix! It was a strange bug that took us a while to find.

@justo justo unassigned jimfb Feb 21, 2017

@aweary

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

@justo just curious, but did you actually unassign @jimfb ^?

@justo

This comment has been minimized.

Copy link

commented Feb 21, 2017

I have no idea, as soon as I posted the comment that showed up...

@aweary

This comment has been minimized.

Copy link
Member

commented Feb 21, 2017

@justo thanks, I've seen it a couple other times recently. It looks like its potentially a bug with Github.

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v0.14.9 #29

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.