Closed
Bug 843236
Opened 12 years ago
Closed 11 years ago
Defect - Send the correct DOM keycodes in keyboard events from metro widget for US and non-US keyboards
Categories
(Firefox for Metro Graveyard :: Input, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 25
People
(Reporter: TimAbraldes, Assigned: masayuki)
References
Details
(Whiteboard: feature=defect c=tbd u=tbd p=0)
Attachments
(2 files, 1 obsolete file)
2.08 KB,
text/html
|
Details | |
36.25 KB,
patch
|
TimAbraldes
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Blocks: metrov1triage
Assignee | ||
Comment 1•12 years ago
|
||
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.
Updated•12 years ago
|
Assignee | ||
Comment 2•12 years ago
|
||
FYI: Perhaps, I can work on separating keyboard message handling code from nsWindow to KeyboardLayout in this April. Then, MetroInput can use same method.
Reporter | ||
Comment 3•12 years ago
|
||
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.
Flags: needinfo?(asa)
Comment 4•12 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)
Assignee | ||
Comment 5•12 years ago
|
||
Updated•12 years ago
|
Updated•11 years ago
|
Blocks: metrov1defect&change
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•11 years ago
|
Priority: -- → P2
Assignee | ||
Comment 6•11 years ago
|
||
I'll try to fix this bug on next week.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Updated•11 years ago
|
QA Contact: jbecerra
Assignee | ||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
Sorry for the spam.
Attachment #769710 -
Attachment is obsolete: true
Attachment #769710 -
Flags: review?(tabraldes)
Attachment #769712 -
Flags: review?(tabraldes)
Reporter | ||
Comment 9•11 years ago
|
||
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+
Assignee | ||
Comment 10•11 years ago
|
||
(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)
Reporter | ||
Comment 11•11 years ago
|
||
(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)
Assignee | ||
Comment 12•11 years ago
|
||
Comment 13•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Updated•11 years ago
|
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•