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

fixed (workarounded) the tedious bug in FF 3.x about charCode in keyup/keydown events #282

Closed
wants to merge 4 commits into from

Conversation

thg2k
Copy link

@thg2k thg2k commented Mar 21, 2011

this shouldn't break any existing code, and fixes a problem in FF 3.x which really bugs me and my firebug while debugging website. a real must for me.

@rwaldron
Copy link
Member

Is there a ticket for this? http://bugs.jquery.com

@thg2k
Copy link
Author

thg2k commented Mar 22, 2011

i created it. #8595
it takes too much time guys :(

@thg2k thg2k closed this Mar 22, 2011
@thg2k thg2k reopened this Mar 22, 2011
@rwaldron
Copy link
Member

Commit does not confirm to jQuery Style Guide: http://docs.jquery.com/JQuery_Core_Style_Guidelines

@thg2k
Copy link
Author

thg2k commented Mar 23, 2011

omg what was I thinking when I did this? :)
OK i'm willing to conform, but I can see you are quite inconsistent with the spacing around parenthesis anyway. Here are a few examples from jQuery 1.5.1:

        if ( match && (match[1] || !context) ) {
            if ( deep && copy && ( jQuery.isPlainObject(copy) || (copyIsArray = jQuery.isArray(copy)) ) ) {
        exec = !pass && exec && jQuery.isFunction(value);
                object.promise().then( iCallback(lastIndex), deferred.reject );
        if ( context && context instanceof jQuery && !(context instanceof jQuerySubclass) ) {
    if ( (value && typeof value === "string") || value === undefined ) {
        if ( jQuery.isWindow( elem ) && ( elem !== window && !elem.frameElement ) ) {
        if ( (!special._default || special._default.call( elem, event ) === false) &&

as you can see, the inner parenthesis are sometimes spaced sometimes now. I'll resubmit my patch with spacing the way I understood are right from the guidelines, but they constrast with the examples above.

@thg2k
Copy link
Author

thg2k commented Mar 23, 2011

i'm not very familiar with github. I pushed to the same branch, and the pull requested got automagically updated. Is that good for you?

@rwaldron
Copy link
Member

I'll be sure to bring that up at the next jquery core devs meeting - in the meantime, you're STILL missing curly braces around the body of your if condition block

http://docs.jquery.com/JQuery_Core_Style_Guidelines#Blocks

@rwaldron
Copy link
Member

Also, strict equality ===

http://docs.jquery.com/JQuery_Core_Style_Guidelines#Equality

@rwaldron
Copy link
Member

As for the auto updating branch, yes - that part is correct

@thg2k
Copy link
Author

thg2k commented Mar 23, 2011

two lines three coding style bugs? you are hurting my ego. How about now?
the problem is that now they are 4 commits, i guess they would still be 4 commits once you merge, it's quite unneeded. I would like to merge them in one commit, but I don't know any easy way to do that.
If you suggest me an easy way to do that with git I will fix it, assuming that it's not as simple as copy/pasting three lines of code in a clean branch, something that would work programmatically.. thanks!

@rwaldron
Copy link
Member

I never intended to hurt anyone's ego... jQuery has to maintain a strict quality of code and that includes making it readable like it was written by one person. If i were you - I'd just paste the changes into a new branch. Aside from that - you provided no reduced test case in your ticket... I can't reproduce this issue, if you provided a test case it would make reviewing your patch significantly easier.

In case you weren't aware, we are all volunteering our time - so don't give me a hard time because you didn't thoroughly read the style guide or because you just assumed that we'd pull in any contributed code - tested or not. Which brings me to another point - is this issue testable? Has your patch been tested and passing in all supported browsers? have you bothered to read the bug patching guide?

http://docs.jquery.com/Tips_for_jQuery_Bug_Patching

@dmethvin
Copy link
Member

Sorry but this event-copy loop is the most time critical part of jQuery.event.fix and we wouldn't want to make it slower just to get rid of a spurious console warning. I don't think we want to add more logic to it. See http://forum.jquery.com/topic/fix-is-slow for more information. A pull request like this would need to come with performance tests on a real event object (not just a Javascript object).

@dmethvin dmethvin closed this Mar 30, 2011
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants