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

ui/views/cocoa/bridged_content_view.mm

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "ui/views/cocoa/bridged_content_view.h"
 
 #include "base/logging.h"
 #import "base/mac/mac_util.h"
 #import "base/mac/scoped_nsobject.h"
 #import "base/mac/sdk_forward_declarations.h"
@@ skipping 489 matching lines @@
 
 - (void)insertTextInternal:(id)text {
   if (!hostedView_)
     return;
 
   if ([text isKindOfClass:[NSAttributedString class]])
     text = [text string];
 
   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
     // interpretKeyEvents: then use InsertChar() to allow editing events to be
     // merged. We use ui::VKEY_UNKNOWN as the key code since it's not feasible
     // to determine the correct key code for each unicode character. Also a
     // correct keycode is not needed in the current context. Send ui::EF_NONE as
     // the key modifier since |text| already accounts for the pressed key
     // modifiers.
 
     // Also, note we don't use |keyDownEvent_| to generate the synthetic
     // ui::KeyEvent since for text inserted using an IME, [keyDownEvent_
     // characters] might not be the same as |text|. This is because
     // |keyDownEvent_| will correspond to the event that caused the composition
     // text to be confirmed, say, Return key press.
     if (isCharacterEvent) {
       textInputClient_->InsertChar(ui::KeyEvent([text characterAtIndex:0],
                                                 ui::VKEY_UNKNOWN, ui::EF_NONE));
     } 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.
   }
 }
 
 - (views::DragDropClientMac*)dragDropClient {
   views::BridgedNativeWidget* bridge =
       views::NativeWidgetMac::GetBridgeForNativeWindow([self window]);
   return bridge ? bridge->drag_drop_client() : nullptr;
 }
 
@@ skipping 922 matching lines @@
   return [hostedView_->GetNativeViewAccessible() accessibilityHitTest:point];
 }
 
 - (id)accessibilityFocusedUIElement {
   if (!hostedView_)
     return nil;
   return [hostedView_->GetNativeViewAccessible() accessibilityFocusedUIElement];
 }
 
 @end