MacViews: Fix accelerator handling while Omnibox is in focus.

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 257 matching lines @@
 
 @interface BridgedContentView ()
 
 // Returns the active menu controller corresponding to |hostedView_|,
 // nil otherwise.
 - (MenuController*)activeMenuController;
 
 // Passes |event| to the InputMethod for dispatch.
 - (void)handleKeyEvent:(ui::KeyEvent*)event;
 
+// Allows accelerators to be handled at different points in AppKit key event
+// dispatch. Checks for an unhandled event passed in to -keyDown: and passes it
+// to the Widget for processing. Returns YES if the Widget handles it.
+- (BOOL)handleUnhandledKeyDownAsKeyEvent;
+
 // 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
 // which lives in /System/Library/Frameworks/AppKit.framework/Resources. Do
@@ skipping 160 matching lines @@
   if (!hostedView_)
     return;
 
   DCHECK(event);
   if (DispatchEventToMenu([self activeMenuController], event))
     return;
 
   hostedView_->GetWidget()->GetInputMethod()->DispatchKeyEvent(event);
 }
 
+- (BOOL)handleUnhandledKeyDownAsKeyEvent {
+  if (!keyDownEvent_)
+    return NO;
+
+  ui::KeyEvent event(keyDownEvent_);
+  [self handleKeyEvent:&event];
+  keyDownEvent_ = nil;
+  return event.handled();
+}
+
 - (void)handleAction:(ui::TextEditCommand)command
              keyCode:(ui::KeyboardCode)keyCode
              domCode:(ui::DomCode)domCode
           eventFlags:(int)eventFlags {
   if (!hostedView_)
     return;
 
   // Generate a synthetic event with the keycode toolkit-views expects.
   ui::KeyEvent event(ui::ET_KEY_PRESSED, keyCode, domCode, eventFlags);
 
@@ skipping 37 matching lines @@
     // 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));
     }
+
+    // If -insertTextInternal: was invoked from -keyDown:, we don't want
+    // to do -handleUnhandledKeyDownAsKeyEvent.
+    keyDownEvent_ = nil;

[themblsha, 2016/11/22 14:33:48]

Fixex TextfieldTest.TextInputType_InsertionTest (and several other tests), as
otherwise we'd get duplicate text entry.

[tapted, 2016/11/23 02:09:30]

Acknowledged. The comment could be more terse, too, since it's probably needed
below. E.g.

    keyDownEvent_ = nil;  // Handled.

[themblsha, 2016/11/23 15:16:39]

Done.

     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];
+    // Currently it doesn't seem that this code path could be reached from
+    // -keyDown:. If it does, it needs an additional test case, and to clear the
+    // keyDownEvent_ to avoid duplicate text processing.
+    DCHECK(!keyDownEvent_);

[themblsha, 2016/11/22 14:33:48]

This didn't cause any problems with the tests I've tried so placed a DCHECK just
in case.

[tapted, 2016/11/23 02:09:30]

It's easy to hit this DCHECK - just focus something that isn't a textfield and
press a key. E.g. focus a button after turning on Full Keyboard Access in OSX
System Preferences.

(pressing a key on a dialog with no textfields will probably also trigger this)

[themblsha, 2016/11/23 15:16:39]

Ah. Duplicated "keyDownEvent_ = nil;  // Handled." here as well.

   }
 }
 
 - (views::DragDropClientMac*)dragDropClient {
   views::BridgedNativeWidget* bridge =
       views::NativeWidgetMac::GetBridgeForNativeWindow([self window]);
   return bridge ? bridge->drag_drop_client() : nullptr;
 }
 
 - (void)undo:(id)sender {
@@ skipping 218 matching lines @@
     case ui::TEXT_INPUT_TYPE_NONE:
     case ui::TEXT_INPUT_TYPE_PASSWORD:
       return nil;
     default:
       return [super inputContext];
   }
 }
 
 // NSResponder implementation.
 
+- (BOOL)_wantsKeyDownForEvent:(NSEvent*)event {
+  // This is a SPI that AppKit apparently calls after |performKeyEquivalent:|
+  // returned NO. If this function returns |YES|, Cocoa sends the event to
+  // |keyDown:| instead of doing other things with it. Ctrl-tab will be sent
+  // to us instead of doing key view loop control, ctrl-left/right get handled
+  // correctly, etc.
+  // (However, there are still some keys that Cocoa swallows, e.g. the key
+  // equivalent that Cocoa uses for toggling the input language. In this case,
+  // that's actually a good thing, though -- see http://crbug.com/26115 .)
+  return YES;
+}
+
 - (void)keyDown:(NSEvent*)theEvent {
   // Convert the event into an action message, according to OSX key mappings.
   keyDownEvent_ = theEvent;
   [self interpretKeyEvents:@[ theEvent ]];
-  keyDownEvent_ = nil;
+
+  // If |keyDownEvent_| wasn't cleared during -interpretKeyEvents:, it wasn't
+  // handled. Give Widget accelerators a chance to handle it.
+  [self handleUnhandledKeyDownAsKeyEvent];
+  DCHECK(!keyDownEvent_);
 }
 
 - (void)keyUp:(NSEvent*)theEvent {
   ui::KeyEvent event(theEvent);
   [self handleKeyEvent:&event];
 }
 
 - (void)scrollWheel:(NSEvent*)theEvent {
   if (!hostedView_)
     return;
@@ skipping 498 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 (keyDownEvent_ && [NSStringFromSelector(selector) hasPrefix:@"insert"]) {
-    ui::KeyEvent event(keyDownEvent_);
-    [self handleKeyEvent:&event];
+  if (keyDownEvent_ && [NSStringFromSelector(selector) hasPrefix:@"insert"])
+    return;  // Handle in -keyDown:.
+
+  if ([self respondsToSelector:selector]) {
+    [self performSelector:selector withObject:nil];
+    keyDownEvent_ = nil;
     return;
   }
 
-  if ([self respondsToSelector:selector])
-    [self performSelector:selector withObject:nil];
-  else
+  // For events that AppKit sends via doCommandBySelector:, first attempt to
+  // handle as a Widget accelerator. Forward along the responder chain only if
+  // the Widget doesn't handle it.
+  if (![self handleUnhandledKeyDownAsKeyEvent])
     [[self nextResponder] doCommandBySelector:selector];
 }
 
 - (NSRect)firstRectForCharacterRange:(NSRange)range
                          actualRange:(NSRangePointer)actualNSRange {
   gfx::Range actualRange;
   gfx::Rect rect = GetFirstRectForRangeHelper(textInputClient_,
                                               gfx::Range(range), &actualRange);
   if (actualNSRange)
     *actualNSRange = actualRange.ToNSRange();
@@ skipping 122 matching lines @@
   return [hostedView_->GetNativeViewAccessible() accessibilityHitTest:point];
 }
 
 - (id)accessibilityFocusedUIElement {
   if (!hostedView_)
     return nil;
   return [hostedView_->GetNativeViewAccessible() accessibilityFocusedUIElement];
 }
 
 @end