757688 - Refactor KeyboardLayout

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

I'm now working on bug 680830. When I'm thinking about some complex cases of keyup events, current KeyboardLayout isn't useful because it's stateful but only stores latest keydown. So, it cannot handle if multiple keys are pressed. I think that most function can be non-stateful.

Comment 1

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

Attachment: part.1 Make KeyboardLayout::IsDeadKey() stateless

Comment 2

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

Attachment: part.1 Make KeyboardLayout::IsDeadKey() stateless

Comment 3

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

Attachment: part.2 Move nsModifierKeyState to KeyboardLayout and redesign it

Comment 4

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

Attachment: part.1 Make KeyboardLayout::IsDeadKey() stateless

Comment 5

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

Attachment: part.2 Move nsModifierKeyState to KeyboardLayout and redesign it

Comment 6

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

Attachment: part.3 Use widget::Modifiers for the arguments of public methods of KeyboardLayout

Comment 7

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

Attachment: part.1 Make KeyboardLayout::IsDeadKey() stateless

First of all, KeyboardLayout::OnKeyDown() stores a lot of information which include what can be computed anytime. That makes some methods stateful. We should make them stateless as far as possible.

This patch removes mLast* from KeyboardLayout. They are used for IsDeadKey() only. It doesn't make sense.

Comment 8

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

Attachment: part.2 Move nsModifierKeyState to KeyboardLayout and redesign it

nsModifierKeyState is useful class for initializing modifier state and specifies the state to other methods. So, it's useful for KeyboardLayout methods. And it's really handling key states. I think that it should be defined in KeyboardLayout.h.

Comment 9

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

Attachment: part.3 Don't use VirtualKey::ShiftState for the arguments of public methods of KeyboardLayout

This removes ShiftState from nsWindow. It is defined in KeyboardLayout.h and used by arguments of KeyboardLayout methods. Instead, we can use ModifierKeyState class or Modifiers flags. This makes other callers simpler.

Comment 10

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

Attachment: part.4 Remove GetShiftState() and move SetShiftState() to VirtualKey

KeyboardLayout::OnKeyDown() should take ModifierKeyState for its argument. Then, it doesn't need to get actual modifier key state from ::GetKeyboardState() and we can get rid of KeyboardLayout::GetShiftState().

Additionally, moves and renames KeyboardLayout::SetShiftState() to VirtualKey::FillKbdState() because ShiftState is now owned and used by VirtualKey class only.

Comment 11

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

Attachment: part.5 Make KeyboardLayout stateless for non-dead keys

If KeyboardLayout::OnKeyDown() returns the characters which are inputted by the key event, KeyboardLayout::GetUniChars() won't necessary. And also KeyboardLayout::mChars, mModifiersOfChars and mNumOfChars.

Then, we can rename GetUniCharsWithShiftState() to GetUniCharsAndModifiers().

However, there is a problem. It's too ugly to use PRUnichar[5], Modifiers[5] and PRUint8 for the result.

For solving this problem, this patch makes UniCharsAndModifiers struct. This makes nsWindow::OnKeyDown() simpler.

Comment 12

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

Attachment: part.6 Add numpad keys to printable key table and remove KeyboardLayout::IsNumpadKey()

This registers the NumPad keys are printable keys. This makes nsWindow simpler. I'll post -w version of this patch.

Comment 13

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

Attachment: part.6 with -w

Comment 14

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

Attachment: part.7 Make nsWindow for Windows possible to test dead keys

This bug and bug 680830 change a lot of code in key event handling. And I'm not a Western language user. So, I may take some mistakes for dead key handling.

This patch allows to test dead key handling code in KeyboardLayout::OnKeyDown().

KeyboardLayout::LoadLayout() resets dead key state if different keyboard layout is loaded. This patch allows delayed loading when restores original keyboard layout. I.e., tests can perform testing for two or more key inputs.

