Fix minimize button sliding into the wrong position when exiting tablet mode

When PWA menu buttons are visible in the title bar we fail to animate the
minimize button to the correct position when exiting tablet mode.

This CL fixes the bug by removing the assumption that the minimize button
is the left-most button and generalises the slide animation to all buttons
left of the size button.

Bug: 802144
Change-Id: I8ad560c966f9b0fa04beabcf6b96850fe784b5aa
Reviewed-on: https://chromium-review.googlesource.com/892186
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: calamity <calamity@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533133}

ash/frame/caption_buttons/frame_caption_button_container_view.cc

@@ -31,20 +31,21 @@ namespace ash {
 
 namespace {
 
-// Duration of the animation of the position of |minimize_button_|.
+// Duration of the animation of the position of buttons to the left of
+// |size_button_|.
 const int kPositionAnimationDurationMs = 500;
 
 // Duration of the animation of the alpha of |size_button_|.
 const int kAlphaAnimationDurationMs = 250;
 
 // Delay during |tablet_mode_animation_| hide to wait before beginning to
-// animate the position of |minimize_button_|.
+// animate the position of buttons to the left of |size_button_|.
 const int kHidePositionDelayMs = 100;
 
 // Duration of |tablet_mode_animation_| hiding.
 // Hiding size button 250
 // |------------------------|
-// Delay 100      Slide minimize button 500
+// Delay 100      Slide other buttons 500
 // |---------|-------------------------------------------------|
 const int kHideAnimationDurationMs =
     kHidePositionDelayMs + kPositionAnimationDurationMs;
@@ -54,7 +55,7 @@ const int kHideAnimationDurationMs =
 const int kShowAnimationAlphaDelayMs = 100;
 
 // Duration of |tablet_mode_animation_| showing.
-// Slide minimize button 500
+// Slide other buttons 500
 // |-------------------------------------------------|
 // Delay 100   Show size button 250
 // |---------|-----------------------|
@@ -82,7 +83,7 @@ float SizeButtonHideDuration() {
 }
 
 // Value of |tablet_mode_animation_| hiding to begin animating the position of
-// |minimize_button_|.
+// buttons to the left of |size_button_|.
 float HidePositionStartValue() {
   return 1.0f -
          static_cast<float>(kHidePositionDelayMs) / kHideAnimationDurationMs;
@@ -221,7 +222,7 @@ void FrameCaptionButtonContainerView::Layout() {
   views::View::Layout();
 
   // This ensures that the first frame of the animation to show the size button
-  // pushes the minimize button into the center.
+  // pushes the buttons to the left of the size button into the center.
   if (tablet_mode_animation_->is_animating())
     AnimationProgressed(tablet_mode_animation_.get());
 }
@@ -250,19 +251,19 @@ void FrameCaptionButtonContainerView::AnimationProgressed(
     const gfx::Animation* animation) {
   double current_value = animation->GetCurrentValue();
   int size_alpha = 0;
-  int minimize_x = 0;
+  int x_slide = 0;
   if (tablet_mode_animation_->IsShowing()) {
-    double scaled_value =
+    double scaled_value_alpha =
         CapAnimationValue((current_value - SizeButtonShowStartValue()) /
                           SizeButtonShowDuration());
     double tweened_value_alpha =
-        gfx::Tween::CalculateValue(gfx::Tween::EASE_OUT, scaled_value);
+        gfx::Tween::CalculateValue(gfx::Tween::EASE_OUT, scaled_value_alpha);
     size_alpha = gfx::Tween::LinearIntValueBetween(tweened_value_alpha, 0, 255);
 
     double tweened_value_slide =
         gfx::Tween::CalculateValue(gfx::Tween::EASE_OUT, current_value);
-    minimize_x = gfx::Tween::LinearIntValueBetween(tweened_value_slide,
-                                                   size_button_->x(), 0);
+    x_slide = gfx::Tween::LinearIntValueBetween(tweened_value_slide,
+                                                size_button_->width(), 0);
   } else {
     double scaled_value_alpha =
         CapAnimationValue((1.0f - current_value) / SizeButtonHideDuration());
@@ -272,13 +273,21 @@ void FrameCaptionButtonContainerView::AnimationProgressed(
 
     double scaled_value_position = CapAnimationValue(
         (HidePositionStartValue() - current_value) / HidePositionStartValue());
-    double tweened_value_position =
+    double tweened_value_slide =
         gfx::Tween::CalculateValue(gfx::Tween::EASE_OUT, scaled_value_position);
-    minimize_x = gfx::Tween::LinearIntValueBetween(tweened_value_position, 0,
-                                                   size_button_->x());
+    x_slide = gfx::Tween::LinearIntValueBetween(tweened_value_slide, 0,
+                                                size_button_->width());
   }
   size_button_->SetAlpha(size_alpha);
-  minimize_button_->SetX(minimize_x);
+
+  // Slide all buttons to the left of the size button. Usually this is just the
+  // minimize button but it can also include a PWA menu button.
+  int previous_x = 0;
+  for (int i = 0; i < child_count() && child_at(i) != size_button_; ++i) {
+    views::View* button = child_at(i);
+    button->SetX(previous_x + x_slide);
+    previous_x += button->width();
+  }
 }
 
 void FrameCaptionButtonContainerView::SetButtonIcon(FrameCaptionButton* button,

ash/frame/caption_buttons/frame_caption_button_container_view.h

@@ -146,8 +146,9 @@ class ASH_EXPORT FrameCaptionButtonContainerView
   // CaptionButtonIcon.
   std::map<CaptionButtonIcon, const gfx::VectorIcon*> button_icon_map_;
 
-  // Animation that affects the position of |minimize_button_| and the
-  // visibility of |size_button_|.
+  // Animation that affects the visibility of |size_button_| and the position of
+  // buttons to the left of it. Usually this is just the minimize button but it
+  // can also include a PWA menu button.
   std::unique_ptr<gfx::SlideAnimation> tablet_mode_animation_;
 
   DISALLOW_COPY_AND_ASSIGN(FrameCaptionButtonContainerView);

ash/frame/caption_buttons/frame_caption_button_container_view_unittest.cc

@@ -148,15 +148,26 @@ TEST_F(FrameCaptionButtonContainerViewTest,
        TestUpdateSizeButtonVisibilityAnimation) {
   FrameCaptionButtonContainerView container(
       CreateTestWidget(MAXIMIZE_ALLOWED, MINIMIZE_ALLOWED));
+
+  // Add an extra button to the left of the size button to verify that it is
+  // repositioned similarly to the minimize button. This simulates the PWA menu
+  // button being added to the left of the minimize button.
+  FrameCaptionButton* extra_button =
+      new FrameCaptionButton(&container, CAPTION_BUTTON_ICON_BACK);
+  container.AddChildViewAt(extra_button, 0);
+
   InitContainer(&container);
   container.Layout();
 
   FrameCaptionButtonContainerView::TestApi test(&container);
+  gfx::Rect initial_extra_button_bounds = extra_button->bounds();
   gfx::Rect initial_minimize_button_bounds = test.minimize_button()->bounds();
   gfx::Rect initial_size_button_bounds = test.size_button()->bounds();
   gfx::Rect initial_close_button_bounds = test.close_button()->bounds();
   gfx::Rect initial_container_bounds = container.bounds();
 
+  ASSERT_EQ(initial_minimize_button_bounds.x(),
+            initial_extra_button_bounds.right());
   ASSERT_EQ(initial_size_button_bounds.x(),
             initial_minimize_button_bounds.right());
   ASSERT_EQ(initial_close_button_bounds.x(),
@@ -173,8 +184,10 @@ TEST_F(FrameCaptionButtonContainerViewTest,
   EXPECT_TRUE(test.minimize_button()->visible());
   EXPECT_FALSE(test.size_button()->visible());
   EXPECT_TRUE(test.close_button()->visible());
+  gfx::Rect extra_button_bounds = extra_button->bounds();
   gfx::Rect minimize_button_bounds = test.minimize_button()->bounds();
   gfx::Rect close_button_bounds = test.close_button()->bounds();
+  EXPECT_EQ(minimize_button_bounds.x(), extra_button_bounds.right());
   EXPECT_EQ(close_button_bounds.x(), minimize_button_bounds.right());
   EXPECT_EQ(initial_size_button_bounds, test.size_button()->bounds());
   EXPECT_EQ(initial_close_button_bounds.size(), close_button_bounds.size());
@@ -191,6 +204,7 @@ TEST_F(FrameCaptionButtonContainerViewTest,
   EXPECT_TRUE(test.minimize_button()->visible());
   EXPECT_TRUE(test.size_button()->visible());
   EXPECT_TRUE(test.close_button()->visible());
+  EXPECT_EQ(initial_extra_button_bounds, extra_button->bounds());
   EXPECT_EQ(initial_minimize_button_bounds, test.minimize_button()->bounds());
   EXPECT_EQ(initial_size_button_bounds, test.size_button()->bounds());
   EXPECT_EQ(initial_close_button_bounds, test.close_button()->bounds());