search

cvan / keyboardevent-key-polyfill

polyfill for `KeyboardEvent.prototype.key`
https://cvan.io/keyboardevent-key-polyfill/
Creative Commons Zero v1.0 Universal
47 stars 15 forks source link

Please don't overwrite event.key if it already exists #20

Open nikparo opened 6 years ago

nikparo commented 6 years ago

This polyfill currently overwrites the native KeyboardEvent.prototype.key on Internet Explorer and Edge. I understand that this is to ensure that IE and Edge give the standard value for some keys, but this polyfill is still in many ways inferior to the native implementation. It is missing many keys, it has no locale support #11, and the returned key for keypresses are wrong #15.

I have recently written a shim that fixes most bad key values of non-standard, native event.key implementations: https://github.com/nikparo/keyboardevent-key-standardiser-shim/ . My shim would play well with this one, unfortunately this one doesn't play well with mine :/

Would you please remove the isEdgeOrIE check? Thanks.

danielbsig commented 6 years ago

Is the isEdgeOrIE check the only thing that needs changing? I noticed you forked the repo but didn't modify this check. Did that change cause some other side effects?

nikparo commented 6 years ago

I think that fork was only created for a pull request (https://github.com/cvan/keyboardevent-key-polyfill/pull/19). I don't actually use this or that in any code at the moment, for no good reason other than that I already had a polyfill in place in my main project that covers what I need.

I see no reason removing the check would cause side-effects, as long as you pair it with my shim to standardise the keys. (We use it in producation and have recieved no bug reports) The code would be something like:

function polyfill () {
  if (!('KeyboardEvent' in window) || ('key' in KeyboardEvent.prototype)) {
    return false;
  }
  ...