Fixes regression in links ending up too small. The problem is that we

changed IsFocusable to check IsVisibleInRootView. This meant that any
views (like label) whose preferred size takes into account focusable
and cached the preferred size before they were visible in the root
view would end up with the wrong size because they didn't recalculate
when IsVisibleInRootView changed.

I've separated out the two states so that IsFocusable no longer checks
IsVisibleInRootView and is protected. All consumers now call
IsFocusableInRootView which checks IsFocusable and
IsVisibleInRootView.

BUG=49635
TEST=see bug.

Review URL: http://codereview.chromium.org/3046008

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

8 files changed, 44 insertions, 39 deletions

diff --git a/views/controls/menu/menu_controller.cc b/views/controls/menu/menu_controller.cc
index 3375a6e..21a7708 100644
--- a/views/controls/menu/menu_controller.cc
+++ b/views/controls/menu/menu_controller.cc
@@ -91,7 +91,7 @@ static View* GetFirstFocusableView(View* view, int start, bool forward) {
         return deepest;
     }
   }
-  return view->IsFocusable() ? view : NULL;
+  return view->IsFocusableInRootView() ? view : NULL;
 }
 
 // Returns the first child of |start| that is focusable.
diff --git a/views/controls/slider/slider.cc b/views/controls/slider/slider.cc
index 581bc9a..00dba0c 100644
--- a/views/controls/slider/slider.cc
+++ b/views/controls/slider/slider.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -68,10 +68,6 @@ gfx::Size Slider::GetPreferredSize() {
   return gfx::Size();
 }
 
