Normalize keyCode in Native/Keyboard.js #435

esad · Nov 17, 2015

Are there any plans for a cross-browser test suite where we could test this and similar things?

Cross-browser testing was discussed on the mailing list and possibly here on GitHub before. I don't know much about this myself, but others on the list may have concrete proposals.

For the specific issue here, testing "by hand" should be sufficient to justify inclusion of the fix. The fact that the fix is exactly what jQuery does also bodes well.

One question, though: After your fix (locally), if you press the A key on your keyboard, do you get code 65 or 97?

jvoigtlaender · Nov 17, 2015

Cross-browser testing was discussed on the mailing list and possibly here on GitHub before. I don't know much about this myself, but others on the list may have concrete proposals.

For the specific issue here, testing "by hand" should be sufficient to justify inclusion of the fix. The fact that the fix is exactly what jQuery does also bodes well.

One question, though: After your fix (locally), if you press the A key on your keyboard, do you get code 65 or 97?

Actually, testing using http://javascript.info/tutorial/keyboard-events tells me that Chrome on Windows will not fire keypress for arrow keys. So even after this fix, we will have to edit the documentation of Keyboard.presses to say something like what https://developer.mozilla.org/en-US/docs/Web/Events/keypress says, namely "... is fired when a key is pressed down and that key normally produces a character value".

jvoigtlaender · Nov 17, 2015

Actually, testing using http://javascript.info/tutorial/keyboard-events tells me that Chrome on Windows will not fire keypress for arrow keys. So even after this fix, we will have to edit the documentation of Keyboard.presses to say something like what https://developer.mozilla.org/en-US/docs/Web/Events/keypress says, namely "... is fired when a key is pressed down and that key normally produces a character value".

@evancz, can we have this fix merged and get a patch release of core?

It's tiring to have to answer the issues that people open at various repositories about this bug, when I know there is no reason for this bug to be there, since a fix exists.

jvoigtlaender · Dec 6, 2015

@evancz, can we have this fix merged and get a patch release of core?

It's tiring to have to answer the issues that people open at various repositories about this bug, when I know there is no reason for this bug to be there, since a fix exists.

Is this new? I guess browsers are deprecating things?

Isn't there an issue open about switching from keyCode which is being deprecated to key which is the new one that actually works? But not all browsers actually support it yet? And it doesn't work exactly the same as keyCode?

Point is, there are more questions to ask here. Not every polyfill is equal. This is one that seemed to not be an issue for a couple years. What is different now? Why is this the right one given the new facts?

evancz · Dec 7, 2015

Is this new? I guess browsers are deprecating things?

Isn't there an issue open about switching from keyCode which is being deprecated to key which is the new one that actually works? But not all browsers actually support it yet? And it doesn't work exactly the same as keyCode?

Point is, there are more questions to ask here. Not every polyfill is equal. This is one that seemed to not be an issue for a couple years. What is different now? Why is this the right one given the new facts?

Is this new? I guess browsers are deprecating things?

It's not new. Well, depends on the definition of "new". The first error report about this that I know of was in early August. That does not necessarily mean that it was working across browsers before August.

Isn't there an issue open about switching from keyCode which is being deprecated to key which is the new one that actually works? But not all browsers actually support it yet? And it doesn't work exactly the same as keyCode?

I'm not aware of such an issue being open. Not in the core repository, as far as I know. The one issue being open that is related to key stuff is the one that is linked to in the first comment above, and which is addressed by this pull request here.

Point is, there are more questions to ask here. Not every polyfill is equal. This is one that seemed to not be an issue for a couple years. What is different now? Why is this the right one given the new facts?

When you say "This [polyfill] is one that seemed to not be an issue for a couple of years.", which polyfill are you referring to? The current core code just uses event.keyCode. There is no polyfill in sight. Instead, this pull request suggests a polyfill, and the polyfill it suggests is the one that is used by jQuery. Also, I am not sure that anything "is different now". What indication do you have that the current implementation of Keyboard.presses in core has ever worked across browsers? From what I have read about key stuff (see the other thread for links to some of the stuff I have read), I have no reason to believe that it can have. So I don't see the point of hesitation here. The current implementation is not working in all cases. The proposed implementation is known to work in strictly more cases. (Specifically, from what I have read, it will still work in all cases in which the current implementation works. And we know that it works in cases in which the current one doesn't.)

jvoigtlaender · Dec 7, 2015

Is this new? I guess browsers are deprecating things?

It's not new. Well, depends on the definition of "new". The first error report about this that I know of was in early August. That does not necessarily mean that it was working across browsers before August.

Isn't there an issue open about switching from keyCode which is being deprecated to key which is the new one that actually works? But not all browsers actually support it yet? And it doesn't work exactly the same as keyCode?

I'm not aware of such an issue being open. Not in the core repository, as far as I know. The one issue being open that is related to key stuff is the one that is linked to in the first comment above, and which is addressed by this pull request here.

