Aura: Fix activated windows sometimes not switching to foreground

This was caused by a mismatch in window and layer stacking.  We use a NULL layer delegate as a signal that a window is hidden or closed and is animating out.  The bottom-of-window status bubble frequently shows and hides, and hence is commonly in this state.  If you tried to activate a background window while the status bubble was animating out the window would activate and start processing events, but its layer would not be brought to front, so it would visually appear behind other windows.  The problem was probably introduced in either http://crrev.com/119453 (which introduced stacking changes for NULL layer delegate windows) or http://crrev.com/119753 (which fixed a similar issue with transient child windows).  This patch ensures that window stacking order always matches layer stacking order and adds unit tests for this situation.

BUG=112562
TEST=aura_unittests WindowTest.Stack*

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

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

3 files changed, 221 insertions, 48 deletions

diff --git a/ui/aura/window.cc b/ui/aura/window.cc
index 765682c..8dbd6c5 100644
--- a/ui/aura/window.cc
+++ b/ui/aura/window.cc
@@ -240,43 +240,36 @@ void Window::StackChildAtTop(Window* child) {
   StackChildAbove(child, children_.back());
 }
 
-void Window::StackChildAbove(Window* child, Window* other) {
-  DCHECK_NE(child, other);
+void Window::StackChildAbove(Window* child, Window* target) {
+  DCHECK_NE(child, target);
   DCHECK(child);
-  DCHECK(other);
+  DCHECK(target);
   DCHECK_EQ(this, child->parent());
-  DCHECK_EQ(this, other->parent());
-
-  const size_t child_i =
-      std::find(children_.begin(), children_.end(), child) - children_.begin();
-  const size_t other_i =
-      std::find(children_.begin(), children_.end(), other) - children_.begin();
-  if (child_i == other_i + 1)
+  DCHECK_EQ(this, target->parent());
+
+  const size_t target_i =
+      std::find(children_.begin(), children_.end(), target) - children_.begin();
+
+  // By convention we don't stack on top of windows with layers with NULL
+  // delegates.  Walk backward to find a valid target window.
+  // See tests WindowTest.StackingMadrigal and StackOverClosingTransient
+  // for an explanation of this.
+  size_t final_target_i = target_i;
+  while (final_target_i > 0 &&
+         children_[final_target_i]->layer()->delegate() == NULL)
+    --final_target_i;
+  Window* final_target = children_[final_target_i];
+
+  // If we couldn't find a valid target position, don't move anything.
+  if (final_target->layer()->delegate() == NULL)
     return;
 
-  const size_t dest_i = child_i < other_i ? other_i : other_i + 1;
-  children_.erase(children_.begin() + child_i);
-  children_.insert(children_.begin() + dest_i, child);
-
-  // See test WindowTest.StackingMadrigal for an explanation of this and the
-  // check below in the transient loop.
-  if (other->layer()->delegate())
-    layer()->StackAbove(child->layer(), other->layer());
-
-  // Stack any transient children that share the same parent to be in front of
-  // 'child'.
-  Window* last_transient = child;
-  for (Windows::iterator i = child->transient_children_.begin();
-       i != child->transient_children_.end(); ++i) {
-    Window* transient_child = *i;
-    if (transient_child->parent_ == this) {
-      StackChildAbove(transient_child, last_transient);
-      if (transient_child->layer()->delegate())
-        last_transient = transient_child;
-    }
-  }
+  // Don't try to stack a child above itself.
+  if (child == final_target)
+    return;
 
-  child->OnStackingChanged();
+  // Move the child and all its transients.
+  StackChildAboveImpl(child, final_target);
 }
 
 void Window::AddChild(Window* child) {
@@ -524,6 +517,9 @@ bool Window::StopsEventPropagation() const {
   return it != children_.end();
 }
 
+///////////////////////////////////////////////////////////////////////////////
+// Window, private:
+
 void Window::SetBoundsInternal(const gfx::Rect& new_bounds) {
   gfx::Rect actual_new_bounds(new_bounds);
 
@@ -638,6 +634,43 @@ void Window::OnParentChanged() {
       WindowObserver, observers_, OnWindowParentChanged(this, parent_));
 }
 
+void Window::StackChildAboveImpl(Window* child, Window* target) {
+  DCHECK_NE(child, target);
+  DCHECK(child);
+  DCHECK(target);
+  DCHECK_EQ(this, child->parent());
+  DCHECK_EQ(this, target->parent());
+
+  const size_t child_i =
+      std::find(children_.begin(), children_.end(), child) - children_.begin();
+  const size_t target_i =
+      std::find(children_.begin(), children_.end(), target) - children_.begin();
+
+  // Don't move the child if it is already in the right place.
+  if (child_i == target_i + 1)
+    return;
+
+  const size_t dest_i = child_i < target_i ? target_i : target_i + 1;
+  children_.erase(children_.begin() + child_i);
+  children_.insert(children_.begin() + dest_i, child);
+
+  layer()->StackAbove(child->layer(), target->layer());
+
+  // Stack any transient children that share the same parent to be in front of
+  // 'child'.
+  Window* last_transient = child;
+  for (Windows::iterator it = child->transient_children_.begin();
+       it != child->transient_children_.end(); ++it) {
+    Window* transient_child = *it;
+    if (transient_child->parent_ == this) {
+      StackChildAboveImpl(transient_child, last_transient);
+      last_transient = transient_child;
+    }
+  }
+
+  child->OnStackingChanged();
+}
+
 void Window::OnStackingChanged() {
   FOR_EACH_OBSERVER(WindowObserver, observers_, OnWindowStackingChanged(this));
 }
diff --git a/ui/aura/window.h b/ui/aura/window.h
index c1b49f1..62619dd 100644
--- a/ui/aura/window.h
+++ b/ui/aura/window.h
@@ -123,9 +123,6 @@ class AURA_EXPORT Window : public ui::LayerDelegate {
   // Returns the window's bounds in screen coordinates.
   gfx::Rect GetScreenBounds() const;
 
-  // Returns true if this window is active.
-  bool IsActive() const;
-
   virtual void SetTransform(const ui::Transform& transform);
 
   // Assigns a LayoutManager to size and place child windows.
@@ -154,9 +151,10 @@ class AURA_EXPORT Window : public ui::LayerDelegate {
   // Stacks the specified child of this Window at the front of the z-order.
   void StackChildAtTop(Window* child);
 
-  // Stacks |child| above |other|.  Does nothing if |child| is already above
-  // |other|.
-  void StackChildAbove(Window* child, Window* other);
+  // Stacks |child| above |target|.  Does nothing if |child| is already above
+  // |target|.  Does not stack on top of windows with NULL layer delegates,
+  // see WindowTest.StackingMadrigal for details.
+  void StackChildAbove(Window* child, Window* target);
 
   // Tree operations.
   // TODO(beng): Child windows are currently not owned by the hierarchy. We
@@ -175,7 +173,7 @@ class AURA_EXPORT Window : public ui::LayerDelegate {
   //   destroyed. This means a transient child is destroyed if either its parent
   //   or transient parent is destroyed.
   // . If a transient child and its transient parent share the same parent, then
-  //   transient children are always ordered above the trasient parent.
+  //   transient children are always ordered above the transient parent.
   // Transient windows are typically used for popups and menus.
   void AddTransientChild(Window* child);
   void RemoveTransientChild(Window* child);
@@ -319,6 +317,9 @@ class AURA_EXPORT Window : public ui::LayerDelegate {
   // Called when this window's parent has changed.
   void OnParentChanged();
 
+  // Stacks |child| above |target| without checking for NULL layer delegates.
+  void StackChildAboveImpl(Window* child, Window* target);
+
   // Called when this window's stacking order among its siblings is changed.
   void OnStackingChanged();
 
diff --git a/ui/aura/window_unittest.cc b/ui/aura/window_unittest.cc
index dbf8367..24ded2e 100644
--- a/ui/aura/window_unittest.cc
+++ b/ui/aura/window_unittest.cc
@@ -1220,7 +1220,7 @@ TEST_F(WindowTest, AcquireLayer) {
   EXPECT_EQ(1U, parent->children().size());
 }
 
-TEST_F(WindowTest, DontRestackWindowsWhoseLayersHaveNoDelegate) {
+TEST_F(WindowTest, StackWindowsWhoseLayersHaveNoDelegate) {
   scoped_ptr<Window> window1(CreateTestWindowWithId(1, NULL));
   scoped_ptr<Window> window2(CreateTestWindowWithId(2, NULL));
 
@@ -1233,17 +1233,46 @@ TEST_F(WindowTest, DontRestackWindowsWhoseLayersHaveNoDelegate) {
   EXPECT_EQ(RootWindow::GetInstance()->layer()->children().back(),
             window1->layer());
 
-  // This brings window2 (but NOT its layer) to the front.
+  // Since window1 does not have a delegate, window2 should not move in
+  // front of it, nor should its layer.
   window1->layer()->set_delegate(NULL);
   RootWindow::GetInstance()->StackChildAbove(window2.get(), window1.get());
-  EXPECT_EQ(RootWindow::GetInstance()->children().front(), window1.get());
-  EXPECT_EQ(RootWindow::GetInstance()->children().back(), window2.get());
+  EXPECT_EQ(RootWindow::GetInstance()->children().front(), window2.get());
+  EXPECT_EQ(RootWindow::GetInstance()->children().back(), window1.get());
   EXPECT_EQ(RootWindow::GetInstance()->layer()->children().front(),
             window2->layer());
   EXPECT_EQ(RootWindow::GetInstance()->layer()->children().back(),
             window1->layer());
 }
 
+TEST_F(WindowTest, StackTransientsWhoseLayersHaveNoDelegate) {
+  // Create a window with several transients, then a couple windows on top.
+  scoped_ptr<Window> window1(CreateTestWindowWithId(1, NULL));
+  scoped_ptr<Window> window11(CreateTransientChild(11, window1.get()));
+  scoped_ptr<Window> window12(CreateTransientChild(12, window1.get()));
+  scoped_ptr<Window> window13(CreateTransientChild(13, window1.get()));
+  scoped_ptr<Window> window2(CreateTestWindowWithId(2, NULL));
+  scoped_ptr<Window> window3(CreateTestWindowWithId(3, NULL));
+
+  // Remove the delegates of a couple of transients, as if they are closing
+  // and animating out.
+  window11->layer()->set_delegate(NULL);
+  window13->layer()->set_delegate(NULL);
+
+  // Move window1 to the front.  All transients should move with it, and their
+  // order should be preserved.
+  RootWindow* root = RootWindow::GetInstance();
+  root->StackChildAtTop(window1.get());
+
+  ASSERT_EQ(6u, root->children().size());
+  EXPECT_EQ(window2.get(), root->children()[0]);
+  EXPECT_EQ(window3.get(), root->children()[1]);
+  EXPECT_EQ(window1.get(), root->children()[2]);
+  EXPECT_EQ(window11.get(), root->children()[3]);
+  EXPECT_EQ(window12.get(), root->children()[4]);
+  EXPECT_EQ(window13.get(), root->children()[5]);
+}
+
 class TestVisibilityClient : public client::VisibilityClient {
 public:
   TestVisibilityClient() : ignore_visibility_changes_(false) {
@@ -1439,7 +1468,7 @@ class StackingMadrigalVisibilityClient : public client::VisibilityClient {
 // This test attempts to reconstruct a circumstance that can happen when the
 // aura client attempts to manipulate the visibility and delegate of a layer
 // independent of window visibility.
-// A use case is where the client attempts to keep a window's visible onscreen
+// A use case is where the client attempts to keep a window visible onscreen
 // even after code has called Hide() on the window. The use case for this would
 // be that window hides are animated (e.g. the window fades out). To prevent
 // spurious updating the client code may also clear window's layer's delegate,
@@ -1469,14 +1498,16 @@ TEST_F(WindowTest, StackingMadrigal) {
   EXPECT_TRUE(WindowIsAbove(window11.get(), window1.get()));
   EXPECT_TRUE(LayerIsAbove(window11.get(), window1.get()));
 
+  // A new transient should still be above window1.  It will appear behind
+  // window11 because we don't stack windows on top of targets with NULL
+  // delegates.
   scoped_ptr<Window> window12(CreateTransientChild(12, window1.get()));
   window12->Show();
 
-  EXPECT_TRUE(WindowIsAbove(window12.get(), window11.get()));
-  EXPECT_TRUE(LayerIsAbove(window12.get(), window11.get()));
+  EXPECT_TRUE(WindowIsAbove(window12.get(), window1.get()));
+  EXPECT_TRUE(LayerIsAbove(window12.get(), window1.get()));
 
-  // Prior to the NULL check in the transient restacking loop in
-  // Window::StackChildAbove() introduced with this change, attempting to stack
+  // In earlier versions of the StackChildAbove() method, attempting to stack
   // window1 above window12 at this point would actually restack the layers
   // resulting in window12's layer being below window1's layer (though the
   // windows themselves would still be correctly stacked, so events would pass
@@ -1488,6 +1519,114 @@ TEST_F(WindowTest, StackingMadrigal) {
   EXPECT_TRUE(LayerIsAbove(window12.get(), window1.get()));
 }
 
+// Test for an issue where attempting to stack a primary window on top of a
+// transient with a NULL layer delegate causes that primary window to be moved,
+// but the layer order not changed to match.  http://crbug.com/112562
+TEST_F(WindowTest, StackOverClosingTransient) {
+  scoped_ptr<Window> window1(CreateTestWindowWithId(1, NULL));
+  scoped_ptr<Window> transient1(CreateTransientChild(11, window1.get()));
+  scoped_ptr<Window> window2(CreateTestWindowWithId(2, NULL));
+  scoped_ptr<Window> transient2(CreateTransientChild(21, window2.get()));
+
+  // Both windows and layers are stacked in creation order.
+  RootWindow* root = RootWindow::GetInstance();
+  ASSERT_EQ(4u, root->children().size());
+  EXPECT_EQ(root->children()[0], window1.get());
+  EXPECT_EQ(root->children()[1], transient1.get());
+  EXPECT_EQ(root->children()[2], window2.get());
+  EXPECT_EQ(root->children()[3], transient2.get());
+  ASSERT_EQ(4u, root->layer()->children().size());
+  EXPECT_EQ(root->layer()->children()[0], window1->layer());
+  EXPECT_EQ(root->layer()->children()[1], transient1->layer());
+  EXPECT_EQ(root->layer()->children()[2], window2->layer());
+  EXPECT_EQ(root->layer()->children()[3], transient2->layer());
+
+  // This brings window1 and its transient to the front.
+  //root_window->StackChildAbove(window1.get(), window2.get());
+  root->StackChildAtTop(window1.get());
+
+  EXPECT_EQ(root->children()[0], window2.get());
+  EXPECT_EQ(root->children()[1], transient2.get());
+  EXPECT_EQ(root->children()[2], window1.get());
+  EXPECT_EQ(root->children()[3], transient1.get());
+  EXPECT_EQ(root->layer()->children()[0], window2->layer());
+  EXPECT_EQ(root->layer()->children()[1], transient2->layer());
+  EXPECT_EQ(root->layer()->children()[2], window1->layer());
+  EXPECT_EQ(root->layer()->children()[3], transient1->layer());
+
+  // Pretend we're closing the top-most transient, then bring window2 to the
+  // front.  This mimics activating a browser window while the status bubble
+  // is fading out.  The transient should stay topmost.
+  transient1->layer()->set_delegate(NULL);
+  root->StackChildAtTop(window2.get());
+
+  EXPECT_EQ(root->children()[0], window1.get());
+  EXPECT_EQ(root->children()[1], window2.get());
+  EXPECT_EQ(root->children()[2], transient2.get());
+  EXPECT_EQ(root->children()[3], transient1.get());
+  EXPECT_EQ(root->layer()->children()[0], window1->layer());
+  EXPECT_EQ(root->layer()->children()[1], window2->layer());
+  EXPECT_EQ(root->layer()->children()[2], transient2->layer());
+  EXPECT_EQ(root->layer()->children()[3], transient1->layer());
+
+  // Close the transient.  Remaining windows are stable.
+  transient1.reset();
+
+  ASSERT_EQ(3u, root->children().size());
+  EXPECT_EQ(root->children()[0], window1.get());
+  EXPECT_EQ(root->children()[1], window2.get());
+  EXPECT_EQ(root->children()[2], transient2.get());
+  ASSERT_EQ(3u, root->layer()->children().size());
+  EXPECT_EQ(root->layer()->children()[0], window1->layer());
+  EXPECT_EQ(root->layer()->children()[1], window2->layer());
+  EXPECT_EQ(root->layer()->children()[2], transient2->layer());
+
+  // Open another window on top.
+  scoped_ptr<Window> window3(CreateTestWindowWithId(3, NULL));
+
+  ASSERT_EQ(4u, root->children().size());
+  EXPECT_EQ(root->children()[0], window1.get());
+  EXPECT_EQ(root->children()[1], window2.get());
+  EXPECT_EQ(root->children()[2], transient2.get());
+  EXPECT_EQ(root->children()[3], window3.get());
+  ASSERT_EQ(4u, root->layer()->children().size());
+  EXPECT_EQ(root->layer()->children()[0], window1->layer());
+  EXPECT_EQ(root->layer()->children()[1], window2->layer());
+  EXPECT_EQ(root->layer()->children()[2], transient2->layer());
+  EXPECT_EQ(root->layer()->children()[3], window3->layer());
+
+  // Pretend we're closing the topmost non-transient window, then bring
+  // window2 to the top.  It should not move.
+  window3->layer()->set_delegate(NULL);
+  root->StackChildAtTop(window2.get());
+
+  ASSERT_EQ(4u, root->children().size());
+  EXPECT_EQ(root->children()[0], window1.get());
+  EXPECT_EQ(root->children()[1], window2.get());
+  EXPECT_EQ(root->children()[2], transient2.get());
+  EXPECT_EQ(root->children()[3], window3.get());
+  ASSERT_EQ(4u, root->layer()->children().size());
+  EXPECT_EQ(root->layer()->children()[0], window1->layer());
+  EXPECT_EQ(root->layer()->children()[1], window2->layer());
+  EXPECT_EQ(root->layer()->children()[2], transient2->layer());
+  EXPECT_EQ(root->layer()->children()[3], window3->layer());
+
+  // Bring window1 to the top.  It should move ahead of window2, but not
+  // ahead of window3 (with NULL delegate).
+  root->StackChildAtTop(window1.get());
+
+  ASSERT_EQ(4u, root->children().size());
+  EXPECT_EQ(root->children()[0], window2.get());
+  EXPECT_EQ(root->children()[1], transient2.get());
+  EXPECT_EQ(root->children()[2], window1.get());
+  EXPECT_EQ(root->children()[3], window3.get());
+  ASSERT_EQ(4u, root->layer()->children().size());
+  EXPECT_EQ(root->layer()->children()[0], window2->layer());
+  EXPECT_EQ(root->layer()->children()[1], transient2->layer());
+  EXPECT_EQ(root->layer()->children()[2], window1->layer());
+  EXPECT_EQ(root->layer()->children()[3], window3->layer());
+}
+
 class RootWindowAttachmentObserver : public WindowObserver {
  public:
   RootWindowAttachmentObserver() : added_count_(0), removed_count_(0) {}