Bug Tracker

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#13180 closed bug (fixed)

Delegated event not firing for click within SVG use element

Reported by: dankellett Owned by: gibson042
Priority: low Milestone: 1.9
Component: event Version: 1.8.1
Keywords: Cc: gibson042, timmywil
Blocked by: Blocking:


I have created a jsbin illustrating the potential bug. http://jsbin.com/welcome/71517/

The first js statement attaches an event using the 'live' functionality of the .on() method. This event does not fire when you click on the chart image, but will fire when you click on the SVG outside of the graph image. The event will write 'live click' to the console.

The second statement attaches an event using a regular direct event attachment to the svg container. This does work when you click on the chart and elsewhere in the SVG. The event will write 'on click' to the console.

I would expect that the two event handlers would fire no matter where you click within the SVG. The difference being that the 'live' type attachment would allow me to detach and reattach a new SVG target to the DOM without having to reattach the event handler.

NOTE: the jsbin is using version 1.8.1, if a later version is used, a different javascript error is thrown: "Object #<SVGElementInstance> has no method 'getAttribute"

Change History (12)

comment:1 Changed 4 years ago by dankellett

From my tests, this issue occurs in Chrome and IE9, but not FF. Windows Server 2008.

Regarding the guidelines to use jsfiddle; I attempted to use jsfiddle instead of jsbin, but I could not get the SVG to render in jsfiddle.

Last edited 4 years ago by dankellett (previous) (diff)

comment:2 Changed 4 years ago by dmethvin

Cc: gibson042 timmywil added
Component: unfiledevent
Milestone: None1.9
Priority: undecidedlow
Status: newopen

Thanks for the clear explanation and test case!

Delegated events have never worked on SVG elements since they have no DOM methods we can use. I opened a documentation ticket for that. https://github.com/jquery/api.jquery.com/issues/211

In 1.8+ jQuery throws an error on this case in Sizzle, but prior to that it silently ignores delegate clicks within SVG elements. I think we should do the latter, both for backcompat and because delegated event handlers have no way of protecting themselves against events that bubble up from within SVG compartments. @gibson042 or @timmywil?

comment:3 Changed 4 years ago by gibson042

Here's what's going on:

  1. For delegated events, jQuery.event.dispatch checks every element from the event target up to the delegate to see if one matches the delegation selector.
  2. When possible, that check takes a fast approach that passes a seed set to Sizzle
  3. Sizzle runs Expr.filter matchers against the seed, which in many cases (including this one) assume the availability of .getAttribute or other DOM methods
  4. However, the SVGElementInstance elements generated by SVG use elements have a rather limited interface which does not include any of those methods, so passing them to a matcher is almost guaranteed to throw.

If wontfix is not an acceptable resolution for the reasons you mentioned, we should do... something... in dispatch. The most user-friendly method would be similar to cur = cur.correspondingUseElement || cur within the cur = event.target loop. !cur.nodeType (since parentNode points to the wrong place anyway) is smaller, but would mean losing the event entirely.

Either way, there will be a (small) performance hit in this hot path from the additional property access on cur.

comment:4 Changed 4 years ago by dmethvin

Owner: set to dmethvin
Status: openassigned

Fixing the problem in .dispatch() does make the most sense. It would be a shame to mess up the matchers with this, since we really shouldn't be passing SVG elements to them in the first place. I'll look at this a bit.

comment:5 Changed 4 years ago by dmethvin

I think we can just ignore delegated events if the target is SVG and don't even do the bubbling; we'd still allow them on directly bound events as we always have. Since the matcher needs .getAttribute() we can use that as our marker; for oldIE's sake we have to use typeof. The only other place where I know we don't have .getAttribute() is that #12945 bug with Flash in IE9, and if Flash objects start bubbling DOM events out of their guts I don't want to live in this world anymore. That just adds one more condition to the "are there delegated events" check.

if ( delegateCount && typeof event.target.getAttribute !== "undefined" &&
    !(event.button && event.type === "click") ) {

comment:6 Changed 4 years ago by gibson042

Why not !event.target.nodeType? The interface is really restricted. Although now that I think about it, it might be possible to bubble up into an SVGElementInstance, which would necessitate dealing with them in the loop.

comment:7 Changed 4 years ago by gibson042

Upon further inspection, it appears that SVGElementInstances are never the parentNodes of anything else, which makes our task easier. As for the fix, we can either black-hole events inside of SVG <use> elements or jump into the real DOM with correspondingUseElement. The former seems unfairly harsh, but the latter a bit dirty.

In either case, it would be nice to abstract out delegate matching logic into another jQuery.event method so it can potentially be overridden by people who have strong opinions about the way we've addressed the issues.

comment:8 Changed 4 years ago by gibson042

Owner: changed from dmethvin to gibson042

comment:10 Changed 4 years ago by Richard Gibson

Resolution: fixed
Status: assignedclosed

Fix #13180: don't delegate into SVG <use>

Changeset: f860e0bd2f7dd228a14704d78ed5208cbe870d01

comment:11 Changed 4 years ago by Richard Gibson

comment:12 Changed 4 years ago by gibson042

Summary: Live binding click event on image within SVG not firing.Delegated event not firing for click within SVG use element
Note: See TracTickets for help on using tickets.