Closed Bug 630810 Opened 13 years ago Closed 12 years ago

Should convert from native keycode to DOM keycode for every keys on Windows

Categories

(Core :: Widget: Win32, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: masayuki, Assigned: masayuki)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 12 obsolete files)

19.99 KB, patch
Details | Diff | Splinter Review
89.67 KB, patch
jimm
: review+
Details | Diff | Splinter Review
We have defined the DOM keycode values from virtual keycodes of Windows. By this fact, we're setting raw keycode value to nsKeyEvent::keyCode for most keys on Windows. This makes bad things:

1. We're dispatching DOM key events with unexpected keycode. For example, we don't define a DOM keycode for Windows key. But we set native keycode which is VK_WIN_L or VK_WIN_R.
2. If we dispatch such unexpected key event, we may have compatibility problem with our future release. E.g., the key code may be used by another key.
3. We're using some numbers which are not used on Windows (by reserved) for other platform specific key, e.g., NS_VK_META. If Windows defines new key to such point, it causes new compatibility issue on stable branch too.

Against for these issues, we should convert from native keycode to DOM keycode event like other platforms even if the result isn't changed.
Summary: Should convert from native keycode to DOM keycode for every keys → Should convert from native keycode to DOM keycode for every keys on Windows
Attached patch Patch v1.0 (obsolete) — Splinter Review
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #509381 - Attachment is obsolete: true
Attached patch Patch v1.2.1 (obsolete) — Splinter Review
This patch defines some keycodes for all ASCII characters can be indicated by keycode. And also defines Win keycode for both VK_WIN_L and VK_WIN_R.

For OEM keys, this patch returns a keycode for the unshifted character of the key. If the character isn't in ASCII range, uses shifted character. If it's also out of ASCII range, returns 0.

By these changes:

* Windows key have a specific DOM keycode.
* We will never dispatch DOM key events with undefined DOM keycode.
* On all keyboard layouts, every OEM key has a consistent keycode which doesn't depend on any modifier keys.
* On all keyboard layouts, keycode of every OEM key matches its character. For example, VK_OEM_1 is mapped to ';' and ':'. We're currently mapping it to NS_VK_SEMICOLON. But some keyboard layout may use VK_OEM_1 for other characters. This patch prevents such strange situation.
Attachment #511272 - Attachment is obsolete: true
Attachment #521848 - Flags: review?(jmathies)
I'm not a base widget peer, you should probably ask roc about the widget changes. You'll also need a dom events peer, which is where I'd start. I honestly can't review this as I'm not familiar with the code at all.
Comment on attachment 521848 [details] [diff] [review]
Patch v1.2.1

Okay.

Smaug, would you review about the keycode changes? This patch makes the behavior which I wrote in bug 631165.
Attachment #521848 - Flags: review?(jmathies) → review?(Olli.Pettay)
I need to re-review what DOM 3 Events say about this all
(although it does not specify key/charCodes)
smaug: ping
Status: NEW → ASSIGNED
Sorry for the delay.
All hands and work week and then bad jetlag has taken my time.
I'll try to get to this asap.
Comment on attachment 521848 [details] [diff] [review]
Patch v1.2.1

Ok, this is very tricky to review.

+    // Following keycodes are OEM keys which are keycodes for non-alphabet and
+    // non-numeric keys, we should compute each keycode of them from unshifted
+    // character which is inputted by each key.  But if the unshifted character
+    // is not an ASCII character but shifted character is an ASCII character,
+    // we should refer it.
Where is this information coming?


