[Lockscreen Media Controls] Fix control button position.

This CL changes button row layout tree so that play/pause
button is always centered regardless the visibility of previous/next
track button.

Bug: 1010645
Change-Id: Iba274ccd820346885768fa7243ff523313df3632
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1834899
Commit-Queue: Jazz Xu <jazzhsu@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702606}

ash/login/ui/lock_screen_media_controls_view.cc

@@ -70,6 +70,9 @@ constexpr int kChangeTrackIconSize = 14;
 constexpr int kSeekingIconsSize = 26;
 constexpr gfx::Size kMediaControlsButtonRowSize =
     gfx::Size(300, kMediaButtonSize.height());
+constexpr gfx::Size kMediaButtonGroupSize =
+    gfx::Size(2 * kMediaButtonSize.width() + kMediaButtonRowSeparator,
+              kMediaButtonSize.height());
 constexpr int kArtworkCornerRadius = 4;
 constexpr int kMediaActionButtonCornerRadius = kMediaButtonSize.width() / 2;
 
@@ -282,6 +285,13 @@ LockScreenMediaControlsView::LockScreenMediaControlsView(
 
   // |button_row_| contains the buttons for controlling playback.
   auto button_row = std::make_unique<NonAccessibleView>();
+
+  // left_control_group contains previous_track_button and seek_backward_button.
+  auto left_control_group = std::make_unique<NonAccessibleView>();
+
+  // right_control_group contains next_track_button and seek_forward_button.
+  auto right_control_group = std::make_unique<NonAccessibleView>();
+
   auto* button_row_layout =
       button_row->SetLayoutManager(std::make_unique<views::BoxLayout>(
           views::BoxLayout::Orientation::kHorizontal, kButtonRowInsets,
@@ -290,18 +300,44 @@ LockScreenMediaControlsView::LockScreenMediaControlsView(
       views::BoxLayout::CrossAxisAlignment::kCenter);
   button_row_layout->set_main_axis_alignment(
       views::BoxLayout::MainAxisAlignment::kCenter);
+
+  auto* left_control_group_layout =
+      left_control_group->SetLayoutManager(std::make_unique<views::BoxLayout>(
+          views::BoxLayout::Orientation::kHorizontal, gfx::Insets(),
+          kMediaButtonRowSeparator));
+  left_control_group_layout->set_cross_axis_alignment(
+      views::BoxLayout::CrossAxisAlignment::kCenter);
+  left_control_group_layout->set_main_axis_alignment(
+      views::BoxLayout::MainAxisAlignment::kEnd);
+
+  auto* right_control_group_layout =
+      right_control_group->SetLayoutManager(std::make_unique<views::BoxLayout>(
+          views::BoxLayout::Orientation::kHorizontal, gfx::Insets(),
+          kMediaButtonRowSeparator));
+  right_control_group_layout->set_cross_axis_alignment(
+      views::BoxLayout::CrossAxisAlignment::kCenter);
+  right_control_group_layout->set_main_axis_alignment(
+      views::BoxLayout::MainAxisAlignment::kStart);
+
   button_row->SetPreferredSize(kMediaControlsButtonRowSize);
   button_row_ = contents_view_->AddChildView(std::move(button_row));
 
-  button_row_->AddChildView(std::make_unique<MediaActionButton>(
-      this, kChangeTrackIconSize, MediaSessionAction::kPreviousTrack,
-      l10n_util::GetStringUTF16(
-          IDS_ASH_LOCK_SCREEN_MEDIA_CONTROLS_ACTION_PREVIOUS_TRACK)));
+  left_control_group->SetPreferredSize(kMediaButtonGroupSize);
+  right_control_group->SetPreferredSize(kMediaButtonGroupSize);
+
+  media_action_buttons_.push_back(
+      left_control_group->AddChildView(std::make_unique<MediaActionButton>(
+          this, kChangeTrackIconSize, MediaSessionAction::kPreviousTrack,
+          l10n_util::GetStringUTF16(
+              IDS_ASH_LOCK_SCREEN_MEDIA_CONTROLS_ACTION_PREVIOUS_TRACK))));
+
+  media_action_buttons_.push_back(
+      left_control_group->AddChildView(std::make_unique<MediaActionButton>(
+          this, kSeekingIconsSize, MediaSessionAction::kSeekBackward,
+          l10n_util::GetStringUTF16(
+              IDS_ASH_LOCK_SCREEN_MEDIA_CONTROLS_ACTION_SEEK_BACKWARD))));
 
-  button_row_->AddChildView(std::make_unique<MediaActionButton>(
-      this, kSeekingIconsSize, MediaSessionAction::kSeekBackward,
-      l10n_util::GetStringUTF16(
-          IDS_ASH_LOCK_SCREEN_MEDIA_CONTROLS_ACTION_SEEK_BACKWARD)));
+  button_row_->AddChildView(std::move(left_control_group));
 
   // |play_pause_button_| toggles playback. If the current media is playing, the
   // tag will be |MediaSessionAction::kPause|. If the current media is paused,
@@ -311,16 +347,21 @@ LockScreenMediaControlsView::LockScreenMediaControlsView(
           this, kPlayPauseIconSize, MediaSessionAction::kPause,
           l10n_util::GetStringUTF16(
               IDS_ASH_LOCK_SCREEN_MEDIA_CONTROLS_ACTION_PAUSE)));
+  media_action_buttons_.push_back(play_pause_button_);
 
-  button_row_->AddChildView(std::make_unique<MediaActionButton>(
-      this, kSeekingIconsSize, MediaSessionAction::kSeekForward,
-      l10n_util::GetStringUTF16(
-          IDS_ASH_LOCK_SCREEN_MEDIA_CONTROLS_ACTION_SEEK_FORWARD)));
+  media_action_buttons_.push_back(
+      right_control_group->AddChildView(std::make_unique<MediaActionButton>(
+          this, kSeekingIconsSize, MediaSessionAction::kSeekForward,
+          l10n_util::GetStringUTF16(
+              IDS_ASH_LOCK_SCREEN_MEDIA_CONTROLS_ACTION_SEEK_FORWARD))));
+
+  media_action_buttons_.push_back(
+      right_control_group->AddChildView(std::make_unique<MediaActionButton>(
+          this, kChangeTrackIconSize, MediaSessionAction::kNextTrack,
+          l10n_util::GetStringUTF16(
+              IDS_ASH_LOCK_SCREEN_MEDIA_CONTROLS_ACTION_NEXT_TRACK))));
 
-  button_row_->AddChildView(std::make_unique<MediaActionButton>(
-      this, kChangeTrackIconSize, MediaSessionAction::kNextTrack,
-      l10n_util::GetStringUTF16(
-          IDS_ASH_LOCK_SCREEN_MEDIA_CONTROLS_ACTION_NEXT_TRACK)));
+  button_row_->AddChildView(std::move(right_control_group));
 
   // Set child view data to default values initially, until the media controller
   // observers are triggered by a change in media session state.
@@ -632,8 +673,7 @@ void LockScreenMediaControlsView::UpdateActionButtonsVisibility() {
                                                  ignored_actions, kMaxActions);
 
   bool should_invalidate = false;
-  for (auto* view : button_row_->children()) {
-    views::Button* action_button = views::Button::AsButton(view);
+  for (views::Button* action_button : media_action_buttons_) {
     bool should_show = base::Contains(
         visible_actions,
         media_message_center::GetActionFromButtonTag(*action_button));

ash/login/ui/lock_screen_media_controls_view.h

@@ -251,6 +251,8 @@ class ASH_EXPORT LockScreenMediaControlsView
   MediaActionButton* play_pause_button_ = nullptr;
   media_message_center::MediaControlsProgressView* progress_ = nullptr;
 
+  std::vector<views::Button*> media_action_buttons_;
+
   // Callbacks.
   const MediaControlsEnabled media_controls_enabled_;
   const base::RepeatingClosure hide_media_controls_;

ash/login/ui/lock_screen_media_controls_view_unittest.cc

@@ -178,16 +178,16 @@ class LockScreenMediaControlsViewTest : public LoginTestBase {
   }
 
   views::Button* GetButtonForAction(MediaSessionAction action) const {
-    const auto& children = button_row()->children();
-    const auto it = std::find_if(
-        children.begin(), children.end(), [action](const views::View* v) {
-          return views::Button::AsButton(v)->tag() == static_cast<int>(action);
-        });
+    const auto& buttons = media_action_buttons();
+    const auto it = std::find_if(buttons.begin(), buttons.end(),
+                                 [action](const views::Button* b) {
+                                   return b->tag() == static_cast<int>(action);
+                                 });
 
-    if (it == children.end())
+    if (it == buttons.end())
       return nullptr;
 
-    return views::Button::AsButton(*it);
+    return *it;
   }
 
   TestMediaController* media_controller() const {
@@ -226,6 +226,10 @@ class LockScreenMediaControlsViewTest : public LoginTestBase {
     return header_row()->close_button_for_testing();
   }
 
+  std::vector<views::Button*>& media_action_buttons() const {
+    return media_controls_view_->media_action_buttons_;
+  }
+
   bool CloseButtonHasImage() const {
     return !close_button()
                 ->GetImage(views::Button::ButtonState::STATE_NORMAL)
@@ -342,10 +346,10 @@ TEST_F(LockScreenMediaControlsViewTest, ButtonsSanityCheck) {
   EnableAllActions();
 
   EXPECT_TRUE(button_row()->GetVisible());
-  EXPECT_EQ(5u, button_row()->children().size());
+  EXPECT_EQ(5u, media_action_buttons().size());
 
   for (int i = 0; i < 5; /* size of |button_row| */ i++) {
-    auto* child = button_row()->children()[i];
+    auto* child = media_action_buttons()[i];
 
     ASSERT_TRUE(IsMediaButtonType(child->GetClassName()));