Point is, there are more questions to ask here. Not every polyfill is equal. This is one that seemed to not be an issue for a couple years. What is different now? Why is this the right one given the new facts?

When you say "This [polyfill] is one that seemed to not be an issue for a couple of years.", which polyfill are you referring to? The current core code just uses event.keyCode. There is no polyfill in sight. Instead, this pull request suggests a polyfill, and the polyfill it suggests is the one that is used by jQuery. Also, I am not sure that anything "is different now". What indication do you have that the current implementation of Keyboard.presses in core has ever worked across browsers? From what I have read about key stuff (see the other thread for links to some of the stuff I have read), I have no reason to believe that it can have. So I don't see the point of hesitation here. The current implementation is not working in all cases. The proposed implementation is known to work in strictly more cases. (Specifically, from what I have read, it will still work in all cases in which the current implementation works. And we know that it works in cases in which the current one doesn't.)

The proposed implementation is known to work in strictly more cases.

What does "work" mean exactly. I don't want to debate, I just feel like I don't have enough information. Is there a link you can point me to that says "here's how things work"? I know the situation is more complicated than this, and I recall that keyCode and charCode produce different results on different platforms.

A code review process that requires reading very long conversational threads that link to many other conversations is difficult. If I need to read for a half hour to assess a browser compatibility thing that has existed forever (as far as we know) for a feature that is going to increasingly not vital to how things work, it's just less likely to happen.

I hope I'm not sounding harsh here. It's just that there's a reason why certain things are slow. If you want it to go faster, the 30 minutes or hour of reading needs to be less.

evancz · Dec 7, 2015

The proposed implementation is known to work in strictly more cases.

What does "work" mean exactly. I don't want to debate, I just feel like I don't have enough information. Is there a link you can point me to that says "here's how things work"? I know the situation is more complicated than this, and I recall that keyCode and charCode produce different results on different platforms.

A code review process that requires reading very long conversational threads that link to many other conversations is difficult. If I need to read for a half hour to assess a browser compatibility thing that has existed forever (as far as we know) for a feature that is going to increasingly not vital to how things work, it's just less likely to happen.

I hope I'm not sounding harsh here. It's just that there's a reason why certain things are slow. If you want it to go faster, the 30 minutes or hour of reading needs to be less.

Some PR's come in and say things like this:

I have noticed that problem X exists on platform Blah under these conditions. Here is a link to an issue with an http://sscce.org, Or better yet, I included the SSCCE right here, just paste it into /try and see it.

The root problem is Y. There are a couple different perspectives on how to resolve Y. Here are links to the major ways that are out there. Given the state of affairs, I think Z is the best way to resolve this. In cases where it falls back on other stuff, here's what it'll do.

Now I have enough information to just merge stuff. If something comes in saying "here's some code" there's a lot more work to do. Someone has to do all this research, and that's in competition with all the other things that need doing. If that work gets spread through a bunch of comments, it's still hard. Now a lot of people have said things and I potentially have more claims to verify because now I'm moderating different perspectives.

So I think it'd be good to moderate PRs like this a lot more. I have liked the process of "change the initial description to capture all the necessary information as the thread progresses" as a way to organize this stuff in a good way.

evancz · Dec 7, 2015

Some PR's come in and say things like this:

I have noticed that problem X exists on platform Blah under these conditions. Here is a link to an issue with an http://sscce.org, Or better yet, I included the SSCCE right here, just paste it into /try and see it.

The root problem is Y. There are a couple different perspectives on how to resolve Y. Here are links to the major ways that are out there. Given the state of affairs, I think Z is the best way to resolve this. In cases where it falls back on other stuff, here's what it'll do.

Now I have enough information to just merge stuff. If something comes in saying "here's some code" there's a lot more work to do. Someone has to do all this research, and that's in competition with all the other things that need doing. If that work gets spread through a bunch of comments, it's still hard. Now a lot of people have said things and I potentially have more claims to verify because now I'm moderating different perspectives.

So I think it'd be good to moderate PRs like this a lot more. I have liked the process of "change the initial description to capture all the necessary information as the thread progresses" as a way to organize this stuff in a good way.

I have checked at http://javascript.info/tutorial/keyboard-events, using my current versions of Firefox, Chrome and Internet Explorer, that whenever more than one of keyCode, which, and charCode are defined for a keyup, keydown, or keypress event, then the values they carry are the same.

@esad also tested in his environment, to positive results.

That is what I can say about "works".

You say "I recall that keyCode and charCode produce different results on different platforms". If by that you meant that on one platform keyCode and charCode give different results from each other, I have found nothing (by experiment or through reading) that confirms this. If you meant that keyCode (or charCode) may give a different result on one platform than on another platform, then this would already affect the current implementation just the same. So there would be no detriment through the proposed change.

About this:

A code review process that requires reading very long conversational threads that link to many other conversations is difficult. If I need to read for a half hour to assess a browser compatibility thing that has existed forever (as far as we know) for a feature that is going to increasingly not vital to how things work, it's just less likely to happen.

