Port more focus controller unit tests and fix our focus rules.

mojo/services/window_manager/basic_focus_rules.cc

 // Copyright 2014 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.
 
 #include "mojo/services/window_manager/basic_focus_rules.h"
 
 #include "base/macros.h"
 #include "mojo/services/public/cpp/view_manager/view.h"
 
 namespace mojo {
 
+namespace {
+
+// TODO(erg): aura::Window::CanFocus() exists. mojo::View::CanFocus() does
+// not. This is a hack that does everything that Window::CanFocus() currently
+// does that doesn't require a delegate or an EventClient.
+bool ViewCanFocus(View* view) {
+  if (view->GetRoot() == view)

[sky, 2014/11/21 22:42:19]

I don't get this logic. Isn't the same as:

return view->GetRoot()->visible()?

Which doesn't seem right either. Maybe you want view->IsDrawn()? (I see a bug in
that implementation, I'll fix it shortly).

[Elliot Glaysher, 2014/11/24 20:29:53]

Correct, this is equivalent.
So the aura version is this:

bool Window::CanFocus() const {
  if (IsRootWindow())
    return IsVisible();

  // A bunch of checks we can't implement.

  return parent_->CanFocus();
}

IsDrawn() is probably not what we want. It appears to have the additional
restriction that the current view has to be visible, which isn't present in the
aura version which only checks if the root is visible. More importantly,
IsDrawn() appears to always be false in unit tests because we aren't actually
connected to a view manager.

As for why I wrote it this way, it was to emphasize that there's a check on each
view as it goes up the hierarchy. I've added a comment here explaining that.

+    return view->visible();
+
+  return ViewCanFocus(view->parent());
+}
+
+}  // namespace
+
 BasicFocusRules::BasicFocusRules(mojo::View* window_container)
     : window_container_(window_container) {
 }
 
 BasicFocusRules::~BasicFocusRules() {}
 
+bool BasicFocusRules::SupportsChildActivation(mojo::View* view) const {
+  return true;
+}
+
 bool BasicFocusRules::IsToplevelView(mojo::View* view) const {
-  return view->parent() == window_container_;
+  // The window must be in a valid hierarchy.
+  if (!view->GetRoot())

[sky, 2014/11/21 22:42:19]

I don't think this is the check you're after. GetRoot() always returns non-null.
I think you want the old logic, possibly with a fixed IsDrawn().

[Elliot Glaysher, 2014/11/24 20:29:53]

Made the old logic work on unit tests.

+    return false;
+
+  // The window must exist within a container that supports activation.
+  // The window cannot be blocked by a modal transient.
+  return SupportsChildActivation(view->parent());
 }
 
 bool BasicFocusRules::CanActivateView(mojo::View* view) const {
-  // TODO(erg): This needs to check visibility, along with focus, and several
-  // other things (see wm::BaseFocusRules).
-  return view->parent() == window_container_;
+  if (!view)
+    return true;
+
+  // Only toplevel windows can be activated
+  if (!IsToplevelView(view))
+    return false;
+
+  // The view must be visible.
+  if (!view->visible())
+    return false;
+
+  // TODO(erg): The aura version of this class asks the aura::Window's
+  // ActivationDelegate whether the window is activatable.
+
+  // A window must be focusable to be activatable. We don't call
+  // CanFocusWindow() from here because it will call back to us via
+  // GetActivatableWindow().
+  if (!ViewCanFocus(view))
+    return false;
+
+  // TODO(erg): In the aura version, we also check whether the window is
+  // blocked by a modal transient window.
+
+  return true;
 }
 
 bool BasicFocusRules::CanFocusView(mojo::View* view) const {
-  return true;
+  // It is possible to focus a NULL window, it is equivalent to clearing focus.
+  if (!view)
+    return true;
+
+  // The focused view is always inside the active view, so views that aren't
+  // activatable can't contain the focused view.
+  View* activatable = GetActivatableView(view);
+  if (!activatable || !activatable->Contains(view))
+    return false;
+  return ViewCanFocus(view);
 }
 
 mojo::View* BasicFocusRules::GetToplevelView(mojo::View* view) const {
-  while (view->parent() != window_container_) {
-    view = view->parent();
-    // Unparented hierarchy, there is no "top level" window.
-    if (!view)
+  View* parent = view->parent();
+  View* child = view;
+  while (parent) {
+    if (IsToplevelView(child))
+      return child;
+
+    parent = parent->parent();
+    child = child->parent();
+  }
+
+  return nullptr;
+}
+
+mojo::View* BasicFocusRules::GetActivatableView(mojo::View* view) const {
+  View* parent = view->parent();
+  View* child = view;
+  while (parent) {
+    if (CanActivateView(child))
+      return child;
+
+    // TODO(erg): In the aura version of this class, we have a whole bunch of
+    // checks to support modal transient windows, and transient parents.
+
+    parent = parent->parent();
+    child = child->parent();
+  }
+
+  return nullptr;
+}
+
+mojo::View* BasicFocusRules::GetFocusableView(mojo::View* view) const {
+  if (CanFocusView(view))
+    return view;
+
+  // |view| may be in a hierarchy that is non-activatable, in which case we
+  // need to cut over to the activatable hierarchy.
+  View* activatable = GetActivatableView(view);
+  if (!activatable) {
+    // There may not be a related activatable hierarchy to cut over to, in which
+    // case we try an unrelated one.
+    View* toplevel = GetToplevelView(view);
+    if (toplevel)
+      activatable = GetNextActivatableView(toplevel);
+    if (!activatable)
       return nullptr;
   }
 
-  return view;
-}
+  if (!activatable->Contains(view)) {
+    // If there's already a child window focused in the activatable hierarchy,
+    // just use that (i.e. don't shift focus), otherwise we need to at least cut
+    // over to the activatable hierarchy.
+    View* focused = GetFocusableView(activatable);
+    return activatable->Contains(focused) ? focused : activatable;
+  }
 
-mojo::View* BasicFocusRules::GetActivatableView(mojo::View* view) const {
-  return GetToplevelView(view);
-}
-
-mojo::View* BasicFocusRules::GetFocusableView(mojo::View* view) const {
+  while (view && !CanFocusView(view))
+    view = view->parent();
   return view;
 }
 
 mojo::View* BasicFocusRules::GetNextActivatableView(
     mojo::View* activatable) const {
-  const mojo::View::Children& children = activatable->parent()->children();
-  for (mojo::View::Children::const_reverse_iterator it = children.rbegin();
-       it != children.rend(); ++it) {
-    if (*it != activatable)
-      return *it;
+  DCHECK(activatable);
+
+  // In the basic scenarios handled by BasicFocusRules, the pool of activatable
+  // windows is limited to the |ignore|'s siblings.
+  const View::Children& siblings = activatable->parent()->children();
+  DCHECK(!siblings.empty());
+
+  for (auto rit = siblings.rbegin(); rit != siblings.rend(); ++rit) {
+    View* cur = *rit;
+    if (cur == activatable)
+      continue;
+    if (CanActivateView(cur))
+      return cur;
   }
   return nullptr;
 }
 
 }  // namespace mojo