855975 - Separate key message handlers from nsWindow to KeyboardLayout or NativeKey

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.1 Make widget::KeyboardLayout a singleton class

First of all, widget::KeyboardLayout should be a singleton class instead of a normal class but created only on instance. This makes widget::KeyboardLayout available on everywhere.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.2 Wrap MapVirtualKeyEx() API for converting native key event to VK or Unichar with widget::NativeKey

For simpler code, this patch just wraps ::MapVirtualKeyEx() with widget::NativeKey.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.3 Wrap MapVirtualKeyEx() API for converting VK to ScanCode with widget::KeyboardLayout

Similar to part.2, but this patch wraps ::MapVirtualKeyEx() for converting virtual keycode to scancode with widget::KeyboardLayout.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.4 Move nsWindow::InitKeyEvent() to widget::NativeKey::InitKeyEvent()

This patch moves nsWindow::InitKeyEvent() to widget::NativeKey::InitKeyEvent().

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.5 Move nsWindow::DispatchKeyEvent() to widget::NativeKey::DispatchKeyEvent()

This patch moves nsWindow::DispatchKeyEvent() to widget::NativeKey::DispatchKeyEvent().

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.6 Move nsWindow::OnKeyUp() to widget::NativeKey::HandleKeyUpMessage()

This patch moves nsWindow::OnKeyUp() to widget::NativeKey::HandleKeyUpMessage().

widget::NativeKey stores the native message with mMsg. All code using hwnd for handling message should use mMsg.hwnd rather than mWidget->GetWindowHandle(). mWidget should be just a widget on which DOM events are fired. So, this change allows widget::NativeKey handles messages for a window on any window.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.7 Implement widget::NativeKey::DispatchKeyDownEvent()

This patch implements widget::NativeKey::DispatchKeyDownEvent(). This will be private method later.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.8 Move nsWindow::OnChar() to widget::NativeKey::HandleCharMessage()

This patch just moves nsWindow::OnChar() to widget::NativeKey::HandleCharMessage().

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.9 Move keypress events dispatcher of nsWindow::OnKeyDown() to widget::NativeKey::DispatchKeyPressEventsWithKeyboardLayout()

This patch moves the "key hell" part of nsWindow::OnKeyDown() to widget::NativeKey::DispatchKeyPressEventsWithKeyboardLayout().

This method will be a private method later.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.10 Move nsWindow::DispatchPluginEvent() to nsWindowBase

This patch moves nsWindow::DispatchPluginEvent() to nsWindowBase::DispatchPluginEvent().

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.11 Sort out the if statement which checks whether the native keydown message needs to be handled without following char messages

This patch make a method of widget::NativeKey, NeedsToHandleWithoutFollowingCharMessages().

It checks whether all following WM_CHAR or WM_SYSCHAR messages should be removed and the keydown message handler needs to dispatch keypress events without the removed messages.

For internal use of NeedsToHandleWithoutFollowingCharMessages(), this also makes IsDeadKey() and IsPrintableKey(). These methods are also useful for making the nsWindow::OnKeyDown() simpler.

All methods which are made by this patch will be private methods later.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.12 Move special keypress event handling from nsWindow::OnKeyDown() to widget::NativeKey::DispatchKeyPressEventsAndDiscardsCharMessages

The main purpose of this patch is, the internal block of
|if (nativeKey.NeedsToHandleWithoutFollowingCharMessages()) {}|
moves to an independent method.

For that, this patch also moves nsWindow::RemoveMessageAndDispatchPluginEvent() and widget::IMEHandler::IsIMEDoingKakuteiUndo() to widget::NativeKey::RemoveMessageAndDispatchPluginEvent() and widget::NativeKey::IsIMEDoingKakuteiUndo().

Additionally, while I test this patch, I realized that WinUtils::InitMSG() doesn't initialize MSG::hwnd. It's bad for this bug's refactoring. Therefore, I also change the method by this patch.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.13 widget::NativeKey should manage following WM_*CHAR message if it's initialized with WM_*KEYDOWN

This patch adds mCharMsg to widget::NativeKey. It stores following WM_CHAR, WM_SYSCHAR or WM_DEADCHAR message of a WM_KEYDOWN or WM_SYSKEYDOWN message.

Then, this patch makes normal text input path of nsWindow::OnKeyDown() with making widget::NativeKey::IsFollowedByCharMessage() and widget::NativeKey::RemoveFollowingCharMessage().

These new methods will be private later.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.14 Move following WM_*CHAR message handler from nsWindow::OnKeyDown() to widget::NativeKey::DispatchKeyPressEventForFollowingCharMessage()

