MacViews: Correctly handle character events when there's an active TextInputClient.

ui/views/cocoa/bridged_content_view.mm

Index: ui/views/cocoa/bridged_content_view.mm
diff --git a/ui/views/cocoa/bridged_content_view.mm b/ui/views/cocoa/bridged_content_view.mm
index 15ac3500bc7c36ed0e51d48931e75860ac9ef2ce..b11bf6727f963a50e9c88d2f8e5298694de530a5 100644
--- a/ui/views/cocoa/bridged_content_view.mm
+++ b/ui/views/cocoa/bridged_content_view.mm
@@ -507,6 +507,21 @@ ui::TextEditCommand GetTextEditCommandForMenuAction(SEL action) {
 
   bool isCharacterEvent = keyDownEvent_ && [text length] == 1;
 
+  // Pass the character event to the View hierarchy. Cases this handles (non-
+  // exhaustive)-
+  //    - Space key presses on controls. Unlike Tab and newline which have
+  //      corresponding action messages, an insertText: message is generated for
+  //      the Space key (insertText:replacementRange: when there's an active
+  //      input context).
+  //    - Menu mnemonic selection.
+  // Currently there seems to be no use case to pass non-character events
+  // routed from insertText: handlers, to the View hierarchy.
+  if (isCharacterEvent) {
+    ui::KeyEvent charEvent = GetCharacterEventFromNSEvent(keyDownEvent_);

[tapted, 2017/01/11 14:54:20]

I only discovered it by chance, but there's a super-subtle bug introduced by
having this here that results in the DCHECK on line 207 getting hit, e.g., when
pressing Alt+q. That inserts "œ" which has [text length] == 1, but [[event
characters] length] == 2.

I think we still want mnemonic selection for alt+q (although perhaps
MenuController should be a textInputClient / PrefixSelector like combobox..). On
native mac menus, it jumps to 'o', which is presumably [event characters][0], so
I think we just need to update the DCHECK on line 207 (e.g. to check >0) with a
comment about Alt+q, unless you see something else.

[karandeepb, 2017/01/13 03:13:38]

Alt+q causes [[event character] length] == 1 in my testing, however [[event
characters] length] = 2 and [text length] =1 is possible as you said (See code
comments). I discovered some more subtle behavior. Hopefully the comments in
code should be self explanatory. Decided to not use keyDownEvent_ = nil to mark
the key event as handled, since it's useful to have the correct key down event
for the duration of keyDown:, especially when multiple insertText: action
messages can be generated for a single key down.

Let me know if any of this does not make sense to you.

[tapted, 2017/01/13 20:02:44]

Acknowledged.

+    [self handleKeyEvent:&charEvent];
+    keyDownEvent_ = nil;  // Handled.
+  }

[tapted, 2017/01/11 14:54:20]

part of me is surprised we don't accidentally handle events twice this way, but
I guess `DispatchKeyEvent` never actually makes it to InsertChar.

[karandeepb, 2017/01/13 03:13:38]

Reading the code, I think the flow on Mac is-
InputMethodMac::DispatchKeyEvent -> InputMethodBase::DispatchKeyEventPostIME ->
BridgedNativeWidget::DispatchKeyEventPostIME -> Widget::OnKeyEvent. 

which is quite different from other platforms where Input method interacts with
the TextInputClient. 

Unrelated, seeing the code in BridgedNativeWidget::DispatchKeyEventPostIME,
seems there's something wrong. We call Widget::OnKeyEvent and also
FocusManager::OnKeyEvent. Widget::OnKeyEvent also calls
FocusManager::OnKeyEvent. May also be related to crbug.com/679522.

[tapted, 2017/01/13 20:02:44]

nothing seems too out of place yet.. but yes - something to investigate :/

+
   // Forward the |text| to |textInputClient_| if no menu is active.
   if (textInputClient_ && ![self activeMenuController]) {
     // If a single character is inserted by keyDown's call to
@@ -528,23 +543,6 @@ ui::TextEditCommand GetTextEditCommandForMenuAction(SEL action) {
     } else {
       textInputClient_->InsertText(base::SysNSStringToUTF16(text));
     }
-
-    keyDownEvent_ = nil;  // Handled.
-    return;
-  }
-
-  // Only handle the case where no. of characters is 1. Cases not handled (not
-  // an exhaustive list):
-  // - |text| contains a unicode surrogate pair, i.e. a single grapheme which
-  //   requires two 16 bit characters. Currently Views menu only supports
-  //   mnemonics using a single 16 bit character, so it is ok to ignore this
-  //   case.
-  // - Programmatically created events.
-  // - Input from IME. But this case should not occur since inputContext is
-  //   nil.
-  if (isCharacterEvent) {
-    ui::KeyEvent charEvent = GetCharacterEventFromNSEvent(keyDownEvent_);
-    [self handleKeyEvent:&charEvent];
     keyDownEvent_ = nil;  // Handled.
   }
 }