Bug Tracker

Modify

Ticket #11756 (closed bug: worksforme)

Opened 2 years ago

Last modified 2 years ago

event.currentTarget documentation inaccurate for delegated events

Reported by: jmm Owned by: jmm
Priority: undecided Milestone: None
Component: unfiled Version: 1.7.2
Keywords: Cc:
Blocking: Blocked by:

Description

Summary

The  event.currentTarget documentation doesn't seem to be quite accurate for delegated events. It says:

Description: The current DOM element within the event bubbling phase.

But it seems that for delegated events, currentTarget is hijacked to mean "the element that matches the selector" (the selector part of the .on( events [, selector] [, data] , handler(eventObject) ) signature). In other words, it simulates the currentTarget that would exist if the listener was directly bound to the element matching the selector.

I get the impression that has been the case since delegate() was added in 1.4.2, for example from ticket #10563

There's now event.delegateTarget, which is really the current element within the event bubbling phase, isn't it?

Testcase

 http://jsfiddle.net/uLvy8/

The situation I'm describing is illustrated by the following evaluating to true (in FF 7 Win, Chrome 18 Win) in a delegated handler:

event.currentTarget !== event.originalEvent.currentTarget &&

event.delegateTarget === event.originalEvent.currentTarget

Notes

My interpretation of this is:

  • A long time ago (1.4.2), a useful piece of data (the DOM element that matches the selector in a delegation ) was added to jQuery.Event, which was good.
  • The data was added by co-opting the existing DOM Event.currentTarget property, changing its meaning. I'm wondering if that was as good of an idea.
  • 1.7 added event.delegateTarget to provide (restore) access to the data that is lost as a result of currentTarget being co-opted to store other data.
  • Perhaps if this were being created from scratch today currentTarget and delegateTarget would be reversed, so that currentTarget corresponds to the element where the listener is registered (the actual current element within the bubbling phase) and delegateTarget corresponds to the element matching the selector.

I speculate that because, if there's going to be an additional property (delegateTarget) for these events to convey data above and beyond what the DOM includes, it seems to me like it would make sense to preserve the meaning of the existing property, currentTarget, and embed the new information in the new, non-standard property.

But, I imagine it's either intentional (?), or too late to change the meaning of currentTarget for delegated events anyway. In any case, it would be good for the currentTarget documentation and other relevant parts to accurately describe the situation.

As an aside, where the  `.on()` documentation says:

jQuery passes the handler an event object it can use to analyze and change the status of the event. This object is a *normalized subset* of data provided by the browser;

With the addition of things like delegateTarget (and in some cases other things, like currentTarget), it's not strictly true that the event object is a subset of data provided by the browser. I guess to be really accurate it could say something like "includes a *normalized subset* of data included in the  W3C DOM Event interface, as well as additional data provided by the browser and / or jQuery".

Change History

comment:1 Changed 2 years ago by dmethvin

  • Owner set to jmm
  • Status changed from new to pending

I think you've got it about right.

I think the reason event.currentTarget was munged was so that .live() and .bind() would be equivalent from the view of the event handler. Since we're trying to abstract the delegation perhaps that made sense. Really it hardly matters since I bet 98% of people use this instead of event.currentTarget.

If we were starting again then yes, it would make sense to leave currentTarget alone and make people look at delegateTarget. But given that several years have gone by we are likely to break someone's code. There isn't a lot of reason to do that.

For most people's view it's a normalized subset, the importance of saying subset there is that many people seem to expect every property of the native event object to be present in the jQuery one.

Given that information, what action do you think needs to be taken to close this ticket?

comment:2 Changed 2 years ago by jmm

  • Status changed from pending to new

I think the reason event.currentTarget was munged was so that .live() and .bind() would be equivalent from the view of the event handler.

Ok, thanks for the background.

Really it hardly matters since I bet 98% of people use this instead of event.currentTarget.

If we were starting again then yes, it would make sense to leave currentTarget alone and make people look at delegateTarget.

Perhaps most people do mostly use this. If it's that prevalent, then it does sound like there would have been little downside to requiring people to use delegateTarget in that case, unless it's considered essential that currentTarget === this when this is a DOM object.

I often write in an OO fashion and use $.proxy() or some other mechanism for making this in my listeners refer to my own objects. This is beside your point, just an aside about my personal usage.

In general, using this for this purpose seems like kind of a waste since the data it would refer to is accessible via the other variables (target, currentTarget, delegateTarget) and this can be put to a more useful purpose. Not that I expect that to change, I'm just saying.

The amount of this usage may decline if things like Backbone catch on, which encourages setting up event handling in a way that binds this to its view objects in the listeners.

But given that several years have gone by we are likely to break someone's code. There isn't a lot of reason to do that.

Yeah, I figured there's no way it can be changed now, unfortunately.

For most people's view it's a normalized subset, the importance of saying subset there is that many people seem to expect every property of the native event object to be present in the jQuery one.

Right. Well maybe it would make sense to just change it to "This object includes a normalized subset...". I guess it's a technicality though.

