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

7 files changed, 40 insertions, 36 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.