Bug 36616

Summary: Dvorak-Qwerty keyboard layout gives unexpected results in javascript keydown
Product: WebKit Reporter: Andrew Pouliot <andpoul>
Component: WebCore Misc.Assignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Major CC: aestes, ap, eric, harrison
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.6   
Attachments:
Description Flags
Here is the test case
none
Test case: logs both keypress and keydown
none
proposed fix
darin: review+
regression tests... who would believe? darin: review+

Description Andrew Pouliot 2010-03-25 12:47:38 PDT
Webkit is reporting incorrect key codes to web apps when the meta/command key is down in the dvorak - qwerty command layout.

Steps to reproduce:

1. Enable the "Dvorak - Qwerty <command>" layout in system preferences and switch to it

2. Open test page in Safari

3. Type command-k with the text field selected (hit command and the k key on the keyboard)

console log is :
key pressed. code:84 char:0 meta:true fromCharCode:T keyIdentifier:U+0054 keyLocation:0

expected:
key pressed. code:75 char:0 meta:true fromCharCode:K keyIdentifier:U+004B keyLocation:0

The point of the dvorak-qwerty layout is that it should send qwerty-mapped characters when the command key is held down, otherwise behaving like a dvorak keyboard. This is so that commands are still in their familiar and ergonomic places ie command-c, etc.

The impacts of this bug on dvorak-qwerty users are far and wide, as many websites trap keyboard shortcuts and prevent the default action. Thus, typing command-V, which usually results in a "paste" action can instead trigger whatever is mapped to command-K in the web app.

For a real-world example, try to use the http://stackoverflow.com code editor in this layout.
Comment 1 Andrew Pouliot 2010-03-25 12:48:05 PDT
Created attachment 51671 [details]
Here is the test case
Comment 2 Andrew Pouliot 2010-03-25 17:44:35 PDT
I'm currently looking at KeyEventMac.mm where it creates a PlatformKeyboardEvent from an NSEvent. 

If I print out the NSEvent going into here, I see:

NSEvent: type=KeyDown loc=(0,1158) time=56678.2 flags=0x100108 win=0x0 winNum=13159 ctxt=0x0 chars="k" unmodchars="t" repeat=0 keyCode=40"


In this case, I think the modified chars reflect the intent of the user. There may be other cases like option-e on the mac (dead key), where it might be desirable to send "e"=unmodified rather than ""=modified on to javascript. Does anyone depend on this behavior?

I'm still trying to track down where in code the calculation of the keyCode/keyIdentifier to report to javascript is.
Comment 3 Andrew Pouliot 2010-03-25 19:27:59 PDT
Created attachment 51711 [details]
Test case: logs both keypress and keydown

Now logging both keypress and keydown.
Comment 4 Andrew Pouliot 2010-03-25 19:32:16 PDT
Interesting: it took my reading the source code to realize that the behavior
for keypress is different than that of keydown.

Keydown is working correctly, ie returning k, whereas keydown returns T.

I updated the test case and here are the results for typing command-k:

onkeydown :  code:84 char:0 meta:true fromCharCode:T keyIdentifier:U+0054
keyLocation:0
onkeypress :  code:107 char:107 meta:true fromCharCode:k keyIdentifier:
keyLocation:0

Is keypress the canonical event to use for making keyboard commands, and I
didn't realize it, or is this still a bug with keydown?

In any case, the behavior of keydown is causing problems "in the real world,"
as few developers even are aware of the existence of alternate keyboard
layouts.
Comment 5 Alexey Proskuryakov 2010-03-26 17:51:04 PDT
I think it's a bug with keydown. The code that provides keyCode in keydown is nonsense - we end up with 0 for non-Roman keyboard layouts!

Will need to check IE to be sure that this nonsense isn't expected behavior.
Comment 6 Alexey Proskuryakov 2010-03-30 11:02:43 PDT
Created attachment 52056 [details]
proposed fix
Comment 7 Alexey Proskuryakov 2010-03-30 13:02:58 PDT
Committed <http://trac.webkit.org/changeset/56804>.
Comment 8 Alexey Proskuryakov 2010-03-30 14:18:01 PDT
Created attachment 52082 [details]
regression tests... who would believe?
Comment 9 Alexey Proskuryakov 2010-03-30 14:27:45 PDT
DumpRenderTree fix committed revision 56808.
Comment 10 Eric Seidel (no email) 2010-03-30 14:28:17 PDT
Why can't we use hashmaps for any of this?  These long if-blocks are super ugly, and we're adding more of them. :(
Comment 11 David Harrison 2010-04-20 16:46:09 PDT
This patch is thought to have caused https://bugs.webkit.org/show_bug.cgi?id=37776