Fix Keyboard.presses

[jvoigtlaender]

The issue this pull request addresses can be demonstrated with this program (pasteable into http://elm-lang.org/try):

import Graphics.Element exposing (..)
import Keyboard


main : Signal Element
main =
  Signal.map show Keyboard.presses

What is expected is that whenever a key is pressed, the corresponding code is printed. But in reality the behavior depends on the browser, and in no browser does it correspond to what the documentation says. In particular,

• in Firefox keys like a don't work, but special keys like the arrows do work,
• in Chrome it is the other way round.

More specifically,

• in Firefox key a gives code 0, while special keys give meaningful codes,
• in Chrome key a gives a meaningful code, while special keys fire no event at all.

Error reports related to this have come up again and again. See also https://github.com/elm-lang/core/issues/326, https://github.com/elm-lang/core/issues/427, https://github.com/elm-lang/core/pull/435, https://github.com/elm-lang/core/issues/462, elm/compiler#1069, elm-lang/elm-platform#123, elm/elm-lang.org#440, elm-lang@016d44a.

This pull request here fixes the behavior. Of necessity, it does so in a way that will require a change to the signal's documentation. Because the signal's documentation claims something that is impossible to implement across current browsers, and likely was never true.

The main data and cause for all this is that Firefox and Chrome (and other browsers that behave like Chrome, while I haven't found a browser that behaves like Firefox here) behave quite differently concerning keydown, keyup, and keypress events, as follows:

• On Firefox:
  • pressing an arrow key fires keydown and keyup with keyCode and which set to the same non-zero value, but charCode set to zero
  • pressing an arrow key fires keypress with keyCode set to the same non-zero value as above, but which and charCode set to zero
  • pressing a character key fires keydown and keyup with keyCode and which set to the same non-zero value, but charCode set to zero
  • pressing a character key fires keypress with which and charCode set to the same non-zero value, but keyCode set to zero
• On Chrome:
  • pressing an arrow key fires keydown and keyup with keyCode and which set to the same non-zero value, but charCode set to zero
  • pressing an arrow key does not fire keypress at all
  • pressing a character key fires keydown and keyup with keyCode and which set to the same non-zero value, but charCode set to zero
  • pressing a character key fires keypress with keyCode and which and charCode all set to the same non-zero value

As a first thing to note, it will be impossible to let Signal.keypress have an event for arrow keys in Chrome. And Chrome is in its rights here, because that's how the specification says it should be. See https://developer.mozilla.org/en-US/docs/Web/Events/keypress, which says: "... is fired when a key is pressed down and that key normally produces a character value". So the only way to get consistent behavior across browsers here is to stop (Keyboard.presses in) Firefox from giving codes like 38 for up-arrow.

As a second thing to note, using keyCode to detect which key was pressed (as is currently done) will not work in Firefox for character keys. By looking at the above analysis, it would be okay (for the two browsers above) to simply use which, because it contains the right thing in all situations. One could be concerned, though, about historical or other browsers. So, to be on the safe side, I have used what jQuery uses. That is battle tested. And whenever which is defined (which is as far as I can tell always the case), it is equivalent to simply using which right away.

I don't see any reasonable alternatives to either using which or what jQuery uses. In particular, I don't think it makes sense to strive for a solution that "does not change anything of the existing behavior". As demonstrated above, there is no well-defined "existing behavior" at the moment, and probably never was.

[jvoigtlaender]

@evancz, I have tried hard now to bring the above post (the one and only in this pull request) into a form that meets your requirements as set out here and here. If anything is missing or amiss, please let me know. Otherwise, let's finally get this fix into a core release. 😄

[jvoigtlaender]

@evancz, I have tried hard now to bring the above post (the one and only in this pull request) into a form that meets your requirements as set out here and here. If anything is missing or amiss, please let me know. Otherwise, let's finally get this fix into a core release. 😄

[marreman]

Maybe using a polyfill approach is better? Pushing cross-browser concerns to the edges of the code base? Also, they are very easy to remove once all targeted browsers are following standards.

[marreman]

Maybe using a polyfill approach is better? Pushing cross-browser concerns to the edges of the code base? Also, they are very easy to remove once all targeted browsers are following standards.

[jvoigtlaender]

@marreman, I'm not sure what exactly you are proposing to do differently. The native Keyboard.js file is pretty much at the edge of the code base. Is your point just to move the one line change there into a separate function that can later be replaced by some more simple projection? Or some more substantial deviation from this specific pull request? (I don't think moving the Signal.filter from Keyboard.elm to some equivalent logic on the .js side would be worth it, but do you have a concrete proposal?)

[jvoigtlaender]

@marreman, I'm not sure what exactly you are proposing to do differently. The native Keyboard.js file is pretty much at the edge of the code base. Is your point just to move the one line change there into a separate function that can later be replaced by some more simple projection? Or some more substantial deviation from this specific pull request? (I don't think moving the Signal.filter from Keyboard.elm to some equivalent logic on the .js side would be worth it, but do you have a concrete proposal?)

