Bug 89742 - [Win] key event's location does not work on Windows platform.
Summary: [Win] key event's location does not work on Windows platform.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Windows Vista
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-21 23:27 PDT by Takashi Sakamoto
Modified: 2012-11-15 16:30 PST (History)
11 users (show)

See Also:


Attachments
Patch (8.15 KB, patch)
2012-06-24 20:46 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (7.68 KB, patch)
2012-09-03 02:22 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (7.67 KB, patch)
2012-09-04 01:06 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (7.84 KB, patch)
2012-09-04 04:02 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (8.09 KB, patch)
2012-09-05 04:29 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (8.21 KB, patch)
2012-09-05 21:59 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (8.01 KB, patch)
2012-09-27 23:23 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (8.01 KB, patch)
2012-09-27 23:50 PDT, Takashi Sakamoto
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Takashi Sakamoto 2012-06-21 23:27:59 PDT
As WM_KEYDOWN or WM_KEYUP doesn't provide WM_LCONTROL, VM_RCONTROL, ..., WebInputFactory::keyboardEvent (i.e. Source/WebKit/chromium/src/win/WebInputEventFactory.cpp) should check which key is pressed, left or right.
Comment 1 Takashi Sakamoto 2012-06-24 20:46:09 PDT
Created attachment 149232 [details]
Patch
Comment 2 Ryosuke Niwa 2012-08-07 23:28:52 PDT
Comment on attachment 149232 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:10
> +        As WM_KEYDOWN, WM_KEYUP, WM_SYSKEYDOWN, and WM_SYSKEYUP doesn't provide
> +        a virtual keycode which distinguish between left-hand and right-hand
> +        keys. So modified keybordEvent to see a scancode and an extended key

You mean in wParam? It's inaccurate to say those window messages don't supply key code that distinguishes left and right keys because they DO in lParam.

> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:99
> +static int windowsKeycodeWithLocation(WPARAM wparam, LPARAM lParam)

Why is "param" in lParam capitalized?
It appears that the convention is not to capitalize it.

> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:101
> +    int keycode = static_cast<int>(wparam);

Why are we casting it to int?

> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:118
> +

Where is the closing } ??

> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:124
> +    int scancode = (lParam >> 16) & 0xFF;
> +    // WindowsVista or new Windows can support 0xe0 or 0xe1 to specify
> +    // the extended scancode. In this case, if extended key bit is set,
> +    // extended scancode is 0xe000 | scancode or 0xe100 | scancode.
> +    int virtualKeycode = ::MapVirtualKey(scancode, MAPVK_VSC_TO_VK_EX);
> +    return virtualKeyCode ? virtualKeyCode : keycode;

For which virtual key code is this code used?

> Tools/DumpRenderTree/win/EventSender.cpp:486
> +        // Keyboard Scan Code Specification:
> +        // http://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/scancode.doc
> +        keyData += 0x1d << 16; // Left control's scancode

Why are we not calling MapVirtualKey with MAPVK_VK_TO_VSC?
Comment 3 Alexey Proskuryakov 2012-08-08 09:44:09 PDT
Comment on attachment 149232 [details]
Patch

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

r- mostly due to Ryosuke's comments - it's clear that a new patch and another round of review will be needed.

> Source/WebKit/chromium/ChangeLog:14
> +        * src/win/WebInputEventFactory.cpp:
> +        (WebKit::windowsKeycodeWithLocation):

This code seems port neutral. Why does it have to be in chromium directory? What do other ports do?

> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:123
> +    // WindowsVista or new Windows can support 0xe0 or 0xe1 to specify
> +    // the extended scancode. In this case, if extended key bit is set,
> +    // extended scancode is 0xe000 | scancode or 0xe100 | scancode.
> +    int virtualKeycode = ::MapVirtualKey(scancode, MAPVK_VSC_TO_VK_EX);

I have a lot of difficulty understanding this comment, and how it relates to the code below.
Comment 4 Ryosuke Niwa 2012-08-23 23:29:17 PDT
Comment on attachment 149232 [details]
Patch

r- per my and ap's comments.
Comment 5 Takashi Sakamoto 2012-09-03 02:22:06 PDT
Created attachment 161883 [details]
Patch
Comment 6 Takashi Sakamoto 2012-09-03 02:24:01 PDT
Comment on attachment 149232 [details]
Patch

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

