Clean-up of the accelerator code.

The View::CanProcessTabKeyEvents and View::ShouldLookUpAccelerator have both been replaced with a new method, SkipDefaultKeyEventProcessing.
This new method provides for a view that has focus a way to prevent a key event from being processed for tab traversal or accelerators.
Also, fixed a regression where the Ctrl-Tab accelerator was not working anymore when the omnibox was focused.

BUG=11538
TEST=Thoroughly test accelerators, making sure they work when the page, the omnibox and the find-bar text-field have focus.
     Also test that tab traversal still work as expected in the browser and in the option dialog.


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

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

8 files changed, 61 insertions, 57 deletions

diff --git a/views/controls/menu/chrome_menu.cc b/views/controls/menu/chrome_menu.cc
index 203fb88..a1a6809 100644
--- a/views/controls/menu/chrome_menu.cc
+++ b/views/controls/menu/chrome_menu.cc
@@ -23,6 +23,7 @@
 #include "skia/ext/skia_utils_win.h"
 #include "views/border.h"
 #include "views/drag_utils.h"
+#include "views/focus/focus_manager.h"
 #include "views/view_constants.h"
 #include "views/widget/root_view.h"
 #include "views/widget/widget_win.h"
@@ -998,6 +999,10 @@ void SubmenuView::ReleaseCapture() {
   host_->ReleaseCapture();
 }
 