I hope I'm not sounding harsh here. It's just that there's a reason why certain things are slow. If you want it to go faster, the 30 minutes or hour of reading needs to be less.

the best I can say is:

Not today, but during the earlier discussions on this (without your participation), I did read up on this stuff, and came to the conclusion that the proposed fix is a strict improvement. Unfortunately, I cannot provide those links now for your reading, since I have not preserved them. All I can say is: I have already invested the "30 minutes or hour of reading". My proposal to accept the pull request here is the outcome of that. If you need to do the same thing before you can follow that proposal, I have failed by not preserving those links.
About the "a feature that is going to increasingly not vital to how things work": My almost immediate reaction over in the issue where this came up originally (https://github.com/elm-lang/core/issues/326) was indeed to propose to deprecate Keyboard.presses. Some people then said that they really need exactly that functionality.

jvoigtlaender · Dec 7, 2015

I have checked at http://javascript.info/tutorial/keyboard-events, using my current versions of Firefox, Chrome and Internet Explorer, that whenever more than one of keyCode, which, and charCode are defined for a keyup, keydown, or keypress event, then the values they carry are the same.

@esad also tested in his environment, to positive results.

That is what I can say about "works".

You say "I recall that keyCode and charCode produce different results on different platforms". If by that you meant that on one platform keyCode and charCode give different results from each other, I have found nothing (by experiment or through reading) that confirms this. If you meant that keyCode (or charCode) may give a different result on one platform than on another platform, then this would already affect the current implementation just the same. So there would be no detriment through the proposed change.

About this:

A code review process that requires reading very long conversational threads that link to many other conversations is difficult. If I need to read for a half hour to assess a browser compatibility thing that has existed forever (as far as we know) for a feature that is going to increasingly not vital to how things work, it's just less likely to happen.

I hope I'm not sounding harsh here. It's just that there's a reason why certain things are slow. If you want it to go faster, the 30 minutes or hour of reading needs to be less.

the best I can say is:

Not today, but during the earlier discussions on this (without your participation), I did read up on this stuff, and came to the conclusion that the proposed fix is a strict improvement. Unfortunately, I cannot provide those links now for your reading, since I have not preserved them. All I can say is: I have already invested the "30 minutes or hour of reading". My proposal to accept the pull request here is the outcome of that. If you need to do the same thing before you can follow that proposal, I have failed by not preserving those links.
About the "a feature that is going to increasingly not vital to how things work": My almost immediate reaction over in the issue where this came up originally (https://github.com/elm-lang/core/issues/326) was indeed to propose to deprecate Keyboard.presses. Some people then said that they really need exactly that functionality.

Just read your other comment, after posting my latest one. What you say is all fine. The ideal world would be if pull requests would come with this complete information and structure. But it will not always be like that. People work through issues and come to a conclusion. You have to decide whether you trust those conclusions. With a more structured approach that decision would be easier. But maybe you have to set a bar where you can trust some conclusions (if you see that people have tested etc.) even without having convinced yourself about the validity from scratch.

But I don't know. I'm not in your position.

jvoigtlaender · Dec 7, 2015

Just read your other comment, after posting my latest one. What you say is all fine. The ideal world would be if pull requests would come with this complete information and structure. But it will not always be like that. People work through issues and come to a conclusion. You have to decide whether you trust those conclusions. With a more structured approach that decision would be easier. But maybe you have to set a bar where you can trust some conclusions (if you see that people have tested etc.) even without having convinced yourself about the validity from scratch.

But I don't know. I'm not in your position.

Let's just focus on getting a better process now. Here is how I experienced this thing:

For me, this issue ended with "@evancz, can we have this fix merged and get a patch release of core?" Before that, there is basically no description of what it's doing or why, and then you asking a bunch of questions with no resolution.

So I am going through my backlog tonight, trying to get things merged, and this one looked easy. I asked for more information. Now we're like 5 very long messages in, and there is one link to a 2011 post in there. This is a crazy topic where browsers change the meaning of things over time if comments like this were ever true.

I'm not trying to be a jerk here. I'm literally going through my backlog to try to merge stuff. In this case I needed more info, so I asked for it. You have much more context on this, so I guess it felt like I did too.

And I get that issues and PRs come in messy. I see them too ;) It's pretty much all of them.

But to make things work they must get cleaned up. Either I do it and it takes a long time for me, or sometimes the person writing the PR does it. What I was trying to say is, we have the power to edit titles and comments on these issues. We can tell people who send the PRs to do this work. "This looks like a good change, but please edit your title / original post to have X and Y." Or "I like this but blah blah blah, please reopen it with these things fixed."

We can also close them and open organized versions ourselves where the code and description is cleaned up. When I go through and organize issues, this is exactly what I do. I close 3 or 4 vague (but secretly related) issues and put them in one that actually describes what's going on in a clear way.

So I'm going to close this issue and just do whatever needs to be done to fix this. Next time, we can try to use a process where we can develop a PR into something that is organized and nice.

evancz · Dec 7, 2015