Thank you for reviewing.

>> Source/WebKit/chromium/ChangeLog:10
>> +        keys. So modified keybordEvent to see a scancode and an extended key
> 
> You mean in wParam? It's inaccurate to say those window messages don't supply key code that distinguishes left and right keys because they DO in lParam.

Yes, I mean wParam. I know lParam has extended key bit. However, it is not A VIRTUAL KEYCODE. We have to re-create a new virtual keycode by using the given scancode, extended key bit, and virtual keycode. So I wrote "So modified keybordEvent to see a scancode and an extended key bit in lParam to distinguish."

>> Source/WebKit/chromium/ChangeLog:14
>> +        (WebKit::windowsKeycodeWithLocation):
> 
> This code seems port neutral. Why does it have to be in chromium directory? What do other ports do?

I think, this code is very windows-specific. As far as I know, Linux+GTK uses XKeyEvent and it contains a key symbol which distinguishes between left- and right-hand keys (e.g. keysym 0xffe1, Shift_L or keysym 0xffe2, Shift_R).
(I read chromium/src/content/browser/renderer_host/native_web_keyboard_event_gtk.cc)

>> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:99
>> +static int windowsKeycodeWithLocation(WPARAM wparam, LPARAM lParam)
> 
> Why is "param" in lParam capitalized?
> It appears that the convention is not to capitalize it.

Done.

>> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:118
>> +
> 
> Where is the closing } ??

I'm sorry. I have just prepared windows development environment. (So I was planning to see WebKit bot's result.) Now I can build and do tests in Windows. I tested my new patch.

>> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:123
>> +    int virtualKeycode = ::MapVirtualKey(scancode, MAPVK_VSC_TO_VK_EX);
> 
> I have a lot of difficulty understanding this comment, and how it relates to the code below.

I rewrote the comment and moved the comment before "if ((lParam >> 16) & KF_EXTENDED)". I mean, for example, the code always returns VK_RCONTROL in the case where the given lparam's extended key bit is set and the wparam is VK_CONTROL. However, it might be better to use MapVirtualKey to recreate a new virtual keycode. I think, the conversion should be done by Windows API.

>> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:124
>> +    return virtualKeyCode ? virtualKeyCode : keycode;
> 
> For which virtual key code is this code used?

Done.

>> Tools/DumpRenderTree/win/EventSender.cpp:486
>> +        keyData += 0x1d << 16; // Left control's scancode
> 
> Why are we not calling MapVirtualKey with MAPVK_VK_TO_VSC?

Done.
Comment 7 Build Bot 2012-09-03 02:50:35 PDT
Comment on attachment 161883 [details]
Patch

Attachment 161883 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13729804
Comment 8 Takashi Sakamoto 2012-09-04 01:06:58 PDT
Created attachment 161982 [details]
Patch
Comment 9 Build Bot 2012-09-04 01:31:14 PDT
Comment on attachment 161982 [details]
Patch

