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

ui/views/controls/menu/menu_controller.cc

Index: ui/views/controls/menu/menu_controller.cc
diff --git a/ui/views/controls/menu/menu_controller.cc b/ui/views/controls/menu/menu_controller.cc
index 030600f84cc8b37673ff65c744c67c5c68e11f35..e0c651a370ae062d785a745acafda237a58cc992 100644
--- a/ui/views/controls/menu/menu_controller.cc
+++ b/ui/views/controls/menu/menu_controller.cc
@@ -76,6 +76,8 @@ const int kCenteredContextMenuYOffset = -15;
 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).
@@ -1023,24 +1025,52 @@ void MenuController::OnDragComplete(bool should_close) {
 }
 
 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);
-
-  // MenuController may have been deleted, so check for an active instance
-  // before accessing member variables.
-  if (GetActiveInstance())
-    TerminateNestedMessageLoopIfNecessary();
+  event->StopPropagation();
+
+  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;
 }