[marreman]

After reading more on the subject, I realize that my comment was a bit premature. It is not as easy as I thought. keyCode, which and charCode are all deprecated and key is still just part of a working draft and only implemented in Firefox and IE9+.

I was thinking of something like this in some part of the code base that is dedicated to normalizing browser behavior:

Object.defineProperty(KeyboardEvent.prototype, 'keyCode', {
  get: function () {
    // make things consistent..
  }
});

[marreman]

After reading more on the subject, I realize that my comment was a bit premature. It is not as easy as I thought. keyCode, which and charCode are all deprecated and key is still just part of a working draft and only implemented in Firefox and IE9+.

I was thinking of something like this in some part of the code base that is dedicated to normalizing browser behavior:

Object.defineProperty(KeyboardEvent.prototype, 'keyCode', {
  get: function () {
    // make things consistent..
  }
});

[jvoigtlaender]

I see. If the Elm runtime/core had such a centralized place for normalizing browser behavior, it would be good to handle this case there as well. But that's not how core is structured at the moment. If there is something to polyfill (like requestAnimationFrame), it happens near the use site, not in a centralized place. So the same makes sense to do here. And the specific issue that Firefox fires keypress on non-character keys couldn't be handled with your proposed approach anyway.

So I stand by the existing pull request here.

[jvoigtlaender]

I see. If the Elm runtime/core had such a centralized place for normalizing browser behavior, it would be good to handle this case there as well. But that's not how core is structured at the moment. If there is something to polyfill (like requestAnimationFrame), it happens near the use site, not in a centralized place. So the same makes sense to do here. And the specific issue that Firefox fires keypress on non-character keys couldn't be handled with your proposed approach anyway.

So I stand by the existing pull request here.

[marreman]

I'm sorry for bothering with ignorant comments. Just very eager to help! +1 for your proposal! :P

[marreman]

I'm sorry for bothering with ignorant comments. Just very eager to help! +1 for your proposal! :P

[jvoigtlaender]

Oh, no problem, and no need to apologize.

[jvoigtlaender]

Oh, no problem, and no need to apologize.

[evancz]

Things are going to work different in an upcoming release. I think this issue will be less, but if it's not we can pick it back up on the appropriate repo.

[evancz]

Things are going to work different in an upcoming release. I think this issue will be less, but if it's not we can pick it back up on the appropriate repo.

[jvoigtlaender]

It turned out that elm-lang/keyboard still has this problem, the issue did not turn out to be less with that release. As said in the previous comment, it should then be picked back up on that repo.

It now has been picked up there, so anybody who happens to come by here, be directed to:

• elm-lang/keyboard#6
• and, related, also: elm-lang/keyboard#5

[jvoigtlaender]

It turned out that elm-lang/keyboard still has this problem, the issue did not turn out to be less with that release. As said in the previous comment, it should then be picked back up on that repo.

It now has been picked up there, so anybody who happens to come by here, be directed to:

• elm-lang/keyboard#6
• and, related, also: elm-lang/keyboard#5