MacViews: Modify insertText handlers to correctly handle space key and simplify menu event dispatch.

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 22add15d3c0fbf4c6d4f64a941a0052b542fe0f8..79849b53600b13c1e1d8ad3c34bc7658821aa3b5 100644
--- a/ui/views/cocoa/bridged_content_view.mm
+++ b/ui/views/cocoa/bridged_content_view.mm
@@ -67,11 +67,10 @@ gfx::Point MovePointToWindow(const NSPoint& point,
 
 // Checks if there's an active MenuController during key event dispatch. If
 // there is one, it gets preference, and it will likely swallow the event.
-bool DispatchEventToMenu(views::Widget* widget, ui::KeyboardCode key_code) {
+bool DispatchEventToMenu(views::Widget* widget, const ui::KeyEvent& event) {
   MenuController* menuController = MenuController::GetActiveInstance();
   if (menuController && menuController->owner() == widget) {
-    if (menuController->OnWillDispatchKeyEvent(0, key_code) ==
-        ui::POST_DISPATCH_NONE)
+    if (menuController->OnWillDispatchKeyEvent(event) == ui::POST_DISPATCH_NONE)
       return true;
   }
   return false;
@@ -197,13 +196,26 @@ base::string16 AttributedSubstringForRangeHelper(
   return substring;
 }
 
+// Returns a character event corresponding to |event|. |event| must be a
+// character event itself.
+ui::KeyEvent GetCharacterEventFromNSEvent(NSEvent* event) {
+  DCHECK([event type] == NSKeyDown || [event type] == NSKeyUp);
+  DCHECK_EQ(1u, [[event characters] length]);
+
+  base::char16 character = [[event characters] characterAtIndex:0];
+  // [NSEvent characters] already considers the pressed key modifiers. Hence
+  // send ui::EF_NONE as the key modifier to the KeyEvent constructor.
+  // E.g. For Alt+S, [NSEvent characters] is 'ß' and not 'S'.
+  return ui::KeyEvent(character, static_cast<ui::KeyboardCode>(character),
+                      ui::EF_NONE);
+}
+
 }  // namespace
 
 @interface BridgedContentView ()
 
-// Translates keycodes and modifiers on |theEvent| to ui::KeyEvents and passes
-// the event to the InputMethod for dispatch.
-- (void)handleKeyEvent:(NSEvent*)theEvent;
+// Passes |event| to the InputMethod for dispatch.
+- (void)handleKeyDownEvent:(ui::KeyEvent*)event;
 
 // Handles an NSResponder Action Message by mapping it to a corresponding text
 // editing command from ui_strings.grd and, when not being sent to a
@@ -357,16 +369,14 @@ base::string16 AttributedSubstringForRangeHelper(
 
 // BridgedContentView private implementation.
 
-- (void)handleKeyEvent:(NSEvent*)theEvent {
+- (void)handleKeyDownEvent:(ui::KeyEvent*)event {
   if (!hostedView_)
     return;
-
-  DCHECK(theEvent);
-  ui::KeyEvent event(theEvent);
-  if (DispatchEventToMenu(hostedView_->GetWidget(), event.key_code()))
+  DCHECK(event);
+  if (DispatchEventToMenu(hostedView_->GetWidget(), *event))
     return;
 
-  hostedView_->GetWidget()->GetInputMethod()->DispatchKeyEvent(&event);
+  hostedView_->GetWidget()->GetInputMethod()->DispatchKeyEvent(event);
 }
 
 - (void)handleAction:(int)commandId
@@ -376,7 +386,10 @@ base::string16 AttributedSubstringForRangeHelper(
   if (!hostedView_)
     return;
 
-  if (DispatchEventToMenu(hostedView_->GetWidget(), keyCode))
+  // Generate a synthetic event with the keycode toolkit-views expects.
+  ui::KeyEvent event(ui::ET_KEY_PRESSED, keyCode, domCode, eventFlags);
+
+  if (DispatchEventToMenu(hostedView_->GetWidget(), event))
     return;
 
   // If there's an active TextInputClient, schedule the editing command to be
@@ -385,8 +398,6 @@ base::string16 AttributedSubstringForRangeHelper(
       textInputClient_->IsEditCommandEnabled(commandId))
     textInputClient_->SetEditCommandForNextKeyEvent(commandId);
 
-  // Generate a synthetic event with the keycode toolkit-views expects.
-  ui::KeyEvent event(ui::ET_KEY_PRESSED, keyCode, domCode, eventFlags);
   hostedView_->GetWidget()->GetInputMethod()->DispatchKeyEvent(&event);
 }
 
@@ -620,6 +631,12 @@ base::string16 AttributedSubstringForRangeHelper(
   inKeyDown_ = NO;
 }
 
+- (void)keyUp:(NSEvent*)theEvent {
+  // Generate a synthetic ui::KeyEvent for toolkit-views.

[karandeepb, 2016/06/06 02:04:42]

Since keyUp wasn't implemented till now, Views::OnKeyReleased wasn't probably
being invoked up till now. We should check that this does not cause any
undesired behavior as far as MacViews is concerned.

+  ui::KeyEvent event(theEvent);
+  hostedView_->GetWidget()->GetInputMethod()->DispatchKeyEvent(&event);
+}
+
 - (void)scrollWheel:(NSEvent*)theEvent {
   if (!hostedView_)
     return;
@@ -644,7 +661,22 @@ base::string16 AttributedSubstringForRangeHelper(
 // handle the case when inputContext: is nil. When inputContext: returns non-nil
 // text goes directly to insertText:replacementRange:.
 - (void)insertText:(id)text {
-  [self insertText:text replacementRange:NSMakeRange(NSNotFound, 0)];
+  // This is only to prevent crash in case of tests.
+  if(!text)
+    return;
+
+  // For a string of length 1, invoke handleKeyDownEvent, so that a synthetic
+  // ui::KeyEvent is generated.
+  // TODO(karandeepb): Is there a case where [text length] != 1, since an IME
+  // should not be active when inputContext is nil.
+  DCHECK_EQ(nil, [self inputContext]);
+  if ([text length] == 1) {
+    ui::KeyEvent charEvent = GetCharacterEventFromNSEvent([NSApp currentEvent]);
+    [self handleKeyDownEvent:&charEvent];
+  }
+  else {
+    NOTREACHED();
+  }
 }
 
 // Selection movement and scrolling.
@@ -1039,7 +1071,8 @@ base::string16 AttributedSubstringForRangeHelper(
   // Like the renderer, handle insert action messages as a regular key dispatch.
   // This ensures, e.g., insertTab correctly changes focus between fields.
   if (inKeyDown_ && [NSStringFromSelector(selector) hasPrefix:@"insert"]) {
-    [self handleKeyEvent:[NSApp currentEvent]];
+    ui::KeyEvent event([NSApp currentEvent]);
+    [self handleKeyDownEvent:&event];
     return;
   }
 
@@ -1070,43 +1103,17 @@ base::string16 AttributedSubstringForRangeHelper(
   if ([text isKindOfClass:[NSAttributedString class]])
     text = [text string];
 
-  MenuController* menuController = MenuController::GetActiveInstance();
-  if (menuController && menuController->owner() == hostedView_->GetWidget()) {
-    // Handle menu mnemonics (e.g. "sav" jumps to "Save"). Handles both single-
-    // characters and input from IME. For IME, swallow the entire string unless
-    // the very first character gives ui::POST_DISPATCH_PERFORM_DEFAULT.
-    bool swallowedAny = false;
-    for (NSUInteger i = 0; i < [text length]; ++i) {
-      if (!menuController ||
-          menuController->OnWillDispatchKeyEvent([text characterAtIndex:i],
-                                                 ui::VKEY_UNKNOWN) ==

[karandeepb, 2016/06/06 02:04:42]

Can IME's be active in case of an active menu, as this code seems to imply?

[tapted, 2016/06/06 04:44:05]

That does kinda seem like something that should be supported, but maybe it's not
possible to do that in OSX right now.

-              ui::POST_DISPATCH_PERFORM_DEFAULT) {
-        if (swallowedAny)
-          return;  // Swallow remainder.
-        break;
-      }
-      swallowedAny = true;
-      // Ensure the menu remains active.
-      menuController = MenuController::GetActiveInstance();
-    }
-  }
-
-  if (!textInputClient_)
-    return;
+  // Verify inputContext is not nil, i.e. |textInputClient_| is valid and no
+  // menu is active.
+  DCHECK([self inputContext]);
 
   textInputClient_->DeleteRange(gfx::Range(replacementRange));
 
   // If a single character is inserted by keyDown's call to interpretKeyEvents:
-  // then use InsertChar() to allow editing events to be merged. The second
-  // argument is the key modifier, which interpretKeyEvents: will have already
-  // processed, so don't send it to InsertChar() as well. E.g. Alt+S puts 'ß' in
-  // |text| but sending 'Alt' to InsertChar would filter it out since it thinks
-  // it's a command. Actual commands (e.g. Cmd+S) won't go through insertText:.
+  // then use InsertChar() to allow editing events to be merged.
   if (inKeyDown_ && [text length] == 1) {
-    ui::KeyEvent char_event(
-        [text characterAtIndex:0],
-        static_cast<ui::KeyboardCode>([text characterAtIndex:0]), ui::EF_NONE);
-    textInputClient_->InsertChar(char_event);
+    textInputClient_->InsertChar(
+        GetCharacterEventFromNSEvent([NSApp currentEvent]));
   } else {
     textInputClient_->InsertText(base::SysNSStringToUTF16(text));
   }