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

Also handle lower case ASCII chars #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

pbor
Copy link

@pbor pbor commented Mar 3, 2016

On my chrome when I press the "a" key, I get this.which = 97, so
we also should duplicate the a-z ASCII range in the array.

@levithomason
Copy link
Contributor

It looks like you're mapping the keypress event while this library is mapping on the keydown event:

image

The range in the polyfill right now covers lower and upper case for the range of keydown event which and keyCode values.

On my chrome when I press the "a" key, I get this.which = 97, so
we also should duplicate the a-z ASCII range in the array.
@pbor
Copy link
Author

pbor commented May 17, 2016

Sorry for the delay.

Yes, I am using the polyfill for both keyup and keydown. With my change this seems to work as expected. Do you think it is a problem?

@levithomason
Copy link
Contributor

levithomason commented May 17, 2016

I think there is a conflict. This polyfill assumes the event is a keydown event only. It is mapping which and keyCode values to the new key values. This PR maps the which/keyCode range 97 - 123. This range overlaps the keydown values for function keys. Example:

image

Unless there were some logic added to differentiate which type of event was invoked (keypress vs keydown) there is no way to tell if 112 should indicate F1 or P.

The Event.type tells whether it is a keypress or keydown event. This could be used to map the values you are trying to map. If this was done, this polyfill should likely have two complete maps. One for keydown, and another for keypress.

Lastly, I'm not sure if the specs say key property will be available on all events or just keydown. Also, I've moved from this polyfill to a ponyfill that does the same thing.

Hope that helps, cheers!

@cvan
Copy link
Owner

cvan commented Oct 6, 2017

sorry, just seeing this now. thanks for the PR and the discussion.

I think the fix here would probably be to fire different events on keypress vs. keydown. let me know what y'all think. I can open a separate issue and follow-up PR.

@levithomason
Copy link
Contributor

Just a heads up, I've moved away from the polyfill in favor of a function. This way, the prototype isn't mutated.

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