GMC: Always display playback controls LTR

Regardless of how the UI is laid out, media playback controls should be
laid out in a left-to-right manner, as suggested by the material.io
bidirectionality guide.

Additionally, the images for playback control buttons will no longer be
mirrored about their horizontal axes in an RTL layout. This includes
the play button and the PiP button. The PiP button is not mirrored
because the PiP display is always in the bottom right corner regardless
of UI layout.

Finally, after having worked with l10n and talking to UX engineers in
Israel, we have determined that the appropriate focus order for this
dialog should traverse the playback controls left-to-right first and
then focus the picture-in-picture button. This CL implements this order.

Bug: 1040021
Change-Id: I0a57ec7d879ab312ec1ce7bb592d8f79edceaa69
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2253134
Commit-Queue: Noah Rose Ledesma <noahrose@google.com>
Reviewed-by: Tommy Steimel <steimel@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#789662}

chrome/browser/ui/views/global_media_controls/media_dialog_view_interactive_browsertest.cc

@@ -495,6 +495,21 @@ class MediaDialogViewBrowserTest : public InProcessBrowserTest {
     return true;
   }
 
+  views::ImageButton* GetButtonForAction(MediaSessionAction action) {
+    return GetButtonForAction(
+        MediaDialogView::GetDialogViewForTesting()->children().front(),
+        static_cast<int>(action));
+  }
+
+  // Returns true if |target| exists in |base|'s forward focus chain
+  bool ViewFollowsInFocusChain(views::View* base, views::View* target) {
+    for (views::View* cur = base; cur; cur = cur->GetNextFocusableView()) {
+      if (cur == target)
+        return true;
+    }
+    return false;
+  }
+
  protected:
   std::unique_ptr<TestWebContentsPresentationManager> presentation_manager_;
   TestMediaRouter* media_router_ = nullptr;
@@ -508,12 +523,6 @@ class MediaDialogViewBrowserTest : public InProcessBrowserTest {
     closure_loop.Run();
   }
 
-  views::ImageButton* GetButtonForAction(MediaSessionAction action) {
-    return GetButtonForAction(
-        MediaDialogView::GetDialogViewForTesting()->children().front(),
-        static_cast<int>(action));
-  }
-
   // Recursively tries to find a views::ImageButton for the given
   // MediaSessionAction. This operates under the assumption that
   // media_message_center::MediaNotificationViewImpl sets the tags of its action
@@ -605,6 +614,80 @@ IN_PROC_BROWSER_TEST_F(MediaDialogViewBrowserTest,
   EXPECT_FALSE(IsDialogVisible());
 }
 
