Fixes focus traversal bug

When on the last focusable view in a Container FocusManager would
start searching from Widget. Problem is child Widgets share the same
FocusManager, meaning focus in a child widget wraps to the parent,
which is unexpected in most situations. In theory there could be
situations where we should wrap to the parent, but I don't think we
have any.

BUG=276213
TEST=covered by test
R=dmazzoni@chromium.org

Review URL: https://chromiumcodereview.appspot.com/23475012

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

ui/views/focus/focus_manager.cc

@@ -139,7 +139,7 @@ bool FocusManager::ContainsView(View* view) {
 }
 
 void FocusManager::AdvanceFocus(bool reverse) {
-  View* v = GetNextFocusableView(focused_view_, reverse, false);
+  View* v = GetNextFocusableView(focused_view_, NULL, reverse, false);
   // Note: Do not skip this next block when v == focused_view_.  If the user
   // tabs past the last focusable element in a webpage, we'll get here, and if
   // the TabContentsContainerView is the only focusable view (possible in
@@ -219,6 +219,7 @@ bool FocusManager::RotatePaneFocus(Direction direction,
 }
 
 View* FocusManager::GetNextFocusableView(View* original_starting_view,
+                                         Widget* starting_widget,
                                          bool reverse,
                                          bool dont_loop) {
   FocusTraversable* focus_traversable = NULL;
@@ -262,7 +263,8 @@ View* FocusManager::GetNextFocusableView(View* original_starting_view,
       }
     }
   } else {
-    focus_traversable = widget_->GetFocusTraversable();
+    Widget* widget = starting_widget ? starting_widget : widget_;
+    focus_traversable = widget->GetFocusTraversable();
   }
 
   // Traverse the FocusTraversable tree down to find the focusable view.
@@ -303,9 +305,12 @@ View* FocusManager::GetNextFocusableView(View* original_starting_view,
     // infinitely looping in empty windows.
     if (!dont_loop && original_starting_view) {
       // Easy, just clear the selection and press tab again.
-      // By calling with NULL as the starting view, we'll start from the
-      // top_root_view.
-      return GetNextFocusableView(NULL, reverse, true);
+      // By calling with NULL as the starting view, we'll start from either
+      // the starting views widget or |widget_|.
+      Widget* widget = original_starting_view->GetWidget();
+      if (widget->widget_delegate()->ShouldAdvanceFocusToTopLevelWidget())
+        widget = widget_;
+      return GetNextFocusableView(NULL, widget, reverse, true);
     }
   }
   return NULL;

ui/views/focus/focus_manager.h

@@ -308,8 +308,16 @@ class VIEWS_EXPORT FocusManager {
   }
 
  private:
-  // Returns the next focusable view.
-  View* GetNextFocusableView(View* starting_view, bool reverse, bool dont_loop);
+  // Returns the next focusable view. Traversal starts at |starting_view|. If
+  // |starting_view| is NULL |starting_widget| is consuled to determine which
+  // Widget to start from. See
+  // WidgetDelegate::ShouldAdvanceFocusToTopLevelWidget() for details. If both
+  // |starting_view| and |starting_widget| are NULL, traversal starts at
+  // |widget_|.
+  View* GetNextFocusableView(View* starting_view,
+                             Widget* starting_widget,
+                             bool reverse,
+                             bool dont_loop);
 
   // Returns the focusable view found in the FocusTraversable specified starting
   // at the specified view. This traverses down along the FocusTraversable

ui/views/focus/focus_manager_unittest.cc

@@ -829,4 +829,85 @@ TEST_F(FocusManagerTest, StoreFocusedView) {
   EXPECT_EQ(&view, GetFocusManager()->GetStoredFocusView());
 }
 
