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

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"
 #include "base/strings/sys_string_conversions.h"
@@ skipping 49 matching lines @@
   NSPoint point_in_window =
       ui::ConvertPointFromScreenToWindow(target_window, point_in_screen);
   NSRect content_rect =
       [target_window contentRectForFrameRect:[target_window frame]];
   return gfx::Point(point_in_window.x,
                     NSHeight(content_rect) - point_in_window.y);
 }
 
 // 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;
 }
 
 // Returns true if |client| has RTL text.
 bool IsTextRTL(const ui::TextInputClient* client) {
   gfx::Range text_range;
   base::string16 text;
   return client && client->GetTextRange(&text_range) &&
@@ skipping 105 matching lines @@
   // This is a special case for which the complete string should should be
   // returned. NSTextView also follows this, though the same is not mentioned in
   // NSTextInputClient documentation.
   if (!requested_range.IsValid())
     *actual_range = text_range;
 
   client->GetTextFromRange(*actual_range, &substring);
   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]);
+
+  // [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([[event characters] characterAtIndex:0],
+                      ui::KeyboardCodeFromNSEvent(event), ui::EF_NONE);

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

Modified this. Earlier we were just casting the character to ui::KeyboardCode,
which is incorrect. 

+}
+
 }  // 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
 // TextInputClient, the keyCode that toolkit-views expects internally.
 // For example, moveToLeftEndOfLine: would pass ui::VKEY_HOME in non-RTL locales
 // even though the Home key on Mac defaults to moveToBeginningOfDocument:.
 // This approach also allows action messages a user
 // may have remapped in ~/Library/KeyBindings/DefaultKeyBinding.dict to be
 // catered for.
 // Note: default key bindings in Mac can be read from StandardKeyBinding.dict
@@ skipping 133 matching lines @@
     return;
 
   views::FocusManager* focusManager =
       hostedView_->GetWidget()->GetFocusManager();
   if (focusManager)
     focusManager->SetKeyboardAccessible([NSApp isFullKeyboardAccessEnabled]);
 }
 
 // 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);

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

nit: blank line before

[karandeepb, 2016/06/06 11:21:10]

Done.

+  if (DispatchEventToMenu(hostedView_->GetWidget(), *event))
     return;
 
-  hostedView_->GetWidget()->GetInputMethod()->DispatchKeyEvent(&event);
+  hostedView_->GetWidget()->GetInputMethod()->DispatchKeyEvent(event);
 }
 
 - (void)handleAction:(int)commandId
              keyCode:(ui::KeyboardCode)keyCode
              domCode:(ui::DomCode)domCode
           eventFlags:(int)eventFlags {
   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
   // performed.
   if (commandId && textInputClient_ &&
       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);
 }
 
 - (void)onFullKeyboardAccessModeChanged:(NSNotification*)notification {
   DCHECK([[notification name]
       isEqualToString:kFullKeyboardAccessChangedNotification]);
   [self updateFullKeyboardAccess];
 }
 
 - (void)undo:(id)sender {
@@ skipping 213 matching lines @@
 
 // NSResponder implementation.
 
 - (void)keyDown:(NSEvent*)theEvent {
   // Convert the event into an action message, according to OSX key mappings.
   inKeyDown_ = YES;
   [self interpretKeyEvents:@[ theEvent ]];
   inKeyDown_ = NO;
 }
 
+- (void)keyUp:(NSEvent*)theEvent {
+  // Generate a synthetic ui::KeyEvent for toolkit-views.
+  ui::KeyEvent event(theEvent);
+  hostedView_->GetWidget()->GetInputMethod()->DispatchKeyEvent(&event);

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

Should allow the menu to intercept keyUp as well, otherwise the up/downs won't
be balanced

[karandeepb, 2016/06/06 11:21:10]

Done. 

+}
+
 - (void)scrollWheel:(NSEvent*)theEvent {
   if (!hostedView_)
     return;
 
   ui::MouseWheelEvent event(theEvent);
   hostedView_->GetWidget()->OnMouseEvent(&event);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // NSResponder Action Messages. Keep sorted according NSResponder.h (from the
 // 10.9 SDK). The list should eventually be complete. Anything not defined will
 // beep when interpretKeyEvents: would otherwise call it.
 // TODO(tapted): Make this list complete, except for insert* methods which are
 // dispatched as regular key events in doCommandBySelector:.
 
 // views::Textfields are single-line only, map Paragraph and Document commands
 // to Line. Also, Up/Down commands correspond to beginning/end of line.
 
 // The insertText action message forwards to the TextInputClient unless a menu
 // is active. Note that NSResponder's interpretKeyEvents: implementation doesn't
 // direct insertText: through doCommandBySelector:, so this is still needed to
 // 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)];
+  DCHECK_EQ(nil, [self inputContext]);
+
+  // We only handle the case where no of characters is 1. Cases not handled (not

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

nit: 'We only' -> 'Only'. Also this needs to say why it's safe to ignore the
other cases.

[karandeepb, 2016/06/06 11:21:10]

Done.

+  // an exhaustive list):
+  // - |text| contains a unicode surrogate pair, i.e. a single grapheme which
+  //   requires two 16 bit characters.
+  // - Programmatically created events.
+  // - Input from IME. But this case should not occur since inputContext is nil.
+  if ([text length] == 1) {
+    ui::KeyEvent charEvent = GetCharacterEventFromNSEvent([NSApp currentEvent]);
+    [self handleKeyDownEvent:&charEvent];
+  }
 }
 
 // Selection movement and scrolling.
 
 - (void)moveForward:(id)sender {
   IsTextRTL(textInputClient_) ? [self moveLeft:sender]
                               : [self moveRight:sender];
 }
 
 - (void)moveRight:(id)sender {
@@ skipping 374 matching lines @@
 
 - (NSUInteger)characterIndexForPoint:(NSPoint)aPoint {
   NOTIMPLEMENTED();
   return 0;
 }
 
 - (void)doCommandBySelector:(SEL)selector {
   // 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;
   }
 
   if ([self respondsToSelector:selector])
     [self performSelector:selector withObject:nil];
   else
     [[self nextResponder] doCommandBySelector:selector];
 }
 
 - (NSRect)firstRectForCharacterRange:(NSRange)range
@@ skipping 10 matching lines @@
   return textInputClient_ && textInputClient_->HasCompositionText();
 }
 
 - (void)insertText:(id)text replacementRange:(NSRange)replacementRange {
   if (!hostedView_)
     return;
 
   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) ==
-              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));
   }
 }
 
 - (NSRange)markedRange {
   if (!textInputClient_)
     return NSMakeRange(NSNotFound, 0);
 
   gfx::Range range;
@@ skipping 65 matching lines @@
   }
 
   return [super accessibilityAttributeValue:attribute];
 }
 
 - (id)accessibilityHitTest:(NSPoint)point {
   return [hostedView_->GetNativeViewAccessible() accessibilityHitTest:point];
 }
 
 @end