Make accelerators work on Signin modal dialog.

Previously, only some accelerators worked in Windows/Linux [e.g. ctr + V works,
ctr + T did not]. All accelerators were busted on macOS Views. Some accelerators
were busted on macOS Cocoa.

The root cause is that SigninViewControllerDelegateViews::HandleKeyboardEvent
was not implemented on Views, and
SigninViewControllerDelegateMac::HandleKeyboardEvent was incorrectly implemented
on macOS.

This CL fixes both implementations to use the now standard logic for
redispatching key events. This CL also fixes the implementation of
CommandDispatcher on macOS to correctly detect whether an event is being
redispatched.

Change-Id: I76b1ef263fce171ae182d316f541f836ab6388ee
Bug: 776304
Reviewed-on: https://chromium-review.googlesource.com/1097206
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568052}

chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm

@@ -568,7 +568,7 @@ void NativeAppWindowCocoa::HandleKeyboardEvent(
       event.GetType() == content::NativeWebKeyboardEvent::kChar) {
     return;
   }
-  [window() redispatchKeyEvent:event.os_event];
+  [[window() commandDispatcher] redispatchKeyEvent:event.os_event];
 }
 
 void NativeAppWindowCocoa::UpdateDraggableRegionViews() {

chrome/browser/ui/cocoa/browser_window_cocoa.mm

@@ -614,7 +614,7 @@ void BrowserWindowCocoa::HandleKeyboardEvent(
 
   ChromeEventProcessingWindow* event_window =
       base::mac::ObjCCastStrict<ChromeEventProcessingWindow>(window());
-  [event_window redispatchKeyEvent:event.os_event];
+  [[event_window commandDispatcher] redispatchKeyEvent:event.os_event];
 }
 
 void BrowserWindowCocoa::CutCopyPaste(int command_id) {

chrome/browser/ui/cocoa/browser_window_utils.mm

@@ -53,7 +53,7 @@ CGFloat GetPatternVerticalOffsetWithTabStrip(bool tabStripVisible) {
       static_cast<ChromeEventProcessingWindow*>(window);
   DCHECK([event_window isKindOfClass:[ChromeEventProcessingWindow class]]);
 
-  return [event_window redispatchKeyEvent:event];
+  return [[event_window commandDispatcher] redispatchKeyEvent:event];
 }
 
 + (NSString*)scheduleReplaceOldTitle:(NSString*)oldTitle

chrome/browser/ui/cocoa/chrome_event_processing_window.mm

@@ -40,8 +40,8 @@
   commandHandler_.reset([commandHandler retain]);
 }
 
-- (BOOL)redispatchKeyEvent:(NSEvent*)event {
-  return [commandDispatcher_ redispatchKeyEvent:event];
+- (CommandDispatcher*)commandDispatcher {
+  return commandDispatcher_.get();
 }
 
 - (BOOL)defaultPerformKeyEquivalent:(NSEvent*)event {

chrome/browser/ui/cocoa/extensions/extension_view_mac.mm

@@ -81,7 +81,7 @@ void ExtensionViewMac::HandleKeyboardEvent(
   ChromeEventProcessingWindow* event_window =
       base::mac::ObjCCastStrict<ChromeEventProcessingWindow>(
           [GetNativeView() window]);
-  [event_window redispatchKeyEvent:event.os_event];
+  [[event_window commandDispatcher] redispatchKeyEvent:event.os_event];
 }
 
 void ExtensionViewMac::OnLoaded() {

chrome/browser/ui/cocoa/profiles/signin_view_controller_delegate_mac.mm

@@ -219,14 +219,12 @@ void SigninViewControllerDelegateMac::DisplayModal() {
 void SigninViewControllerDelegateMac::HandleKeyboardEvent(
     content::WebContents* source,
     const content::NativeWebKeyboardEvent& event) {
-  int chrome_command_id = [BrowserWindowUtils getCommandId:event];
-  bool can_handle_command = [BrowserWindowUtils isTextEditingEvent:event] ||
-                            chrome_command_id == IDC_CLOSE_WINDOW ||
-                            chrome_command_id == IDC_EXIT;
-  if ([BrowserWindowUtils shouldHandleKeyboardEvent:event] &&
-      can_handle_command) {
-    [[NSApp mainMenu] performKeyEquivalent:event.os_event];
-  }
+  if (![BrowserWindowUtils shouldHandleKeyboardEvent:event])
+    return;
+
+  // Redispatch the event. If it's a keyEquivalent:, this gives
+  // CommandDispatcher the opportunity to finish passing the event to consumers.
+  [[window_ commandDispatcher] redispatchKeyEvent:event.os_event];
 }
 
 void SigninViewControllerDelegateMac::CleanupAndDeleteThis() {

chrome/browser/ui/signin_view_controller_delegate.cc

@@ -89,6 +89,12 @@ void SigninViewControllerDelegate::LoadingStateChanged(
         "inline.login.showCloseButton");
 }
 
+void SigninViewControllerDelegate::HandleKeyboardEvent(
+    content::WebContents* source,
+    const content::NativeWebKeyboardEvent& event) {
+  NOTREACHED();
+}
+
 bool SigninViewControllerDelegate::CanGoBack(
     content::WebContents* web_ui_web_contents) const {
   auto* auth_web_contents = GetAuthFrameWebContents(web_ui_web_contents);

chrome/browser/ui/signin_view_controller_delegate.h

@@ -114,6 +114,11 @@ class SigninViewControllerDelegate
   void LoadingStateChanged(content::WebContents* source,
                            bool to_different_document) override;
 
+  // Subclasses must override this method to correctly handle accelerators.
+  void HandleKeyboardEvent(
+      content::WebContents* source,
+      const content::NativeWebKeyboardEvent& event) override;
+
   // This will be called by this base class when the tab-modal window must be
   // closed. This should close the platform-specific window that is currently
   // showing the sign in flow or the sync confirmation dialog. Note that this

chrome/browser/ui/signin_view_controller_interactive_uitest.cc

+// Copyright 2018 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.
+
+#include "build/build_config.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/signin_view_controller.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/interactive_test_utils.h"
+#include "components/signin/core/browser/signin_metrics.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/test/test_utils.h"
+
+class SignInViewControllerBrowserTest : public InProcessBrowserTest {
+ public:
+  void SetUpOnMainThread() override {
+    // Many hotkeys are defined by the main menu. The value of these hotkeys
+    // depends on the focused window. We must focus the browser window. This is
+    // also why this test must be an interactive_ui_test rather than a browser
+    // test.
+    ASSERT_TRUE(ui_test_utils::ShowAndFocusNativeWindow(
+        browser()->window()->GetNativeWindow()));
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(SignInViewControllerBrowserTest, Accelerators) {
+  ASSERT_EQ(1, browser()->tab_strip_model()->count());
+  browser()->signin_view_controller()->ShowSignin(
+      profiles::BUBBLE_VIEW_MODE_GAIA_SIGNIN, browser(),
+      signin_metrics::AccessPoint::ACCESS_POINT_SETTINGS);
+
+  content::WindowedNotificationObserver wait_for_new_tab(
+      chrome::NOTIFICATION_TAB_PARENTED,
+      content::NotificationService::AllSources());
+// Press Ctrl/Cmd+T, which will open a new tab.
+#if defined(OS_MACOSX)
+  ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
+      browser(), ui::VKEY_T, /*control=*/false, /*shift=*/false, /*alt=*/false,
+      /*command=*/true));
+#else
+  ASSERT_TRUE(ui_test_utils::SendKeyPressSync(
+      browser(), ui::VKEY_T, /*control=*/true, /*shift=*/false, /*alt=*/false,
+      /*command=*/false));
+#endif
+
+  wait_for_new_tab.Wait();
+
+  EXPECT_EQ(2, browser()->tab_strip_model()->count());
+}

chrome/browser/ui/views/frame/browser_frame_mac.mm

@@ -168,5 +168,6 @@ bool BrowserFrameMac::HandleKeyboardEvent(
   DCHECK([window.class conformsToProtocol:@protocol(CommandDispatchingWindow)]);
   NSObject<CommandDispatchingWindow>* command_dispatching_window =
       base::mac::ObjCCastStrict<NSObject<CommandDispatchingWindow>>(window);
-  return [command_dispatching_window redispatchKeyEvent:event.os_event];
+  return [[command_dispatching_window commandDispatcher]
+      redispatchKeyEvent:event.os_event];
 }

chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.cc

@@ -120,6 +120,18 @@ void SigninViewControllerDelegateViews::ResizeNativeView(int height) {
   }
 }
 
+void SigninViewControllerDelegateViews::HandleKeyboardEvent(
+    content::WebContents* source,
+    const content::NativeWebKeyboardEvent& event) {
+  // If this is a MODAL_TYPE_CHILD, then GetFocusManager() will return the focus
+  // manager of the parent window, which has registered accelerators, and the
+  // accelerators will fire. If this is a MODAL_TYPE_WINDOW, then this will have
+  // no effect, since no accelerators have been registered for this standalone
+  // window.
+  unhandled_keyboard_event_handler_.HandleKeyboardEvent(event,
+                                                        GetFocusManager());
+}
+
 void SigninViewControllerDelegateViews::DisplayModal() {
   DCHECK(!modal_signin_widget_);
 

chrome/browser/ui/views/profiles/signin_view_controller_delegate_views.h

@@ -8,6 +8,7 @@
 #include "base/macros.h"
 #include "chrome/browser/ui/profile_chooser_constants.h"
 #include "chrome/browser/ui/signin_view_controller_delegate.h"
+#include "ui/views/controls/webview/unhandled_keyboard_event_handler.h"
 #include "ui/views/window/dialog_delegate.h"
 
 class Browser;
@@ -73,6 +74,11 @@ class SigninViewControllerDelegateViews : public views::DialogDelegateView,
   void PerformClose() override;
   void ResizeNativeView(int height) override;
 
+  // content::WebContentsDelegate:
+  void HandleKeyboardEvent(
+      content::WebContents* source,
+      const content::NativeWebKeyboardEvent& event) override;
+
   void DisplayModal();
 
   // Creates a WebView for a dialog with the specified URL.
@@ -85,6 +91,7 @@ class SigninViewControllerDelegateViews : public views::DialogDelegateView,
   views::WebView* content_view_;
   views::Widget* modal_signin_widget_;  // Not owned.
   ui::ModalType dialog_modal_type_;
+  views::UnhandledKeyboardEventHandler unhandled_keyboard_event_handler_;
 
   DISALLOW_COPY_AND_ASSIGN(SigninViewControllerDelegateViews);
 };

chrome/test/BUILD.gn

@@ -4694,6 +4694,7 @@ if (!is_android) {
       "../browser/ui/search/instant_uitest_base.h",
       "../browser/ui/search/local_ntp_uitest.cc",
       "../browser/ui/send_mouse_move_uitest_win.cc",
+      "../browser/ui/signin_view_controller_interactive_uitest.cc",
       "../browser/ui/startup/startup_browser_creator_interactive_uitest.cc",
       "../browser/ui/tabs/window_activity_watcher_interactive_uitest.cc",
       "../browser/ui/translate/translate_bubble_test_utils.h",

ui/base/cocoa/command_dispatcher.h

@@ -103,9 +103,8 @@ UI_BASE_EXPORT
 // Retains |commandHandler|.
 -(void)setCommandHandler:(id<UserInterfaceItemCommandHandler>) commandHandler;
 
-// This can be implemented with -[CommandDispatcher redispatchKeyEvent:]. It's
-// so that callers can simply return events to the NSWindow.
-- (BOOL)redispatchKeyEvent:(NSEvent*)event;
+// Returns the associated CommandDispatcher.
+- (CommandDispatcher*)commandDispatcher;
 
 // Short-circuit to the default -[NSResponder performKeyEquivalent:] which
 // CommandDispatcher calls as part of its -performKeyEquivalent: flow.

ui/base/cocoa/command_dispatcher.mm

@@ -4,6 +4,7 @@
 
 #import "ui/base/cocoa/command_dispatcher.h"
 
+#include "base/auto_reset.h"
 #include "base/logging.h"
 #include "ui/base/cocoa/cocoa_base_utils.h"
 #import "ui/base/cocoa/user_interface_item_command_handler.h"
@@ -61,8 +62,8 @@ NSEvent* KeyEventForWindow(NSWindow* window, NSEvent* event) {
 
 @implementation CommandDispatcher {
  @private
-  BOOL redispatchingEvent_;
   BOOL eventHandled_;
+  BOOL isRedispatchingKeyEvent_;
 
   // If CommandDispatcher handles a keyEquivalent: [e.g. cmd + w], then it
   // should suppress future key-up events, e.g. [cmd + w (key up)].
@@ -80,8 +81,21 @@ NSEvent* KeyEventForWindow(NSWindow* window, NSEvent* event) {
   return self;
 }
 
+// When an event is being redispatched, its window is rewritten to be the owner_
+// of the CommandDispatcher. However, AppKit may still choose to send the event
+// to the key window. To check of an event is being redispatched, we check the
+// event's window.
+- (BOOL)isEventBeingRedispatched:(NSEvent*)event {
+  if ([event.window conformsToProtocol:@protocol(CommandDispatchingWindow)]) {
+    NSObject<CommandDispatchingWindow>* window =
+        static_cast<NSObject<CommandDispatchingWindow>*>(event.window);
+    return [window commandDispatcher]->isRedispatchingKeyEvent_;
+  }
+  return NO;
+}
+
 - (BOOL)doPerformKeyEquivalent:(NSEvent*)event {
-  // If |redispatchingEvent_| is true, then this is the second time
+  // If the event is being redispatched, then this is the second time
   // performKeyEquivalent: is being called on the event. The first time, a
   // WebContents was firstResponder and claimed to have handled the event [but
   // instead sent the event asynchronously to the renderer process]. The
@@ -91,7 +105,7 @@ NSEvent* KeyEventForWindow(NSWindow* window, NSEvent* event) {
   //
   // We skip all steps before postPerformKeyEquivalent, since those were already
   // triggered on the first pass of the event.
-  if (redispatchingEvent_) {
+  if ([self isEventBeingRedispatched:event]) {
     if ([delegate_ postPerformKeyEquivalent:event
                                      window:owner_
                                isRedispatch:YES]) {
@@ -168,6 +182,9 @@ NSEvent* KeyEventForWindow(NSWindow* window, NSEvent* event) {
 }
 
 - (BOOL)redispatchKeyEvent:(NSEvent*)event {
+  DCHECK(!isRedispatchingKeyEvent_);
+  base::AutoReset<BOOL> resetter(&isRedispatchingKeyEvent_, YES);
+
   DCHECK(event);
   NSEventType eventType = [event type];
   if (eventType != NSKeyDown && eventType != NSKeyUp &&
@@ -187,12 +204,10 @@ NSEvent* KeyEventForWindow(NSWindow* window, NSEvent* event) {
 
   // Redispatch the event.
   eventHandled_ = YES;
-  redispatchingEvent_ = YES;
   [NSApp sendEvent:event];
-  redispatchingEvent_ = NO;
 
-  // If the event was not handled by [NSApp sendEvent:], the sendEvent:
-  // method below will be called, and because |redispatchingEvent_| is YES,
+  // If the event was not handled by [NSApp sendEvent:], the preSendEvent:
+  // method below will be called, and because the event is being redispatched,
   // |eventHandled_| will be set to NO.
   return eventHandled_;
 }
@@ -208,7 +223,7 @@ NSEvent* KeyEventForWindow(NSWindow* window, NSEvent* event) {
       return YES;
   }
 
-  if (redispatchingEvent_) {
+  if ([self isEventBeingRedispatched:event]) {
     // If we get here, then the event was not handled by NSApplication.
     eventHandled_ = NO;
     // Return YES to stop native -sendEvent handling.

ui/views/cocoa/native_widget_mac_nswindow.mm

@@ -189,8 +189,8 @@
   commandHandler_.reset([commandHandler retain]);
 }
 
-- (BOOL)redispatchKeyEvent:(NSEvent*)event {
-  return [commandDispatcher_ redispatchKeyEvent:event];
+- (CommandDispatcher*)commandDispatcher {
+  return commandDispatcher_.get();
 }
 
 - (BOOL)defaultPerformKeyEquivalent:(NSEvent*)event {

ui/views/controls/webview/unhandled_keyboard_event_handler_mac.mm

@@ -13,8 +13,8 @@ namespace views {
 void UnhandledKeyboardEventHandler::HandleNativeKeyboardEvent(
     gfx::NativeEvent event,
     FocusManager* focus_manager) {
-  [base::mac::ObjCCastStrict<NativeWidgetMacNSWindow>([event window])
-      redispatchKeyEvent:event];
+  [[base::mac::ObjCCastStrict<NativeWidgetMacNSWindow>([event window])
+      commandDispatcher] redispatchKeyEvent:event];
 }
 
 }  // namespace views