+bool SubmenuView::SkipDefaultKeyEventProcessing(const views::KeyEvent& e) {
+  return views::FocusManager::IsTabTraversalKeyEvent(e);
+}
+
 void SubmenuView::SetDropMenuItem(MenuItemView* item,
                                   MenuDelegate::DropPosition position) {
   if (drop_item_ == item && drop_position_ == position)
diff --git a/views/controls/menu/chrome_menu.h b/views/controls/menu/chrome_menu.h
index f19af15..6bb25c7 100644
--- a/views/controls/menu/chrome_menu.h
+++ b/views/controls/menu/chrome_menu.h
@@ -16,6 +16,7 @@
 #include "base/task.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "views/controls/menu/controller.h"
+#include "views/event.h"
 #include "views/view.h"
 
 namespace views {
@@ -553,12 +554,12 @@ class SubmenuView : public View {
   // not captured.
   void ReleaseCapture();
 
+  // Overriden from View to prevent tab from doing anything.
+  virtual bool SkipDefaultKeyEventProcessing(const views::KeyEvent& e);
+
   // Returns the parent menu item we're showing children for.
   MenuItemView* GetMenuItem() const { return parent_menu_item_; }
 
-  // Overriden to return true. This prevents tab from doing anything.
-  virtual bool CanProcessTabKeyEvents() { return true; }
-
   // Set the drop item and position.
   void SetDropMenuItem(MenuItemView* item,
                        MenuDelegate::DropPosition position);
diff --git a/views/controls/text_field.cc b/views/controls/text_field.cc
index 0395af3..190be98 100644
--- a/views/controls/text_field.cc
+++ b/views/controls/text_field.cc
@@ -1163,18 +1163,19 @@ void TextField::AboutToRequestFocusFromTabTraversal(bool reverse) {
   SelectAll();
 }
 
-bool TextField::ShouldLookupAccelerators(const KeyEvent& e) {
+bool TextField::SkipDefaultKeyEventProcessing(const KeyEvent& e) {
   // TODO(hamaji): Figure out which keyboard combinations we need to add here,
-  //               similar to LocationBarView::ShouldLookupAccelerators.
+  //               similar to LocationBarView::SkipDefaultKeyEventProcessing.
   if (e.GetCharacter() == VK_BACK)
-    return false;  // We'll handle BackSpace ourselves.
+    return true;  // We'll handle BackSpace ourselves.
 
   // We don't translate accelerators for ALT + NumPad digit, they are used for
   // entering special characters.
-  if (!e.IsAltDown())
+  if (e.IsAltDown() &&
+      win_util::IsNumPadDigit(e.GetCharacter(), e.IsExtendedKey()))
     return true;
 
-  return !win_util::IsNumPadDigit(e.GetCharacter(), e.IsExtendedKey());
+  return false;
 }
 
 void TextField::UpdateEditBackgroundColor() {
diff --git a/views/controls/text_field.h b/views/controls/text_field.h
index 3ae3ed4..6dda5db 100644
--- a/views/controls/text_field.h
+++ b/views/controls/text_field.h
@@ -94,7 +94,7 @@ class TextField : public View {
   virtual void AboutToRequestFocusFromTabTraversal(bool reverse);
 
   // Overridden from Chrome::View.
-  virtual bool ShouldLookupAccelerators(const KeyEvent& e);
+  virtual bool SkipDefaultKeyEventProcessing(const KeyEvent& e);
 
   virtual HWND GetNativeComponent();
 
diff --git a/views/focus/focus_manager.cc b/views/focus/focus_manager.cc
index a7859f2..b33f64c 100644
--- a/views/focus/focus_manager.cc
+++ b/views/focus/focus_manager.cc
@@ -242,17 +242,24 @@ bool FocusManager::OnKeyDown(HWND window, UINT message, WPARAM wparam,
       << "KeystrokeListener list modified during notification";
 
   int virtual_key_code = static_cast<int>(wparam);
+  int repeat_count = LOWORD(lparam);
+  int flags = HIWORD(lparam);
+  KeyEvent key_event(Event::ET_KEY_PRESSED,
+                     virtual_key_code, repeat_count, flags);
+
+  // If the focused view wants to process the key event as is, let it be.
+  if (focused_view_ && focused_view_->SkipDefaultKeyEventProcessing(key_event))
+    return true;
+
   // Intercept Tab related messages for focus traversal.
   // Note that we don't do focus traversal if the root window is not part of the
   // active window hierarchy as this would mean we have no focused view and
   // would focus the first focusable view.
   HWND active_window = ::GetActiveWindow();
   if ((active_window == root_ || ::IsChild(active_window, root_)) &&
-      (virtual_key_code == VK_TAB) && !win_util::IsCtrlPressed()) {
-    if (!focused_view_ || !focused_view_->CanProcessTabKeyEvents()) {
-      AdvanceFocus(win_util::IsShiftPressed());
-      return false;
-    }
+      IsTabTraversalKeyEvent(key_event)) {
+    AdvanceFocus(win_util::IsShiftPressed());
+    return false;
   }
 
   // Intercept arrow key messages to switch between grouped views.
@@ -278,16 +285,6 @@ bool FocusManager::OnKeyDown(HWND window, UINT message, WPARAM wparam,
     return false;
   }
 
-  int repeat_count = LOWORD(lparam);
-  int flags = HIWORD(lparam);
-  if (focused_view_ &&
-      !focused_view_->ShouldLookupAccelerators(KeyEvent(
-          Event::ET_KEY_PRESSED, virtual_key_code,
-          repeat_count, flags))) {
-    // This should not be processed as an accelerator.
-    return true;
-  }
-
   // Process keyboard accelerators.
   // We process accelerators here as we have no way of knowing if a HWND has
   // really processed a key event. If the key combination matches an
@@ -297,9 +294,12 @@ bool FocusManager::OnKeyDown(HWND window, UINT message, WPARAM wparam,
                                       win_util::IsShiftPressed(),
                                       win_util::IsCtrlPressed(),
                                       win_util::IsAltPressed()));
-  if (ProcessAccelerator(accelerator)) {
-    // If a shortcut was activated for this keydown message, do not
-    // propagate the message further.
+  // We give a chance to the focused view to override the accelerator before
+  // processing it.
+  if ((focused_view_ && focused_view_->OverrideAccelerator(accelerator)) ||
+      ProcessAccelerator(accelerator)) {
+    // If a shortcut was activated for this keydown message, do not propagate
+    // the message further.
     return false;
   }
   return true;
@@ -645,15 +645,8 @@ bool FocusManager::ProcessAccelerator(const Accelerator& accelerator) {
   do {
     AcceleratorTarget* target =
         focus_manager->GetTargetForAccelerator(accelerator);
-    if (target) {
-      // If there is focused view, we give it a chance to process that
-      // accelerator.
-      if (!focused_view_ ||
-          !focused_view_->OverrideAccelerator(accelerator)) {
-        if (target->AcceleratorPressed(accelerator))
-          return true;
-      }
-    }
+    if (target && target->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
@@ -671,6 +664,11 @@ AcceleratorTarget* FocusManager::GetTargetForAccelerator(
   return NULL;
 }
 
+// static
+bool FocusManager::IsTabTraversalKeyEvent(const KeyEvent& key_event) {
+  return key_event.GetCharacter() == VK_TAB && !win_util::IsCtrlPressed();
+}
+
 void FocusManager::ViewRemoved(View* parent, View* removed) {
   if (focused_view_ && focused_view_ == removed)
     focused_view_ = NULL;
diff --git a/views/focus/focus_manager.h b/views/focus/focus_manager.h
index 8c33036..98fc1e5 100644
--- a/views/focus/focus_manager.h
+++ b/views/focus/focus_manager.h
@@ -283,6 +283,11 @@ class FocusManager {
   AcceleratorTarget* GetTargetForAccelerator(
       const Accelerator& accelerator) 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
+  // pressed).
+  static bool IsTabTraversalKeyEvent(const KeyEvent& key_event);
+
  private:
 #if defined(OS_WIN)
   explicit FocusManager(HWND root, RootView* root_view);
diff --git a/views/view.cc b/views/view.cc
index f535c50..5430a8f 100644
--- a/views/view.cc
+++ b/views/view.cc
@@ -1212,10 +1212,6 @@ bool View::ExceededDragThreshold(int delta_x, int delta_y) {
           abs(delta_y) > GetVerticalDragThreshold());
 }
 
-bool View::CanProcessTabKeyEvents() {
-  return false;
-}
-
 // Tooltips -----------------------------------------------------------------
 bool View::GetTooltipText(int x, int y, std::wstring* tooltip) {
   return false;
diff --git a/views/view.h b/views/view.h
index ed4d4a0..f3c5b8e 100644
--- a/views/view.h
+++ b/views/view.h
@@ -514,7 +514,8 @@ class View : public AcceleratorTarget {
   // Called on a view (if it is has focus) before an Accelerator is processed.
   // Views that want to override an accelerator should override this method to
   // perform the required action and return true, to indicate that the
-  // accelerator should not be processed any further.
+  // accelerator should not be processed any further (in which case the key
+  // event is eaten).
   virtual bool OverrideAccelerator(const Accelerator& accelerator) {
     return false;
   }
@@ -705,6 +706,19 @@ class View : public AcceleratorTarget {
   // (Shift-Tab).
   virtual void AboutToRequestFocusFromTabTraversal(bool reverse) { }
 
+  // Invoked when a key is pressed before the key event is processed (and
+  // potentially eaten) by the focus manager for tab traversal, accelerators and
+  // other focus related actions.
+  // The default implementation returns false, ensuring that tab traversal and
+  // accelerators processing is performed.
+  // Subclasses should return true if they want to process the key event and not
+  // have it processed as an accelerator (if any) or as a tab traversal (if the
+  // key event is for the TAB key).  In that case, OnKeyPressed will
+  // subsequently be invoked for that event.
+  virtual bool SkipDefaultKeyEventProcessing(const KeyEvent& e) {
+    return false;
+  }
+
   // Invoked when a key is pressed or released.
   // Subclasser should return true if the event has been processed and false
   // otherwise. If the event has not been processed, the parent will be given a
@@ -712,14 +726,6 @@ class View : public AcceleratorTarget {
   virtual bool OnKeyPressed(const KeyEvent& e);
   virtual bool OnKeyReleased(const KeyEvent& e);
 
-  // Whether the view wants to receive Tab and Shift-Tab key events.
-  // If false, Tab and Shift-Tabs key events are used for focus traversal and
-  // are not sent to the view. If true, the events are sent to the view and not
-  // used for focus traversal.
-  // This implementation returns false (so that by default views handle nicely
-  // the keyboard focus traversal).
-  virtual bool CanProcessTabKeyEvents();
-
   // Invoked when the user uses the mousewheel. Implementors should return true
   // if the event has been processed and false otherwise. This message is sent
   // if the view is focused. If the event has not been processed, the parent
@@ -1001,14 +1007,6 @@ class View : public AcceleratorTarget {
   // associated with them (so the root view gets the keyboard messages).
   virtual void Focus();
 
-  // Invoked when a key is pressed before the key event is processed by the
-  // focus manager for accelerators.  This gives a chance to the view to
-  // override an accelerator.  Subclasser should return false if they want to
-  // process the key event and not have it translated to an accelerator (if
-  // any).  In that case, OnKeyPressed will subsequently be invoked for that
-  // event.
-  virtual bool ShouldLookupAccelerators(const KeyEvent& e) { return true; }
-
   // These are cover methods that invoke the method of the same name on
   // the DragController. Subclasses may wish to override rather than install
   // a DragController.