Bug 89742 – [Win] key event's location does not work on Windows platform.

• Component: UI Events
• Status: RESOLVED FIXED

[Takashi Sakamoto]

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.

[Takashi Sakamoto]

Created attachment 149232 [details]
Patch

[Ryosuke Niwa]

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?

[Alexey Proskuryakov]

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.

[Ryosuke Niwa]

Comment on attachment 149232 [details]
Patch

r- per my and ap's comments.

[Takashi Sakamoto]

Created attachment 161883 [details]
Patch

[Takashi Sakamoto]

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.

[Build Bot]

Comment on attachment 161883 [details]
Patch

Attachment 161883 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13729804

[Takashi Sakamoto]

Created attachment 161982 [details]
Patch

[Build Bot]

Comment on attachment 161982 [details]
Patch

Attachment 161982 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13745377

[Takashi Sakamoto]

Created attachment 162003 [details]
Patch

[Jessie Berlin]

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

[Takashi Sakamoto]

Created attachment 162214 [details]
Patch

[Takashi Sakamoto]

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

[Build Bot]

Comment on attachment 162214 [details]
Patch

Attachment 162214 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13742877

[Takashi Sakamoto]

Created attachment 162416 [details]
Patch

[Ryosuke Niwa]

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.

[Takashi Sakamoto]

Created attachment 166148 [details]
Patch

[Takashi Sakamoto]

Created attachment 166154 [details]
Patch

[Takashi Sakamoto]

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.

[Eric Seidel (no email)]

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

[Takashi Sakamoto]

(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

[Brent Fulgham]

Comment on attachment 166154 [details]
Patch

r=me

[Brent Fulgham]

Committed r134855: <http://trac.webkit.org/changeset/134855>