Fix toolbar keyboard focus (shift-alt-t), which was broken by me, and toolbar button context menus on VK_APPS, which was broken by Jonas.

The overall focus issue was caused by my change that made View::RequestFocus() sanity-check that the View was focusable.  The toolbar uses a crazy hack where it purposefully wants to get focus even though it's not focusable.  :P

The context menu issue was caused by Jonas changing the name of a virtual function, presumably not realizing it was a virtual, not just a simple accessor.  I changed the name back and marked this function (and several others) as virtual.  In order to avoid blowing up the source in toolbar_view.cc, I reverted users of the accessor to just using the member variable name.

Also reordered a couple functions to match the order they were originally declared in View.h.
Review URL: http://codereview.chromium.org/27175

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

3 files changed, 32 insertions, 30 deletions

diff --git a/chrome/browser/views/frame/browser_view.cc b/chrome/browser/views/frame/browser_view.cc
index 9d6e7dd..960aef1 100644
--- a/chrome/browser/views/frame/browser_view.cc
+++ b/chrome/browser/views/frame/browser_view.cc
@@ -51,6 +51,7 @@
 #include "chrome/views/hwnd_notification_source.h"
 #include "chrome/views/native_scroll_bar.h"
 #include "chrome/views/non_client_view.h"
+#include "chrome/views/root_view.h"
 #include "chrome/views/view.h"
 #include "chrome/views/window.h"
 #include "grit/chromium_strings.h"
