Fixed alt key as shortcut to focus menus on linux/views. It should only

trigger when you press and release Alt and no other keys. Previously it
could trigger when Alt was released as part of some other key combination.

BUG=40754
TEST=Added new test, accelerator_handler_gtk_unittest.cc

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

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

3 files changed, 239 insertions, 21 deletions

diff --git a/views/focus/accelerator_handler.h b/views/focus/accelerator_handler.h
index c440c6b..67ce92c 100644
--- a/views/focus/accelerator_handler.h
+++ b/views/focus/accelerator_handler.h
@@ -11,9 +11,7 @@
 #include <gdk/gdk.h>
 #endif
 
-#if defined(OS_WIN)
 #include <set>
-#endif
 
 #include "base/message_loop.h"
 
@@ -37,7 +35,16 @@ class AcceleratorHandler : public MessageLoopForUI::Dispatcher {
   // The keys currently pressed and consumed by the FocusManager.
   std::set<WPARAM> pressed_keys_;
 #else  // OS_LINUX
-  guint last_key_pressed_;
+  // The set of hardware keycodes that are currently pressed.  We use this
+  // to keep track of whether or not a key is being pressed in isolation
+  // or not. We must use hardware keycodes because higher-level keycodes
+  // may change between press and release depending on what modifier keys
+  // are pressed in the interim.
+  std::set<int> pressed_hardware_keys_;
+  // The set of vkey codes that were consumed by the FocusManager.
+  std::set<int> consumed_vkeys_;
+  // True if only the menu key (Alt key) was pressed, and no other keys.
+  bool only_menu_pressed_;
 #endif
 
   DISALLOW_COPY_AND_ASSIGN(AcceleratorHandler);
diff --git a/views/focus/accelerator_handler_gtk.cc b/views/focus/accelerator_handler_gtk.cc
index ede8e57..f7fff91 100644
--- a/views/focus/accelerator_handler_gtk.cc
+++ b/views/focus/accelerator_handler_gtk.cc
@@ -14,10 +14,17 @@
 
 namespace views {
 
-AcceleratorHandler::AcceleratorHandler() : last_key_pressed_(0) {
-}
+AcceleratorHandler::AcceleratorHandler() : only_menu_pressed_(false) {}
 
 bool AcceleratorHandler::Dispatch(GdkEvent* event) {
+  // When focus changes, we may not see a full key-press / key-release
+  // pair, so clear the set of keys we think were pressed.
+  if (event->type == GDK_FOCUS_CHANGE) {
+    pressed_hardware_keys_.clear();
+    consumed_vkeys_.clear();
+    only_menu_pressed_ = false;
+  }
+
   // Let Gtk process the event if there is a grabbed widget or it's not
   // a keyboard event.
   if (gtk_grab_get_current() ||
@@ -52,40 +59,51 @@ bool AcceleratorHandler::Dispatch(GdkEvent* event) {
     return true;
   }
 
+  KeyEvent view_key_event(key_event);
+  int key_code = view_key_event.GetKeyCode();
+  int hardware_keycode = key_event->hardware_keycode;
   if (event->type == GDK_KEY_PRESS) {
-    KeyEvent view_key_event(key_event);
+    pressed_hardware_keys_.insert(hardware_keycode);
 
     // If it's the Alt key, don't send it to the focus manager until release
     // (to handle focusing the menu bar).
-    if (view_key_event.GetKeyCode() == base::VKEY_MENU) {
-      last_key_pressed_ = key_event->keyval;
+    if (key_code == base::VKEY_MENU && pressed_hardware_keys_.size() == 1) {
+      only_menu_pressed_ = true;
       return true;
     }
 
-    // FocusManager::OnKeyPressed and OnKeyReleased return false if this
-    // message has been consumed and should not be propagated further.
+    if (pressed_hardware_keys_.size() != 1)
+      only_menu_pressed_ = false;
+
+    // FocusManager::OnKeyEvent will return false if this message has been
+    // consumed and should not be propagated further.
     if (!focus_manager->OnKeyEvent(view_key_event)) {
-      last_key_pressed_ = key_event->keyval;
+      consumed_vkeys_.insert(key_code);
       return true;
     }
   }
 
-  // Key release, make sure to filter-out the key release for key press consumed
-  // as accelerators to avoid unpaired key release.
-  if (event->type == GDK_KEY_RELEASE &&
-      key_event->keyval == last_key_pressed_) {
+  if (event->type == GDK_KEY_RELEASE) {
+    pressed_hardware_keys_.erase(hardware_keycode);
+
+    // Make sure to filter out the key release for a key press consumed
+    // as an accelerator, to avoid calling gtk_main_do_event with an
+    // unpaired key release.
+    if (consumed_vkeys_.find(key_code) != consumed_vkeys_.end()) {
+      consumed_vkeys_.erase(key_code);
+      return true;
+    }
+
     // Special case: the Alt key can trigger an accelerator on release
-    // rather than on press.
-    if (base::WindowsKeyCodeForGdkKeyCode(key_event->keyval) ==
-        base::VKEY_MENU) {
+    // rather than on press, but only if no other keys were pressed.
+    if (key_code == base::VKEY_MENU && only_menu_pressed_) {
       Accelerator accelerator(base::VKEY_MENU, false, false, false);
       focus_manager->ProcessAccelerator(accelerator);
+      return true;
     }
-
-    last_key_pressed_ = 0;
-    return true;
   }
 
+  // Pass the event to gtk if we didn't consume it above.
   gtk_main_do_event(event);
   return true;
 }
diff --git a/views/focus/accelerator_handler_gtk_unittest.cc b/views/focus/accelerator_handler_gtk_unittest.cc
new file mode 100644
index 0000000..ef05312
--- /dev/null
+++ b/views/focus/accelerator_handler_gtk_unittest.cc
@@ -0,0 +1,193 @@
+// Copyright (c) 2010 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 <gdk/gdkkeysyms.h>
+
+#include "gfx/rect.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "views/focus/accelerator_handler.h"
+#include "views/view.h"
+#include "views/window/window_gtk.h"
+#include "views/window/window_delegate.h"
+
+namespace views {
+
+class AcceleratorHandlerGtkTest
+    : public testing::Test,
+      public WindowDelegate,
+      public AcceleratorTarget {
+ public:
+  AcceleratorHandlerGtkTest()
+      : kMenuAccelerator(base::VKEY_MENU, false, false, false),
+        kHomepageAccelerator(base::VKEY_HOME, false, false, true),
+        content_view_(NULL) {
+  }
+
+  virtual void SetUp() {
+    window_ = Window::CreateChromeWindow(
+        NULL, gfx::Rect(0, 0, 500, 500), this);
+    window_->Show();
+    FocusManager* focus_manager = static_cast<WindowGtk*>(window_)->
+        GetFocusManager();
+    focus_manager->RegisterAccelerator(kMenuAccelerator, this);
+    focus_manager->RegisterAccelerator(kHomepageAccelerator, this);
+    menu_pressed_ = false;
+    home_pressed_ = false;
+  }
+
+  virtual void TearDown() {
+    window_->Close();
+
+    // Flush the message loop to make Purify happy.
+    message_loop_.RunAllPending();
+  }
+
+  GdkEventKey CreateKeyEvent(GdkEventType type, guint keyval, guint state) {
+    GdkEventKey evt;
+    memset(&evt, 0, sizeof(evt));
+    evt.type = type;
+    evt.keyval = keyval;
+    // The keyval won't be a "correct" hardware keycode for any real hardware,
+    // but the code should never depend on exact hardware keycodes, just the
+    // fact that the code for presses and releases of the same key match.
+    evt.hardware_keycode = keyval;
+    evt.state = state;
+    GtkWidget* widget = GTK_WIDGET(window_->GetNativeWindow());
+    evt.window = widget->window;
+    return evt;
+  }
+
+  // AcceleratorTarget implementation.
+  virtual bool AcceleratorPressed(const Accelerator& accelerator) {
+    if (accelerator == kMenuAccelerator)
+      menu_pressed_ = true;
+    else if (accelerator == kHomepageAccelerator)
+      home_pressed_ = true;
+    return true;
+  }
+
+  // WindowDelegate Implementation.
+  virtual View* GetContentsView() {
+    if (!content_view_)
+      content_view_ = new View();
+    return content_view_;
+  }
+
+  virtual void InitContentView() {
+  }
+
+ protected:
+  bool menu_pressed_;
+  bool home_pressed_;
+
+ private:
+  Accelerator kMenuAccelerator;
+  Accelerator kHomepageAccelerator;
+  Window* window_;
+  View* content_view_;
+  MessageLoopForUI message_loop_;
+  DISALLOW_COPY_AND_ASSIGN(AcceleratorHandlerGtkTest);
+};
+
+// Test that the homepage accelerator (Alt+Home) is activated on key down
+// and that the menu accelerator (Alt) is never activated.
+TEST_F(AcceleratorHandlerGtkTest, TestHomepageAccelerator) {
+  AcceleratorHandler handler;
+  GdkEventKey evt;
+
+  ASSERT_FALSE(menu_pressed_);
+  ASSERT_FALSE(home_pressed_);
+
+  evt = CreateKeyEvent(GDK_KEY_PRESS, GDK_Menu, 0);
+  EXPECT_TRUE(handler.Dispatch(reinterpret_cast<GdkEvent*>(&evt)));
+
+  ASSERT_FALSE(menu_pressed_);
+  ASSERT_FALSE(home_pressed_);
+
+  evt = CreateKeyEvent(GDK_KEY_PRESS, GDK_Home, GDK_MOD1_MASK);
+  EXPECT_TRUE(handler.Dispatch(reinterpret_cast<GdkEvent*>(&evt)));
+
+  ASSERT_FALSE(menu_pressed_);
+  ASSERT_TRUE(home_pressed_);
+
+  evt = CreateKeyEvent(GDK_KEY_RELEASE, GDK_Home, GDK_MOD1_MASK);
+  EXPECT_TRUE(handler.Dispatch(reinterpret_cast<GdkEvent*>(&evt)));
+  evt = CreateKeyEvent(GDK_KEY_RELEASE, GDK_Menu, 0);
+  EXPECT_TRUE(handler.Dispatch(reinterpret_cast<GdkEvent*>(&evt)));
+
+  ASSERT_FALSE(menu_pressed_);
+  ASSERT_TRUE(home_pressed_);
+}
+
+// Test that the menu accelerator is activated on key up and not key down.
+TEST_F(AcceleratorHandlerGtkTest, TestMenuAccelerator) {
+  AcceleratorHandler handler;
+  GdkEventKey evt;
+
+  ASSERT_FALSE(menu_pressed_);
+
+  evt = CreateKeyEvent(GDK_KEY_PRESS, GDK_Menu, 0);
+  EXPECT_TRUE(handler.Dispatch(reinterpret_cast<GdkEvent*>(&evt)));
+
+  ASSERT_FALSE(menu_pressed_);
+
+  evt = CreateKeyEvent(GDK_KEY_RELEASE, GDK_Menu, 0);
+  EXPECT_TRUE(handler.Dispatch(reinterpret_cast<GdkEvent*>(&evt)));
+
+  ASSERT_TRUE(menu_pressed_);
+}
+
+// Test that the menu accelerator isn't confused by the interaction of the
+// Alt and Shift keys. Try the following sequence on Linux:
+//   Press Alt
+//   Press Shift
+//   Release Alt
+//   Release Shift
+// The key codes for pressing Alt and releasing Alt are different! This
+// caused a bug in a previous version of the code, which is now fixed by
+// keeping track of hardware keycodes, which are consistent.
+TEST_F(AcceleratorHandlerGtkTest, TestAltShiftInteraction) {
+  AcceleratorHandler handler;
+  GdkEventKey evt;
+
+  ASSERT_FALSE(menu_pressed_);
+
+  // Press Shift.
+  evt = CreateKeyEvent(GDK_KEY_PRESS, GDK_Shift_L, 0);
+  evt.hardware_keycode = 0x32;
+  EXPECT_TRUE(handler.Dispatch(reinterpret_cast<GdkEvent*>(&evt)));
+  // Press Alt - but GDK calls this Meta when Shift is also down.
+  evt = CreateKeyEvent(GDK_KEY_PRESS, GDK_Meta_L, 0);
+  evt.hardware_keycode = 0x40;
+  EXPECT_TRUE(handler.Dispatch(reinterpret_cast<GdkEvent*>(&evt)));
+
+  // Release Shift.
+  evt = CreateKeyEvent(GDK_KEY_RELEASE, GDK_Shift_L, 0);
+  evt.hardware_keycode = 0x32;
+  EXPECT_TRUE(handler.Dispatch(reinterpret_cast<GdkEvent*>(&evt)));
+  // Release Alt - with Shift not down, the keyval is now Alt, but
+  // the hardware keycode is unchanged.
+  evt = CreateKeyEvent(GDK_KEY_RELEASE, GDK_Alt_L, 0);
+  evt.hardware_keycode = 0x40;
+  EXPECT_TRUE(handler.Dispatch(reinterpret_cast<GdkEvent*>(&evt)));
+
+  ASSERT_FALSE(menu_pressed_);
+
+  // Press Alt by itself.
+  evt = CreateKeyEvent(GDK_KEY_PRESS, GDK_Alt_L, 0);
+  evt.hardware_keycode = 0x40;
+  EXPECT_TRUE(handler.Dispatch(reinterpret_cast<GdkEvent*>(&evt)));
+
+  // This line fails if we don't keep track of hardware keycodes.
+  ASSERT_FALSE(menu_pressed_);
+
+  // Release Alt - now this should trigger the menu shortcut.
+  evt = CreateKeyEvent(GDK_KEY_RELEASE, GDK_Alt_L, 0);
+  evt.hardware_keycode = 0x40;
+  EXPECT_TRUE(handler.Dispatch(reinterpret_cast<GdkEvent*>(&evt)));
+
+  ASSERT_TRUE(menu_pressed_);
+}
+
+}  // namespace views