Do we need to change non-Windows widget code too?
(In reply to comment #10)
> +    // Following keycodes are OEM keys which are keycodes for non-alphabet and
> +    // non-numeric keys, we should compute each keycode of them from unshifted
> +    // character which is inputted by each key.  But if the unshifted
> character
> +    // is not an ASCII character but shifted character is an ASCII character,
> +    // we should refer it.
> Where is this information coming?

An older draft of DOM3 Events suggested that keyCode holds a system- and implementation-dependent numerical code signifying the unmodified identifier associated with the key pressed. (Currently the keyCode related document was removed, I pointed it bug 447757 comment 23) This idea makes sense for me. I think that keyCode should be an identifier of keys in the keyboard layout. So, Shift state and other modifier key state (except NumLock) shouldn't be changed them.

And also, if a key don't input ASCII character, we need to use 0 for its keyCode. But it's not good behavior for some keyboard layouts. I assume that an ASCII character is inputted by only one normal key on every keyboard layout. Therefore, I think we can also use shifted character for computing keyCode which is an identifier of the key in the keyboard layout.

> Do we need to change non-Windows widget code too?

Of course. This behavior change is mainly needed for Linux and Mac, and consistency between the all tire-1 platforms.

Especially on Linux, keyCode computing doesn't work fine for non-Latin keyboard layout. See bug 447757. We need rules for computing keyCode on all platforms.

For discussing the rules, I filed bug 631165.
Attached patch Patch v1.2.2 (obsolete) — Splinter Review
updated for latest trunk.
Attachment #521848 - Attachment is obsolete: true
Attachment #528268 - Flags: review?(Olli.Pettay)
Attachment #521848 - Flags: review?(Olli.Pettay)
There was just an update to DOM 3 Events. It has now non-normative
section about legacy key events.
http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#keys-keyCode-charCode-which

Need to check if the new section is correct and whether we need changes 
to the patch.
I'll try to review the patch this weekend. (But it is a bit tricky one to review)
 
> And also, if a key don't input ASCII character, we need to use 0 for its
> keyCode. 
Why. D3E states (non-normatively) that
"Return the virtual key code from the operating system."


> But it's not good behavior for some keyboard layouts. I assume that
> an ASCII character is inputted by only one normal key on every keyboard
> layout. Therefore, I think we can also use shifted character for computing
> keyCode which is an identifier of the key in the keyboard layout.
Right, so this is what is defined in D3E.

> > Do we need to change non-Windows widget code too?
> 
> Of course. This behavior change is mainly needed for Linux and Mac, and
> consistency between the all tire-1 platforms.
We need to land all the changes simultaneously, I think. Or at least
they need to be done for the same Fx release.
Comment on attachment 528268 [details] [diff] [review]
Patch v1.2.2


>@@ -532,17 +538,17 @@ KeyboardLayout::GetKeyIndex(PRUint8 aVir
>     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 37, -1,   // 60
>     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,   // 70
>     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,   // 80
>     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,   // 90
>     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,   // A0
>     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 38, 39, 40, 41, 42, 43,   // B0
>     44, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,   // C0
>     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 45, 46, 47, 48, 49,   // D0
>-    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,   // E0
>+    -1, 50, 51, 52, 53, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,   // E0
>     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1    // F0
>   };
Could you explain this change.
Attachment #528268 - Flags: review?(Olli.Pettay) → review+
(In reply to comment #14)
>  
> > And also, if a key don't input ASCII character, we need to use 0 for its
> > keyCode. 
> Why. D3E states (non-normatively) that
> "Return the virtual key code from the operating system."

If we returned native virtual keycode, which can conflict with another our key code or another key on another platform. Furthermore, if some web app developers tested on only one platform, they were confused by complaint from other platform users. By using 0, we can indicate it clearly that we don't support the key by keyCode property.

> > But it's not good behavior for some keyboard layouts. I assume that
> > an ASCII character is inputted by only one normal key on every keyboard
> > layout. Therefore, I think we can also use shifted character for computing
> > keyCode which is an identifier of the key in the keyboard layout.
> Right, so this is what is defined in D3E.

D3E said:

> keyCode is an index of the key itself

and the value isn't defined for compatibility with legacy contents which switch the behavior by UA.

I think that it's better behavior to return non-zero value based on shifted character rather than zero.

> > > Do we need to change non-Windows widget code too?
> > 
> > Of course. This behavior change is mainly needed for Linux and Mac, and
> > consistency between the all tire-1 platforms.
> We need to land all the changes simultaneously, I think. Or at least
> they need to be done for the same Fx release.(In reply to comment #14)

Okay.

I almost finished the work for GTK2 in my local tree. But it's stopped by other pending patches. I'll contact the reviewer.

And I'm working on cocoa widget refactoring (bug 519972), it's needed before this. I'm planing that it's going to be finished the end of this month.

(In reply to comment #15)
> Comment on attachment 528268 [details] [diff] [review] [review]
> Patch v1.2.2
> 
> 
> >@@ -532,17 +538,17 @@ KeyboardLayout::GetKeyIndex(PRUint8 aVir
> >     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 37, -1,   // 60
> >     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,   // 70
> >     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,   // 80
> >     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,   // 90
> >     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,   // A0
> >     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 38, 39, 40, 41, 42, 43,   // B0
> >     44, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,   // C0
> >     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 45, 46, 47, 48, 49,   // D0
> >-    -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,   // E0
> >+    -1, 50, 51, 52, 53, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,   // E0
> >     -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1    // F0
> >   };
> Could you explain this change.
E1, E3 and E4 are non-named virtual keycode for OEM keys. And E2 is VK_OEM_102 key. They can be printable char input key. E.g., E2 is '<' key on French keyboard layout. I'm not sure about other keys. However, if this is negative value, KeyboardLayout::LoadLayout() doesn't initialize the input character about the key. So, we need to unlock the keys.

However, probably, KeyboardLayout::IsPrintableCharKey() is buggy. It should be actual input character too. I'll update the patch.
(In reply to comment #16)

> I almost finished the work for GTK2 in my local tree.
...
> 
> And I'm working on cocoa widget refactoring (bug 519972), it's needed before
> this. I'm planing that it's going to be finished the end of this month.

Awesome!
> However, probably, KeyboardLayout::IsPrintableCharKey() is buggy. It should be actual input character too. I'll update the patch.

The method looks ok for me. The callers doesn't believe that the key always inputs a printable char. Perhaps, the method name should be PossiblePrintableCharKey() or something but this shouldn't be fixed on this bug.
Attachment #528268 - Flags: review?(jmathies)
Comment on attachment 528268 [details] [diff] [review]
Patch v1.2.2

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

::: widget/src/windows/nsWindow.cpp
@@ +6792,1 @@
>              }

> switch (DOMKeyCode) {
> ..
> }

Can we make this big switch conversion a static method in KeyboardLayout?
Attachment #528268 - Flags: review?(jmathies) → review+
Commenting on DOM_VK_WIN:

It may have been an option to use DOM_VK_META here, as the left Microsoft "Windows" key and the Apple "Command" key are physically equivalent.
We use meta for the command key as specified for KeyboardsEvent.metaKey on Macintosh systems:
http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-KeyboardEvent-metaKey

We don't seem to set isMeta for the Windows keys in the NT port and the documentation of keyArg for KeyboardEvent.getModifierState lists "Meta" and "Win" separately in the same list of defined modifier keys.

Functionally, "Windows" and "Command" keys have been used in quite different ways on their different operating systems, so perhaps it is safest to keep them separate.
(In reply to comment #20)
> Functionally, "Windows" and "Command" keys have been used in quite different
> ways on their different operating systems, so perhaps it is safest to keep
> them separate.

I think so, and there is another compatibility issue with GTK. GTK may map the Win key as Super key rather than Meta key. So, even if we use DOM_VK_META for the left win key both on Win and Mac, the same physical key may send DOM_VK_SUPER on Linux. I think this could make the Linux users unhappy. The fact that Win and Mac send different key code may help web app developers to understand the issue.
Attached patch Patch v1.3 (obsolete) — Splinter Review
I moved the big switch and related code to WidgetUtils::GetLatinCharCodeForKeyCode() because the logic will be shared with other platforms.

Roc, would you sr the interface change and check the WidgetUtils' change?
Attachment #528268 - Attachment is obsolete: true
Attachment #539523 - Flags: superreview?(roc)
Why not remove all the definitions of the NS_VK_ constants and have their users just use the nsIDOMKeyEvent constants?
Comment on attachment 539523 [details] [diff] [review]
Patch v1.3

Review of attachment 539523 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #539523 - Flags: superreview?(roc) → superreview+
(In reply to comment #23)
> Why not remove all the definitions of the NS_VK_ constants and have their
> users just use the nsIDOMKeyEvent constants?

I guess that may be too verbose. What you could do to simplify things a little is have a file nsVKList.h which contains lines of the form
DEFINE_VK(COLON)
and then in nsGUIEvent, do
enum {
#define DEFINE_VK(k) NS_VK_##k = nsIDOMEvent::DOM_VK_##k,
#include "nsVKList.h"
#undef DEFINE_VK
};
Attached patch Additional Patch v1.0 (obsolete) — Splinter Review
We need to start "_" for the key names because TAB and DELETE cannot be used.

And also VK_* cannot be used too because they are defined on Windows.
Attachment #543061 - Flags: review?(roc)
Comment on attachment 543061 [details] [diff] [review]
Additional Patch v1.0

Review of attachment 543061 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #543061 - Flags: review?(roc) → review+
Attached patch Patch v1.3.1 (obsolete) — Splinter Review
fix a nit in the tests.
Attachment #539523 - Attachment is obsolete: true
updated for latest trunk.
Attachment #566704 - Attachment is obsolete: true
Attachment #566705 - Attachment is obsolete: true
updating for latest trunk.
Attachment #606097 - Attachment is obsolete: true
Updated for latest trunk.

This patch doesn't change the logic to compute DOM keycode but nsWindow gets the result via NativeKey which was implemented by bug 166240. For confirmation, I'd like you to review this again.
Attachment #610828 - Attachment is obsolete: true
Attachment #621850 - Flags: review?(jmathies)
Attachment #621850 - Flags: review?(jmathies) → review+
http://hg.mozilla.org/mozilla-central/rev/e9e804a67d29
http://hg.mozilla.org/mozilla-central/rev/ad5db4e9d2f2
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Depends on: 757330
No longer depends on: 757330
No longer depends on: 756811
No longer depends on: 769548
Depends on: 772073
Depends on: 786549
Depends on: 787193
Depends on: 797481
Depends on: 797486
Depends on: 879617
Depends on: 892846
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: