Bug 79289 - nativeKeyCode for WebKeyboardEvent not set on Windows
Summary: nativeKeyCode for WebKeyboardEvent not set on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-22 15:45 PST by garykac
Modified: 2012-02-28 11:32 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.40 KB, patch)
2012-02-22 15:53 PST, garykac
no flags Details | Formatted Diff | Diff
Patch (2.17 KB, patch)
2012-02-28 10:17 PST, garykac
no flags Details | Formatted Diff | Diff
Patch (2.32 KB, patch)
2012-02-28 10:29 PST, garykac
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description garykac 2012-02-22 15:45:58 PST
Background:

For most platforms, the |nativeKeyCode| field in the WebKeyboardEvent is used to store the native OS key encoding and the |windowsKeyCode| is used to store the Windows virtual key code equivalent (required for the DOM key events).

Windows, however, doesn't record the native key code but instead stores the Windows virtual key code in both the |nativeKeyCode| and |windowsKeyCode| fields.  This results in loss of information since the Windows virtual key codes contains a subset of the information found in the native key code. For example, the Windows virtual key code cannot distinguish between a left and a right Shift key press - they are both encoded as VK_SHIFT.

This hasn't caused a problem until now because:
(1) DOM events want the Windows virtual key code
(2) the |nativeKeyCode| is only used to calculate the |windowsKeyCode| (thus, it doesn't need to be set on Windows)

However, if downstream components need more accurate information about the key press, then they need access to the native key information on all platforms.

The fix for this is to simply record the key event's LPARAM in the |nativeKeyCode| field (in Source/WebKit/chromium/src/win/WebInputEventFactory.cpp).
This is safe because the |nativeKeyCode| field isn't used anywhere except in platform-specific code to calculate the |windowsKeyCode|.
Comment 1 garykac 2012-02-22 15:53:35 PST
Created attachment 128308 [details]
Patch
Comment 2 Ryosuke Niwa 2012-02-27 15:15:37 PST
Comment on attachment 128308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128308&action=review

> Source/WebKit/chromium/ChangeLog:7
> +

Why are we making this change? r- due to the lack of explanation.
Comment 3 garykac 2012-02-28 10:17:59 PST
Created attachment 129286 [details]
Patch
Comment 4 Ryosuke Niwa 2012-02-28 10:20:58 PST
Comment on attachment 129286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=129286&action=review

> Source/WebKit/chromium/ChangeLog:3
> +	Set the key event's nativeKeyCode on Windows Chromium so that it

Nit: Tab! You also need to put this description below "Reviewed by" line.

> Source/WebKit/chromium/ChangeLog:15
> +	this change will bring Windows in line withe other platforms.

Nit: typo withe.
Comment 5 Ryosuke Niwa 2012-02-28 10:21:22 PST
Comment on attachment 129286 [details]
Patch

r- due to tab characters in the change log.
Comment 6 garykac 2012-02-28 10:29:33 PST
Created attachment 129287 [details]
Patch
Comment 7 Ryosuke Niwa 2012-02-28 10:37:52 PST
Comment on attachment 129287 [details]
Patch

You're not a committer yet, right? If you want me to land it via commit-queue or are you going to ask someone to land your patch?
Comment 8 garykac 2012-02-28 10:39:34 PST
Sending it to the commit-queue would be great. Thanks!
Comment 9 Ryosuke Niwa 2012-02-28 10:42:32 PST
(In reply to comment #8)
> Sending it to the commit-queue would be great. Thanks!

Then please set cq? flag.
Comment 10 garykac 2012-02-28 10:48:03 PST
Comment on attachment 129287 [details]
Patch

cq? set
Comment 11 WebKit Review Bot 2012-02-28 11:32:54 PST
Comment on attachment 129287 [details]
Patch

Clearing flags on attachment: 129287

Committed r109128: <http://trac.webkit.org/changeset/109128>
Comment 12 WebKit Review Bot 2012-02-28 11:32:59 PST
All reviewed patches have been landed.  Closing bug.