[Linux Views] Refactor accelerator handler related code.

This CL removes the accelerator handling logic in
accelerator_handler_gtk.cc and implements a much simpler solution in
WidgetGtk. The new approach always sends a key event to the focused View
and native GtkWidget first and only sends it to the focus manager if it's not
handled by any View or native GtkWidget.

BUG=23383 AcceleratorHandler on Windows should not dispatch the KEYUP messages eaten by the FocusManager
BUG=40966 BrowserKeyEventsTest.AccessKeys is crashy
BUG=49701 [Linux Views]Some Emacs keybindings are broken in omnibox and find in page box.
TEST=Press Alt key in different place (web page, omnibox, find bar, etc.) to see if menu bar can be focused correctly. Press alt-F to popup wrench menu and Escape to close it, then try alt key again.

Review URL: http://codereview.chromium.org/3046041

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@54947 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 89 insertions, 18 deletions

diff --git a/views/widget/gtk_views_window.cc b/views/widget/gtk_views_window.cc
index fb3657d..2eff8f1 100644
--- a/views/widget/gtk_views_window.cc
+++ b/views/widget/gtk_views_window.cc
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <gtk/gtk.h>
+#include "views/event.h"
 #include "views/widget/gtk_views_window.h"
 #include "views/focus/focus_manager.h"
 
@@ -9,15 +11,16 @@ G_BEGIN_DECLS
 
 G_DEFINE_TYPE(GtkViewsWindow, gtk_views_window, GTK_TYPE_WINDOW)
 