This patch moves internal block of |if (nativeKey.IsFollowedByCharMessage()) {) | in nsWindow::OnKeyDown().  I.e., the block handles normal text input path.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.15 Use widget::NativeKey::mCommittedCharsAndModifiers directly in the methods of widget::NativeKey

By the previous patches, nsWindow::OnKeyDown() doesn't use the result of widget::NativeKey::GetCommittedCharsAndModifiers(). In other words, NativeKey instances are the only user of the value.

Therefore, NativeKey should compute the value and store it in its constructor. Then, DispatchKeyPressEvent*() don't need the aInputtingChars argument.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.16 Move keydown message redirect manager from nsWindow to widget::RedirectedKeyDownMessageManager

This makes widget::RedirectedKeyDownMessageManager in KeyboardLayout.h and KeyboardLayout.cpp.

The messy code in nsWindow for this is now sorted out.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.17 Move nsWindow::OnKeyDown() to widget::NativeKey::HandleKeyDownMessage()

This patch moves nsWindow::OnKeyDown() to widget::NativeKey::HandleKeyDownMessage(). For making the patch simple, following patch will change the scope of some methods of widget::NativeKey.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.18 Merge widget::NativeKey::DispatchKeyDown() into widget::NativeKey::HandleKeyDown()

This patch merges widget::NativeKey::DispatchKeyDownEvent() into HandleKeyDownMessage() since it becomes simpler.

Note that RedirectedKeyDownMessageManager::Forget() call is removed by this patch. However, Alt+Space is never handled by HandleKeyDownMessage() with the code, therefore, the message is never redirected. So, we don't need to call it at returning at Alt+Space case.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.19 Move nsWindow::SynthesizeNativeKeyEvent() to widget::KeyboardLayout::SynthesizeNativeKeyEvent()

This patch just moves nsWindow::SynthesizeNativeKeyEvent() to widget::KeyboardLayout::SynthesizeNativeKeyEvent().

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.20 Sort out the scope of the methods of widget::NativKey

This patch makes some methods as private. Additionally, some methods which simply getter of a member such as IsDeadKey() and IsPrintableKey() are removed.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.21 Rename nsFakeCharMessage with widget::NativeKey::FakeCharMsg

This patch moves nsFakeCharMessage in nsWindowDefs.h to widget::NativeKey::FakeCharMsg in KeyboardLayout.h because the struct is only necessary in KeyboardLayout.cpp.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

Attachment: part.22 Move sModifierKeyMap from nsWindowDefs.h to KeyboardLayout.h

sModifierKeyMap in nsWindowDefs.h maps between constants of nsIWidget and native virtual keycode values. Such mapping table should be in KeyboardLayout.h since it's really necessary for key event handling.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=a4e7a76b4421

Comment 24

[Jim Mathies [:jimm]]

Comment on attachment 749679 [details] [diff] [review]
part.2 Wrap MapVirtualKeyEx() API for converting native key event to VK or Unichar with widget::NativeKey

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

::: widget/windows/KeyboardLayout.cpp
@@ +636,5 @@
> +NativeKey::ComputeVirtualKeyCodeFromScanCodeEx() const
> +{
> +  bool VistaOrLater =
> +    (WinUtils::GetWindowsVersion() >= WinUtils::VISTA_VERSION);
> +  NS_ENSURE_TRUE(!mIsExtended || VistaOrLater, 0);

Can you add a comment explaining why we exit on vista and later here?

Comment 25

[Jim Mathies [:jimm]]

Comment on attachment 749684 [details] [diff] [review]
part.4 Move nsWindow::InitKeyEvent() to widget::NativeKey::InitKeyEvent()

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

::: widget/windows/KeyboardLayout.cpp
@@ +385,4 @@
>                       const MSG& aKeyOrCharMessage,
>                       const ModifierKeyState& aModKeyState) :
> +  mWidget(aWidget), mDOMKeyCode(0), mMessage(aKeyOrCharMessage.message),
> +  mModKeyState(aModKeyState), mVirtualKeyCode(0), mOriginalVirtualKeyCode(0)

Is there any chance we might get a null widget here? Maybe we should add a debug assert that checks.

Comment 26

[Jim Mathies [:jimm]]

Comment on attachment 749685 [details] [diff] [review]
part.5 Move nsWindow::DispatchKeyEvent() to widget::NativeKey::DispatchKeyEvent()

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

