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 742 matching lines @@
     case ui::TEXT_INPUT_TYPE_NONE:
     case ui::TEXT_INPUT_TYPE_PASSWORD:
       return nil;
     default:
       return [super inputContext];
   }
 }
 
 // NSResponder implementation.
 
+- (BOOL)performKeyEquivalent:(NSEvent*)theEvent {

[tapted, 2016/11/17 08:20:14]

I think there's more we have to do here. Have a look at -performKeyEquivalent:
in render_widget_host_view_mac.mm . We probably want the firstResponder check,
and then I really like the idea of a shared method like
-keyEvent:wasKeyEquivalent: which both keyDown and performKeyEquivalent call. We
shouldn't need separate performKeyEquivalentEvent_ and keyDownEvent_ members.

I'm also not sure if we want to skip the bubbling in doCommandBySelector -- i.e.
I think we want that NSBeep() -- what are the cases that beep when we don't want
them to?

The private method override `- (BOOL)_wantsKeyDownForEvent:(NSEvent*)event` in
render_widget_host_view_mac.mm looks really useful as well. Maybe it can even
simplify our performKeyEquivalent: further.

[themblsha, 2016/11/17 17:52:16]

Good catch!
 
https://codereview.chromium.org/2074643003/#msg73

Accelerators that result on noop: command will beep, see my older comment for a
list. If we handle both performKeyEquivalent: and keyDown: it could result in
double-beeps, which would be subpar.
Whoa! That's the ticket! Now your original suggestion is more feasible.

+  // Normally having the keyDown: would be enough for processing accelerator
+  // events, but some only result in keyUp: calls. The approach is similar to
+  // what we're doing in keyDown: with the exception that we should not fallback
+  // to next responder in doCommandBySelector:, as this will result in NSBeep().
+
+  // Convert the event into an action message, according to OSX key mappings.
+  performKeyEquivalentEvent_ = theEvent;
+  performKeyEquivalentEventHandled_.reset();
+  [self interpretKeyEvents:@[ theEvent ]];
+  if (auto result = performKeyEquivalentEventHandled_) {
+    DCHECK(!performKeyEquivalentEvent_);
+    performKeyEquivalentEventHandled_.reset();
+    return *result;
+  }
+
+  DCHECK(performKeyEquivalentEvent_);
+  ui::KeyEvent event(performKeyEquivalentEvent_);
+  [self handleKeyEvent:&event];
+  performKeyEquivalentEvent_ = nil;
+  performKeyEquivalentEventHandled_.reset();
+  return event.handled() ? YES : NO;
+}
+
 - (void)keyDown:(NSEvent*)theEvent {
   // Convert the event into an action message, according to OSX key mappings.
   keyDownEvent_ = theEvent;
   [self interpretKeyEvents:@[ theEvent ]];
+
   keyDownEvent_ = nil;
 }
 
 - (void)keyUp:(NSEvent*)theEvent {
   ui::KeyEvent event(theEvent);
   [self handleKeyEvent:&event];
 }
 
 - (void)scrollWheel:(NSEvent*)theEvent {
   if (!hostedView_)
@@ skipping 502 matching lines @@
   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];
+    performKeyEquivalentEvent_ = nil;
+    performKeyEquivalentEventHandled_ = YES;
     return;
   }
 
-  if ([self respondsToSelector:selector])
+  if ([self respondsToSelector:selector]) {
     [self performSelector:selector withObject:nil];
-  else
-    [[self nextResponder] doCommandBySelector:selector];
+    performKeyEquivalentEvent_ = nil;
+    performKeyEquivalentEventHandled_ = YES;
+    return;
+  }
+
+  if (performKeyEquivalentEvent_) {
+    ui::KeyEvent event(performKeyEquivalentEvent_);
+    [self handleKeyEvent:&event];
+    performKeyEquivalentEvent_ = nil;
+    performKeyEquivalentEventHandled_ = event.handled() ? YES : NO;
+    // If the event was not handled we must return NO in performKeyEquivalent:
+    // so the menu bar could have a chance on handling the event. Regardless of
+    // that, we don't want to call doCommandBySelector: on nextResponder, as
+    // this will probably result in NSBeep().
+    return;
+  }
+
+  [[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();
   return gfx::ScreenRectToNSRect(rect);
@@ skipping 126 matching lines @@
   return [hostedView_->GetNativeViewAccessible() accessibilityHitTest:point];
 }
 
 - (id)accessibilityFocusedUIElement {
   if (!hostedView_)
     return nil;
   return [hostedView_->GetNativeViewAccessible() accessibilityFocusedUIElement];
 }
 
 @end