diff options
author | yutak@chromium.org <yutak@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2009-06-12 23:52:20 +0000 |
---|---|---|
committer | yutak@chromium.org <yutak@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2009-06-12 23:52:20 +0000 |
commit | 6d21eb72d7333889badc108afe21dd1bc52e1135 (patch) | |
tree | a45694ee7b3b43fffc8db3dbe113c4ab52e246b9 | |
parent | 8e0c43541d22b356b7eac90cfce4a4ebdd69c8f1 (diff) | |
download | chromium_src-6d21eb72d7333889badc108afe21dd1bc52e1135.zip chromium_src-6d21eb72d7333889badc108afe21dd1bc52e1135.tar.gz chromium_src-6d21eb72d7333889badc108afe21dd1bc52e1135.tar.bz2 |
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
-rw-r--r-- | views/focus/focus_manager.cc | 39 | ||||
-rw-r--r-- | views/focus/focus_manager.h | 8 | ||||
-rw-r--r-- | views/focus/focus_manager_unittest.cc | 24 | ||||
-rw-r--r-- | views/widget/root_view.cc | 114 | ||||
-rw-r--r-- | views/widget/root_view.h | 16 | ||||
-rw-r--r-- | views/widget/widget_win.cc | 7 | ||||
-rw-r--r-- | views/widget/widget_win.h | 2 |
7 files changed, 115 insertions, 95 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()); } diff --git a/views/widget/root_view.cc b/views/widget/root_view.cc index f5f57f4..367c720 100644 --- a/views/widget/root_view.cc +++ b/views/widget/root_view.cc @@ -570,7 +570,7 @@ View* RootView::GetFocusedView() { View* RootView::FindNextFocusableView(View* starting_view, bool reverse, Direction direction, - bool dont_loop, + bool check_starting_view, FocusTraversable** focus_traversable, View** focus_traversable_view) { *focus_traversable = NULL; @@ -582,14 +582,13 @@ View* RootView::FindNextFocusableView(View* starting_view, return NULL; } - bool skip_starting_view = true; if (!starting_view) { // Default to the first/last child starting_view = reverse ? GetChildViewAt(GetChildViewCount() - 1) : GetChildViewAt(0) ; // If there was no starting view, then the one we select is a potential // focus candidate. - skip_starting_view = false; + check_starting_view = true; } else { // The starting view should be part of this RootView. DCHECK(IsParentOf(starting_view)); @@ -597,25 +596,31 @@ View* RootView::FindNextFocusableView(View* starting_view, View* v = NULL; if (!reverse) { - v = FindNextFocusableViewImpl(starting_view, skip_starting_view, + v = FindNextFocusableViewImpl(starting_view, check_starting_view, true, (direction == DOWN) ? true : false, - starting_view->GetGroup()); + starting_view->GetGroup(), + focus_traversable, + focus_traversable_view); } else { // If the starting view is focusable, we don't want to go down, as we are // traversing the view hierarchy tree bottom-up. bool can_go_down = (direction == DOWN) && !starting_view->IsFocusable(); - v = FindPreviousFocusableViewImpl(starting_view, true, + v = FindPreviousFocusableViewImpl(starting_view, check_starting_view, true, can_go_down, - starting_view->GetGroup()); + starting_view->GetGroup(), + focus_traversable, + focus_traversable_view); } + + // Doing some sanity checks. if (v) { - if (v->IsFocusable()) - return v; - *focus_traversable = v->GetFocusTraversable(); - DCHECK(*focus_traversable); - *focus_traversable_view = v; + DCHECK(v->IsFocusable()); + return v; + } + if (*focus_traversable) { + DCHECK(*focus_traversable_view); return NULL; } // Nothing found. @@ -631,23 +636,31 @@ View* RootView::FindNextFocusableView(View* starting_view, // - if the view has no right sibling, go up the parents until you find a parent // with a right sibling and start the search from there. View* RootView::FindNextFocusableViewImpl(View* starting_view, - bool skip_starting_view, + bool check_starting_view, bool can_go_up, bool can_go_down, - int skip_group_id) { - if (!skip_starting_view) { + int skip_group_id, + FocusTraversable** focus_traversable, + View** focus_traversable_view) { + if (check_starting_view) { if (IsViewFocusableCandidate(starting_view, skip_group_id)) return FindSelectedViewForGroup(starting_view); - if (starting_view->GetFocusTraversable()) - return starting_view; + + *focus_traversable = starting_view->GetFocusTraversable(); + if (*focus_traversable) { + *focus_traversable_view = starting_view; + return NULL; + } } // First let's try the left child. if (can_go_down) { if (starting_view->GetChildViewCount() > 0) { View* v = FindNextFocusableViewImpl(starting_view->GetChildViewAt(0), - false, false, true, skip_group_id); - if (v) + true, false, true, skip_group_id, + focus_traversable, + focus_traversable_view); + if (v || *focus_traversable) return v; } } @@ -656,8 +669,10 @@ View* RootView::FindNextFocusableViewImpl(View* starting_view, View* sibling = starting_view->GetNextFocusableView(); if (sibling) { View* v = FindNextFocusableViewImpl(sibling, - false, false, true, skip_group_id); - if (v) + true, false, true, skip_group_id, + focus_traversable, + focus_traversable_view); + if (v || *focus_traversable) return v; } @@ -668,8 +683,10 @@ View* RootView::FindNextFocusableViewImpl(View* starting_view, sibling = parent->GetNextFocusableView(); if (sibling) { return FindNextFocusableViewImpl(sibling, - false, true, true, - skip_group_id); + true, true, true, + skip_group_id, + focus_traversable, + focus_traversable_view); } parent = parent->GetParent(); } @@ -685,36 +702,51 @@ View* RootView::FindNextFocusableViewImpl(View* starting_view, // - start the search on the left sibling. // - if there are no left sibling, start the search on the parent (without going // down). -View* RootView::FindPreviousFocusableViewImpl(View* starting_view, - bool skip_starting_view, - bool can_go_up, - bool can_go_down, - int skip_group_id) { +View* RootView::FindPreviousFocusableViewImpl( + View* starting_view, + bool check_starting_view, + bool can_go_up, + bool can_go_down, + int skip_group_id, + FocusTraversable** focus_traversable, + View** focus_traversable_view) { // Let's go down and right as much as we can. if (can_go_down) { + // Before we go into the direct children, we have to check if this view has + // a FocusTraversable. + *focus_traversable = starting_view->GetFocusTraversable(); + if (*focus_traversable) { + *focus_traversable_view = starting_view; + return NULL; + } + if (starting_view->GetChildViewCount() > 0) { View* view = starting_view->GetChildViewAt(starting_view->GetChildViewCount() - 1); - View* v = FindPreviousFocusableViewImpl(view, false, false, true, - skip_group_id); - if (v) + View* v = FindPreviousFocusableViewImpl(view, true, false, true, + skip_group_id, + focus_traversable, + focus_traversable_view); + if (v || *focus_traversable) return v; } } - if (!skip_starting_view) { - if (IsViewFocusableCandidate(starting_view, skip_group_id)) - return FindSelectedViewForGroup(starting_view); - if (starting_view->GetFocusTraversable()) - return starting_view; + // Then look at this view. Here, we do not need to see if the view has + // a FocusTraversable, since we do not want to go down any more. + if (check_starting_view && + IsViewFocusableCandidate(starting_view, skip_group_id)) { + return FindSelectedViewForGroup(starting_view); } // Then try the left sibling. View* sibling = starting_view->GetPreviousFocusableView(); if (sibling) { return FindPreviousFocusableViewImpl(sibling, - false, true, true, - skip_group_id); + true, true, true, + skip_group_id, + focus_traversable, + focus_traversable_view); } // Then go up the parent. @@ -722,8 +754,10 @@ View* RootView::FindPreviousFocusableViewImpl(View* starting_view, View* parent = starting_view->GetParent(); if (parent) return FindPreviousFocusableViewImpl(parent, - false, true, false, - skip_group_id); + true, true, false, + skip_group_id, + focus_traversable, + focus_traversable_view); } // We found nothing. diff --git a/views/widget/root_view.h b/views/widget/root_view.h index 2f1e391..6488de5 100644 --- a/views/widget/root_view.h +++ b/views/widget/root_view.h @@ -144,7 +144,7 @@ class RootView : public View, virtual View* FindNextFocusableView(View* starting_view, bool reverse, Direction direction, - bool dont_loop, + bool check_starting_view, FocusTraversable** focus_traversable, View** focus_traversable_view); virtual FocusTraversable* GetFocusTraversableParent(); @@ -214,22 +214,26 @@ class RootView : public View, // Returns the next focusable view or view containing a FocusTraversable (NULL // if none was found), starting at the starting_view. - // skip_starting_view, can_go_up and can_go_down controls the traversal of + // check_starting_view, can_go_up and can_go_down controls the traversal of // the views hierarchy. // skip_group_id specifies a group_id, -1 means no group. All views from a // group are traversed in one pass. View* FindNextFocusableViewImpl(View* starting_view, - bool skip_starting_view, + bool check_starting_view, bool can_go_up, bool can_go_down, - int skip_group_id); + int skip_group_id, + FocusTraversable** focus_traversable, + View** focus_traversable_view); // Same as FindNextFocusableViewImpl but returns the previous focusable view. View* FindPreviousFocusableViewImpl(View* starting_view, - bool skip_starting_view, + bool check_starting_view, bool can_go_up, bool can_go_down, - int skip_group_id); + int skip_group_id, + FocusTraversable** focus_traversable, + View** focus_traversable_view); // Convenience method that returns true if a view is focusable and does not // belong to the specified group. diff --git a/views/widget/widget_win.cc b/views/widget/widget_win.cc index 7d8aced..d4cd460 100644 --- a/views/widget/widget_win.cc +++ b/views/widget/widget_win.cc @@ -475,12 +475,13 @@ void WidgetWin::DidProcessMessage(const MSG& msg) { // FocusTraversable View* WidgetWin::FindNextFocusableView( - View* starting_view, bool reverse, Direction direction, bool dont_loop, - FocusTraversable** focus_traversable, View** focus_traversable_view) { + View* starting_view, bool reverse, Direction direction, + bool check_starting_view, FocusTraversable** focus_traversable, + View** focus_traversable_view) { return root_view_->FindNextFocusableView(starting_view, reverse, direction, - dont_loop, + check_starting_view, focus_traversable, focus_traversable_view); } diff --git a/views/widget/widget_win.h b/views/widget/widget_win.h index 2a9562c..b8500c7 100644 --- a/views/widget/widget_win.h +++ b/views/widget/widget_win.h @@ -239,7 +239,7 @@ class WidgetWin : public Widget, virtual View* FindNextFocusableView(View* starting_view, bool reverse, Direction direction, - bool dont_loop, + bool check_starting_view, FocusTraversable** focus_traversable, View** focus_traversable_view); virtual FocusTraversable* GetFocusTraversableParent(); |