Attachment 161982 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13745377
Comment 10 Takashi Sakamoto 2012-09-04 04:02:25 PDT
Created attachment 162003 [details]
Patch
Comment 11 Jessie Berlin 2012-09-04 09:32:32 PDT
(In reply to comment #6)
> (From update of attachment 149232 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149232&action=review
> 
> Thank you for reviewing.
> 
> >> Source/WebKit/chromium/ChangeLog:10
> >> +        keys. So modified keybordEvent to see a scancode and an extended key
> > 
> > You mean in wParam? It's inaccurate to say those window messages don't supply key code that distinguishes left and right keys because they DO in lParam.
> 
> Yes, I mean wParam. I know lParam has extended key bit. However, it is not A VIRTUAL KEYCODE. We have to re-create a new virtual keycode by using the given scancode, extended key bit, and virtual keycode. So I wrote "So modified keybordEvent to see a scancode and an extended key bit in lParam to distinguish."
> 
> >> Source/WebKit/chromium/ChangeLog:14
> >> +        (WebKit::windowsKeycodeWithLocation):
> > 
> > This code seems port neutral. Why does it have to be in chromium directory? What do other ports do?
> 
> I think, this code is very windows-specific. As far as I know, Linux+GTK uses XKeyEvent and it contains a key symbol which distinguishes between left- and right-hand keys (e.g. keysym 0xffe1, Shift_L or keysym 0xffe2, Shift_R).
> (I read chromium/src/content/browser/renderer_host/native_web_keyboard_event_gtk.cc)

Sure, but can it be shared with the Apple Windows port?

> 
> >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:99
> >> +static int windowsKeycodeWithLocation(WPARAM wparam, LPARAM lParam)
> > 
> > Why is "param" in lParam capitalized?
> > It appears that the convention is not to capitalize it.
> 
> Done.
> 
> >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:118
> >> +
> > 
> > Where is the closing } ??
> 
> I'm sorry. I have just prepared windows development environment. (So I was planning to see WebKit bot's result.) Now I can build and do tests in Windows. I tested my new patch.
> 
> >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:123
> >> +    int virtualKeycode = ::MapVirtualKey(scancode, MAPVK_VSC_TO_VK_EX);
> > 
> > I have a lot of difficulty understanding this comment, and how it relates to the code below.
> 
> I rewrote the comment and moved the comment before "if ((lParam >> 16) & KF_EXTENDED)". I mean, for example, the code always returns VK_RCONTROL in the case where the given lparam's extended key bit is set and the wparam is VK_CONTROL. However, it might be better to use MapVirtualKey to recreate a new virtual keycode. I think, the conversion should be done by Windows API.
> 
> >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:124
> >> +    return virtualKeyCode ? virtualKeyCode : keycode;
> > 
> > For which virtual key code is this code used?
> 
> Done.
> 
> >> Tools/DumpRenderTree/win/EventSender.cpp:486
> >> +        keyData += 0x1d << 16; // Left control's scancode
> > 
> > Why are we not calling MapVirtualKey with MAPVK_VK_TO_VSC?
> 
> Done.
Comment 12 Takashi Sakamoto 2012-09-05 04:29:29 PDT
Created attachment 162214 [details]
Patch
Comment 13 Takashi Sakamoto 2012-09-05 04:32:31 PDT
(In reply to comment #11)
> (In reply to comment #6)
> > (From update of attachment 149232 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=149232&action=review
> > 
> > Thank you for reviewing.
> > 
> > >> Source/WebKit/chromium/ChangeLog:10
> > >> +        keys. So modified keybordEvent to see a scancode and an extended key
> > > 
> > > You mean in wParam? It's inaccurate to say those window messages don't supply key code that distinguishes left and right keys because they DO in lParam.
> > 
> > Yes, I mean wParam. I know lParam has extended key bit. However, it is not A VIRTUAL KEYCODE. We have to re-create a new virtual keycode by using the given scancode, extended key bit, and virtual keycode. So I wrote "So modified keybordEvent to see a scancode and an extended key bit in lParam to distinguish."
> > 
> > >> Source/WebKit/chromium/ChangeLog:14
> > >> +        (WebKit::windowsKeycodeWithLocation):
> > > 
> > > This code seems port neutral. Why does it have to be in chromium directory? What do other ports do?
> > 
> > I think, this code is very windows-specific. As far as I know, Linux+GTK uses XKeyEvent and it contains a key symbol which distinguishes between left- and right-hand keys (e.g. keysym 0xffe1, Shift_L or keysym 0xffe2, Shift_R).
> > (I read chromium/src/content/browser/renderer_host/native_web_keyboard_event_gtk.cc)
> 
> Sure, but can it be shared with the Apple Windows port?


I see. I moved the code to WebCore/platform/win/KeyEventWin.cpp.

Best regards,
Takashi Sakamoto

> 
> > 
> > >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:99
> > >> +static int windowsKeycodeWithLocation(WPARAM wparam, LPARAM lParam)
> > > 
> > > Why is "param" in lParam capitalized?
> > > It appears that the convention is not to capitalize it.
> > 
> > Done.
> > 
> > >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:118
> > >> +
> > > 
> > > Where is the closing } ??
> > 
> > I'm sorry. I have just prepared windows development environment. (So I was planning to see WebKit bot's result.) Now I can build and do tests in Windows. I tested my new patch.
> > 
> > >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:123
> > >> +    int virtualKeycode = ::MapVirtualKey(scancode, MAPVK_VSC_TO_VK_EX);
> > > 
> > > I have a lot of difficulty understanding this comment, and how it relates to the code below.
> > 
> > I rewrote the comment and moved the comment before "if ((lParam >> 16) & KF_EXTENDED)". I mean, for example, the code always returns VK_RCONTROL in the case where the given lparam's extended key bit is set and the wparam is VK_CONTROL. However, it might be better to use MapVirtualKey to recreate a new virtual keycode. I think, the conversion should be done by Windows API.
> > 
> > >> Source/WebKit/chromium/src/win/WebInputEventFactory.cpp:124
> > >> +    return virtualKeyCode ? virtualKeyCode : keycode;
> > > 
> > > For which virtual key code is this code used?
> > 
> > Done.
> > 
> > >> Tools/DumpRenderTree/win/EventSender.cpp:486
> > >> +        keyData += 0x1d << 16; // Left control's scancode
> > > 
> > > Why are we not calling MapVirtualKey with MAPVK_VK_TO_VSC?
> > 
> > Done.
Comment 14 Build Bot 2012-09-05 04:54:01 PDT
Comment on attachment 162214 [details]
Patch

Attachment 162214 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13742877
Comment 15 Takashi Sakamoto 2012-09-05 21:59:27 PDT
Created attachment 162416 [details]
Patch
Comment 16 Ryosuke Niwa 2012-09-25 15:57:03 PDT
Comment on attachment 162416 [details]
Patch

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

r- due to style issues.

> Source/WebCore/ChangeLog:9
> +        Additional information of the change such as approach, rationale. Pleas        As WM_KEYDOWN, WM_KEYUP, WM_SYSKEYDOWN, and WM_SYSKEYUP doesn't
> +        directly provide a virtual keycode which distinguish between left-hand

These two lines of changelogs are messed up.

> Source/WebCore/platform/win/KeyEventWin.cpp:189
> +static int windowsKeycodeWithLocation(WPARAM wparam, LPARAM lparam)

It's better to give these argument variable more descriptive names like keycode.

> Source/WebCore/platform/win/KeyEventWin.cpp:196
> +    // 24bit: Indicates whether the key is an extended key, such as
> +    // the right-hand ALT and CTRL keys.

This is quite obvious from the code.

> Source/WebCore/platform/win/KeyEventWin.cpp:197
> +    // However, if we don't need to support WindowsXP or older Windows,

Nit: Windows XP.
Comment 17 Takashi Sakamoto 2012-09-27 23:23:44 PDT
Created attachment 166148 [details]
Patch
Comment 18 Takashi Sakamoto 2012-09-27 23:50:45 PDT
Created attachment 166154 [details]
Patch
Comment 19 Takashi Sakamoto 2012-09-28 00:48:55 PDT
Comment on attachment 162416 [details]
Patch

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

Thank you for reviewing.

>> Source/WebCore/ChangeLog:9
>> +        directly provide a virtual keycode which distinguish between left-hand
> 
> These two lines of changelogs are messed up.

Done.

>> Source/WebCore/platform/win/KeyEventWin.cpp:196
>> +    // the right-hand ALT and CTRL keys.
> 
> This is quite obvious from the code.

Done.

>> Source/WebCore/platform/win/KeyEventWin.cpp:197
>> +    // However, if we don't need to support WindowsXP or older Windows,
> 
> Nit: Windows XP.

Done.
Comment 20 Eric Seidel (no email) 2012-10-08 16:15:41 PDT
Comment on attachment 162416 [details]
Patch

Cleared review? from obsolete attachment 162416 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Comment 21 Takashi Sakamoto 2012-10-09 23:21:08 PDT
(In reply to comment #20)
> (From update of attachment 162416 [details])
> Cleared review? from obsolete attachment 162416 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

I see.
I uploaded a new patch, i.e. https://bug-89742-attachments.webkit.org/attachment.cgi?id=166154.

Best regards,
Takashi Sakamoto
Comment 22 Brent Fulgham 2012-11-15 09:55:13 PST
Comment on attachment 166154 [details]
Patch

r=me
Comment 23 Brent Fulgham 2012-11-15 16:30:48 PST
Committed r134855: <http://trac.webkit.org/changeset/134855>