+namespace {
+
+// Trivial WidgetDelegate implementation that allows setting return value of
+// ShouldAdvanceFocusToTopLevelWidget().
+class AdvanceFocusWidgetDelegate : public WidgetDelegate {
+ public:
+  explicit AdvanceFocusWidgetDelegate(Widget* widget)
+      : widget_(widget),
+        should_advance_focus_to_parent_(false) {}
+  virtual ~AdvanceFocusWidgetDelegate() {}
+
+  void set_should_advance_focus_to_parent(bool value) {
+    should_advance_focus_to_parent_ = value;
+  }
+
+  // WidgetDelegate overrides:
+  virtual bool ShouldAdvanceFocusToTopLevelWidget() const OVERRIDE {
+    return should_advance_focus_to_parent_;
+  }
+  virtual Widget* GetWidget() OVERRIDE { return widget_; }
+  virtual const Widget* GetWidget() const OVERRIDE { return widget_; }
+
+ private:
+  Widget* widget_;
+  bool should_advance_focus_to_parent_;
+
+  DISALLOW_COPY_AND_ASSIGN(AdvanceFocusWidgetDelegate);
+};
+
+}  // namespace
+
+// Verifies focus wrapping happens in the same widget.
+TEST_F(FocusManagerTest, AdvanceFocusStaysInWidget) {
+  // Add |widget_view| as a child of the Widget.
+  View* widget_view = new View;
+  widget_view->set_focusable(true);
+  widget_view->SetBounds(20, 0, 20, 20);
+  GetContentsView()->AddChildView(widget_view);
+
+  // Create a widget with two views, focus the second.
+  scoped_ptr<AdvanceFocusWidgetDelegate> delegate;
+  Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_WINDOW);
+  params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  params.child = true;
+  params.bounds = gfx::Rect(10, 10, 100, 100);
+  params.parent = GetWidget()->GetNativeView();
+  Widget child_widget;
+  delegate.reset(new AdvanceFocusWidgetDelegate(&child_widget));
+  params.delegate = delegate.get();
+  child_widget.Init(params);
+  View* view1 = new View;
+  view1->set_focusable(true);
+  view1->SetBounds(0, 0, 20, 20);
+  View* view2 = new View;
+  view2->set_focusable(true);
+  view2->SetBounds(20, 0, 20, 20);
+  child_widget.client_view()->AddChildView(view1);
+  child_widget.client_view()->AddChildView(view2);
+  child_widget.Show();
+  view2->RequestFocus();
+  EXPECT_EQ(view2, GetFocusManager()->GetFocusedView());
+
+  // Advance focus backwards, which should focus the first.
+  GetFocusManager()->AdvanceFocus(false);
+  EXPECT_EQ(view1, GetFocusManager()->GetFocusedView());
+
+  // Focus forward to |view2|.
+  GetFocusManager()->AdvanceFocus(true);
+  EXPECT_EQ(view2, GetFocusManager()->GetFocusedView());
+
+  // And forward again, wrapping back to |view1|.
+  GetFocusManager()->AdvanceFocus(true);
+  EXPECT_EQ(view1, GetFocusManager()->GetFocusedView());
+
+  // Allow focus to go to the parent, and focus backwards which should now move
+  // up |widget_view| (in the parent).
+  delegate->set_should_advance_focus_to_parent(true);
+  GetFocusManager()->AdvanceFocus(true);
+  EXPECT_EQ(widget_view, GetFocusManager()->GetFocusedView());
+}
+
 }  // namespace views

ui/views/widget/widget.cc

@@ -86,10 +86,15 @@ class DefaultWidgetDelegate : public WidgetDelegate {
   virtual const Widget* GetWidget() const OVERRIDE {
     return widget_;
   }
-
   virtual bool CanActivate() const OVERRIDE {
     return can_activate_;
   }
+  virtual bool ShouldAdvanceFocusToTopLevelWidget() const OVERRIDE {
+    // In most situations where a Widget is used without a delegate the Widget
+    // is used as a container, so that we want focus to advance to the top-level
+    // widget. A good example of this is the find bar.
+    return true;
+  }
 
  private:
   Widget* widget_;

ui/views/widget/widget.h

@@ -769,8 +769,8 @@ class VIEWS_EXPORT Widget : public internal::NativeWidgetDelegate,
 
   ObserverList<WidgetObserver> observers_;
 
-  // Non-owned pointer to the Widget's delegate.  May be NULL if no delegate is
-  // being used.
+  // Non-owned pointer to the Widget's delegate. If a NULL delegate is supplied
+  // to Init() a default WidgetDelegate is created.
   WidgetDelegate* widget_delegate_;
 
   // The root of the View hierarchy attached to this window.

ui/views/widget/widget_delegate.cc

@@ -162,6 +162,10 @@ void WidgetDelegate::GetWidgetHitTestMask(gfx::Path* mask) const {
   DCHECK(mask);
 }
 
+bool WidgetDelegate::ShouldAdvanceFocusToTopLevelWidget() const {
+  return false;
+}
+
 bool WidgetDelegate::ShouldDescendIntoChildForEventHandling(
     gfx::NativeView child,
     const gfx::Point& location) {

ui/views/widget/widget_delegate.h

@@ -159,6 +159,11 @@ class VIEWS_EXPORT WidgetDelegate {
   // Provides the hit-test mask if HasHitTestMask above returns true.
   virtual void GetWidgetHitTestMask(gfx::Path* mask) const;
 
+  // Returns true if focus should advance to the top level widget when
+  // tab/shift-tab is hit and on the last/first focusable view. Default returns
+  // false, which means tab/shift-tab never advance to the top level Widget.
+  virtual bool ShouldAdvanceFocusToTopLevelWidget() const;
+
   // Returns true if event handling should descend into |child|.
   // |location| is in terms of the Window.
   virtual bool ShouldDescendIntoChildForEventHandling(