-static void gtk_views_window_move_focus(GtkWindow *window,
+static void gtk_views_window_move_focus(GtkWindow* window,
                                         GtkDirectionType dir) {
   views::FocusManager* focus_manager =
       views::FocusManager::GetFocusManagerForNativeWindow(window);
   if (focus_manager) {
-    // We only support tab traversing by tab keys, so just ignore all other
-    // cases silently.
-    if (dir == GTK_DIR_TAB_BACKWARD || dir == GTK_DIR_TAB_FORWARD)
-      focus_manager->AdvanceFocus(dir == GTK_DIR_TAB_BACKWARD);
+    GdkEvent* key = gtk_get_current_event();
+    if (key && key->type == GDK_KEY_PRESS) {
+      views::KeyEvent key_event(reinterpret_cast<GdkEventKey*>(key));
+      focus_manager->OnKeyEvent(key_event);
+    }
   } else if (GTK_WINDOW_CLASS(gtk_views_window_parent_class)->move_focus) {
     GTK_WINDOW_CLASS(gtk_views_window_parent_class)->move_focus(window, dir);
   }
diff --git a/views/widget/widget_gtk.cc b/views/widget/widget_gtk.cc
index de4ad9a..93fdcdc 100644
--- a/views/widget/widget_gtk.cc
+++ b/views/widget/widget_gtk.cc
@@ -246,7 +246,8 @@ WidgetGtk::WidgetGtk(Type type)
       has_focus_(false),
       delegate_(NULL),
       always_on_top_(false),
-      is_double_buffered_(false) {
+      is_double_buffered_(false),
+      should_handle_menu_key_release_(false) {
   static bool installed_message_loop_observer = false;
   if (!installed_message_loop_observer) {
     installed_message_loop_observer = true;
@@ -546,9 +547,9 @@ void WidgetGtk::Init(GtkWidget* parent,
   // See views::Views::Focus and views::FocusManager::ClearNativeFocus
   // for more details.
   g_signal_connect(widget_, "key_press_event",
-                   G_CALLBACK(&OnKeyPressThunk), this);
+                   G_CALLBACK(&OnKeyEventThunk), this);
   g_signal_connect(widget_, "key_release_event",
-                   G_CALLBACK(&OnKeyReleaseThunk), this);
+                   G_CALLBACK(&OnKeyEventThunk), this);
 
   // Drag and drop.
   gtk_drag_dest_set(window_contents_, static_cast<GtkDestDefaults>(0),
@@ -861,6 +862,38 @@ void WidgetGtk::ClearNativeFocus() {
   gtk_window_set_focus(GTK_WINDOW(GetNativeView()), NULL);
 }
 
+bool WidgetGtk::HandleKeyboardEvent(GdkEventKey* event) {
+  if (!focus_manager_)
+    return false;
+
+  KeyEvent key(event);
+  int key_code = key.GetKeyCode();
+  bool handled = false;
+
+  // Always reset |should_handle_menu_key_release_| unless we are handling a
+  // VKEY_MENU key release event. It ensures that VKEY_MENU accelerator can only
+  // be activated when handling a VKEY_MENU key release event which is preceded
+  // by an unhandled VKEY_MENU key press event.
+  if (key_code != base::VKEY_MENU || event->type != GDK_KEY_RELEASE)
+    should_handle_menu_key_release_ = false;
+
+  if (event->type == GDK_KEY_PRESS) {
+    // VKEY_MENU is triggered by key release event.
+    if (key_code != base::VKEY_MENU)
+      handled = focus_manager_->OnKeyEvent(key);
+    else
+      should_handle_menu_key_release_ = true;
+  } else if (key_code == base::VKEY_MENU && should_handle_menu_key_release_ &&
+             (key.GetFlags() & ~Event::EF_ALT_DOWN) == 0) {
+    // Trigger VKEY_MENU when only this key is pressed and released, and both
+    // press and release events are not handled by others.
+    Accelerator accelerator(base::VKEY_MENU, false, false, false);
+    handled = focus_manager_->ProcessAccelerator(accelerator);
+  }
+
+  return handled;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // WidgetGtk, protected:
 
@@ -1088,6 +1121,8 @@ gboolean WidgetGtk::OnFocusIn(GtkWidget* widget, GdkEventFocus* event) {
     return false;  // This is the second focus-in event in a row, ignore it.
   has_focus_ = true;
 
+  should_handle_menu_key_release_ = false;
+
   if (type_ == TYPE_CHILD)
     return false;
 
@@ -1114,14 +1149,41 @@ gboolean WidgetGtk::OnFocusOut(GtkWidget* widget, GdkEventFocus* event) {
   return false;
 }
 
-gboolean WidgetGtk::OnKeyPress(GtkWidget* widget, GdkEventKey* event) {
-  KeyEvent key_event(event);
-  return root_view_->ProcessKeyEvent(key_event);
-}
-
-gboolean WidgetGtk::OnKeyRelease(GtkWidget* widget, GdkEventKey* event) {
-  KeyEvent key_event(event);
-  return root_view_->ProcessKeyEvent(key_event);
+gboolean WidgetGtk::OnKeyEvent(GtkWidget* widget, GdkEventKey* event) {
+  KeyEvent key(event);
+
+  // Always reset |should_handle_menu_key_release_| unless we are handling a
+  // VKEY_MENU key release event. It ensures that VKEY_MENU accelerator can only
+  // be activated when handling a VKEY_MENU key release event which is preceded
+  // by an unhandled VKEY_MENU key press event. See also HandleKeyboardEvent().
+  if (key.GetKeyCode() != base::VKEY_MENU || event->type != GDK_KEY_RELEASE)
+    should_handle_menu_key_release_ = false;
+
+  bool handled = false;
+
+  // Dispatch the key event to View hierarchy first.
+  handled = root_view_->ProcessKeyEvent(key);
+
+  // Dispatch the key event to native GtkWidget hierarchy.
+  // To prevent GtkWindow from handling the key event as a keybinding, we need
+  // to bypass GtkWindow's default key event handler and dispatch the event
+  // here.
+  if (!handled && GTK_IS_WINDOW(widget))
+    handled = gtk_window_propagate_key_event(GTK_WINDOW(widget), event);
+
+  // On Linux, in order to handle VKEY_MENU (Alt) accelerator key correctly and
+  // avoid issues like: http://crbug.com/40966 and http://crbug.com/49701, we
+  // should only send the key event to the focus manager if it's not handled by
+  // any View or native GtkWidget.
+  // The flow is different when the focus is in a RenderWidgetHostViewGtk, which
+  // always consumes the key event and send it back to us later by calling
+  // HandleKeyboardEvent() directly, if it's not handled by webkit.
+  if (!handled)
+    handled = HandleKeyboardEvent(event);
+
+  // Always return true for toplevel window to prevents GtkWindow's default key
+  // event handler.
+  return GTK_IS_WINDOW(widget) ? true : handled;
 }
 
 gboolean WidgetGtk::OnQueryTooltip(GtkWidget* widget,
diff --git a/views/widget/widget_gtk.h b/views/widget/widget_gtk.h
index d010b03..7e1bdd1 100644
--- a/views/widget/widget_gtk.h
+++ b/views/widget/widget_gtk.h
@@ -198,6 +198,10 @@ class WidgetGtk
   // Clears the focus on the native widget having the focus.
   virtual void ClearNativeFocus();
 
+  // Handles a keyboard event by sending it to our focus manager.
+  // Returns true if it's handled by the focus manager.
+  bool HandleKeyboardEvent(GdkEventKey* event);
+
  protected:
   // If widget containes another widget, translates event coordinates to the
   // contained widget's coordinates, else returns original event coordinates.
@@ -247,8 +251,7 @@ class WidgetGtk
   CHROMEGTK_CALLBACK_1(WidgetGtk, gboolean, OnButtonRelease, GdkEventButton*);
   CHROMEGTK_CALLBACK_1(WidgetGtk, gboolean, OnFocusIn, GdkEventFocus*);
   CHROMEGTK_CALLBACK_1(WidgetGtk, gboolean, OnFocusOut, GdkEventFocus*);
-  CHROMEGTK_CALLBACK_1(WidgetGtk, gboolean, OnKeyPress, GdkEventKey*);
-  CHROMEGTK_CALLBACK_1(WidgetGtk, gboolean, OnKeyRelease, GdkEventKey*);
+  CHROMEGTK_CALLBACK_1(WidgetGtk, gboolean, OnKeyEvent, GdkEventKey*);
   CHROMEGTK_CALLBACK_4(WidgetGtk, gboolean, OnQueryTooltip,
                        gint, gint, gboolean, GtkTooltip*);
   CHROMEGTK_CALLBACK_1(WidgetGtk, gboolean, OnScroll, GdkEventScroll*);
@@ -436,6 +439,9 @@ class WidgetGtk
   // This is false by default.
   bool is_double_buffered_;
 
+  // Indicates if we should handle the upcoming Alt key release event.
+  bool should_handle_menu_key_release_;
+
   DISALLOW_COPY_AND_ASSIGN(WidgetGtk);
 };