diff options
| -rw-r--r-- | chrome/browser/views/find_bar_win.cc | 25 | ||||
| -rw-r--r-- | chrome/browser/views/find_bar_win.h | 5 | ||||
| -rw-r--r-- | chrome/browser/views/find_bar_win_browsertest.cc | 6 | ||||
| -rw-r--r-- | views/focus/focus_manager.cc | 78 | ||||
| -rw-r--r-- | views/focus/focus_manager.h | 25 | ||||
| -rw-r--r-- | views/focus/focus_manager_unittest.cc | 164 |
6 files changed, 234 insertions, 69 deletions
diff --git a/chrome/browser/views/find_bar_win.cc b/chrome/browser/views/find_bar_win.cc index 4b2d7ec..f3bfdd7 100644 --- a/chrome/browser/views/find_bar_win.cc +++ b/chrome/browser/views/find_bar_win.cc @@ -90,7 +90,7 @@ FindBarWin::FindBarWin(BrowserView* browser_view) : browser_view_(browser_view), find_dialog_animation_offset_(0), focus_manager_(NULL), - old_accel_target_for_esc_(NULL), + esc_accel_target_registered_(false), find_bar_controller_(NULL) { gfx::NativeView parent_view = browser_view->GetWidget()->GetNativeView(); @@ -574,7 +574,7 @@ void FindBarWin::SetFocusChangeListener(gfx::NativeView parent_view) { // FocusManager, which means we need to clean up old listener and start a new // one with the new FocusManager. if (focus_manager_) { - if (old_accel_target_for_esc_) + if (esc_accel_target_registered_) UnregisterEscAccelerator(); focus_manager_->RemoveFocusChangeListener(this); } @@ -603,15 +603,10 @@ FindBarTesting* FindBarWin::GetFindBarTesting() { void FindBarWin::RegisterEscAccelerator() { #if defined(OS_WIN) + DCHECK(!esc_accel_target_registered_); views::Accelerator escape(VK_ESCAPE, false, false, false); - - // TODO(finnur): Once we fix issue 1307173 we should not remember any old - // accelerator targets and just Register and Unregister when needed. - views::AcceleratorTarget* old_target = - focus_manager_->RegisterAccelerator(escape, this); - - if (!old_accel_target_for_esc_) - old_accel_target_for_esc_ = old_target; + focus_manager_->RegisterAccelerator(escape, this); + esc_accel_target_registered_ = true; #else NOTIMPLEMENTED(); #endif @@ -619,14 +614,10 @@ void FindBarWin::RegisterEscAccelerator() { void FindBarWin::UnregisterEscAccelerator() { #if defined(OS_WIN) - // TODO(finnur): Once we fix issue 1307173 we should not remember any old - // accelerator targets and just Register and Unregister when needed. - DCHECK(old_accel_target_for_esc_ != NULL); + DCHECK(esc_accel_target_registered_); views::Accelerator escape(VK_ESCAPE, false, false, false); - views::AcceleratorTarget* current_target = - focus_manager_->GetTargetForAccelerator(escape); - if (current_target == this) - focus_manager_->RegisterAccelerator(escape, old_accel_target_for_esc_); + focus_manager_->UnregisterAccelerator(escape, this); + esc_accel_target_registered_ = false; #else NOTIMPLEMENTED(); #endif diff --git a/chrome/browser/views/find_bar_win.h b/chrome/browser/views/find_bar_win.h index d59c4c23..db7b3e7 100644 --- a/chrome/browser/views/find_bar_win.h +++ b/chrome/browser/views/find_bar_win.h @@ -166,9 +166,8 @@ class FindBarWin : public views::AcceleratorTarget, // The focus manager we register with to keep track of focus changes. views::FocusManager* focus_manager_; - // Stores the previous accelerator target for the Escape key, so that we can - // restore the state once we loose focus. - views::AcceleratorTarget* old_accel_target_for_esc_; + // True if the accelerator target for Esc key is registered. + bool esc_accel_target_registered_; // Tracks and stores the last focused view which is not the FindBarView // or any of its children. Used to restore focus once the FindBarView is diff --git a/chrome/browser/views/find_bar_win_browsertest.cc b/chrome/browser/views/find_bar_win_browsertest.cc index 724b7c6..98a8111 100644 --- a/chrome/browser/views/find_bar_win_browsertest.cc +++ b/chrome/browser/views/find_bar_win_browsertest.cc @@ -563,7 +563,7 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, AcceleratorRestoring) { // See where Escape is registered. views::Accelerator escape(VK_ESCAPE, false, false, false); views::AcceleratorTarget* old_target = - focus_manager->GetTargetForAccelerator(escape); + focus_manager->GetCurrentTargetForAccelerator(escape); EXPECT_TRUE(old_target != NULL); // Open the Find box. @@ -571,7 +571,7 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, AcceleratorRestoring) { // Our Find bar should be the new target. views::AcceleratorTarget* new_target = - focus_manager->GetTargetForAccelerator(escape); + focus_manager->GetCurrentTargetForAccelerator(escape); EXPECT_TRUE(new_target != NULL); EXPECT_NE(new_target, old_target); @@ -580,5 +580,5 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, AcceleratorRestoring) { browser()->find_bar()->EndFindSession(); // The accelerator for Escape should be back to what it was before. - EXPECT_EQ(old_target, focus_manager->GetTargetForAccelerator(escape)); + EXPECT_EQ(old_target, focus_manager->GetCurrentTargetForAccelerator(escape)); } diff --git a/views/focus/focus_manager.cc b/views/focus/focus_manager.cc index 4cf6253..f504fd0 100644 --- a/views/focus/focus_manager.cc +++ b/views/focus/focus_manager.cc @@ -638,67 +638,73 @@ View* FocusManager::FindFocusableView(FocusTraversable* focus_traversable, return v; } -AcceleratorTarget* FocusManager::RegisterAccelerator( +void FocusManager::RegisterAccelerator( const Accelerator& accelerator, AcceleratorTarget* target) { - AcceleratorMap::const_iterator iter = accelerators_.find(accelerator); - AcceleratorTarget* previous_target = NULL; - if (iter != accelerators_.end()) - previous_target = iter->second; - - accelerators_[accelerator] = target; - - return previous_target; + AcceleratorTargetList& targets = accelerators_[accelerator]; + // TODO(yutak): View::RegisterAccelerators() seems to register the same target + // multiple times. Should uncomment below after View is fixed. + // DCHECK(std::find(targets.begin(), targets.end(), target) == targets.end()) + // << "Registering the same target multiple times"; + targets.push_front(target); } void FocusManager::UnregisterAccelerator(const Accelerator& accelerator, AcceleratorTarget* target) { - AcceleratorMap::iterator iter = accelerators_.find(accelerator); - if (iter == accelerators_.end()) { + AcceleratorMap::iterator map_iter = accelerators_.find(accelerator); + if (map_iter == accelerators_.end()) { NOTREACHED() << "Unregistering non-existing accelerator"; return; } - if (iter->second != target) { + AcceleratorTargetList* targets = &map_iter->second; + AcceleratorTargetList::iterator target_iter = + std::find(targets->begin(), targets->end(), target); + if (target_iter == targets->end()) { NOTREACHED() << "Unregistering accelerator for wrong target"; return; } - accelerators_.erase(iter); + targets->erase(target_iter); } void FocusManager::UnregisterAccelerators(AcceleratorTarget* target) { - for (AcceleratorMap::iterator iter = accelerators_.begin(); - iter != accelerators_.end();) { - if (iter->second == target) - accelerators_.erase(iter++); - else - ++iter; + for (AcceleratorMap::iterator map_iter = accelerators_.begin(); + map_iter != accelerators_.end(); ++map_iter) { + AcceleratorTargetList* targets = &map_iter->second; + targets->remove(target); } } bool FocusManager::ProcessAccelerator(const Accelerator& accelerator) { - FocusManager* focus_manager = this; - do { - AcceleratorTarget* target = - focus_manager->GetTargetForAccelerator(accelerator); - if (target && target->AcceleratorPressed(accelerator)) - return true; + AcceleratorMap::iterator map_iter = accelerators_.find(accelerator); + if (map_iter != accelerators_.end()) { + // We have to copy the target list here, because an AcceleratorPressed + // event handler may modify the list. + AcceleratorTargetList targets(map_iter->second); + for (AcceleratorTargetList::iterator iter = targets.begin(); + iter != targets.end(); ++iter) { + if ((*iter)->AcceleratorPressed(accelerator)) + return true; + } + } + + // When dealing with child windows that have their own FocusManager (such + // as ConstrainedWindow), we still want the parent FocusManager to process + // the accelerator if the child window did not process it. + FocusManager* parent_focus_manager = GetParentFocusManager(); + if (parent_focus_manager) + return parent_focus_manager->ProcessAccelerator(accelerator); - // When dealing with child windows that have their own FocusManager (such - // as ConstrainedWindow), we still want the parent FocusManager to process - // the accelerator if the child window did not process it. - focus_manager = focus_manager->GetParentFocusManager(); - } while (focus_manager); return false; } -AcceleratorTarget* FocusManager::GetTargetForAccelerator( - const Accelerator& accelerator) const { - AcceleratorMap::const_iterator iter = accelerators_.find(accelerator); - if (iter != accelerators_.end()) - return iter->second; - return NULL; +AcceleratorTarget* FocusManager::GetCurrentTargetForAccelerator( + const views::Accelerator& accelerator) const { + AcceleratorMap::const_iterator map_iter = accelerators_.find(accelerator); + if (map_iter == accelerators_.end() || map_iter->second.empty()) + return NULL; + return map_iter->second.front(); } // static diff --git a/views/focus/focus_manager.h b/views/focus/focus_manager.h index 133c027..28abcde 100644 --- a/views/focus/focus_manager.h +++ b/views/focus/focus_manager.h @@ -10,6 +10,7 @@ #endif #include <vector> #include <map> +#include <list> #include "base/basictypes.h" #include "base/gfx/native_widget_types.h" @@ -242,17 +243,17 @@ class FocusManager { // their own FocusManager and need to return focus to the browser when closed. FocusManager* GetParentFocusManager() const; - // Register a keyboard accelerator for the specified target. If an - // AcceleratorTarget is already registered for that accelerator, it is - // returned. + // Register a keyboard accelerator for the specified target. If multiple + // targets are registered for an accelerator, a target registered later has + // higher priority. // Note that we are currently limited to accelerators that are either: // - a key combination including Ctrl or Alt // - the escape key // - the enter key // - any F key (F1, F2, F3 ...) // - any browser specific keys (as available on special keyboards) - AcceleratorTarget* RegisterAccelerator(const Accelerator& accelerator, - AcceleratorTarget* target); + void RegisterAccelerator(const Accelerator& accelerator, + AcceleratorTarget* target); // Unregister the specified keyboard accelerator for the specified target. void UnregisterAccelerator(const Accelerator& accelerator, @@ -261,7 +262,11 @@ class FocusManager { // Unregister all keyboard accelerator for the specified target. void UnregisterAccelerators(AcceleratorTarget* target); - // Activate the target associated with the specified accelerator if any. + // Activate the target associated with the specified accelerator. + // First, AcceleratorPressed handler of the most recently registered target + // is called, and if that handler processes the event (i.e. returns true), + // this method immediately returns. If not, we do the same thing on the next + // target, and so on. // Returns true if an accelerator was activated. bool ProcessAccelerator(const Accelerator& accelerator); @@ -281,9 +286,8 @@ class FocusManager { // Returns the AcceleratorTarget that should be activated for the specified // keyboard accelerator, or NULL if no view is registered for that keyboard // accelerator. - // TODO(finnur): http://b/1307173 Make this private once the bug is fixed. - AcceleratorTarget* GetTargetForAccelerator( - const Accelerator& accelerator) const; + AcceleratorTarget* GetCurrentTargetForAccelerator( + const Accelerator& accelertor) const; // Convenience method that returns true if the passed |key_event| should // trigger tab traversal (if it is a TAB key press with or without SHIFT @@ -329,7 +333,8 @@ class FocusManager { bool ignore_set_focus_msg_; // The accelerators and associated targets. - typedef std::map<Accelerator, AcceleratorTarget*> AcceleratorMap; + typedef std::list<AcceleratorTarget*> AcceleratorTargetList; + typedef std::map<Accelerator, AcceleratorTargetList> AcceleratorMap; AcceleratorMap accelerators_; // The list of registered keystroke listeners diff --git a/views/focus/focus_manager_unittest.cc b/views/focus/focus_manager_unittest.cc index 087ecfe..404535c 100644 --- a/views/focus/focus_manager_unittest.cc +++ b/views/focus/focus_manager_unittest.cc @@ -682,4 +682,168 @@ TEST_F(FocusManagerTest, TraversalWithNonEnabledViews) { */ } +// Counts accelerator calls. +class TestAcceleratorTarget : public views::AcceleratorTarget { + public: + explicit TestAcceleratorTarget(bool process_accelerator) + : accelerator_count_(0), process_accelerator_(process_accelerator) {} + + virtual bool AcceleratorPressed(const views::Accelerator& accelerator) { + ++accelerator_count_; + return process_accelerator_; + } + + int accelerator_count() const { return accelerator_count_; } + + private: + int accelerator_count_; // number of times that the accelerator is activated + bool process_accelerator_; // return value of AcceleratorPressed + + DISALLOW_COPY_AND_ASSIGN(TestAcceleratorTarget); +}; + +TEST_F(FocusManagerTest, CallsNormalAcceleratorTarget) { + views::FocusManager* focus_manager = + views::FocusManager::GetFocusManager(test_window_->GetNativeView()); + views::Accelerator return_accelerator(VK_RETURN, false, false, false); + views::Accelerator escape_accelerator(VK_ESCAPE, false, false, false); + + TestAcceleratorTarget return_target(true); + TestAcceleratorTarget escape_target(true); + EXPECT_EQ(return_target.accelerator_count(), 0); + EXPECT_EQ(escape_target.accelerator_count(), 0); + EXPECT_EQ(NULL, + focus_manager->GetCurrentTargetForAccelerator(return_accelerator)); + EXPECT_EQ(NULL, + focus_manager->GetCurrentTargetForAccelerator(escape_accelerator)); + + // Register targets. + focus_manager->RegisterAccelerator(return_accelerator, &return_target); + focus_manager->RegisterAccelerator(escape_accelerator, &escape_target); + + // Checks if the correct target is registered. + EXPECT_EQ(&return_target, + focus_manager->GetCurrentTargetForAccelerator(return_accelerator)); + EXPECT_EQ(&escape_target, + focus_manager->GetCurrentTargetForAccelerator(escape_accelerator)); + + // Hitting the return key. + EXPECT_TRUE(focus_manager->ProcessAccelerator(return_accelerator)); + EXPECT_EQ(return_target.accelerator_count(), 1); + EXPECT_EQ(escape_target.accelerator_count(), 0); + + // Hitting the escape key. + EXPECT_TRUE(focus_manager->ProcessAccelerator(escape_accelerator)); + EXPECT_EQ(return_target.accelerator_count(), 1); + EXPECT_EQ(escape_target.accelerator_count(), 1); + + // Register another target for the return key. + TestAcceleratorTarget return_target2(true); + EXPECT_EQ(return_target2.accelerator_count(), 0); + focus_manager->RegisterAccelerator(return_accelerator, &return_target2); + EXPECT_EQ(&return_target2, + focus_manager->GetCurrentTargetForAccelerator(return_accelerator)); + + // Hitting the return key; return_target2 has the priority. + EXPECT_TRUE(focus_manager->ProcessAccelerator(return_accelerator)); + EXPECT_EQ(return_target.accelerator_count(), 1); + EXPECT_EQ(return_target2.accelerator_count(), 1); + + // Register a target that does not process the accelerator event. + TestAcceleratorTarget return_target3(false); + EXPECT_EQ(return_target3.accelerator_count(), 0); + focus_manager->RegisterAccelerator(return_accelerator, &return_target3); + EXPECT_EQ(&return_target3, + focus_manager->GetCurrentTargetForAccelerator(return_accelerator)); + + // Hitting the return key. + // Since the event handler of return_target3 returns false, return_target2 + // should be called too. + EXPECT_TRUE(focus_manager->ProcessAccelerator(return_accelerator)); + EXPECT_EQ(return_target.accelerator_count(), 1); + EXPECT_EQ(return_target2.accelerator_count(), 2); + EXPECT_EQ(return_target3.accelerator_count(), 1); + + // Unregister return_target2. + focus_manager->UnregisterAccelerator(return_accelerator, &return_target2); + EXPECT_EQ(&return_target3, + focus_manager->GetCurrentTargetForAccelerator(return_accelerator)); + + // Hitting the return key. return_target3 and return_target should be called. + EXPECT_TRUE(focus_manager->ProcessAccelerator(return_accelerator)); + EXPECT_EQ(return_target.accelerator_count(), 2); + EXPECT_EQ(return_target2.accelerator_count(), 2); + EXPECT_EQ(return_target3.accelerator_count(), 2); + + // Unregister targets. + focus_manager->UnregisterAccelerator(return_accelerator, &return_target); + focus_manager->UnregisterAccelerator(return_accelerator, &return_target3); + focus_manager->UnregisterAccelerator(escape_accelerator, &escape_target); + + // Now there is no target registered. + EXPECT_EQ(NULL, + focus_manager->GetCurrentTargetForAccelerator(return_accelerator)); + EXPECT_EQ(NULL, + focus_manager->GetCurrentTargetForAccelerator(escape_accelerator)); + + // Hitting the return key and the escape key. Nothing should happen. + EXPECT_FALSE(focus_manager->ProcessAccelerator(return_accelerator)); + EXPECT_EQ(return_target.accelerator_count(), 2); + EXPECT_EQ(return_target2.accelerator_count(), 2); + EXPECT_EQ(return_target3.accelerator_count(), 2); + EXPECT_FALSE(focus_manager->ProcessAccelerator(escape_accelerator)); + EXPECT_EQ(escape_target.accelerator_count(), 1); +} + +// Unregisters itself when its accelerator is invoked. +class SelfUnregisteringAcceleratorTarget : public views::AcceleratorTarget { + public: + SelfUnregisteringAcceleratorTarget(views::Accelerator accelerator, + views::FocusManager* focus_manager) + : accelerator_(accelerator), + focus_manager_(focus_manager), + accelerator_count_(0) { + } + + virtual bool AcceleratorPressed(const views::Accelerator& accelerator) { + ++accelerator_count_; + focus_manager_->UnregisterAccelerator(accelerator, this); + return true; + } + + int accelerator_count() const { return accelerator_count_; } + + private: + views::Accelerator accelerator_; + views::FocusManager* focus_manager_; + int accelerator_count_; + + DISALLOW_COPY_AND_ASSIGN(SelfUnregisteringAcceleratorTarget); +}; + +TEST_F(FocusManagerTest, CallsSelfDeletingAcceleratorTarget) { + views::FocusManager* focus_manager = + views::FocusManager::GetFocusManager(test_window_->GetNativeView()); + views::Accelerator return_accelerator(VK_RETURN, false, false, false); + SelfUnregisteringAcceleratorTarget target(return_accelerator, focus_manager); + EXPECT_EQ(target.accelerator_count(), 0); + EXPECT_EQ(NULL, + focus_manager->GetCurrentTargetForAccelerator(return_accelerator)); + + // Register the target. + focus_manager->RegisterAccelerator(return_accelerator, &target); + EXPECT_EQ(&target, + focus_manager->GetCurrentTargetForAccelerator(return_accelerator)); + + // Hitting the return key. The target will be unregistered. + EXPECT_TRUE(focus_manager->ProcessAccelerator(return_accelerator)); + EXPECT_EQ(target.accelerator_count(), 1); + EXPECT_EQ(NULL, + focus_manager->GetCurrentTargetForAccelerator(return_accelerator)); + + // Hitting the return key again; nothing should happen. + EXPECT_FALSE(focus_manager->ProcessAccelerator(return_accelerator)); + EXPECT_EQ(target.accelerator_count(), 1); } + +} // anonymous namespace |