-bool Slider::IsFocusable() const {
-  return IsEnabled();
-}
-
 void Slider::SetEnabled(bool enabled) {
   View::SetEnabled(enabled);
   if (native_wrapper_)
diff --git a/views/controls/slider/slider.h b/views/controls/slider/slider.h
index 531ef05..4850a23 100644
--- a/views/controls/slider/slider.h
+++ b/views/controls/slider/slider.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -69,7 +69,6 @@ class Slider : public View {
   // Overridden from View:
   virtual void Layout();
   virtual gfx::Size GetPreferredSize();
-  virtual bool IsFocusable() const;
   virtual void SetEnabled(bool enabled);
   virtual void PaintFocusBorder(gfx::Canvas* canvas);
 
diff --git a/views/focus/focus_manager.cc b/views/focus/focus_manager.cc
index 733d921..ee70d5b 100644
--- a/views/focus/focus_manager.cc
+++ b/views/focus/focus_manager.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -361,8 +361,8 @@ void FocusManager::RestoreFocusedView() {
 
   View* view = view_storage->RetrieveView(stored_focused_view_storage_id_);
   if (view) {
-    if (ContainsView(view) &&
-        (view->IsFocusable() || view->IsAccessibilityFocusable())) {
+    if (ContainsView(view) && (view->IsFocusableInRootView() ||
+                               view->IsAccessibilityFocusableInRootView())) {
       SetFocusedViewWithReason(view, kReasonFocusRestore);
     }
   } else {
diff --git a/views/focus/focus_search.cc b/views/focus/focus_search.cc
index eaeb8ac..f3fac86 100644
--- a/views/focus/focus_search.cc
+++ b/views/focus/focus_search.cc
@@ -102,9 +102,9 @@ bool FocusSearch::IsViewFocusableCandidate(View* v, int skip_group_id) {
 
 bool FocusSearch::IsFocusable(View* v) {
   if (accessibility_mode_)
-    return v && v->IsAccessibilityFocusable();
-  else
-    return v && v->IsFocusable();
+    return v && v->IsAccessibilityFocusableInRootView();
+
+  return v && v->IsFocusableInRootView();
 }
 
 View* FocusSearch::FindSelectedViewForGroup(View* view) {
diff --git a/views/focus/focus_search.h b/views/focus/focus_search.h
index f24864b..d6479c4 100644
--- a/views/focus/focus_search.h
+++ b/views/focus/focus_search.h
@@ -34,8 +34,8 @@ class FocusSearch {
   //           view to FindNextFocusableView you will always get a valid view
   //           out, even if it's the same view.
   // - |accessibility_mode| should be true if full keyboard accessibility is
-  //   needed and you  want to check IsAccessibilityFocusable(),
-  //   rather than IsFocusable().
+  //   needed and you  want to check IsAccessibilityFocusableInRootView(),
+  //   rather than IsFocusableInRootView().
   FocusSearch(View* root, bool cycle, bool accessibility_mode);
   virtual ~FocusSearch() {}
 
@@ -74,7 +74,8 @@ class FocusSearch {
   bool IsViewFocusableCandidate(View* v, int skip_group_id);
 
   // Convenience method; returns true if a view is not NULL and is focusable
-  // (checking IsAccessibilityFocusable() if accessibility_mode_ is true).
+  // (checking IsAccessibilityFocusableInRootView() if accessibility_mode_ is
+  // true).
   bool IsFocusable(View* v);
 
   // Returns the view selected for the group of the selected view. If the view
diff --git a/views/view.cc b/views/view.cc
index 4ae7bef..2c92338 100644
--- a/views/view.cc
+++ b/views/view.cc
@@ -283,15 +283,15 @@ void View::SetEnabled(bool state) {
   }
 }
 
-bool View::IsFocusable() const {
-  return focusable_ && IsEnabled() && IsVisibleInRootView();
-}
-
 void View::SetFocusable(bool focusable) {
   focusable_ = focusable;
 }
 
-bool View::IsAccessibilityFocusable() const {
+bool View::IsFocusableInRootView() const {
+  return IsFocusable() && IsVisibleInRootView();
+}
+
+bool View::IsAccessibilityFocusableInRootView() const {
   return (focusable_ || accessibility_focusable_) && IsEnabled() &&
       IsVisibleInRootView();
 }
@@ -359,10 +359,8 @@ void View::PaintBorder(gfx::Canvas* canvas) {
 }
 
 void View::PaintFocusBorder(gfx::Canvas* canvas) {
-  if (HasFocus() && (IsFocusable() ||
-                     IsAccessibilityFocusable())) {
+  if (HasFocus() && (IsFocusable() || IsAccessibilityFocusableInRootView()))
     canvas->DrawFocusRect(0, 0, width(), height());
-  }
 }
 
 void View::PaintChildren(gfx::Canvas* canvas) {
@@ -665,6 +663,10 @@ void View::PropagateAddNotifications(View* parent, View* child) {
   ViewHierarchyChangedImpl(true, true, parent, child);
 }
 
+bool View::IsFocusable() const {
+  return focusable_ && IsEnabled() && IsVisible();
+}
+
 void View::ThemeChanged() {
   for (int i = GetChildViewCount() - 1; i >= 0; --i)
     GetChildViewAt(i)->ThemeChanged();
@@ -1319,7 +1321,7 @@ bool View::IsVisibleInRootView() const {
 
 void View::RequestFocus() {
   RootView* rv = GetRootView();
-  if (rv && IsFocusable())
+  if (rv && IsFocusableInRootView())
     rv->FocusView(this);
 }
 
diff --git a/views/view.h b/views/view.h
index d06d2fa6..5a72863 100644
--- a/views/view.h
+++ b/views/view.h
@@ -506,17 +506,18 @@ class View : public AcceleratorTarget {
   // IMPORTANT NOTE: loops in the focus hierarchy are not supported.
   void SetNextFocusableView(View* view);
 
-  // Return whether this view can accept the focus.
-  virtual bool IsFocusable() const;
-
   // Sets whether this view can accept the focus.
   // Note that this is false by default so that a view used as a container does
   // not get the focus.
   virtual void SetFocusable(bool focusable);
 
-  // Return whether this view is focusable when the user requires
-  // full keyboard access, even though it may not be normally focusable.
-  virtual bool IsAccessibilityFocusable() const;
+  // Returns true if the view is focusable (IsFocusable) and visible in the root
+  // view. See also IsFocusable.
+  bool IsFocusableInRootView() const;
+
+  // Return whether this view is focusable when the user requires full keyboard
+  // access, even though it may not be normally focusable.
+  bool IsAccessibilityFocusableInRootView() const;
 
   // Set whether this view can be made focusable if the user requires
   // full keyboard access, even though it's not normally focusable.
@@ -966,13 +967,11 @@ class View : public AcceleratorTarget {
   ThemeProvider* GetThemeProvider() const;
 
  protected:
-  // The id of this View. Used to find this View.
-  int id_;
-
-  // The group of this view. Some view subclasses use this id to find other
-  // views of the same group. For example radio button uses this information
-  // to find other radio buttons.
-  int group_;
+  // Returns whether this view can accept focus.
+  // A view can accept focus if it's enabled, focusable and visible.
+  // This method is intended for views to use when calculating preferred size.
+  // The FocusManager and other places use IsFocusableInRootView.
+  virtual bool IsFocusable() const;
 
   // Called when the UI theme has changed, overriding allows individual Views to
   // do special cleanup and processing (such as dropping resource caches).
@@ -1096,6 +1095,14 @@ class View : public AcceleratorTarget {
   static int GetHorizontalDragThreshold();
   static int GetVerticalDragThreshold();
 
+  // The id of this View. Used to find this View.
+  int id_;
+
+  // The group of this view. Some view subclasses use this id to find other
+  // views of the same group. For example radio button uses this information
+  // to find other radio buttons.
+  int group_;
+
   // Whether this view is enabled.
   bool enabled_;