Last Comment Bug 166240 - Implement D3E KeyboardEvent.location (except JOYSTICK)
: Implement D3E KeyboardEvent.location (except JOYSTICK)
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: x86 All
: P3 normal with 10 votes (vote)
: mozilla15
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
:
Mentors:
http://dev.w3.org/2006/webapi/DOM-Lev...
: 493971 (view as bug list)
Depends on: nsdomevent_separate 751881 751929 756504 773651 936313
Blocks: 680829 98160
  Show dependency treegraph
 
Reported: 2002-09-02 12:52 PDT by Ilya Konstantinov
Modified: 2015-01-05 01:41 PST (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.17 KB, text/html)
2012-04-26 02:41 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details
part.1 Add D3E KeyboardEvent.location (5.69 KB, patch)
2012-04-26 03:53 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
bugs: review+
jst: superreview+
Details | Diff | Review
part.2 Add support KeyboardEvent.location on Windows (27.76 KB, patch)
2012-04-26 04:13 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
no flags Details | Diff | Review
part.3 Add support KeyboardEvent.location on GTK (2.92 KB, patch)
2012-04-26 04:28 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
karlt: review+
Details | Diff | Review
part.4 Add support KeyboardEvent.location on Cocoa (2.07 KB, patch)
2012-04-26 04:30 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
smichaud: review+
Details | Diff | Review
part.5 Add support KeyboardEvent.location on Android (809 bytes, patch)
2012-04-26 04:33 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
cpeterson: review+
Details | Diff | Review
part.6 Add support KeyboardEvent.location on Qt (1.11 KB, patch)
2012-04-26 04:35 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
bugs: review+
Details | Diff | Review
part.7 Add support KeyboardEvent.location on Gonk (816 bytes, patch)
2012-04-26 04:36 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
bugs: review+
Details | Diff | Review
part.2 Add support KeyboardEvent.location on Windows (28.28 KB, patch)
2012-04-26 20:40 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
jmathies: review+
Details | Diff | Review
part.8 Add tests for KeyboardEvent.location (synthesized events) (19.51 KB, patch)
2012-04-27 00:44 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured)
bugs: review+
jst: superreview+
Details | Diff | Review

Description Ilya Konstantinov 2002-09-02 12:52:55 PDT
To complete my XBL plugin for Mozilla which enhances BiDi support with useful
keyboard shortcuts, I wanted to bind Windows-like LeftCtrl-Shift and
RightCtrl-Shift shortcuts to left/right directionality changes.

Unfortunatelly (as I found later, as a result of the "key handling revision 3"
document), Mozilla doesn't differentiate between left and right Shift, Ctrl or Alt.

Could this be changed?

To retrieve the side of the modifiers, we can use the same property names as
Internet Explorer does (altLeft, ctrlLeft, shiftLeft):
http://msdn.microsoft.com/workshop/author/dhtml/reference/objects/obj_event.asp
Comment 1 Ilya Konstantinov 2002-10-19 05:30:37 PDT
Note: DOM 3 can differentiate between left and right modifiers in its TextEvent,
but we don't support this part of DOM 3 yet. Our KeyEvent is non-standard anyway
-- should KeyEvent remain frozen (hoping to deprecate it one day) or can we
implement this feature too until TextEvent support arrives?
Comment 2 Ilya Konstantinov 2004-07-23 02:47:27 PDT
Clarifying: This bug would be fixed when DOM3 events (namely KeyboardEvent) are 
in. 
Comment 3 Eyal Rozenberg 2007-12-27 01:51:58 PST
Can someone please add dependence on a relevant bug re adding DOM3 KeyboardEvent? Has there been any progress on this issue since 2004?
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2011-02-02 00:58:22 PST
*** Bug 493971 has been marked as a duplicate of this bug. ***
Comment 5 info 2011-08-18 04:38:04 PDT
Is this issue still being worked on? It's absolutely ridiculous that an issue like this has to stay open for 9 years without any solution whatsoever.

Can someone address this issue please? Give it the "moz" prefix until those DOM3 specifications are finally finished all you want, just add this functionality!

Thanks,
-J
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-04-26 02:41:18 PDT
Created attachment 618602 [details]
testcase
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-04-26 03:53:45 PDT
Created attachment 618609 [details] [diff] [review]
part.1 Add D3E KeyboardEvent.location
Comment 8 Olli Pettay [:smaug] 2012-04-26 04:07:42 PDT
Comment on attachment 618609 [details] [diff] [review]
part.1 Add D3E KeyboardEvent.location