+IN_PROC_BROWSER_TEST_F(MediaDialogViewBrowserTest,
+                       ShowsMetadataAndControlsMediaInRTL) {
+  base::i18n::SetICUDefaultLocale("ar");
+  ASSERT_TRUE(base::i18n::IsRTL());
+
+  // The toolbar icon should not start visible.
+  EXPECT_FALSE(IsToolbarIconVisible());
+
+  // Opening a page with media that hasn't played yet should not make the
+  // toolbar icon visible.
+  OpenTestURL();
+  LayoutBrowser();
+  EXPECT_FALSE(IsToolbarIconVisible());
+
+  // Once playback starts, the icon should be visible, but the dialog should not
+  // appear if it hasn't been clicked.
+  StartPlayback();
+  WaitForStart();
+  WaitForVisibleToolbarIcon();
+  EXPECT_TRUE(IsToolbarIconVisible());
+  EXPECT_FALSE(IsDialogVisible());
+
+  // At this point, the toolbar icon has been set visible. Layout the
+  // browser to ensure it can be clicked.
+  LayoutBrowser();
+
+  // Clicking on the toolbar icon should open the dialog.
+  ClickToolbarIcon();
+  WaitForDialogOpened();
+  EXPECT_TRUE(IsDialogVisible());
+
+  // The view containing playback controls should not be mirrored.
+  EXPECT_FALSE(MediaDialogView::GetDialogViewForTesting()
+                   ->GetNotificationsForTesting()
+                   .begin()
+                   ->second->view_for_testing()
+                   ->playback_button_container_for_testing()
+                   ->GetMirrored());
+
+  // The dialog should contain the title and artist. These are taken from
+  // video-with-metadata.html.
+  WaitForDialogToContainText(base::ASCIIToUTF16("Big Buck Bunny"));
+  WaitForDialogToContainText(base::ASCIIToUTF16("Blender Foundation"));
+
+  // Clicking on the pause button in the dialog should pause the media on the
+  // page.
+  ClickPauseButtonOnDialog();
+  WaitForStop();
+
+  // Clicking on the play button in the dialog should play the media on the
+  // page.
+  ClickPlayButtonOnDialog();
+  WaitForStart();
+
+  // In the RTL UI the picture in picture button should be to the left of the
+  // playback control buttons.
+  EXPECT_LT(
+      GetButtonForAction(MediaSessionAction::kEnterPictureInPicture)
+          ->GetMirroredX(),
+      GetButtonForAction(MediaSessionAction::kPlay)->parent()->GetMirroredX());
+
+  // In the RTL UI the focus order should be the same as it is in the LTR UI.
+  // That is the play/pause button logically proceeds the picture in picture
+  // button.
+  EXPECT_TRUE(ViewFollowsInFocusChain(
+      GetButtonForAction(MediaSessionAction::kPlay)->parent(),
+      GetButtonForAction(MediaSessionAction::kEnterPictureInPicture)));
+
+  // Clicking on the toolbar icon again should hide the dialog.
+  EXPECT_TRUE(IsDialogVisible());
+  ClickToolbarIcon();
+  EXPECT_FALSE(IsDialogVisible());
+}
+
 IN_PROC_BROWSER_TEST_F(MediaDialogViewBrowserTest, ShowsMultipleMediaSessions) {
   // Open a tab and play media.
   OpenTestURL();

components/media_message_center/media_notification_view_impl.cc

@@ -185,6 +185,20 @@ MediaNotificationViewImpl::MediaNotificationViewImpl(
   button_row->SetPreferredSize(kMediaNotificationButtonRowSize);
   button_row_ = main_row_->AddChildView(std::move(button_row));
 
+  auto playback_button_container = std::make_unique<views::View>();
+  auto* playback_button_container_layout =
+      playback_button_container->SetLayoutManager(
+          std::make_unique<views::BoxLayout>(
+              views::BoxLayout::Orientation::kHorizontal, gfx::Insets(),
+              kMediaButtonRowSeparator));
+  playback_button_container_layout->set_cross_axis_alignment(
+      views::BoxLayout::CrossAxisAlignment::kCenter);
+  // Media playback controls should always be presented left-to-right,
+  // regardless of the local UI direction.
+  playback_button_container->SetMirrored(false);
+  playback_button_container_ =
+      button_row_->AddChildView(std::move(playback_button_container));
+
   CreateMediaButton(
       MediaSessionAction::kPreviousTrack,
       l10n_util::GetStringUTF16(
@@ -203,7 +217,9 @@ MediaNotificationViewImpl::MediaNotificationViewImpl(
       IDS_MEDIA_MESSAGE_CENTER_MEDIA_NOTIFICATION_ACTION_PLAY));
   play_pause_button->SetToggledTooltipText(l10n_util::GetStringUTF16(
       IDS_MEDIA_MESSAGE_CENTER_MEDIA_NOTIFICATION_ACTION_PAUSE));
-  play_pause_button_ = button_row_->AddChildView(std::move(play_pause_button));
+  play_pause_button->EnableCanvasFlippingForRTLUI(false);
+  play_pause_button_ =
+      playback_button_container_->AddChildView(std::move(play_pause_button));
 
   CreateMediaButton(
       MediaSessionAction::kSeekForward,
@@ -244,6 +260,7 @@ MediaNotificationViewImpl::MediaNotificationViewImpl(
       IDS_MEDIA_MESSAGE_CENTER_MEDIA_NOTIFICATION_ACTION_ENTER_PIP));
   picture_in_picture_button->SetToggledTooltipText(l10n_util::GetStringUTF16(
       IDS_MEDIA_MESSAGE_CENTER_MEDIA_NOTIFICATION_ACTION_EXIT_PIP));
+  picture_in_picture_button->EnableCanvasFlippingForRTLUI(false);
   picture_in_picture_button_ =
       button_row_->AddChildView(std::move(picture_in_picture_button));
 
@@ -321,7 +338,8 @@ void MediaNotificationViewImpl::ButtonPressed(views::Button* sender,
     return;
   }
 
-  if (sender->parent() == button_row_) {
+  if (sender->parent() == button_row_ ||
+      sender->parent() == playback_button_container_) {
     if (item_) {
       item_->OnMediaSessionActionButtonPressed(GetActionFromButtonTag(*sender));
     }
@@ -474,10 +492,7 @@ void MediaNotificationViewImpl::UpdateActionButtonsVisibility() {
       GetTopVisibleActions(enabled_actions_, ignored_actions,
                            GetMaxNumActions(IsActuallyExpanded()));
 
-  for (auto* view : button_row_->children()) {
-    if (view == pip_button_separator_view_)
-      continue;
-
+  for (auto* view : GetButtons()) {
     views::Button* action_button = views::Button::AsButton(view);
     bool should_show =
         base::Contains(visible_actions, GetActionFromButtonTag(*action_button));
@@ -560,7 +575,8 @@ void MediaNotificationViewImpl::CreateMediaButton(
   button->SetAccessibleName(accessible_name);
   button->SetTooltipText(accessible_name);
   button->SetFocusBehavior(views::View::FocusBehavior::ALWAYS);
-  button_row_->AddChildView(std::move(button));
+  button->EnableCanvasFlippingForRTLUI(false);
+  playback_button_container_->AddChildView(std::move(button));
 }
 
 MediaNotificationBackground*
@@ -634,18 +650,11 @@ void MediaNotificationViewImpl::UpdateForegroundColor() {
       kMediaButtonIconSize, foreground, disabled_icon_color);
 
   // Update action buttons.
-  for (views::View* child : button_row_->children()) {
+  for (views::View* child : playback_button_container_->children()) {
     // Skip the play pause button since it is a special case.
     if (child == play_pause_button_)
       continue;
 
-    if (child == picture_in_picture_button_)
-      continue;
-
-    // Skip if the view is not an image button.
-    if (child->GetClassName() != views::ImageButton::kViewClassName)
-      continue;
-
     views::ImageButton* button = static_cast<views::ImageButton*>(child);
 
     views::SetImageFromVectorIconWithColor(
@@ -658,4 +667,20 @@ void MediaNotificationViewImpl::UpdateForegroundColor() {
   container_->OnColorsChanged(foreground, background);
 }
 
+std::vector<views::View*> MediaNotificationViewImpl::GetButtons() {
+  auto buttons = button_row_->children();
+  buttons.insert(buttons.cbegin(),
+                 playback_button_container_->children().cbegin(),
+                 playback_button_container_->children().cend());
+  buttons.erase(
+      std::remove_if(buttons.begin(), buttons.end(),
+                     [](views::View* view) {
+                       return !(view->GetClassName() ==
+                                    views::ImageButton::kViewClassName ||
+                                view->GetClassName() ==
+                                    views::ToggleImageButton::kViewClassName);
+                     }),
+      buttons.end());
+  return buttons;
+}
 }  // namespace media_message_center

components/media_message_center/media_notification_view_impl.h

@@ -91,6 +91,12 @@ class COMPONENT_EXPORT(MEDIA_MESSAGE_CENTER) MediaNotificationViewImpl
     return picture_in_picture_button_;
   }
 
+  const views::View* playback_button_container_for_testing() const {
+    return playback_button_container_;
+  }
+
+  std::vector<views::View*> get_buttons_for_testing() { return GetButtons(); }
+
   views::Button* GetHeaderRowForTesting() const;
   base::string16 GetSourceTitleForTesting() const;
 
@@ -114,6 +120,10 @@ class COMPONENT_EXPORT(MEDIA_MESSAGE_CENTER) MediaNotificationViewImpl
 
   void UpdateForegroundColor();
 
+  // Returns the buttons contained in the button row and playback button
+  // container.
+  std::vector<views::View*> GetButtons();
+
   // Container that receives OnExpanded events.
   MediaNotificationContainer* const container_;
 
@@ -149,6 +159,7 @@ class COMPONENT_EXPORT(MEDIA_MESSAGE_CENTER) MediaNotificationViewImpl
   // Container views directly attached to this view.
   message_center::NotificationHeaderView* header_row_ = nullptr;
   views::View* button_row_ = nullptr;
+  views::View* playback_button_container_ = nullptr;
   views::View* pip_button_separator_view_ = nullptr;
   views::ToggleImageButton* play_pause_button_ = nullptr;
   views::ToggleImageButton* picture_in_picture_button_ = nullptr;

components/media_message_center/media_notification_view_impl_unittest.cc

@@ -62,12 +62,6 @@ constexpr int kViewWidth = 400;
 constexpr int kViewArtworkWidth = kViewWidth * 0.4;
 const gfx::Size kViewSize(kViewWidth, 400);
 
-// Checks if the view class name is used by a media button.
-bool IsMediaButtonType(const char* class_name) {
-  return class_name == views::ImageButton::kViewClassName ||
-         class_name == views::ToggleImageButton::kViewClassName;
-}
-
 class MockMediaNotificationController : public MediaNotificationController {
  public:
   MockMediaNotificationController() = default;
@@ -227,6 +221,10 @@ class MediaNotificationViewImplTest : public views::ViewsTestBase {
 
   views::View* button_row() const { return view()->button_row_; }
 
+  const views::View* playback_button_container() const {
+    return view()->playback_button_container_;
+  }
+
   views::View* title_artist_row() const { return view()->title_artist_row_; }
 
   views::Label* title_label() const { return view()->title_label_; }
@@ -234,14 +232,12 @@ class MediaNotificationViewImplTest : public views::ViewsTestBase {
   views::Label* artist_label() const { return view()->artist_label_; }
 
   views::Button* GetButtonForAction(MediaSessionAction action) const {
-    const auto& children = button_row()->children();
+    auto buttons = view()->get_buttons_for_testing();
     const auto i = std::find_if(
-        children.begin(), children.end(), [action](const views::View* v) {
-          return (IsMediaButtonType(v->GetClassName()) &&
-                  views::Button::AsButton(v)->tag() ==
-                      static_cast<int>(action));
+        buttons.begin(), buttons.end(), [action](const views::View* v) {
+          return views::Button::AsButton(v)->tag() == static_cast<int>(action);
         });
-    return (i == children.end()) ? nullptr : views::Button::AsButton(*i);
+    return (i == buttons.end()) ? nullptr : views::Button::AsButton(*i);
   }
 
   bool IsActionButtonVisible(MediaSessionAction action) const {
@@ -364,16 +360,14 @@ TEST_F(MAYBE_MediaNotificationViewImplTest, ButtonsSanityCheck) {
   EXPECT_GT(button_row()->width(), 0);
   EXPECT_GT(button_row()->height(), 0);
 
-  EXPECT_EQ(7u, button_row()->children().size());
-
-  for (auto* child : button_row()->children()) {
-    if (!IsMediaButtonType(child->GetClassName()))
-      continue;
+  auto buttons = view()->get_buttons_for_testing();
+  EXPECT_EQ(6u, buttons.size());
 
-    EXPECT_TRUE(child->GetVisible());
-    EXPECT_LT(kMediaButtonIconSize, child->width());
-    EXPECT_LT(kMediaButtonIconSize, child->height());
-    EXPECT_FALSE(views::Button::AsButton(child)->GetAccessibleName().empty());
+  for (auto* button : buttons) {
+    EXPECT_TRUE(button->GetVisible());
+    EXPECT_LT(kMediaButtonIconSize, button->width());
+    EXPECT_LT(kMediaButtonIconSize, button->height());
+    EXPECT_FALSE(views::Button::AsButton(button)->GetAccessibleName().empty());
   }
 
   EXPECT_TRUE(GetButtonForAction(MediaSessionAction::kPlay));