Give up focus if the focused view becomes unfocusable

ui/views/view_unittest.cc

Index: ui/views/view_unittest.cc
diff --git a/ui/views/view_unittest.cc b/ui/views/view_unittest.cc
index 544e9997450b7643b940e0d388a0c42600f241ec..b2712487278df19f61d22aa0e584253b49977a7b 100644
--- a/ui/views/view_unittest.cc
+++ b/ui/views/view_unittest.cc
@@ -2642,6 +2642,55 @@ TEST_F(ViewTest, AddExistingChild) {
 }
 
 ////////////////////////////////////////////////////////////////////////////////
+// FocusManager
+////////////////////////////////////////////////////////////////////////////////
+
+TEST_F(ViewTest, ReviseFocusedViewForUnfocusableView) {
+  // Create a widget with two views and give the first one focus.
+  Widget widget;
+  Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP);
+  params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  widget.Init(params);
+  widget.Activate();

[sky, 2014/08/07 21:40:34]

I suspect doing this is going to require the test be in interactive ui tests.
Maybe you could create a test with a fake NativeWidgetDelegate? I believe there
are tests that create a fake NativeWidgetDelegate already.

[mohsen, 2014/08/08 01:32:07]

This test is not meant to test activation. It is only testing if focus is
advanced properly inside a widget. So, it is not really needed to be an
interactive ui test. The only activation requirement for this test is that
widget is always active. For that, I've subclasses a new widget class that
always returns true in its IsActive() function. Please check if it's fine.

[sky, 2014/08/08 14:52:30]

You're new code triggers calling off to NativeWidgetDelegate::IsActive() below.
I'm worried about the effects of this with other tests running at the same time.
I think it best to mock out NativeWidgetDelegate.

[mohsen, 2014/08/08 17:45:07]

I guess by NativeWidgetDelegate you mean NativeWidgetPrivate. Right?

If yes, its mock implementation needs a lot of pure virtual function overrides
and yet I'm not sure if that would work as some of them might need proper
implementation (not just an empty implementation). The problem as I understand
is that we need Widget::IsActive() return true for the purpose of this test.
Isn't my above ActiveWidget enough for that matter? (Or a similar subclass of
NativeWidgetAura that overrides IsActive()?)

By the way, I believe this test should work properly even without providing any
mock IsActive() implementation. Normally, in views tests, NativeWidgetAura would
be used as the native widget implementation for widgets (rather than
DesktopNativeWidgetAura, even on desktop aura) and as far as I understand
system-level activation and parallel tests will not interfere with
NativeWidgetAura's notion of activation. Is that right?

+
+  View* view1 = new View();
+  view1->SetFocusable(true);
+  widget.GetRootView()->AddChildView(view1);
+  View* view2 = new View();
+  view2->SetFocusable(true);
+  widget.GetRootView()->AddChildView(view2);
+
+  FocusManager* focus_manager = widget.GetFocusManager();
+  ASSERT_TRUE(focus_manager);
+
+  focus_manager->SetFocusedView(view1);
+  EXPECT_EQ(view1, focus_manager->GetFocusedView());
+
+  // Disable the focused view and check if the next view gets focused.
+  view1->SetEnabled(false);
+  EXPECT_EQ(view2, focus_manager->GetFocusedView());
+
+  // Re-enable and re-focus.
+  view1->SetEnabled(true);
+  focus_manager->SetFocusedView(view1);
+  EXPECT_EQ(view1, focus_manager->GetFocusedView());
+
+  // Hide the focused view and check it the next view gets focused.
+  view1->SetVisible(false);
+  EXPECT_EQ(view2, focus_manager->GetFocusedView());
+
+  // Re-show and re-focus.
+  view1->SetVisible(true);
+  focus_manager->SetFocusedView(view1);
+  EXPECT_EQ(view1, focus_manager->GetFocusedView());
+
+  // Set the focused view as not focusable and check if the next view gets
+  // focused.
+  view1->SetFocusable(false);
+  EXPECT_EQ(view2, focus_manager->GetFocusedView());
+}
+
+////////////////////////////////////////////////////////////////////////////////
 // Layers
 ////////////////////////////////////////////////////////////////////////////////