Last Comment Bug 843236 - Defect - Send the correct DOM keycodes in keyboard events from metro widget for US and non-US keyboards
: Defect - Send the correct DOM keycodes in keyboard events from metro widget f...
Status: RESOLVED FIXED
feature=defect c=tbd u=tbd p=0
:
Product: Firefox for Metro
Classification: Client Software
Component: Input (show other bugs)
: Trunk
: x86_64 Windows 8.1
: P2 normal (vote)
: Firefox 25
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
: juan becerra [:juanb]
Mentors:
Depends on: 855975
Blocks: 837293 metrov1it10
  Show dependency treegraph
 
Reported: 2013-02-20 10:46 PST by Tim Abraldes [:TimAbraldes] [:tabraldes]
Modified: 2014-07-24 11:06 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
testcase (2.08 KB, text/html)
2013-05-13 04:45 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details
Use widget::KeyboardLayout and widget::NativeKey for handling key messages on Metrofox (36.86 KB, patch)
2013-07-01 09:21 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Splinter Review
Use widget::KeyboardLayout and widget::NativeKey for handling key messages on Metrofox (36.25 KB, patch)
2013-07-01 09:24 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
timabraldes: review+
Details | Diff | Splinter Review

Description Tim Abraldes [:TimAbraldes] [:tabraldes] 2013-02-20 10:46:12 PST
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] (Mozilla Japan) 2013-02-20 16:34:01 PST
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] (Mozilla Japan) 2013-03-27 14:50:41 PDT
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] 2013-03-28 14:16:03 PDT
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] 2013-04-15 11:57:41 PDT
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] (Mozilla Japan) 2013-05-13 04:45:10 PDT
Created attachment 748756 [details]
testcase
Comment 6 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-06-28 07:06:36 PDT
I'll try to fix this bug on next week.
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2013-07-01 09:21:07 PDT
Created attachment 769710 [details] [diff] [review]
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] (Mozilla Japan) 2013-07-01 09:24:00 PDT
Created attachment 769712 [details] [diff] [review]
Use widget::KeyboardLayout and widget::NativeKey for handling key messages on Metrofox

Sorry for the spam.
Comment 9 Tim Abraldes [:TimAbraldes] [:tabraldes] 2013-07-01 16:37:12 PDT
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] (Mozilla Japan) 2013-07-01 18:39:18 PDT
(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] 2013-07-01 19:11:41 PDT
(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] (Mozilla Japan) 2013-07-01 22:05:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/09b50e6c4194
Comment 13 Ryan VanderMeulen [:RyanVM] 2013-07-02 12:41:19 PDT
https://hg.mozilla.org/mozilla-central/rev/09b50e6c4194

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