And also I found a serious bug of the API. ::UnloadKeyboardLayout() unloads the loaded keyboard layout from the system rather than the caller's thread or process. So, it unloads keyboard layouts which are already added by the users. This patch also checks whether the keyboard layout has been loaded on the system already.

Comment 15

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

Attachment: part.8 Make sure test_keycodes.xul emulates correct key events

If neither Ctrl key nor Alt key is pressed, the result characters of KeyboardLayout::OnKeyDown() is never used for generating keypress event.

That means that most tests in test_keycodes.xul cannot test the result of KeyboardLayout::OnKeyDown(). That really wastes.

This patch adds debug code in handler of fake char message. If the result and fake char message which is specified by tests mismatches, it aborts. So, we can see red in tinderbox.

By this change, I can see some bugs in test_keycodes.xul.

1. The test suite doesn't handle alrGr:1 case.
  1. It doesn't send Ctrl key and Alt key flags to sendNativeKeyEvent().
  2. It doesn't assume that two or more modifier keys are pressed same time. therefore, checking keydown/keyup events' ctrlKey, metaKey, altKey and shiftKey doesn't work. They are checked in isStateChangingModifierKeyEvent() by this patch. And current checking code will run only when the event is keypress event.
  3. It doesn't assume that some modifier keys are consumed by widget when the sent key event causes text input. This adds SHOULD_NOT_CAUSE_INPUT flag for it.

2. Shift + NS_NUMPAD[0-9] don't generate any characters and the key combination generates NumLock unlocked key codes. So, it doesn't make sense to test them.

3. Testing French keyboard's </> key, the results are wrong.

Comment 16

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

I think that we can make nsWindow::OnKeyDown() simpler too. However, it should be done later. It doesn't block bug 680830 and is not so important.

Comment 17

[Jim Mathies [:jimm]]

Comment on attachment 631306 [details] [diff] [review]
part.1 Make KeyboardLayout::IsDeadKey() stateless

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

::: widget/windows/nsWindow.cpp
@@ +6296,5 @@
>      case NS_VK_WIN:
>        return noDefault;
>    }
>  
> +  PRUint8 currentShiftState = KeyboardLayout::GetShiftState(aModKeyState);

You don't appear to be using this call in part1 but I'm assuming I'll come across this in subsequent patches.

Comment 18

[Jim Mathies [:jimm]]

Comment on attachment 631307 [details] [diff] [review]
part.2 Move nsModifierKeyState to KeyboardLayout and redesign it

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

::: widget/windows/KeyboardLayout.cpp
@@ +123,5 @@
> +    case NS_SIMPLE_GESTURE_EVENT:
> +    case NS_MOZTOUCH_EVENT:
> +      break;
> +    default:
> +      return;

Let's assert on this so we get a warning when someone calls InitMouseEvent with an unsupported event.

Comment 19

[Jim Mathies [:jimm]]

Comment on attachment 631312 [details] [diff] [review]
part.4 Remove GetShiftState() and move SetShiftState() to VirtualKey

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

::: widget/windows/KeyboardLayout.cpp
@@ +277,5 @@
>  
> +// static
> +void
> +VirtualKey::FillKbdState(PBYTE aKbdState,
> +                         ShiftState aShiftState)

make ShiftState const?

Comment 20

[Jim Mathies [:jimm]]

Comment on attachment 631313 [details] [diff] [review]
part.5 Make KeyboardLayout stateless for non-dead keys

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