@@ -700,7 +701,10 @@ void BrowserView::FocusToolbar() {
   // Do not restore the button that previously had accessibility focus, if
   // focus is set by using the toolbar focus keyboard shortcut.
   toolbar_->set_acc_focused_view(NULL);
-  toolbar_->RequestFocus();
+  // HACK: Do not use RequestFocus() here, as the toolbar is not marked as
+  // "focusable".  Instead bypass the sanity check in RequestFocus() and just
+  // force it to focus, which will do the right thing.
+  GetRootView()->FocusView(toolbar_);
 }
 
 void BrowserView::DestroyBrowser() {
diff --git a/chrome/browser/views/toolbar_view.cc b/chrome/browser/views/toolbar_view.cc
index 2dc3858..295c6ef 100644
--- a/chrome/browser/views/toolbar_view.cc
+++ b/chrome/browser/views/toolbar_view.cc
@@ -360,10 +360,10 @@ void BrowserToolbarView::Paint(ChromeCanvas* canvas) {
 void BrowserToolbarView::DidGainFocus() {
   // Check to see if MSAA focus should be restored to previously focused button,
   // and if button is an enabled, visibled child of toolbar.
-  if (!acc_focused_view() ||
-      (acc_focused_view()->GetParent()->GetID() != VIEW_ID_TOOLBAR) ||
-      !acc_focused_view()->IsEnabled() ||
-      !acc_focused_view()->IsVisible()) {
+  if (!acc_focused_view_ ||
+      (acc_focused_view_->GetParent()->GetID() != VIEW_ID_TOOLBAR) ||
+      !acc_focused_view_->IsEnabled() ||
+      !acc_focused_view_->IsVisible()) {
     // Find first accessible child (-1 to start search at parent).
     int first_acc_child = GetNextAccessibleViewIndex(-1, false);
 
@@ -378,15 +378,15 @@ void BrowserToolbarView::DidGainFocus() {
   int view_index = VIEW_ID_TOOLBAR;
 
   // Set hot-tracking for child, and update focused_view for MSAA focus event.
-  if (acc_focused_view()) {
-    acc_focused_view()->SetHotTracked(true);
+  if (acc_focused_view_) {
+    acc_focused_view_->SetHotTracked(true);
 
     // Show the tooltip for the view that got the focus.
     if (GetWidget()->GetTooltipManager())
       GetWidget()->GetTooltipManager()->ShowKeyboardTooltip(acc_focused_view_);
 
     // Update focused_view with MSAA-adjusted child id.
-    view_index = acc_focused_view()->GetID();
+    view_index = acc_focused_view_->GetID();
   }
 
   HWND hwnd = GetWidget()->GetHWND();
@@ -397,9 +397,9 @@ void BrowserToolbarView::DidGainFocus() {
 }
 
 void BrowserToolbarView::WillLoseFocus() {
-  if (acc_focused_view()) {
+  if (acc_focused_view_) {
     // Resetting focus state.
-    acc_focused_view()->SetHotTracked(false);
+    acc_focused_view_->SetHotTracked(false);
   }
   // Any tooltips that are active should be hidden when toolbar loses focus.
   if (GetWidget() && GetWidget()->GetTooltipManager())
@@ -680,14 +680,6 @@ void BrowserToolbarView::OnGetProfilesDone(
       l10n_util::GetString(IDS_SELECT_PROFILE_DIALOG_NEW_PROFILE_ENTRY));
 }
 
-bool BrowserToolbarView::GetAccessibleRole(VARIANT* role) {
-  DCHECK(role);
-
-  role->vt = VT_I4;
-  role->lVal = ROLE_SYSTEM_TOOLBAR;
-  return true;
-}
-
 bool BrowserToolbarView::GetAccessibleName(std::wstring* name) {
   if (!accessible_name_.empty()) {
     (*name).assign(accessible_name_);
@@ -696,6 +688,14 @@ bool BrowserToolbarView::GetAccessibleName(std::wstring* name) {
   return false;
 }
 
+bool BrowserToolbarView::GetAccessibleRole(VARIANT* role) {
+  DCHECK(role);
+
+  role->vt = VT_I4;
+  role->lVal = ROLE_SYSTEM_TOOLBAR;
+  return true;
+}
+
 void BrowserToolbarView::SetAccessibleName(const std::wstring& name) {
   accessible_name_.assign(name);
 }
@@ -729,8 +729,8 @@ int BrowserToolbarView::GetNextAccessibleViewIndex(int view_index,
 }
 
 void BrowserToolbarView::ShowContextMenu(int x, int y, bool is_mouse_gesture) {
-  if (GetAccFocusedChildView())
-    GetAccFocusedChildView()->ShowContextMenu(x, y, is_mouse_gesture);
+  if (acc_focused_view_)
+    acc_focused_view_->ShowContextMenu(x, y, is_mouse_gesture);
 }
 
 int BrowserToolbarView::GetDragOperations(views::View* sender, int x, int y) {
diff --git a/chrome/browser/views/toolbar_view.h b/chrome/browser/views/toolbar_view.h
index 4e4a560..1712ef4 100644
--- a/chrome/browser/views/toolbar_view.h
+++ b/chrome/browser/views/toolbar_view.h
@@ -87,18 +87,20 @@ class BrowserToolbarView : public views::View,
   // (such as user editing) as well.
   void Update(TabContents* tab, bool should_restore_state);
 
-  void OnInputInProgress(bool in_progress);
+  virtual void OnInputInProgress(bool in_progress);
+
+  // Returns a brief, identifying string, containing a unique, readable name.
+  virtual bool GetAccessibleName(std::wstring* name);
 
   // Returns the MSAA role of the current view. The role is what assistive
   // technologies (ATs) use to determine what behavior to expect from a given
   // control.
-  bool GetAccessibleRole(VARIANT* role);
-
-  // Returns a brief, identifying string, containing a unique, readable name.
-  bool GetAccessibleName(std::wstring* name);
+  virtual bool GetAccessibleRole(VARIANT* role);
 
   // Assigns an accessible string name.
-  void SetAccessibleName(const std::wstring& name);
+  virtual void SetAccessibleName(const std::wstring& name);
+
+  virtual View* GetAccFocusedChildView() { return acc_focused_view_; }
 
   // Returns the index of the next view of the toolbar, starting from the given
   // view index (skipping the location bar), in the given navigation direction
@@ -106,10 +108,6 @@ class BrowserToolbarView : public views::View,
   // first accessible child, based on the above policy.
   int GetNextAccessibleViewIndex(int view_index, bool nav_left);
 
-  views::View* acc_focused_view() {
-    return acc_focused_view_;
-  }
-
   void set_acc_focused_view(views::View* acc_focused_view) {
     acc_focused_view_ = acc_focused_view;
   }