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

ui/views/controls/menu/menu_controller.cc

 // Copyright (c) 2012 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 "ui/views/controls/menu/menu_controller.h"
 
 #include "base/i18n/case_conversion.h"
 #include "base/i18n/rtl.h"
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 58 matching lines @@
 const int kCloseOnExitTime = 1200;
 
 // If a context menu is invoked by touch, we shift the menu by this offset so
 // that the finger does not obscure the menu.
 const int kCenteredContextMenuYOffset = -15;
 
 // The spacing offset for the bubble tip.
 const int kBubbleTipSizeLeftRight = 12;
 const int kBubbleTipSizeTopBottom = 11;
 
+const int kKeyFlagsMask = ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN;

[tapted, 2016/06/07 01:23:28]

Since this file is bigger, there should be a comment for this - but I guess we
could just move it down to inside OnWillDispatchKeyEvent(..) as well. (we
probably also want COMMAND instead of ALT on Mac, but we should see if that's
actually a problem before adding this complexity).

[karandeepb, 2016/06/07 04:13:46]

All the character events we are passing currently have no modifier flags set. I
think this is fine for the moment. We can modify it later if this causes any
problems.

+
 // The maximum distance (in DIPS) that the mouse can be moved before it should
 // trigger a mouse menu item activation (regardless of how long the menu has
 // been showing).
 const float kMaximumLengthMovedToActivate = 4.0f;
 
 // Returns true if the mnemonic of |menu| matches key.
 bool MatchesMnemonic(MenuItemView* menu, base::char16 key) {
   return key != 0 && menu->GetMnemonic() == key;
 }
 
@@ skipping 927 matching lines @@
   current_mouse_event_target_ = nullptr;
   if (showing_ && should_close && GetActiveInstance() == this) {
     CloseAllNestedMenus();
     Cancel(EXIT_ALL);
   } else if (async_run_) {
     ExitAsyncRun();
   }
 }
 
 ui::PostDispatchAction MenuController::OnWillDispatchKeyEvent(
-    base::char16 character,
-    ui::KeyboardCode key_code) {
+    ui::KeyEvent* event) {
   if (exit_type() == MenuController::EXIT_ALL ||
       exit_type() == MenuController::EXIT_DESTROYED) {
+    // If the event has arrived after the menu's exit type had changed but
+    // before its message loop terminated, the event will continue its normal
+    // propagation for the following reason:
+    // If the user accepts a menu item in a nested menu, the menu item action is
+    // run after the base::RunLoop for the innermost menu has quit but before
+    // the base::RunLoop for the outermost menu has quit. If the menu item
+    // action starts a base::RunLoop, the outermost menu's base::RunLoop will
+    // not quit till the action's base::RunLoop ends. IDC_BOOKMARK_BAR_OPEN_ALL
+    // sometimes opens a modal dialog. The modal dialog starts a base::RunLoop
+    // and keeps the base::RunLoop running for the duration of its lifetime.
     TerminateNestedMessageLoopIfNecessary();
     return ui::POST_DISPATCH_PERFORM_DEFAULT;
   }
 
-  if (character)
-    SelectByChar(character);
-  else
-    OnKeyDown(key_code);
+  event->StopPropagation();
 
-  // MenuController may have been deleted, so check for an active instance
-  // before accessing member variables.
-  if (GetActiveInstance())
-    TerminateNestedMessageLoopIfNecessary();
+  if (event->type() == ui::ET_KEY_PRESSED) {
+    OnKeyDown(event->key_code());
+    // Menu controller might have been deleted.
+    if (!MenuController::GetActiveInstance())

[tapted, 2016/06/07 01:23:28]

Convention in the rest of this file is to omit the MenuController:: prefixes.
Here and on MenuController::EXIT_NONE below. (we should fix the ones that I put
in above for MenuController::EXIT_ALL/DESTROYED too)

[karandeepb, 2016/06/07 04:13:46]

Done.

+      return ui::POST_DISPATCH_NONE;
 
+    // Do not check mnemonics if the Alt or Ctrl modifiers are pressed. For
+    // example Ctrl+<T> is an accelerator, but <T> only is a mnemonic.
+    const int flags = event->flags();
+    if (exit_type() == MenuController::EXIT_NONE &&
+        (flags & kKeyFlagsMask) == 0) {
+      base::char16 c = event->GetCharacter();
+      SelectByChar(c);
+      // Menu controller might have been deleted.
+      if (!MenuController::GetActiveInstance())
+        return ui::POST_DISPATCH_NONE;
+    }
+  }
+
+  if (!TerminateNestedMessageLoopIfNecessary()) {
+    ui::Accelerator accelerator(*event);
+    ViewsDelegate::ProcessMenuAcceleratorResult result =
+        ViewsDelegate::GetInstance()->ProcessAcceleratorWhileMenuShowing(
+            accelerator);
+    if (result == ViewsDelegate::ProcessMenuAcceleratorResult::CLOSE_MENU)
+      CancelAll();
+  }
   return ui::POST_DISPATCH_NONE;
 }
 
 void MenuController::UpdateSubmenuSelection(SubmenuView* submenu) {
   if (submenu->IsShowing()) {
     gfx::Point point = display::Screen::GetScreen()->GetCursorScreenPoint();
     const SubmenuView* root_submenu =
         submenu->GetMenuItem()->GetRootMenuItem()->GetSubmenu();
     View::ConvertPointFromScreen(
         root_submenu->GetWidget()->GetRootView(), &point);
@@ skipping 1624 matching lines @@
   if (hot_button_)
     hot_button_->SetHotTracked(false);
   hot_button_ = hot_button;
   if (hot_button) {
     hot_button->SetHotTracked(true);
     hot_button->NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true);
   }
 }
 
 }  // namespace views