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

15 files changed, 101 insertions, 93 deletions

diff --git a/chrome/browser/autocomplete/autocomplete_edit_view_win.cc b/chrome/browser/autocomplete/autocomplete_edit_view_win.cc
index 88facc6..be81645 100644
--- a/chrome/browser/autocomplete/autocomplete_edit_view_win.cc
+++ b/chrome/browser/autocomplete/autocomplete_edit_view_win.cc
@@ -1641,8 +1641,8 @@ bool AutocompleteEditViewWin::OnKeyDownOnlyWritable(TCHAR key,
   // WM_SYSKEYDOWN, so we need to check (flags & KF_ALTDOWN) in various places
   // in this function even with a WM_SYSKEYDOWN handler.
 
-  // Update LocationBarView::ShouldLookupAccelerators() as well when you add
-  // some key combinations here.
+  // Update LocationBarView::SkipDefaultKeyEventProcessing() as well when you
+  // add some key combinations here.
   int count = repeat_count;
   switch (key) {
     case VK_RETURN:
diff --git a/chrome/browser/views/dom_view.cc b/chrome/browser/views/dom_view.cc
index fbbd2f5..7213bcf 100644
--- a/chrome/browser/views/dom_view.cc
+++ b/chrome/browser/views/dom_view.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/views/dom_view.h"
 
 #include "chrome/browser/tab_contents/tab_contents.h"
+#include "views/focus/focus_manager.h"
 
 DOMView::DOMView() : initialized_(false), tab_contents_(NULL) {
   SetFocusable(true);
@@ -30,3 +31,10 @@ void DOMView::LoadURL(const GURL& url) {
   DCHECK(initialized_);
   tab_contents_->controller().LoadURL(url, GURL(), PageTransition::START_PAGE);
 }
+
+bool DOMView::SkipDefaultKeyEventProcessing(const views::KeyEvent& e) {
+  // Don't move the focus to the next view when tab is pressed, we want the
+  // key event to be propagated to the render view for doing the tab traversal
+  // there.
+  return views::FocusManager::IsTabTraversalKeyEvent(e);
+}
diff --git a/chrome/browser/views/dom_view.h b/chrome/browser/views/dom_view.h
index da26bc4..303e57d 100644
--- a/chrome/browser/views/dom_view.h
+++ b/chrome/browser/views/dom_view.h
@@ -11,6 +11,7 @@
 #include "base/scoped_ptr.h"
 #include "googleurl/src/gurl.h"
 #include "views/controls/hwnd_view.h"
+#include "views/event.h"
 
 class Profile;
 class SiteInstance;
@@ -33,7 +34,8 @@ class DOMView : public views::HWNDView {
   void LoadURL(const GURL& url);
 
  protected:
-  virtual bool CanProcessTabKeyEvents() { return true; }
+  // Overridden from View.
+  virtual bool SkipDefaultKeyEventProcessing(const views::KeyEvent& e);
 
   scoped_ptr<TabContents> tab_contents_;
 
diff --git a/chrome/browser/views/location_bar_view.cc b/chrome/browser/views/location_bar_view.cc
index c53a7e9..2f22fd2 100644
--- a/chrome/browser/views/location_bar_view.cc
+++ b/chrome/browser/views/location_bar_view.cc
@@ -33,6 +33,7 @@
 #include "grit/theme_resources.h"
 #include "views/background.h"
 #include "views/border.h"
+#include "views/focus/focus_manager.h"
 #include "views/widget/root_view.h"
 #include "views/widget/widget.h"
 
@@ -238,11 +239,6 @@ void LocationBarView::Paint(ChromeCanvas* canvas) {
                       std::max(height() - top_margin - kVertMargin, 0));
 }
 
-bool LocationBarView::CanProcessTabKeyEvents() {
-  // We want to receive tab key events when the hint is showing.
-  return keyword_hint_view_.IsVisible();
-}
-
 void LocationBarView::VisibleBoundsInRootChanged() {
   location_entry_->ClosePopup();
 }
@@ -851,42 +847,48 @@ void LocationBarView::KeywordHintView::Layout() {
   }
 }
 
-bool LocationBarView::ShouldLookupAccelerators(const views::KeyEvent& e) {
+bool LocationBarView::SkipDefaultKeyEventProcessing(const views::KeyEvent& e) {
+  if (keyword_hint_view_.IsVisible() &&
+      views::FocusManager::IsTabTraversalKeyEvent(e)) {
+    // We want to receive tab key events when the hint is showing.
+    return true;
+  }
+
   int c = e.GetCharacter();
-  // We don't translate accelerators for ALT + numpad digit, they are used for
+  // We don't process ALT + numpad digit as accelerators, they are used for
   // entering special characters.
   if (e.IsAltDown() && win_util::IsNumPadDigit(c, e.IsExtendedKey()))
-    return false;
+    return true;
 
   // Skip accelerators for key combinations omnibox wants to crack. This list
   // should be synced with AutocompleteEditViewWin::OnKeyDownOnlyWritable().
+  // (but for tab which is dealt with above).
   //
-  // We cannot return false for all keys because we still need to handle some
+  // We cannot return true for all keys because we still need to handle some
   // accelerators (e.g., F5 for reload the page should work even when the
   // Omnibox gets focused).
   switch (c) {
   case VK_RETURN:
-    return false;
+    return true;
 
   case VK_UP:
   case VK_DOWN:
-    return e.IsAltDown();
+    return !e.IsAltDown();
 
   case VK_DELETE:
   case VK_INSERT:
-    return e.IsAltDown() || !e.IsShiftDown();
+    return !e.IsAltDown() && e.IsShiftDown();
 
   case 'X':
   case 'V':
-    return e.IsAltDown() || !e.IsControlDown();
+    return !e.IsAltDown() && e.IsControlDown();
 
   case VK_BACK:
-  case VK_TAB:
   case 0xbb:
-    return false;
+    return true;
 
   default:
-    return true;
+    return false;
   }
 }
 
diff --git a/chrome/browser/views/location_bar_view.h b/chrome/browser/views/location_bar_view.h
index a6ea987..4c30b4b 100644
--- a/chrome/browser/views/location_bar_view.h
+++ b/chrome/browser/views/location_bar_view.h
@@ -83,9 +83,6 @@ class LocationBarView : public LocationBar,
   // No focus border for the location bar, the caret is enough.
   virtual void PaintFocusBorder(ChromeCanvas* canvas) { }
 
-  // Overridden from View so we can use <tab> to go into keyword search mode.
-  virtual bool CanProcessTabKeyEvents();
-
   // Called when any ancestor changes its size, asks the AutocompleteEditModel
   // to close its popup.
   virtual void VisibleBoundsInRootChanged();
@@ -136,7 +133,7 @@ class LocationBarView : public LocationBar,
   void Focus();
 
   // Overridden from Chrome::View.
-  virtual bool ShouldLookupAccelerators(const views::KeyEvent& e);
+  virtual bool SkipDefaultKeyEventProcessing(const views::KeyEvent& event);
 
  private:
   // View used when the user has selected a keyword.
diff --git a/chrome/browser/views/tab_contents_container_view.cc b/chrome/browser/views/tab_contents_container_view.cc
index cb08fcb..def7279 100644
--- a/chrome/browser/views/tab_contents_container_view.cc
+++ b/chrome/browser/views/tab_contents_container_view.cc
@@ -110,12 +110,6 @@ void TabContentsContainerView::AboutToRequestFocusFromTabTraversal(
   tab_contents_->SetInitialFocus(reverse);
 }
 
-bool TabContentsContainerView::CanProcessTabKeyEvents() {
-  // TabContents with no RootView are supposed to deal with the focus traversal
-  // explicitly.  For that reason, they receive tab key events as is.
-  return !!tab_contents_;
-}
-
 views::FocusTraversable* TabContentsContainerView::GetFocusTraversableParent() {
   return GetRootView();
 }
@@ -158,15 +152,16 @@ bool TabContentsContainerView::GetAccessibleRole(
   return true;
 }
 
-bool TabContentsContainerView::ShouldLookupAccelerators(
+bool TabContentsContainerView::SkipDefaultKeyEventProcessing(
     const views::KeyEvent& e) {
-  // Don't look-up accelerators if we are showing a non-crashed TabContents.
+  // Don't look-up accelerators or tab-traverse if we are showing a non-crashed
+  // TabContents.
   // We'll first give the page a chance to process the key events.  If it does
   // not process them, they'll be returned to us and we'll treat them as
   // accelerators then.
   if (tab_contents_ && !tab_contents_->is_crashed())
-    return false;
-  return true;
+    return true;
+  return false;
 }
 
 void TabContentsContainerView::Observe(NotificationType type,
diff --git a/chrome/browser/views/tab_contents_container_view.h b/chrome/browser/views/tab_contents_container_view.h
index cc901ba..a742c49 100644
--- a/chrome/browser/views/tab_contents_container_view.h
+++ b/chrome/browser/views/tab_contents_container_view.h
@@ -14,6 +14,7 @@ class TabContents;
 
 #include "chrome/common/notification_registrar.h"
 #include "views/controls/hwnd_view.h"
+#include "views/event.h"
 #include "views/focus/focus_manager.h"
 
 // This View contains the TabContents.
@@ -35,7 +36,6 @@ class TabContentsContainerView : public views::HWNDView,
   virtual void Focus();
   virtual void RequestFocus();
   virtual void AboutToRequestFocusFromTabTraversal(bool reverse);
-  virtual bool CanProcessTabKeyEvents();
   virtual bool GetAccessibleRole(AccessibilityTypes::Role* role);
 
   // Overridden from HWNDView.
@@ -49,8 +49,8 @@ class TabContentsContainerView : public views::HWNDView,
 
  protected:
   // Web content should be given first crack at accelerators. This function
-  // returns false if the current tab is a webcontents.
-  virtual bool ShouldLookupAccelerators(const views::KeyEvent& e);
+  // returns true if not the sad tab.
+  virtual bool SkipDefaultKeyEventProcessing(const views::KeyEvent& e);
 
  private:
   // Add or remove observers for events that we care about.
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.