Fix reversed focus traversal order issue.

This change fixes several problems in handling focus traversal and resolves TabbedPane's traversal order issue.

Reversed focus traversal is very tricky, and the original traversal code overlooked several things.
(1) A focusable view may have a FocusTraverasble. 
(2) When we are going down, we have to check if the current view has a FocusTraversable.
(3) When we are going up from a FocusTraversable, the parent view may gain the next focus.
This change fixes these issues.

BUG=6871 
TEST=See issue 6871.
Review URL: http://codereview.chromium.org/125062

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

3 files changed, 26 insertions, 45 deletions

diff --git a/views/focus/focus_manager.cc b/views/focus/focus_manager.cc
index ab67301..abbc456 100644
--- a/views/focus/focus_manager.cc
+++ b/views/focus/focus_manager.cc
@@ -360,12 +360,19 @@ View* FocusManager::GetNextFocusableView(View* original_starting_view,
 
   View* starting_view = NULL;
   if (original_starting_view) {
-    // If the starting view has a focus traversable, use it.
-    // This is the case with WidgetWins for example.
-    focus_traversable = original_starting_view->GetFocusTraversable();
-
-    // Otherwise default to the root view.
-    if (!focus_traversable) {
+    if (!reverse) {
+      // If the starting view has a focus traversable, use it.
+      // This is the case with WidgetWins for example.
+      focus_traversable = original_starting_view->GetFocusTraversable();
+
+      // Otherwise default to the root view.
+      if (!focus_traversable) {
+        focus_traversable = original_starting_view->GetRootView();
+        starting_view = original_starting_view;
+      }
+    } else {
+      // When you are going back, starting view's FocusTraversable should not be
+      // used.
       focus_traversable = original_starting_view->GetRootView();
       starting_view = original_starting_view;
     }
@@ -374,8 +381,7 @@ View* FocusManager::GetNextFocusableView(View* original_starting_view,
   }
 
   // Traverse the FocusTraversable tree down to find the focusable view.
-  View* v = FindFocusableView(focus_traversable, starting_view,
-                              reverse, dont_loop);
+  View* v = FindFocusableView(focus_traversable, starting_view, reverse);
   if (v) {
     return v;
   } else {
@@ -386,15 +392,17 @@ View* FocusManager::GetNextFocusableView(View* original_starting_view,
     while (parent_focus_traversable) {
       FocusTraversable* new_focus_traversable = NULL;
       View* new_starting_view = NULL;
-      v = parent_focus_traversable ->FindNextFocusableView(
-          starting_view, reverse, FocusTraversable::UP, dont_loop,
-          &new_focus_traversable, &new_starting_view);
+      // When we are going backward, the parent view might gain the next focus.
+      bool check_starting_view = reverse;
+      v = parent_focus_traversable->FindNextFocusableView(
+          starting_view, reverse, FocusTraversable::UP,
+          check_starting_view, &new_focus_traversable, &new_starting_view);
 
       if (new_focus_traversable) {
         DCHECK(!v);
 
         // There is a FocusTraversable, traverse it down.
-        v = FindFocusableView(new_focus_traversable, NULL, reverse, dont_loop);
+        v = FindFocusableView(new_focus_traversable, NULL, reverse);
       }
 
       if (v)
@@ -563,14 +571,13 @@ FocusManager* FocusManager::GetParentFocusManager() const {
 // FocusTraversable hierarchy.
 View* FocusManager::FindFocusableView(FocusTraversable* focus_traversable,
                                       View* starting_view,
-                                      bool reverse,
-                                      bool dont_loop) {
+                                      bool reverse) {
   FocusTraversable* new_focus_traversable = NULL;
   View* new_starting_view = NULL;
   View* v = focus_traversable->FindNextFocusableView(starting_view,
                                                      reverse,
                                                      FocusTraversable::DOWN,
-                                                     dont_loop,
+                                                     false,
                                                      &new_focus_traversable,
                                                      &new_starting_view);
 
@@ -584,7 +591,7 @@ View* FocusManager::FindFocusableView(FocusTraversable* focus_traversable,
     v = focus_traversable->FindNextFocusableView(starting_view,
                                                  reverse,
                                                  FocusTraversable::DOWN,
-                                                 dont_loop,
+                                                 false,
                                                  &new_focus_traversable,
                                                  &new_starting_view);
   }
diff --git a/views/focus/focus_manager.h b/views/focus/focus_manager.h
index c5e2be9a..59e9b3b 100644
--- a/views/focus/focus_manager.h
+++ b/views/focus/focus_manager.h
@@ -107,8 +107,7 @@ class FocusTraversable {
   //   previous (reverse is true) view.
   // - |direction| specifies whether we are traversing down (meaning we should
   //   look into child views) or traversing up (don't look at child views).
-  // - |dont_loop| if true specifies that if there is a loop in the focus
-  //   hierarchy, we should keep traversing after the last view of the loop.
+  // - |check_starting_view| is true if starting_view may obtain the next focus.
   // - |focus_traversable| is set to the focus traversable that should be
   //   traversed if one is found (in which case the call returns NULL).
   // - |focus_traversable_view| is set to the view associated with the
@@ -118,7 +117,7 @@ class FocusTraversable {
   virtual View* FindNextFocusableView(View* starting_view,
                                       bool reverse,
                                       Direction direction,
-                                      bool dont_loop,
+                                      bool check_starting_view,
                                       FocusTraversable** focus_traversable,
                                       View** focus_traversable_view) = 0;
 
@@ -309,8 +308,7 @@ class FocusManager {
   // Returns NULL if no focusable view were found.
   View* FindFocusableView(FocusTraversable* focus_traversable,
                           View* starting_view,
-                          bool reverse,
-                          bool dont_loop);
+                          bool reverse);
 
   // The RootView of the window associated with this FocusManager.
   RootView* top_root_view_;
diff --git a/views/focus/focus_manager_unittest.cc b/views/focus/focus_manager_unittest.cc
index f4142db..4821112 100644
--- a/views/focus/focus_manager_unittest.cc
+++ b/views/focus/focus_manager_unittest.cc
@@ -892,21 +892,9 @@ TEST_F(FocusTraversalTest, NormalTraversal) {
   // Let's traverse in reverse order.
   for (int i = 0; i < 3; ++i) {
     for (int j = arraysize(kTraversalIDs) - 1; j >= 0; --j) {
-      // TODO(jcampan): Remove this hack.  The reverse order of traversal in
-      // Tabbed Panes is broken (we go to the tab before going to the content).
-      if (kTraversalIDs[j] == kStyleContainerID)
-        j--;  // Ignore the tab.
-
       GetFocusManager()->AdvanceFocus(true);
       View* focused_view = GetFocusManager()->GetFocusedView();
       EXPECT_TRUE(focused_view != NULL);
-
-      // TODO(jcampan): Remove this hack, same as above.
-      if (focused_view->GetID() == kStyleContainerID) {
-        j++;  // Ignore the tab.
-        continue;
-      }
-
       if (focused_view)
         EXPECT_EQ(kTraversalIDs[j], focused_view->GetID());
     }
@@ -963,21 +951,9 @@ TEST_F(FocusTraversalTest, TraversalWithNonEnabledViews) {
   // Same thing in reverse.
   for (int i = 0; i < 3; ++i) {
     for (int j = arraysize(kTraversalIDs) - 1; j >= 0; --j) {
-      // TODO(jcampan): Remove this hack.  The reverse order of traversal in
-      // Tabbed Panes is broken (we go to the tab before going to the content).
-      if (kTraversalIDs[j] == kStyleContainerID)
-        j--;  // Ignore the tab.
-
       GetFocusManager()->AdvanceFocus(true);
       focused_view = GetFocusManager()->GetFocusedView();
       EXPECT_TRUE(focused_view != NULL);
-
-      // TODO(jcampan): Remove this hack, same as above.
-      if (focused_view->GetID() == kStyleContainerID) {
-        j++;  // Ignore the tab.
-        continue;
-      }
-
       if (focused_view)
         EXPECT_EQ(kTraversalIDs[j], focused_view->GetID());
     }