::: widget/windows/KeyboardLayout.h
@@ +118,5 @@
> +{
> +  // Dead-key + up to 4 characters
> +  PRUnichar chars[5];
> +  Modifiers modifiers[5];
> +  PRUint32  length;

I'm not sure what our convention is here, but I think I'd prefer we use normal member naming. That way there's no confusion in the cpp code which references them.

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 631320 [details] [diff] [review]
part.7 Make nsWindow for Windows possible to test dead keys

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

::: widget/windows/KeyboardLayout.h
@@ +364,5 @@
> +  /**
> +   * LoadLayout() loads the keyboard layout.  If aLoadLater is true,
> +   * it will be done when OnKeyDown() is called.
> +   */
> +  void LoadLayout(HKL aLayout, bool aLoadLater = false);

Curious, why would we want to delay the load until a key down?

Comment 22

[Jim Mathies [:jimm]]

Comment on attachment 631324 [details] [diff] [review]
part.8 Make sure test_keycodes.xul emulates correct key events

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

With all these test changes please make sure a push to try and post results here.

Comment 23

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

(In reply to Jim Mathies [:jimm] from comment #17)
> Comment on attachment 631306 [details] [diff] [review]
> part.1 Make KeyboardLayout::IsDeadKey() stateless
> 
> Review of attachment 631306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsWindow.cpp
> @@ +6296,5 @@
> >      case NS_VK_WIN:
> >        return noDefault;
> >    }
> >  
> > +  PRUint8 currentShiftState = KeyboardLayout::GetShiftState(aModKeyState);
> 
> You don't appear to be using this call in part1 but I'm assuming I'll come
> across this in subsequent patches.

It's used in next line :-)

Comment 24

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

(In reply to Jim Mathies [:jimm] from comment #20)
> Comment on attachment 631313 [details] [diff] [review]
> part.5 Make KeyboardLayout stateless for non-dead keys
> 
> Review of attachment 631313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/KeyboardLayout.h
> @@ +118,5 @@
> > +{
> > +  // Dead-key + up to 4 characters
> > +  PRUnichar chars[5];
> > +  Modifiers modifiers[5];
> > +  PRUint32  length;
> 
> I'm not sure what our convention is here, but I think I'd prefer we use
> normal member naming. That way there's no confusion in the cpp code which
> references them.

Some public members of struct/class are not prefixed by "m". E.g., most events in nsGUIEvent.h. However, the others have "m" prefix. So, I think that either is okay. I'll add "m" for them.

Comment 25

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

(In reply to Jim Mathies [:jimm] from comment #21)
> Comment on attachment 631320 [details] [diff] [review]
> part.7 Make nsWindow for Windows possible to test dead keys
> 
> Review of attachment 631320 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/KeyboardLayout.h
> @@ +364,5 @@
> > +  /**
> > +   * LoadLayout() loads the keyboard layout.  If aLoadLater is true,
> > +   * it will be done when OnKeyDown() is called.
> > +   */
> > +  void LoadLayout(HKL aLayout, bool aLoadLater = false);
> 
> Curious, why would we want to delay the load until a key down?

During the tests, LoadLayout is called twice a SynthesizeNativeKey() call. One is for changing testing keyboard layout. The other is for restoring original keyboard layout.

When we test dead keys, the restore breaks the dead key state. It's the reason why we cannot test dead keys.

By this change, the restoring is delayed to next keydown. Therefore, it's never restored actually during tests. So, the dead key state isn't broken during testing same keyboard layout.

Comment 26

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

Finally, the tests passed! Windows machines are too slow :-(
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=7abf9311d4d5

Comment 27

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/efa71abf5c39
https://hg.mozilla.org/integration/mozilla-inbound/rev/82d06f0f02d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/3de03e1fb9dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d7af55c4536
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd9de8d2e181
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3bed04319ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5b8c6a545cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4b46347c04b

Thank you, Jim for your review!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/efa71abf5c39
https://hg.mozilla.org/mozilla-central/rev/82d06f0f02d0
https://hg.mozilla.org/mozilla-central/rev/3de03e1fb9dd
https://hg.mozilla.org/mozilla-central/rev/5d7af55c4536
https://hg.mozilla.org/mozilla-central/rev/fd9de8d2e181
https://hg.mozilla.org/mozilla-central/rev/d3bed04319ab
https://hg.mozilla.org/mozilla-central/rev/f5b8c6a545cf
https://hg.mozilla.org/mozilla-central/rev/c4b46347c04b