::: widget/windows/KeyboardLayout.cpp
@@ +698,5 @@
>    if (!sInstance) {
>      sInstance = new KeyboardLayout();
> +    nsCOMPtr<nsIIdleServiceInternal> idleService =
> +      do_GetService("@mozilla.org/widget/idleservice;1");
> +    sIdleService = idleService.forget().get();

Please add a comment here noting this addrefs.

Comment 27

[Jim Mathies [:jimm]]

Comment on attachment 749701 [details] [diff] [review]
part.7 Implement widget::NativeKey::DispatchKeyDownEvent()

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

::: widget/windows/KeyboardLayout.h
@@ +331,5 @@
>    bool DispatchKeyEvent(nsKeyEvent& aKeyEvent,
>                          const MSG* aMsgSentToPlugin = nullptr) const;
>  
>    /**
> +   * Dspatces keydown event.  Retrns true if tte event is consumed.

nit - 'tte'

Comment 28

[Jim Mathies [:jimm]]

Comment on attachment 749766 [details] [diff] [review]
part.20 Sort out the scope of the methods of widget::NativKey

Thanks for breaking this up so well, it made it much easier to follow along and review.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57461a161f93
https://hg.mozilla.org/integration/mozilla-inbound/rev/21fb386d7995
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfb52492b116
https://hg.mozilla.org/integration/mozilla-inbound/rev/49920d7e1518
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d71ddc7a304
https://hg.mozilla.org/integration/mozilla-inbound/rev/0435d559b314
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f923c894ca4
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed82af5475f
https://hg.mozilla.org/integration/mozilla-inbound/rev/91ba04b50639
https://hg.mozilla.org/integration/mozilla-inbound/rev/fef076fed8ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3aebac825b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/658bfe5bb0ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/13350f2b2f04
https://hg.mozilla.org/integration/mozilla-inbound/rev/60420c75c496
https://hg.mozilla.org/integration/mozilla-inbound/rev/269686777a14
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b1704ec0e50
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfef49bc5691
https://hg.mozilla.org/integration/mozilla-inbound/rev/81aadfdcd59d
https://hg.mozilla.org/integration/mozilla-inbound/rev/f65ebd90ad9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/41e9211d9daa
https://hg.mozilla.org/integration/mozilla-inbound/rev/6919e1b2926e
https://hg.mozilla.org/integration/mozilla-inbound/rev/90a6e3a943b6

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/90a6e3a943b6
https://hg.mozilla.org/mozilla-central/rev/6919e1b2926e
https://hg.mozilla.org/mozilla-central/rev/41e9211d9daa
https://hg.mozilla.org/mozilla-central/rev/f65ebd90ad9e
https://hg.mozilla.org/mozilla-central/rev/81aadfdcd59d
https://hg.mozilla.org/mozilla-central/rev/cfef49bc5691
https://hg.mozilla.org/mozilla-central/rev/7b1704ec0e50
https://hg.mozilla.org/mozilla-central/rev/269686777a14
https://hg.mozilla.org/mozilla-central/rev/60420c75c496
https://hg.mozilla.org/mozilla-central/rev/13350f2b2f04
https://hg.mozilla.org/mozilla-central/rev/658bfe5bb0ea
https://hg.mozilla.org/mozilla-central/rev/d3aebac825b9
https://hg.mozilla.org/mozilla-central/rev/fef076fed8ab
https://hg.mozilla.org/mozilla-central/rev/91ba04b50639
https://hg.mozilla.org/mozilla-central/rev/0ed82af5475f
https://hg.mozilla.org/mozilla-central/rev/1f923c894ca4
https://hg.mozilla.org/mozilla-central/rev/0435d559b314
https://hg.mozilla.org/mozilla-central/rev/0d71ddc7a304
https://hg.mozilla.org/mozilla-central/rev/49920d7e1518
https://hg.mozilla.org/mozilla-central/rev/bfb52492b116
https://hg.mozilla.org/mozilla-central/rev/21fb386d7995
https://hg.mozilla.org/mozilla-central/rev/57461a161f93

Comment 31

[Jacek Caban]

I pushed a fixup for a typo in include (extra space in file name):

https://hg.mozilla.org/integration/mozilla-inbound/rev/69680cd3fe39

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/69680cd3fe39

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away until 10/14)]

(In reply to Jacek Caban from comment #31)
> I pushed a fixup for a typo in include (extra space in file name):
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/69680cd3fe39

Thank you and sorry for my mistake.

Just for my curiosity, does it cause failing to build with MinGW or something?

Comment 34

[Jacek Caban]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (Offline 5/28 - 6/1) from comment #33)
> Thank you and sorry for my mistake.

No problem.

> Just for my curiosity, does it cause failing to build with MinGW or
> something?

Yes, I found it because it broke MinGW build. I think GCC tried to find a file that contained this space in its name.