457523 - Caps Lock key events are never fired from cocoa widgets

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

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

Attachment: Patch v1.0

Caps Lock key events are not fired from cocoa widgets. The event is needed in bug 259059.

At flagsChanged, we can send keydown events, however, we cannot get the timing of key up. But the key up event is not needed in bug 259059. So, currently, the attached patch is enough.

Comment 1

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

The reason what I don't check the event type is: The document of Apple doesn't check the event type. ( http://developer.apple.com/qa/qa2007/qa1519.html )

And the reasons what I add a new if block for the CapsLock case:

1. The CapsLock key behavior is different from other modifier keys behavior. When other modifier keys are pressed, the state is turned on. And when they are released, the state is turned off. However, When CapsLock key is pressed, the state is toggled. So, the state is not turned off by key up the state.

2. In the loop, we get the flag off timing by following way: mLastModifierState has the flag, but the current modifier flags doesn't have the state. However, mLastModifierState is only set in flagsChanged. So, if the CapsLock state was turned on when another window has focus, mLastModifierState may not correct state.

Even if I move the CapsLock related code to the loop, I need to change the code in the loop. It's not good for current other modifier key processing.

Comment 2

[Josh Aas]

Comment on attachment 340782 [details] [diff] [review]
Patch v1.0

+  return (resp == (NSResponder*)self) ? YES : NO;

This can just be "return (resp == (NSResponder*)self);"

   // Fire key up/down events for the modifier keys (shift, alt, ctrl, command).
-  if ([theEvent type] == NSFlagsChanged) {
+  else if ([theEvent type] == NSFlagsChanged) {

Please put the comment here inside the "else if" block.

Add a comment in the code explaining why you aren't processing the "([theEvent keyCode] == kCapsLockKeyCode)" case the same way you process the "([theEvent type] == NSFlagsChanged)" case (mLastModifierState...).

Comment 3

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

Attachment: Patch v1.1

Comment 4

[Josh Aas]

Comment on attachment 341132 [details] [diff] [review]
Patch v1.1

+  // CapsLock state and other modifier states are different:
+  //   CapsLock state is *toggled* when the CapsLock key is pressed.  However,
+  //   when the other modifier keys are pressed, the modifier flags are set.
+  //   And when the keys are release, the modifier flags are removed.  And also
+  //   mLastModifierState is set only when this view is first responder.
+  //   Therefore, we cannot trust mLastModifierState for CapsLock state.
+  //   Because CapsLock may be turned on when another window was active.  So,
+  //   CapsLock key processing and other modifier key processing should be
+  //   separated.

That comment is a little unclear, maybe use this:

CapsLock state and other modifier states are different:
CapsLock state does not revert when the CapsLock key goes up, as the modifier state does for other modifier keys on key up. Also, mLastModifierState is set only when this view is the first responder. We cannot trust mLastModifierState to accurately reflect the state of CapsLock since CapsLock maybe have been toggled when another window was active.

Thanks!

Comment 5

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

Attachment: Patch v1.1.1

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 341228 [details] [diff] [review]
Patch v1.1.1

Can we have an automated test for this?

Comment 7

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

(In reply to comment #6)
> (From update of attachment 341228 [details] [diff] [review])
> Can we have an automated test for this?

This key event uses irregular method that is "flagsChanged", it's not used "keyDown" and "keyUp". What do you think? You should think that I should change |SynthesizeNativeKeyEvent|?

Comment 8

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

(In reply to comment #7)
> You should think that I should change |SynthesizeNativeKeyEvent|?

I meant that "Do you think that I should add a new feature which is flagsChanged events testable code for special keys in |SynthesizeNativeKeyEvent|?", sorry.

Comment 9

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

checked-in the patch, first.

I'll work for the test after bug 440457. Because it also changes the test file.