Given that information, what action do you think needs to be taken to close this ticket?

I think the event.currentTarget documentation should be updated to explain exactly what data it will contain in various situations (directly bound and delegated events) and clarify that it isn't necessarily the current element in the bubbling phase and not necessarily the same as the currentTarget value of the native event object. Also, explain that delegateTarget may contain what you might expect currentTarget to, and when it will.

I don't know what's involved in submitting a patch for the documentation or if I'll have time to figure it out, but if I wrote some new copy, might that be accepted and added to the docs by someone who already knows how?

I could submit a new ticket if I notice other areas of the documentation that could use clarification on this issue and this ticket is closed by then.

Thanks

comment:3 Changed 2 years ago by ajpiano

jmm - We've begun tracking documentation bugs in Github,  https://github.com/jquery/api.jquery.com/issues! You can file this there, and you can submit a patch by forking the repository and submitting a pull request on the docs for event.currentTarget ( https://github.com/jquery/api.jquery.com/blob/master/entries/event.currentTarget.xml). Right now those changes have to be rolled into our Wordpress install manually, but it will soon be automated. In the meantime, if you get the PR in, someone will be able to assist you with getting it in to the live docs. Thanks for your interest in helping out!

comment:4 Changed 2 years ago by jmm

Considering the following, is it definitely too late to swap the meaning of currentTarget and delegateTarget, restoring currentTarget to its standard meaning?:

  • on() is still relatively new.
  • jQuery's treatment of currentTarget has not even been documented up to now.
  • It's been speculated that a massive majority of people use this and don't even use currentTarget.

I'm suggesting:

  • Restore the standard meaning of currentTarget when using on().
  • Leave the existing treatment of currentTarget for older methods like live() and delegate().
  • Store the element that matches the selector in a delegation in delegateTarget. Or, if there's more peril in changing that than currentTarget, create a new property like virtualCurrentTarget / effectiveCurrentTarget / ?
  • Accurately document the treatment of currentTarget according to registration method / jQuery version.

comment:5 Changed 2 years ago by dmethvin

  • Status changed from new to pending

I just don't see any need to change any behavior or add new variables. This has not been a point of confusion. Are there situations where the current variables cannot provide the information you need?

comment:6 Changed 2 years ago by jmm

  • Status changed from pending to new

Are there situations where the current variables cannot provide the information you need?

No, the data is available with current variables in 1.7+ now that delegateTarget has been added. To me it just seems like an undesirable outcome that the new, non-standard property (delegateTarget) -- and not currentTarget -- is what corresponds to the standard currentTarget.

In hindsight it seems like the decision to use currentTarget like this maybe wasn't ideal, and perhaps there was a missed opportunity to straighten it out when on() and delegateTarget were introduced. If it's too late to straighten out now, that's that. If it's not too late, would it not make sense to seize the opportunity to get currentTarget back on track, mirroring the standard property of the same name, once and for all, and have delegateTarget contain the data that's overlaid on the DOM?

As I mentioned, the treatment of currentTarget, diverging from the DOM, has not actually been documented, at least not in the currentTarget entry.

This has not been a point of confusion.

I can't speak for anyone else, but it was a source of confusion for me when the documentation says currentTarget, which has the same name as a standard DOM property, is "the current DOM element within the event bubbling phase" and it's not really. But if 98% of people use this and not currentTarget, then I'm not too surprised it hasn't been on the radar.

comment:7 Changed 2 years ago by dmethvin

  • Status changed from new to closed
  • Resolution set to worksforme

I think we're good with what we currently have so I'll close this ticket. If you have specific wording suggestions for the docs you can post them here.

comment:8 Changed 2 years ago by jmm

I actually submitted a  pull request for a documentation patch and then closed it when I got the idea that maybe it's actually not too late to change the treatment of currentTarget (as I talked about in comment 4).

I can't help but think that if there is still enough leeway to make a change to this behavior, due to the newness of on() and lack of documentation of this treatment of currentTarget, then documenting it will be the last nail in the coffin of opportunity to change it.

If there's any chance that changing the behavior will be considered, then it wouldn't make sense to use my documentation patch. If there's zero chance it will be changed, as it seems you're saying, then I'll re-open the pull request.

comment:9 Changed 2 years ago by dmethvin

I should reference #10563 here, which indicates the compat impact of changing this is not hypothetical.

comment:10 Changed 2 years ago by jmm

Understood, which is why I was only floating the idea of changing it for listeners registered with on(). However, as a developer it's dangerous to rely on undocumented behavior. The current docs describe the standard meaning of currentTarget, not the actual treatment in jQuery:

The current DOM element within the event bubbling phase.

It makes no distinction between direct and delegated registrations.

If the issue is backward compatibility, that seems relevant to me. If it's intentional that currentTarget be coerced this way because on balance it's considered more useful than preserving the standard meaning, that's a different story.

Anyway, if the official position of jQuery is that it's not changing, the last paragraph of my previous post sums it up for me.

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.