Last Comment Bug 773651 - on windows XP KeyboardEvent.location for CTRL keys is always set to DOM_KEY_LOCATION_LEFT
: on windows XP KeyboardEvent.location for CTRL keys is always set to DOM_KEY_L...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla17
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
: Jim Mathies [:jimm]
Mentors:
Depends on:
Blocks: 166240 680830
  Show dependency treegraph
 
Reported: 2012-07-13 07:57 PDT by Louis Jolibois
Modified: 2012-07-19 07:31 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (4.56 KB, patch)
2012-07-13 09:41 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch (225.75 KB, patch)
2012-07-14 22:34 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Patch (225.78 KB, patch)
2012-07-15 01:38 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
jmathies: review+
Details | Diff | Splinter Review

Description Louis Jolibois 2012-07-13 07:57:03 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.1 (KHTML, like Gecko) Chrome/22.0.1201.0 Safari/537.1

Steps to reproduce:

On a windows XP platform, go to https://bugzilla.mozilla.org/attachment.cgi?id=618602 and hit both CTRL keys one after the other


Actual results:

Both keys returns the same location as being DOM_KEY_LOCATION_LEFT:

keyup: target=<INPUT>, location=DOM_KEY_LOCATION_LEFT
keydown: target=<INPUT>, location=DOM_KEY_LOCATION_LEFT
keyup: target=<INPUT>, location=DOM_KEY_LOCATION_LEFT
keydown: target=<INPUT>, location=DOM_KEY_LOCATION_LEFT


Expected results:

The right CTRL key location should have been DOM_KEY_LOCATION_RIGHT
Comment 1 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-07-13 09:41:52 PDT
Created attachment 641935 [details] [diff] [review]
Patch

I'll test this patch this weekend if I have much time.

But unfortunately, currently, tryserver isn't available, sigh...
Comment 2 Louis Jolibois 2012-07-13 14:15:06 PDT
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #1)
> Created attachment 641935 [details] [diff] [review]
> Patch
> 
> I'll test this patch this weekend if I have much time.
> 
> But unfortunately, currently, tryserver isn't available, sigh...

For what it's worth, I just tested it and everything works fine so far on my old xp.
Thanks for the quick and efficient answer.
Comment 3 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-07-14 22:34:26 PDT
Created attachment 642338 [details] [diff] [review]
Patch

We shouldn't overwrite the mVirtualKeyCode with wrong scancode.
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-07-15 01:38:48 PDT
Created attachment 642352 [details] [diff] [review]
Patch

Jimm, could you review this quickly if you realized this request on the 15th?

This patch fix the keycode values for shift/ctrl/alt of NativeKey::mOriginalVirtualKeyCode and NativKey::mVirtualKeyCode.

mOriginalVirtualKeyCode must have VK_SHIFT/VK_CONTROL/VK_MENU.

mVirtualKeyCode must have VK_LSHIFT/VK_RSHIFT/VK_LCONTROL/VK_RCONTROL/VK_LMENU/VK_RMENU.

If the left-right distinguished key code comes, this patch sets VK_SHIFT/VK_CONTROL/VK_MENU manually and does NOT compute the mVirtualKeyCode. Instead of that, it just sets the received keycode to mVirtualKeyCode.

If the received keycode isn't one of VK_SHIFT/VK_CONTROL/VK_MENU, this patch does NOT compute the mVirtualKeyCode because the lParam might be broken value if the message is sent/posted by other applications. This patch just set the received keycode to the mVirtualKey in this case too.

If the received keycode is one of VK_SHIFT/VK_CONTROL/VK_MENU, this patch computes the left-right distinguished keycode with MapVirtualKeyEx() and scan code in lParam.

But if it's impossible on XP or WinServer2003, this patch *guess* the LR difference from the extended key flag if the received keycode is VK_CONTROL or VK_MENU. On the other hand, if the received keycode is VK_SHIFT, always use VK_LSHIFT because both shift keys are not extended keys. So, we have no hint in this case.

If the result of MapVirtualKeyEx() isn't indicate the left-right distinguished keycode of the received keycode, this patch *guess* the mVirtualKeyCode with extended key flag.

So, this patch supports the generated key messages without scancode this might help compatibility with other utils.

And for the conformation of the change, this patch adds the KeyboardEvent.location tests into test_keycodes.xul.
Comment 5 Jim Mathies [:jimm] 2012-07-18 11:06:52 PDT
Comment on attachment 642352 [details] [diff] [review]
Patch

Sorry for the delay. This looks ok overall.
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-07-18 18:30:46 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/205aaea8796d

No problem, this is really my fault and I requested the review in the last weekend.

Thanks.
Comment 7 Ed Morley [:emorley] 2012-07-19 07:31:42 PDT
https://hg.mozilla.org/mozilla-central/rev/205aaea8796d

Note You need to log in before you can comment on or make changes to this bug.