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

Improve normalization of charCode, fix FF not reporting Enter #1956

Merged
merged 1 commit into from
Jul 31, 2014
Merged

Improve normalization of charCode, fix FF not reporting Enter #1956

merged 1 commit into from
Jul 31, 2014

Conversation

syranide
Copy link
Contributor

#1955

Haven't tested it yet (it's late), I'll run it through some manual tests tomorrow.

@syranide
Copy link
Contributor Author

cc @zpao

@syranide
Copy link
Contributor Author

Pushed an update, can play around with it here http://dev.cetrez.com/jsx/2/indexx.html (KeyDown left, KeyPress right, type in the right-most input).

I decided that it would be better to target any malfunctioning browser and not just rely on FF behaving in a specific way. Any function keys (all below 32 except Enter) are ignored for KeyPress (AFAIK only FF does this incorrectly, but now they should all behave the same).

Also made a little precautionary change for IE8, AFAIK IE8 does not ever return 0 in keyCode, but if it would, "which" would have become null rather than 0.

@syranide syranide changed the title Restore enter key in onKeyPress for FireFox Restore enter key in onKeyPress for FireFox + minor IE8 precaution Jul 30, 2014
@syranide syranide changed the title Restore enter key in onKeyPress for FireFox + minor IE8 precaution Improve normalization of charCode, fix FF not reporting Enter Jul 30, 2014
@syranide
Copy link
Contributor Author

OK, there was a bunch of unexpected browser edge-case inconsistencies, but I think I've gotten all kinks out of charCode now. Reading the code it all seems to makes sense to me (i.e, no weird hacking). I'll ponder it a bit and run some final tests tomorrow, but this should be the definitive fix.


"use strict";

var invariant = require('invariant');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self, remove this.

@syranide
Copy link
Contributor Author

I think this should be it, it's hard to be absolutely certain, but as far as I'm able to discern it works as expected. As above, I think the code by itself makes sense so that's always a good sign.

Test link is updated: http://dev.cetrez.com/jsx/2/indexx.html

PS. I also removed an invariant from getEventKey, it seemed like a bad idea to have it there, and replaced it with the uninitialized key-value '' instead, which seems consistent with the standard. It should never occur, but this seems safer and simpler. I guess it could happen if you're dispatching custom non-standard keyboard events and I'd rather have it not throw an invariant then.

@zpao
Copy link
Member

zpao commented Jul 31, 2014

cc @yungsters @josh @salier


"use strict";

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe briefly mention that charCode returns the keyCode of printable characters (excluding Tab, including Enter).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm? I'm not sure if I fully understand, charCode is "character code" (which is why we can use String.fromCharCode) and keyCode is "virtual key code". All similarities between the two are circumstancial and (theoretically) implementation-defined AFAIK, but for our purposes it's rather safe to assume that keyCode = 13 represents charCode = 13 (since keyboards at least agree on enter key being 13 and we "must" assume that it's a keyboard typing).

I should however explain some of that there I agree :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added doc comment.

@yungsters
Copy link
Contributor

The code makes sense to me. Thanks for digging into this, @syranide!

@syranide
Copy link
Contributor Author

@yungsters Np, I break it, I fix it :)

*/
function getEventCharCode(nativeEvent) {
var charCode,
keyCode = nativeEvent.keyCode;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: var for each.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought "we" preferred this style, I hate it though so I'm all too happy to change it :)

Fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Nah, we don't like this style. It shouldn't exist in any of the other code we have up here (maybe some build scripts or tests but we're a bit lax there)

zpao added a commit that referenced this pull request Jul 31, 2014
Improve normalization of charCode, fix FF not reporting Enter
@zpao zpao merged commit 5d288de into facebook:master Jul 31, 2014
@syranide syranide deleted the ffenter branch July 31, 2014 22:12
@syranide syranide mentioned this pull request Sep 29, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants