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>
Severity: Major CC: aestes, ap, eric, harrison
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.6   
Description Flags
Here is the test case
Test case: logs both keypress and keydown
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

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
onkeypress :  code:107 char:107 meta:true fromCharCode:k keyIdentifier:

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
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