I assume tests will be added in some other patch.
IMO, this patch shouldn't be pushed before we support also other than
DOM_KEY_LOCATION_STANDARD.
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-04-26 04:13:31 PDT
Created attachment 618615 [details] [diff] [review]
part.2 Add support KeyboardEvent.location on Windows

Windows's widget needs some clean up for this.

For checking the key location, We need to call MapVirtualKeyEx() with scan code. I made mozilla::widget::NativeKeyInfo class in KeyboardLayout.h. It initializes all members at created. And computes the left-right distinguished key code from the key/char message.

The instance is made in stack when methods make nsKeyEvent instance. And OnKeyDown() passes the instance to OnChar() and InitKeyEvent().

OnCharRaw() is replaced with OnChar(). On current code, they are not needed to be separated.

InitKeyEvent() is separated from DispatchKeyEvent(). It initializes only common members between all callers.
Comment 10 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-04-26 04:19:25 PDT
Comment on attachment 618615 [details] [diff] [review]
part.2 Add support KeyboardEvent.location on Windows

Sorry for the spam. This may have bug.
Comment 11 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-04-26 04:28:06 PDT
Created attachment 618618 [details] [diff] [review]
part.3 Add support KeyboardEvent.location on GTK
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-04-26 04:30:04 PDT
Created attachment 618619 [details] [diff] [review]
part.4 Add support KeyboardEvent.location on Cocoa

