843236 - Defect - Send the correct DOM keycodes in keyboard events from metro widget for US and non-US keyboards

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Input, defect, P2)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Masayuki Nakano expressed concern in bug 825337 comment 1 that the DOM keycodes we send from metro widget will not be correct for non-US keyboards.

From looking at the code, it seems that MetroInput::GetMozKeyCode and MetroInput::InitializeVirtualKeyMap are duplicating functionality that KeyboardLayout::ConvertNativeKeyCodeToDOMKeyCode will give us, and that the KeyboardLayout implementation also handles non-US keyboards.  We should probably just modify MetroInput to use KeyboardLayout::ConvertNativeKeyCodeToDOMKeyCode, and remove the pieces in MetroInput that attempt to implement the same functionality.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

See bug 842927 and bug 680830. MetroWidget probably can use KeyboardLayout class and NativeKey class. I think that key event handler should be shared by these class. Then, its maintenance will be easier.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

FYI: Perhaps, I can work on separating keyboard message handling code from nsWindow to KeyboardLayout in this April. Then, MetroInput can use same method.

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Thanks Nakano-san!  Having our desktop and our immersive/Metro code utilize a common back-end for processing keyboard input would be a big step forward for the functionality of metro widget and for the maintainability of both platforms.


Asa: This bug, like bug 837293, is important for immersive/Metro Firefox to work well on machines whose primary input mechanism is not a US-english keyboard.  I think you were in the process of creating a story for bug 837293 to block; I believe that this bug should also block whatever story you create.

Comment 4

[Asa Dotzler [:asa]]

Yes, this should probably be a work item and block the same Story that (also a work item, I think) bug 837293 should block. That story should be something like "As a Metro Firefox user, I can  use my international  keyboard effectively." or something like that. I'll try to get that written up soon.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: testcase

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'll try to fix this bug on next week.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Use widget::KeyboardLayout and widget::NativeKey for handling key messages on Metrofox

I give up using MetroInput for handling the key messages since it's pretty high level layer for us. We have no way to get following WM_CHAR message at coming keydown event into MetroInput. Additionally, we will handle key messages lower level than current, see bug 887695. So, directly handling WM_(SYS)?KEY(DOWN|UP) message is the only way to share the key handling code between Desktop and Metro.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Use widget::KeyboardLayout and widget::NativeKey for handling key messages on Metrofox

Sorry for the spam.

Comment 9

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 769712 [details] [diff] [review]
Use widget::KeyboardLayout and widget::NativeKey for handling key messages on Metrofox

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

Looks great!  I tested locally and ran the mochitest-metro suite, and everything worked as expected.  Yay for deleting lots of lines of code :)

::: widget/windows/winrt/MetroWidget.cpp
@@ +147,5 @@
>    // Global initialization
>    if (!gInstanceCount) {
>      UserActivity();
>      nsTextStore::Initialize();
> +    KeyboardLayout::GetInstance()->OnLayoutChange(::GetKeyboardLayout(0));

It might make sense to create a "mKeyboardLayout" member of MetroWidget to replace the calls to KeyboardLayout::GetInstance.

@@ +446,5 @@
>  
>  nsresult
>  MetroWidget::SynthesizeNativeKeyEvent(int32_t aNativeKeyboardLayout,
>                                        int32_t aNativeKeyCode,
>                                        uint32_t aModifierFlags,

If there aren't too many callers of this function, perhaps we should remove it entirely and update the callers to use KeyboardLayout::SynthesizeNativeKeyEvent directly.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #9)

Thank you for your quick review!

> ::: widget/windows/winrt/MetroWidget.cpp
> @@ +147,5 @@
> >    // Global initialization
> >    if (!gInstanceCount) {
> >      UserActivity();
> >      nsTextStore::Initialize();
> > +    KeyboardLayout::GetInstance()->OnLayoutChange(::GetKeyboardLayout(0));
> 
> It might make sense to create a "mKeyboardLayout" member of MetroWidget to
> replace the calls to KeyboardLayout::GetInstance.

Really? KeyboardLayout::GetInstance() returns just a singleton instance. So, the run-time cost is very cheap. Additionally, it's called only from:

1. Creating the 1st instance of the MetroWidget (for initialization)
2. At WM_INPUTLANGCHANGE is received. This is dispatch only when user changes the keyboard layout. This is usually never used in most locales since most locales needs only one keyboard layout for inputting text.
3. MetroWidget::SynthesizeNativeKeyEvent(), this is automated test API. We don't need to worry the cost.

So, I believe that adding mKeyboardLayout just wastes the memory (it's very small, though). Do you really want the change??

> @@ +446,5 @@
> >  
> >  nsresult
> >  MetroWidget::SynthesizeNativeKeyEvent(int32_t aNativeKeyboardLayout,
> >                                        int32_t aNativeKeyCode,
> >                                        uint32_t aModifierFlags,
> 
> If there aren't too many callers of this function, perhaps we should remove
> it entirely and update the callers to use
> KeyboardLayout::SynthesizeNativeKeyEvent directly.

No. it's derived from nsIWidget interface which is an abstract interface of native widget. And I said above, this may be used by automated tests (needs chrome permission).

Comment 11

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #10)
> (In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #9)
> 
> Thank you for your quick review!

You're welcome!

> [...]
> 
> So, I believe that adding mKeyboardLayout just wastes the memory (it's very
> small, though). Do you really want the change??

I don't feel strongly either way; I find the code to be readable both ways and making the change won't meaningfully affect performance.  Feel free to leave as is!

> > @@ +446,5 @@
> > >  
> > >  nsresult
> > >  MetroWidget::SynthesizeNativeKeyEvent(int32_t aNativeKeyboardLayout,
> > >                                        int32_t aNativeKeyCode,
> > >                                        uint32_t aModifierFlags,
> > 
> > If there aren't too many callers of this function, perhaps we should remove
> > it entirely and update the callers to use
> > KeyboardLayout::SynthesizeNativeKeyEvent directly.
> 
> No. it's derived from nsIWidget interface which is an abstract interface of
> native widget. And I said above, this may be used by automated tests (needs
> chrome permission).

Ah I see.  Looks good then!

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09b50e6c4194

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/09b50e6c4194