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

RESOLVED FIXED in Firefox 25

Status

Firefox for Metro
Input
P2
normal
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: TimAbraldes, Assigned: masayuki)

Tracking

Trunk
Firefox 25
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=defect c=tbd u=tbd p=0)

Attachments

(2 attachments, 1 obsolete attachment)

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.

Updated

5 years ago
Blocks: 841214
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.
Blocks: 833153
No longer blocks: 841214
FYI: Perhaps, I can work on separating keyboard message handling code from nsWindow to KeyboardLayout in this April. Then, MetroInput can use same method.
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.
Blocks: 841214
No longer blocks: 833153
Flags: needinfo?(asa)
Depends on: 855975

Comment 4

4 years ago
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.
Flags: needinfo?(asa)
Created attachment 748756 [details]
testcase

Updated

4 years ago
Blocks: 837293
No longer blocks: 841214

Updated

4 years ago
Blocks: 859003
Summary: 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 for US and non-US keyboards
Whiteboard: feature=defect c=tbd u=tbd p=0

Updated

4 years ago
Priority: -- → P2
I'll try to fix this bug on next week.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED

Updated

4 years ago
QA Contact: jbecerra
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.
Attachment #769710 - Flags: review?(tabraldes)
Created attachment 769712 [details] [diff] [review]
Use widget::KeyboardLayout and widget::NativeKey for handling key messages on Metrofox

Sorry for the spam.
Attachment #769710 - Attachment is obsolete: true
Attachment #769710 - Flags: review?(tabraldes)
Attachment #769712 - Flags: review?(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.
Attachment #769712 - Flags: review?(tabraldes) → review+
(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).
Flags: needinfo?(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!
Flags: needinfo?(tabraldes)
https://hg.mozilla.org/integration/mozilla-inbound/rev/09b50e6c4194
https://hg.mozilla.org/mozilla-central/rev/09b50e6c4194
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25

Updated

4 years ago
Blocks: 882638
No longer blocks: 859003
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.