Skip to main content

Bug Tracker

Side navigation

#7930 closed bug (fixed)

Opened January 09, 2011 06:11PM UTC

Closed September 08, 2011 06:37PM UTC

Last modified November 22, 2011 12:29AM UTC

Methods on custom objects are being called when an event with the same name is triggered on it

Reported by: timmolendijk Owned by: dmethvin
Priority: low Milestone: 1.next
Component: event Version: 1.4.4
Keywords: Cc:
Blocked by: Blocking:
Description

I don't understand why this happens:

var customObject = { hello: function() { alert("Hello!"); } };
$(customObject).trigger('hello');  // An alert will pop up!
$(customObject).triggerHandler('hello');  // No alert will pop up.

Why is

customObject.hello
being called when an
hello
event is triggered on
customObject
? I wasn't expecting it and I cannot find any hint towards it in the docs. My guess is that it's an unintended side-effect of the new (explicit) support for using
jQuery.fn.trigger
on custom objects. Am I right?

Attachments (0)
Change History (8)

Changed January 10, 2011 03:27AM UTC by dmethvin comment:1

owner: → dmethvin
status: newassigned

It's meant to handle the situation where an event named X is triggered on a DOM element and there's a corresponding X method on the element. Events like change, focus, and blur for example.

So yes, in those cases it will call the correspondingly named function on the plain object if it exists. Actually it will try to call the property regardless of whether it's a function or not, because it's impossible to tell with IE.

Sounds like we should put a check into jQuery.event.trigger to see if the element is a DOM element, before someone decides this is a feature.

Changed January 10, 2011 02:54PM UTC by jitter comment:2

_comment0: I already assessed this one yesterday but didn't come around to add my findings. Sorry Dave. I basically figured out the same thing as you (code branch in trigger() is only meant for DOM element nodes to use the browsers native event method if applicable) \ \ This also is straightforward to fix imo. We could just change [https://github.com/jquery/jquery/blob/c5c1f18adb4dfae5366eafada12ed02fe1b7064d/src/event.js#L388 this line] to \ {{{ \ !isClick && target && jQuery.acceptData( target ) ) { \ }}} \ This is better readable, uses acceptData instead repeating the very same thing in the conditional. It also has the nice side effect of fixing a potential other bug: that this branch gets run even when target is e.g. undefined. \ \ So when you tackle this I guess you could do some refactoring and save us a few bytes, as there are at least two other places where a similar pattern (along `elem && elem.nodeName && jQuery.noData[elem.nodeName.toLowerCase()]`) is used which (I guess) can be switch to using acceptData() too. (Maybe even incorporate the check if elem is defined/undefined into the acceptData too?) \ \ Replying to [comment:1 dmethvin]: \ > Sounds like we should put a check into jQuery.event.trigger to see if the element is a DOM element, before someone decides this is a feature. \ This "faulty" undocumented behavior is in jQuery since 1.1 (!!!!) as you can see with this [http://jsfiddle.net/jitter/DEkqZ/ live test case]. So by now there surely are some people relying on this hidden "gem" but this should get fixed anyway.1294671478228393
_comment1: I already assessed this one yesterday but didn't come around to add my findings. Sorry Dave. I basically figured out the same thing as you (code branch in trigger() is only meant for DOM element nodes to use the browsers native event method if applicable) \ \ This also is straightforward to fix imo. It should be fine to just change [https://github.com/jquery/jquery/blob/c5c1f18adb4dfae5366eafada12ed02fe1b7064d/src/event.js#L388 this line] to \ {{{ \ !isClick && target && jQuery.acceptData( target ) ) { \ }}} \ This is better readable, uses acceptData instead repeating the very same thing in the conditional. It also has the nice side effect of fixing a potential other bug: that this branch gets run even when target is e.g. undefined. \ \ So when you tackle this I guess you could do some refactoring and save us a few bytes, as there are at least two other places where a similar pattern (along `elem && elem.nodeName && jQuery.noData[elem.nodeName.toLowerCase()]`) is used which (I guess) can be switch to using acceptData() too. (Maybe even incorporate the check if elem is defined/undefined into the acceptData too?) \ \ Replying to [comment:1 dmethvin]: \ > Sounds like we should put a check into jQuery.event.trigger to see if the element is a DOM element, before someone decides this is a feature. \ This "faulty" undocumented behavior is in jQuery since 1.1 (!!!!) as you can see with this [http://jsfiddle.net/jitter/DEkqZ/ live test case]. So by now there surely are some people relying on this hidden "gem" but this should get fixed anyway.1294671578886296
component: unfiledevent
priority: undecidedlow

I already assessed this one yesterday but didn't come around to add my findings. Sorry Dave. I basically figured out the same thing as you (code branch in trigger() is only meant for DOM element nodes to use the browsers native event method if applicable)

This also is straightforward to fix imo. It should be fine to just change this line to

!isClick && target && jQuery.acceptData( target ) ) {

This is better readable, uses acceptData instead of repeating the very same thing in the conditional. It also has the nice side effect of fixing a potential other bug: that this branch gets run even when target is e.g. undefined.

So when you tackle this I guess you could do some refactoring and save us a few bytes, as there are at least two other places where a similar pattern (along elem && elem.nodeName && jQuery.noData[elem.nodeName.toLowerCase()]) is used. These can probably be switched to use acceptData() too. Maybe we even can incorporate the check if elem is defined/undefined into acceptData() too?

Replying to [comment:1 dmethvin]:

Sounds like we should put a check into jQuery.event.trigger to see if the element is a DOM element, before someone decides this is a feature.

This "faulty" undocumented behavior is in jQuery since 1.1 (!!!!) as you can see with this live test case. So by now there surely are some people relying on this hidden "gem" but this should get fixed anyway.

Changed March 14, 2011 01:54PM UTC by scottgonzalez comment:3

Whether or not this is changed should be defined by #7818.

Changed May 07, 2011 04:22AM UTC by rwaldron comment:4

#9146 is a duplicate of this ticket.

Changed July 27, 2011 02:34PM UTC by rwaldron comment:5

#7930 is a duplicate of this ticket.

Changed September 08, 2011 06:37PM UTC by dmethvin comment:6

resolution: → fixed
status: assignedclosed

This behavior has been institutionalized and will be documented; see #7818 for the draft documentation about what is valid to call on a jQuery-wrapped plain object. Per the fix for #6170 this behavior is not allowed for window objects to avoid calling globals.

Changed November 10, 2011 03:40PM UTC by rwaldron comment:7

#10747 is a duplicate of this ticket.

Changed November 22, 2011 12:29AM UTC by dmethvin comment:8

#10837 is a duplicate of this ticket.