This patch doesn't trust NSAlphaShiftKeyMask because it's applied to arrow keys too :-(
Comment 13 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-04-26 04:33:45 PDT
Created attachment 618621 [details] [diff] [review]
part.5 Add support KeyboardEvent.location on Android

We should support only MOBILE for now.

If we can distinguish the kind of input device by KeyEvent.getDeviceId(), we might be set same location values for full keyboard connected by Bluetooth and set JOYSTICK. But I don't familiar with Android and shouldn't be problem for now because WebKit hasn't supported .location yet.
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-04-26 04:35:37 PDT
Created attachment 618623 [details] [diff] [review]
part.6 Add support KeyboardEvent.location on Qt

Looks like Qt's key event doesn't provide enough information for us about the key location. For Maemo, we should set MOBILE for now.
Comment 15 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-04-26 04:36:54 PDT
Created attachment 618624 [details] [diff] [review]
part.7 Add support KeyboardEvent.location on Gonk

I don't know about Gonk. But I think setting MOBILE is enough for now.
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-04-26 04:41:18 PDT
(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 618609 [details] [diff] [review]
> part.1 Add D3E KeyboardEvent.location
> 
> I assume tests will be added in some other patch.
> IMO, this patch shouldn't be pushed before we support also other than
> DOM_KEY_LOCATION_STANDARD.

Wow, I was surprised at your quick review! Thank you.

Okay, I'll make nsIDOMWindowUtils::sendKeyEvent() tests tomorrow.
Comment 17 Steven Michaud [:smichaud] (Retired) 2012-04-26 07:25:37 PDT
Comment on attachment 618619 [details] [diff] [review]
part.4 Add support KeyboardEvent.location on Cocoa

Looks fine to me.
Comment 18 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-04-26 20:40:02 PDT
Created attachment 618910 [details] [diff] [review]
part.2 Add support KeyboardEvent.location on Windows

See comment 9 for the detail of this patch but I renamed NativeKeyInfo to NativeKey.
Comment 19 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-04-27 00:44:32 PDT
Created attachment 618950 [details] [diff] [review]
part.8 Add tests for KeyboardEvent.location (synthesized events)

The interface change doesn't break addon compatibility if they use true or false for aPreventDefault.
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-04-27 02:36:53 PDT
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=c570a89d7b0b
Comment 21 Olli Pettay [:smaug] 2012-04-27 05:09:08 PDT
Comment on attachment 618621 [details] [diff] [review]
part.5 Add support KeyboardEvent.location on Android

Actually, on Android the key events may be coming from a real
keyboard. Think about Asus Transformer with a keyboard.
Is there some way to know what kind of input device is used?
Comment 22 Olli Pettay [:smaug] 2012-04-27 05:10:30 PDT
Comment on attachment 618624 [details] [diff] [review]
part.7 Add support KeyboardEvent.location on Gonk

Should be ok.
Comment 23 Wesley Johnston (:wesj) 2012-04-27 05:40:01 PDT
Comment on attachment 618621 [details] [diff] [review]
part.5 Add support KeyboardEvent.location on Android

Chris is our resident IME expert right now. Throwing this to him.
Comment 24 Chris Peterson [:cpeterson] 2012-04-30 11:34:02 PDT
Comment on attachment 618621 [details] [diff] [review]
part.5 Add support KeyboardEvent.location on Android

Review of attachment 618621 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Comment 25 Jim Mathies [:jimm] 2012-05-01 02:52:17 PDT
Comment on attachment 618910 [details] [diff] [review]
part.2 Add support KeyboardEvent.location on Windows

Review of attachment 618910 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/KeyboardLayout.cpp
@@ +284,5 @@
> +  if (!mIsExtended ||
> +      WinUtils::GetWindowsVersion() < WinUtils::VISTA_VERSION) {
> +    return mScanCode;
> +  }
> +  return (0xE000 | mScanCode);

Please add a comment explaining this constant.

::: widget/windows/nsWindow.cpp
@@ +5738,5 @@
> +    UINT scanCode = ::MapVirtualKeyEx(aNativeKeyCode, MAPVK_VK_TO_VSC,
> +                                      gKbdLayout.GetLayout());
> +    LPARAM lParam = static_cast<LPARAM>(scanCode << 16);
> +    if (keySpecific == VK_RCONTROL || keySpecific == VK_RMENU) {
> +      lParam |= 0x1000000;

ditto here.
Comment 26 Chris Peterson [:cpeterson] 2012-05-01 10:31:11 PDT
Comment on attachment 618621 [details] [diff] [review]
part.5 Add support KeyboardEvent.location on Android

Review of attachment 618621 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/android/nsWindow.cpp
@@ +1758,5 @@
>      event.InitBasicModifiers(gMenu,
>                               key.IsAltPressed(),
>                               key.IsShiftPressed(),
>                               false);
> +    event.location = nsIDOMKeyEvent::DOM_KEY_LOCATION_MOBILE;

How will DOM_KEY_LOCATION_MOBILE events be handled differently than DOM_KEY_LOCATION_STANDARD events?

If an Android device has an external hardware keyboard (like the ASUS Transformer), do we still want key events to be marked DOM_KEY_LOCATION_MOBILE?
Comment 27 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-05-01 16:52:20 PDT
Thank you for your reviews!

(In reply to Chris Peterson (:cpeterson) from comment #26)
> Comment on attachment 618621 [details] [diff] [review]
> part.5 Add support KeyboardEvent.location on Android
> 
> Review of attachment 618621 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/nsWindow.cpp
> @@ +1758,5 @@
> >      event.InitBasicModifiers(gMenu,
> >                               key.IsAltPressed(),
> >                               key.IsShiftPressed(),
> >                               false);
> > +    event.location = nsIDOMKeyEvent::DOM_KEY_LOCATION_MOBILE;
> 
> How will DOM_KEY_LOCATION_MOBILE events be handled differently than
> DOM_KEY_LOCATION_STANDARD events?
> 
> If an Android device has an external hardware keyboard (like the ASUS
> Transformer), do we still want key events to be marked
> DOM_KEY_LOCATION_MOBILE?

I'm not sure for that.
http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-ID-KeyboardEvent-KeyLocationCode

The definition of MOBILE is:

> The key activation originated on a mobile device, either on a physical keypad or a virtual keyboard.

I guess that the author assumes the "phsical keypad" is non-PC like keyboard of cellphones. So, I *think* that if we can detect which hardware causes the key event on Android, we should set STANDARD, LEFT, RIGHT or NUMPAD for PC keys on PC keyboard. But I don't know if it's possible on current Android's API. Chris, do you think it's possible? If it's possible, I'd like you to work on it because I don't have much knowledge for Android and environment for testing.

And I think that if it's possible and needs a lot of work, we can do it in another bug. Fortunately, we have much time for Fx15 and the users who're using external keyboard with Android must not be so many.

Do you agree with the values for external PC keyboard and working on another bug if we need a large patch for it, smaug?
Comment 28 Chris Peterson [:cpeterson] 2012-05-01 17:56:33 PDT
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 4/29 - 5/6) from comment #27)
> I guess that the author assumes the "phsical keypad" is non-PC like keyboard
> of cellphones. So, I *think* that if we can detect which hardware causes the
> key event on Android, we should set STANDARD, LEFT, RIGHT or NUMPAD for PC
> keys on PC keyboard. But I don't know if it's possible on current Android's
> API. Chris, do you think it's possible? If it's possible, I'd like you to
> work on it because I don't have much knowledge for Android and environment
> for testing.

I propose that we use your DOM_KEY_LOCATION_MOBILE patch as-is. If external PC-style keyboards become important, I can implement that change later.

I'm not sure how to differentiate between a built-in hardware keyboard and an external PC-style keyboard, but I agree that should be a different bug.
Comment 29 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-05-03 01:48:31 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/03c2b4944d97
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b790392d570
https://hg.mozilla.org/integration/mozilla-inbound/rev/3472062aae7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/27b17363e5ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5a2056342d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/efa1a19df3e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/691e7e02395e
https://hg.mozilla.org/integration/mozilla-inbound/rev/de5745bce8bc

(In reply to Chris Peterson (:cpeterson) from comment #28)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (away: 4/29 - 5/6)
> from comment #27)
> > I guess that the author assumes the "phsical keypad" is non-PC like keyboard
> > of cellphones. So, I *think* that if we can detect which hardware causes the
> > key event on Android, we should set STANDARD, LEFT, RIGHT or NUMPAD for PC
> > keys on PC keyboard. But I don't know if it's possible on current Android's
> > API. Chris, do you think it's possible? If it's possible, I'd like you to
> > work on it because I don't have much knowledge for Android and environment
> > for testing.
> 
> I propose that we use your DOM_KEY_LOCATION_MOBILE patch as-is. If external
> PC-style keyboards become important, I can implement that change later.

ASUS has released a notebook with Android which can be used as tablet PC when the display separated from keyboard. On such device, I think that it's more important than the other cases.

> I'm not sure how to differentiate between a built-in hardware keyboard and
> an external PC-style keyboard, but I agree that should be a different bug.

Okay.
Comment 30 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-05-03 01:52:07 PDT
like this.
http://usa.asus.com/Eee/Eee_Pad/Eee_Pad_Transformer_TF101/
Comment 33 Louis Jolibois 2012-07-13 04:01:19 PDT
As stated in the source code the left/right distinction for CTRL keys does not work on windows < vista because MapVirtualKeyEx is not able to distinguish them.
However, the distinction for the ENTER key (normal or from numpad) works because it is done after mapping to the virtual key in the GetKeyLocation() method:

    case VK_RETURN:
      return !mIsExtended ? nsIDOMKeyEvent::DOM_KEY_LOCATION_STANDARD :
                            nsIDOMKeyEvent::DOM_KEY_LOCATION_NUMPAD;

Is there a reason we could not do the same for the VK_CONTROL key (and presumably others) in order to get full support of the key location feature for Windows XP also ?
The mIsExtended should enables us to tell whether it was the right or the left CONTROL key or am I misunderstanding something here ?
Comment 34 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2012-07-13 07:33:17 PDT
(In reply to drinounet from comment #33)
> As stated in the source code the left/right distinction for CTRL keys does
> not work on windows < vista because MapVirtualKeyEx is not able to
> distinguish them.
> However, the distinction for the ENTER key (normal or from numpad) works
> because it is done after mapping to the virtual key in the GetKeyLocation()
> method:
> 
>     case VK_RETURN:
>       return !mIsExtended ? nsIDOMKeyEvent::DOM_KEY_LOCATION_STANDARD :
>                             nsIDOMKeyEvent::DOM_KEY_LOCATION_NUMPAD;
> 
> Is there a reason we could not do the same for the VK_CONTROL key (and
> presumably others) in order to get full support of the key location feature
> for Windows XP also ?
> The mIsExtended should enables us to tell whether it was the right or the
> left CONTROL key or am I misunderstanding something here ?

Oh, really? Could you file a bug for that? And cc me. I'll check it next week. But it might be too late for Fx16...
Comment 35 Louis Jolibois 2012-07-13 08:03:48 PDT
Thanks, i just filed it here: https://bugzilla.mozilla.org/show_bug.cgi?id=773651
Comment 36 Jean-Yves Perrier [:teoli] 2015-01-05 01:41:39 PST
I've created the last bit of docs missing:
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.location

(Thanks :masayuki for the rest!)

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