The default bug view has changed. See this FAQ.

on windows XP KeyboardEvent.location for CTRL keys is always set to DOM_KEY_LOCATION_LEFT

RESOLVED FIXED in mozilla17

Status

()

Core
Widget: Win32
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Louis Jolibois, Assigned: masayuki)

Tracking

Trunk
mozilla17
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
Component: Untriaged → General
OS: Windows 7 → Windows XP
Hardware: x86_64 → x86
See Also: → bug 166240
(Assignee)

Updated

5 years ago
Assignee: nobody → masayuki
Status: UNCONFIRMED → ASSIGNED
Component: General → Widget: Win32
Ever confirmed: true
Product: Firefox → Core
Version: 16 Branch → Trunk
(Assignee)

Comment 1

5 years ago
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...
(Reporter)

Comment 2

5 years ago
(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.
(Assignee)

Comment 3

5 years ago
Created attachment 642338 [details] [diff] [review]
Patch

We shouldn't overwrite the mVirtualKeyCode with wrong scancode.
Attachment #641935 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
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.
Attachment #642338 - Attachment is obsolete: true
Attachment #642352 - Flags: review?(jmathies)
(Assignee)

Updated

5 years ago
Blocks: 166240, 680830

Comment 5

5 years ago
Comment on attachment 642352 [details] [diff] [review]
Patch

Sorry for the delay. This looks ok overall.
Attachment #642352 - Flags: review?(jmathies) → review+
(Assignee)

Comment 6

5 years ago
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.
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/205aaea8796d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.