Don't reset starting layout if no progress has been made.

Resetting the starting layout loses context about views that are fading
in our out, causing popping of layouts in some cases (especially during
slide animations).

Having a layout reset multiple times can happen when one view is hidden
and another view shown within the same host view with an animating
layout. Ensuring that only the final change "sticks" makes for a smooth
transition from the pre-modification state.

Bug: 1037947
Change-Id: Ibc1dbb2f249d1edfd21294202467d2292d0591a7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1981333
Commit-Queue: Dana Fried <dfried@chromium.org>
Reviewed-by: Taylor Bergquist <tbergquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#727514}

ui/views/layout/animating_layout_manager.cc

@@ -491,9 +491,9 @@ bool AnimatingLayoutManager::RecalculateTarget() {
   if (target_layout_ == proposed_layout)
     return false;
 
-  starting_layout_ = current_layout_;
   target_layout_ = proposed_layout;
   if (current_offset_ > kResetAnimationThreshold) {
+    starting_layout_ = current_layout_;
     starting_offset_ = 0.0;
     current_offset_ = 0.0;
     animation_delegate_->Animate();
@@ -501,7 +501,12 @@ bool AnimatingLayoutManager::RecalculateTarget() {
       is_animating_ = true;
       NotifyIsAnimatingChanged();
     }
-  } else {
+  } else if (current_offset_ > starting_offset_) {
+    // Only update the starting layout if the animation has progressed. This has
+    // the effect of "batching up" changes that all happen on the same frame,
+    // keeping the same starting point. (A common example of this is multiple
+    // child views' visibility changing.)
+    starting_layout_ = current_layout_;
     starting_offset_ = current_offset_;
   }
   CalculateFadeInfos();

ui/views/layout/animating_layout_manager_unittest.cc

@@ -186,16 +186,17 @@ class AnimatingLayoutManagerTest : public testing::Test {
         view_->AddChildViewAt(std::make_unique<TestView>(), index);
   }
 
-  void EnsureLayout(const ProposedLayout& expected) {
+  void EnsureLayout(const ProposedLayout& expected, const char* message = "") {
     for (size_t i = 0; i < expected.child_layouts.size(); ++i) {
       const auto& expected_child = expected.child_layouts[i];
       const View* const child = expected_child.child_view;
-      EXPECT_EQ(view_, child->parent()) << " view " << i << " parent differs.";
+      EXPECT_EQ(view_, child->parent())
+          << " view " << i << " parent differs " << message;
       EXPECT_EQ(expected_child.visible, child->GetVisible())
-          << " view " << i << " visibility.";
+          << " view " << i << " visibility " << message;
       if (expected_child.visible) {
         EXPECT_EQ(expected_child.bounds, child->bounds())
-            << " view " << i << " bounds";
+            << " view " << i << " bounds " << message;
       }
     }
   }
@@ -1497,6 +1498,72 @@ TEST_F(AnimatingLayoutManagerTest, FlexLayout_RemoveShowingViewDoesNotCrash) {
   EXPECT_FALSE(layout()->is_animating());
 }
 
+// Regression test for crbug.com/1037947
+TEST_F(AnimatingLayoutManagerTest, FlexLayout_DoubleSlide) {
+  layout()->SetShouldAnimateBounds(true);
+  layout()->SetOrientation(LayoutOrientation::kHorizontal);
+  layout()->SetDefaultFadeMode(
+      AnimatingLayoutManager::FadeInOutMode::kSlideFromTrailingEdge);
+  child(1)->SetVisible(false);
+  auto* const flex_layout =
+      layout()->SetTargetLayoutManager(std::make_unique<FlexLayout>());
+  flex_layout->SetOrientation(LayoutOrientation::kHorizontal);
+  flex_layout->SetCrossAxisAlignment(LayoutAlignment::kCenter);
+
+  layout()->ResetLayout();
+  SizeAndLayout();
+
+  const ProposedLayout expected_start{
+      {20, 10},
+      {{child(0), true, {{0, 0}, kChildViewSize}},
+       {child(1), false},
+       {child(2), true, {{10, 0}, kChildViewSize}}}};
+  EnsureLayout(expected_start, "before visibility changes");
+
+  child(0)->SetVisible(false);
+  child(1)->SetVisible(true);
+
+  EXPECT_TRUE(layout()->is_animating());
+  animation_api()->IncrementTime(base::TimeDelta::FromMilliseconds(500));
+  view()->Layout();
+  EXPECT_TRUE(layout()->is_animating());
+
+  const ProposedLayout expected_middle{
+      {20, 10},
+      {{child(0), true, {{5, 0}, kChildViewSize}},
+       {child(1), true, {{5, 0}, kChildViewSize}},
+       {child(2), true, {{10, 0}, kChildViewSize}}}};
+  EnsureLayout(expected_middle, "during first slide");
+
+  // Complete the layout.
+  animation_api()->IncrementTime(base::TimeDelta::FromMilliseconds(500));
+  view()->Layout();
+
+  EXPECT_FALSE(layout()->is_animating());
+  const ProposedLayout expected_end{
+      {20, 10},
+      {{child(0), false},
+       {child(1), true, {{0, 0}, kChildViewSize}},
+       {child(2), true, {{10, 0}, kChildViewSize}}}};
+  EnsureLayout(expected_end, "after first slide");
+
+  // Reverse the layout.
+  child(0)->SetVisible(true);
+  child(1)->SetVisible(false);
+
+  EXPECT_TRUE(layout()->is_animating());
+  animation_api()->IncrementTime(base::TimeDelta::FromMilliseconds(500));
+  view()->Layout();
+  EXPECT_TRUE(layout()->is_animating());
+  EnsureLayout(expected_middle, "during second slide");
+
+  // Complete the layout.
+  animation_api()->IncrementTime(base::TimeDelta::FromMilliseconds(500));
+  view()->Layout();
+  EXPECT_FALSE(layout()->is_animating());
+  EnsureLayout(expected_start, "after second slide");
+}
+
 TEST_F(AnimatingLayoutManagerTest, FlexLayout_FadeInOnAdded) {
   constexpr gfx::Insets kChildMargins(5);
   layout()->SetShouldAnimateBounds(false);