diff options
| -rw-r--r-- | chrome/chrome_tests.gypi | 1 | ||||
| -rw-r--r-- | views/focus/accelerator_handler.h | 13 | ||||
| -rw-r--r-- | views/focus/accelerator_handler_gtk.cc | 54 | ||||
| -rw-r--r-- | views/focus/accelerator_handler_gtk_unittest.cc | 193 |
4 files changed, 240 insertions, 21 deletions
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index 06b93af..8712276 100644 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -1091,6 +1091,7 @@ ['OS=="linux" and toolkit_views==1', { 'sources': [ '<@(views_unit_tests_sources)', + '../views/focus/accelerator_handler_gtk_unittest.cc', ], # We must use 'sources/' instead of 'source!' as there is a # target-default 'sources/' including gtk_unittest and 'source/' takes 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 |