Skip to content
Permalink
Browse files
Adding in the change recommended by Alexander Farkas to fix some bugs… 
… in the change delegation logic. Fixes #5851.
  • Loading branch information
@jeresig
jeresig committed Jan 18, 2010
1 parent 257a469 commit 435772e29b4ac4ccfdefbc4045d43f714e153381
Showing 1 changed file with 2 additions and 2 deletions.
4 src/event.js
View file
@@ -670,9 +670,9 @@ function testChange( e ) {
jQuery.data( elem, "_change_data", val );
}

if ( elem.type !== "select" && (data != null || val) ) {
if ( data != null || val ) {
e.type = "change";
return jQuery.event.trigger( e, arguments[1], this );
return jQuery.event.trigger( e, arguments[1], elem );
}
}

8 comments on commit 435772e

@GarrettS
Copy link

@GarrettS GarrettS commented on 435772e Feb 9, 2010

Choose a reason for hiding this comment

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

This code should result in SyntaxError. A FunctionDeclaration cannot appear in a Block; a Block can have only Statements.

FunctionDeclataion is not a Statement and so is not allowed there. Implementations behave differently when encountering a FunctionDeclaration where a only Statement is allowed. Spidermonkey has FunctionStatement extension. Indeed this is fairly common knowledge on comp.lang.javascript.

The code is long and indented inconsistently with tabs (either zero, one, or two). It is painful to read (or magic, depending on the reader). Instead, the code should be brief, should be properly indented using spaces, and should use meaningful identifiers.

@GarrettS
Copy link

@GarrettS GarrettS commented on 435772e Feb 9, 2010

Choose a reason for hiding this comment

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

In case it was not clear, the LOC I refer to is:
http://github.com/jquery/jquery/blob/master/src/event.js#L688

Where you have



if ( !jQuery.support.changeBubbles ) {


var formElems = /textarea|input|select/i;


// (GS) FunctionDeclaration not allowed here.

function getVal( elem ) {

Functions getVal and testChange can only appear there as FunctionExpression, not as FunctionDeclaration, so

var getVal = function( elem )

is syntactially valid there.

@jeresig
Copy link
Member Author

@jeresig jeresig commented on 435772e Feb 9, 2010

Choose a reason for hiding this comment

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

I'm confused - why is this a comment on a completely unrelated commit and not in the jQuery forum (where we discuss things like this)? I'll allow this this once, in the future please hold discussions over there.

As to the tab formatting - it looks like it may have gotten malformed in one of the rewrites of the logic, I've adjusted it in commit 5267824.

As to your statement: "This code should result in SyntaxError." - obviously there is a difference between the specification and what actually happens in practice (namely that no browser that we support throws a SyntaxError on the above code). Regardless, there wasn't much need for those functions to actually be declarations within that context so I've adjusted it in the aforementioned commit.

@GarrettS
Copy link

@GarrettS GarrettS commented on 435772e Feb 9, 2010

Choose a reason for hiding this comment

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

That doesn't sound much like "thanks" to me.

@jdalton
Copy link
Member

@jdalton jdalton commented on 435772e Feb 9, 2010

Choose a reason for hiding this comment

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

John,

You will allow this, oh my.

It seems Garrett at least tried to help and provided a line number to boot. He is absolutely correct about function declarations in blocks, for additional reading check out kangax's excellent post on named-function-expressions. There is nothing stopping the next version of a supported browser from throwing an error (who knows?). Why take the chance.

In fact, ECMA 5th edition states on page 86 section 12:

NOTE: Several widely used implementations of ECMAScript are
known to support the use of FunctionDeclaration as a
Statement. However there are significant and
irreconcilable variations among the implementations in the
semantics applied to such FunctionDeclarations. Because of
these irreconcilable difference, the use of a
FunctionDeclaration as a Statement results in code that is
not reliably portable among implementations. It is
recommended that ECMAScript implementations either
disallow this usage of FunctionDeclaration or issue a
warning when such a usage is encountered. Future editions
of ECMAScript may define alternative portable means for
declaring functions in a Statement context.

I don't think its cool to lash out at a dev for trying to help just because he drops the comp.lang.javascript tag (my interpretation). Garrett is a great dev, I have learned plenty from him. Who knows you might too.

@GarrettS
Copy link

@GarrettS GarrettS commented on 435772e Feb 9, 2010

Choose a reason for hiding this comment

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

Kangax' Named Function Expressions article is excellent. The relevant part on FunctionStatement explains it well in gory details:
http://yura.thinkweb2.com/named-function-expressions/#function-statements

Although no known browser throws a SyntaxError, browsers do interpretat FunctionDeclaration in where only Statement is allowed differently. This makes it a bad idea. IIRC PrototypeJS had that mistake a few years back with each (or was it _each?).

Again, the problem with using FunctionDeclaration as a Statement is that some implementations will interpret the FunctionDeclaration and add it to the scope immediately upon entering the execution context. Others may interpret a FunctionStatement, but only when executing that statement. The way the scope is affected varies between browsers and versions.

OTOH, comments on my commits are welcome (even for something that I did that was dumb).

@jeresig
Copy link
Member Author

@jeresig jeresig commented on 435772e Feb 9, 2010

Choose a reason for hiding this comment

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

@garrett: Perhaps you missed my thanks in the commit message? Definitely appreciate the input.

@jdalton: When you get the volume of questions, support requests, possible bug reports, and other miscellany that I do related to jQuery it significantly helps to have everyone use the appropriate resources for communication. For example, I completely ignore bug reports sent via Twitter at this point. They're simply too short to contain meaning and trying to extracting additional information is akin to teeth-pulling. It's pretty simple: If you think you've found an issue but you aren't sure about and would like to discuss it further: Bring it to the forum. If you've absolutely found an issue: File it in the bug tracker. This way all discussion related to a particular issue can be contained and easily referenced. But see this discussion: It's in this completely unrelated commit - how am I going to be able to find it again and figure out how or why this discussion came about? or how about when I go back to look at the changes from ticket #5851 and see this non-relevant discussion taking place? We provide organized communication tools for a reason - it makes our jobs somewhat sane and it makes it easier for you, as well.

@garrett: "Again, the problem with using FunctionDeclaration as a Statement is that some implementations will interpret the FunctionDeclaration and add it to the scope immediately upon entering the execution context." Absolutely - I remember running across a similar issue a while back that was related to that. That is why, in this particular case, these functions weren't competing with any other function declarations.

You may have misinterpreted my request: Comments on commits are absolutely welcome - but you didn't comment on the changes related to this commit, you were just using this mechanism to communicate. The forum (or even better, the bug tracker) would've been a much better place for this discussion.

@GarrettS
Copy link

@GarrettS GarrettS commented on 435772e Feb 10, 2010

Choose a reason for hiding this comment

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

Yes I did miss that but see it now.

I initially wanted to file a bug but did not see the "issues" tab for this project. Not sure why; I see it for Prototype but not jQuery.
http://github.com/sstephenson/prototype/issues

When I click on the link you linked for "#5581", I get a 404 page at the URL: http://github.com/jquery/jquery/issues/#issue/5851

This code I commented on here is right inside that conditional. It's the closest match I found. I'm sure there is a much older commit that has the exact LOC but I didn't search way back.

Yes I did find it ironic that the SyntaxError sat there for so long but I did try and file an issue.

As far as forums go, well, I am here on Github for "social coding". I really like to commit comments. I have gotten good comments from kangax and a few others lately (D Perini). I don't like forum censorship and thread locking. I much prefer Github where I can give and receive code comments about various projects.

Please sign in to comment.