GMC: Show the dismiss button on hover/focus

This CL changes the GMC dismiss button to show whenever the media
notification is hovered or focused instead of whenever the media is
paused. This means that users can dismiss while media is playing, which
will stop playback.

Bug: 1008907
Change-Id: I5342a4e1e7238a16203687e57dfe6608b2ab35be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1859584Reviewed-by: Jazz Xu <jazzhsu@chromium.org>
Commit-Queue: Tommy Steimel <steimel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706692}

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

@@ -61,8 +61,8 @@ MediaNotificationContainerImplView::MediaNotificationContainerImplView(
       foreground_color_(kDefaultForegroundColor),
       background_color_(kDefaultBackgroundColor) {
   SetLayoutManager(std::make_unique<views::FillLayout>());
-
   SetPreferredSize(kNormalSize);
+  set_notify_enter_exit_on_child(true);
 
   swipeable_container_ = std::make_unique<views::View>();
   swipeable_container_->set_owned_by_client();
@@ -76,7 +76,7 @@ MediaNotificationContainerImplView::MediaNotificationContainerImplView(
   dismiss_button_container_->SetPreferredSize(kDismissButtonSize);
   dismiss_button_container_->SetLayoutManager(
       std::make_unique<views::FillLayout>());
-  dismiss_button_container_->SetVisible(true);
+  dismiss_button_container_->SetVisible(false);
 
   auto dismiss_button = std::make_unique<DismissButton>(this);
   dismiss_button->SetPreferredSize(kDismissButtonSize);
@@ -104,6 +104,32 @@ MediaNotificationContainerImplView::~MediaNotificationContainerImplView() {
     observer.OnContainerDestroyed(id_);
 }
 
+void MediaNotificationContainerImplView::AddedToWidget() {
+  if (GetFocusManager())
+    GetFocusManager()->AddFocusChangeListener(this);
+}
+
+void MediaNotificationContainerImplView::RemovedFromWidget() {
+  if (GetFocusManager())
+    GetFocusManager()->RemoveFocusChangeListener(this);
+}
+
+void MediaNotificationContainerImplView::OnMouseEntered(
+    const ui::MouseEvent& event) {
+  UpdateDismissButtonVisibility();
+}
+
+void MediaNotificationContainerImplView::OnMouseExited(
+    const ui::MouseEvent& event) {
+  UpdateDismissButtonVisibility();
+}
+
+void MediaNotificationContainerImplView::OnDidChangeFocus(
+    views::View* focused_before,
+    views::View* focused_now) {
+  UpdateDismissButtonVisibility();
+}
+
 void MediaNotificationContainerImplView::OnExpanded(bool expanded) {
   SetPreferredSize(expanded ? kExpandedSize : kNormalSize);
   PreferredSizeChanged();
@@ -112,13 +138,6 @@ void MediaNotificationContainerImplView::OnExpanded(bool expanded) {
     observer.OnContainerExpanded(expanded);
 }
 
-void MediaNotificationContainerImplView::OnMediaSessionInfoChanged(
-    const media_session::mojom::MediaSessionInfoPtr& session_info) {
-  dismiss_button_container_->SetVisible(
-      !session_info || session_info->playback_state !=
-                           media_session::mojom::MediaPlaybackState::kPlaying);
-}
-
 void MediaNotificationContainerImplView::OnMediaSessionMetadataChanged() {
   for (auto& observer : observers_)
     observer.OnContainerMetadataChanged();
@@ -195,6 +214,17 @@ void MediaNotificationContainerImplView::UpdateDismissButtonBackground() {
       background_color_, kDismissButtonBackgroundRadius));
 }
 
+void MediaNotificationContainerImplView::UpdateDismissButtonVisibility() {
+  bool has_focus = false;
+  if (GetFocusManager()) {
+    views::View* focused_view = GetFocusManager()->GetFocusedView();
+    if (focused_view)
+      has_focus = Contains(focused_view);
+  }
+
+  dismiss_button_container_->SetVisible(IsMouseHovered() || has_focus);
+}
+
 void MediaNotificationContainerImplView::DismissNotification() {
   for (auto& observer : observers_)
     observer.OnContainerDismissed(id_);

chrome/browser/ui/views/global_media_controls/media_notification_container_impl_view.h

@@ -14,6 +14,7 @@
 #include "components/media_message_center/media_notification_view.h"
 #include "ui/views/animation/slide_out_controller_delegate.h"
 #include "ui/views/controls/button/button.h"
+#include "ui/views/focus/focus_manager.h"
 #include "ui/views/view.h"
 
 namespace media_message_center {
@@ -34,17 +35,30 @@ class MediaNotificationContainerImplView
       public media_message_center::MediaNotificationContainer,
       public MediaNotificationContainerImpl,
       public views::SlideOutControllerDelegate,
-      public views::ButtonListener {
+      public views::ButtonListener,
+      public views::FocusChangeListener {
  public:
   MediaNotificationContainerImplView(
       const std::string& id,
       base::WeakPtr<media_message_center::MediaNotificationItem> item);
   ~MediaNotificationContainerImplView() override;
 
+  // views::View:
+  void AddedToWidget() override;
+  void RemovedFromWidget() override;
+  void OnMouseEntered(const ui::MouseEvent& event) override;
+  void OnMouseExited(const ui::MouseEvent& event) override;
+
+  // views::FocusChangeListener:
+  void OnWillChangeFocus(views::View* focused_before,
+                         views::View* focused_now) override {}
+  void OnDidChangeFocus(views::View* focused_before,
+                        views::View* focused_now) override;
+
   // media_message_center::MediaNotificationContainer:
   void OnExpanded(bool expanded) override;
   void OnMediaSessionInfoChanged(
-      const media_session::mojom::MediaSessionInfoPtr& session_info) override;
+      const media_session::mojom::MediaSessionInfoPtr& session_info) override {}
   void OnMediaSessionMetadataChanged() override;
   void OnVisibleActionsChanged(
       const std::set<media_session::mojom::MediaSessionAction>& actions)
@@ -78,6 +92,8 @@ class MediaNotificationContainerImplView
 
   void UpdateDismissButtonBackground();
 
+  void UpdateDismissButtonVisibility();
+
   void DismissNotification();
 
   // Updates the forced expanded state of |view_|.

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

@@ -5,12 +5,17 @@
 #include "chrome/browser/ui/views/global_media_controls/media_notification_container_impl_view.h"
 
 #include "base/strings/utf_string_conversions.h"
+#include "build/build_config.h"
 #include "chrome/browser/ui/global_media_controls/media_notification_container_observer.h"
 #include "services/media_session/public/mojom/media_session.mojom.h"
 #include "testing/gmock/include/gmock/gmock.h"
+#include "ui/display/test/scoped_screen_override.h"
+#include "ui/display/test/test_screen.h"
 #include "ui/events/base_event_utils.h"
+#include "ui/events/test/event_generator.h"
 #include "ui/views/test/button_test_api.h"
 #include "ui/views/test/views_test_base.h"
+#include "ui/views/widget/widget_utils.h"
 
 using media_session::mojom::MediaPlaybackState;
 using media_session::mojom::MediaSessionAction;
@@ -37,11 +42,28 @@ class MockMediaNotificationContainerObserver
   DISALLOW_COPY_AND_ASSIGN(MockMediaNotificationContainerObserver);
 };
 
+// Fake display::Screen implementation that allows us to set a cursor location.
+class FakeCursorLocationScreen : public display::test::TestScreen {
+ public:
+  FakeCursorLocationScreen() = default;
+  ~FakeCursorLocationScreen() override = default;
+
+  void SetCursorScreenPoint(gfx::Point point) { cursor_position_ = point; }
+
+  // display::test::TestScreen implementation.
+  gfx::Point GetCursorScreenPoint() override { return cursor_position_; }
+
+ private:
+  gfx::Point cursor_position_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeCursorLocationScreen);
+};
+
 }  // anonymous namespace
 
 class MediaNotificationContainerImplViewTest : public views::ViewsTestBase {
  public:
-  MediaNotificationContainerImplViewTest() = default;
+  MediaNotificationContainerImplViewTest() : screen_override_(&fake_screen_) {}
   ~MediaNotificationContainerImplViewTest() override = default;
 
   // ViewsTestBase:
@@ -53,14 +75,12 @@ class MediaNotificationContainerImplViewTest : public views::ViewsTestBase {
     params.bounds = gfx::Rect(400, 300);
     params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
     widget_.Init(std::move(params));
-    views::View* container_view = new views::View();
-    widget_.SetContentsView(container_view);
 
     auto notification_container =
         std::make_unique<MediaNotificationContainerImplView>(
             kTestNotificationId, nullptr);
-    notification_container_ =
-        container_view->AddChildView(std::move(notification_container));
+    notification_container_ = notification_container.get();
+    widget_.SetContentsView(notification_container.release());
 
     observer_ = std::make_unique<MockMediaNotificationContainerObserver>();
     notification_container_->AddObserver(observer_.get());
@@ -84,12 +104,47 @@ class MediaNotificationContainerImplViewTest : public views::ViewsTestBase {
 
   bool IsDismissButtonVisible() { return GetDismissButton()->IsDrawn(); }
 
+  void SimulateHoverOverContainer() {
+    fake_screen_.SetCursorScreenPoint(
+        notification_container_->GetBoundsInScreen().CenterPoint());
+
+    ui::MouseEvent event(ui::ET_MOUSE_ENTERED, gfx::Point(), gfx::Point(),
+                         ui::EventTimeForNow(), 0, 0);
+    notification_container_->OnMouseEntered(event);
+  }
+
+  void SimulateNotHoveringOverContainer() {
+    gfx::Rect container_bounds = notification_container_->GetBoundsInScreen();
+    gfx::Point point_outside_container =
+        container_bounds.bottom_right() + gfx::Vector2d(1, 1);
+    fake_screen_.SetCursorScreenPoint(point_outside_container);
+
+    ui::MouseEvent event(ui::ET_MOUSE_EXITED, gfx::Point(), gfx::Point(),
+                         ui::EventTimeForNow(), 0, 0);
+    notification_container_->OnMouseExited(event);
+  }
+
   void SimulateDismissButtonClicked() {
     ui::MouseEvent event(ui::ET_MOUSE_PRESSED, gfx::Point(), gfx::Point(),
                          ui::EventTimeForNow(), 0, 0);
     views::test::ButtonTestApi(GetDismissButton()).NotifyClick(event);
   }
 
+  void SimulatePressingDismissButtonWithKeyboard() {
+    GetFocusManager()->SetFocusedView(
+        notification_container_->GetDismissButtonForTesting());
+
+// On Mac OS, we need to use the space bar to press a button.
+#if defined(OS_MACOSX)
+    ui::KeyboardCode button_press_keycode = ui::VKEY_SPACE;
+#else
+    ui::KeyboardCode button_press_keycode = ui::VKEY_RETURN;
+#endif  // defined(OS_MACOSX)
+
+    ui::test::EventGenerator generator(GetRootWindow(&widget_));
+    generator.PressKey(button_press_keycode, 0);
+  }
+
   void SimulateSessionPlaying() { SimulateSessionInfo(true); }
 
   void SimulateSessionPaused() { SimulateSessionInfo(false); }
@@ -133,6 +188,10 @@ class MediaNotificationContainerImplViewTest : public views::ViewsTestBase {
     GetView()->UpdateWithMediaArtwork(gfx::ImageSkia());
   }
 
+  views::FocusManager* GetFocusManager() {
+    return notification_container_->GetFocusManager();
+  }
+
   MockMediaNotificationContainerObserver& observer() { return *observer_; }
 
   MediaNotificationContainerImplView* notification_container() {
@@ -179,6 +238,9 @@ class MediaNotificationContainerImplViewTest : public views::ViewsTestBase {
   // Set of actions currently enabled.
   std::set<MediaSessionAction> actions_;
 
+  FakeCursorLocationScreen fake_screen_;
+  display::test::ScopedScreenOverride screen_override_;
+
   DISALLOW_COPY_AND_ASSIGN(MediaNotificationContainerImplViewTest);
 };
 
@@ -188,17 +250,57 @@ TEST_F(MediaNotificationContainerImplViewTest, SwipeToDismiss) {
 }
 
 TEST_F(MediaNotificationContainerImplViewTest, ClickToDismiss) {
-  // We don't show the dismiss button when playing.
-  SimulateSessionPlaying();
+  // Ensure that the mouse is not over the container and that nothing is
+  // focused. The dismiss button should not be visible.
+  SimulateNotHoveringOverContainer();
+  ASSERT_EQ(nullptr, GetFocusManager()->GetFocusedView());
+  ASSERT_FALSE(notification_container()->IsMouseHovered());
+  EXPECT_FALSE(IsDismissButtonVisible());
+
+  // Hovering over the notification should show the dismiss button.
+  SimulateHoverOverContainer();
+  EXPECT_TRUE(IsDismissButtonVisible());
+
+  // Moving the mouse away from the notification should hide the dismiss button.
+  SimulateNotHoveringOverContainer();
   EXPECT_FALSE(IsDismissButtonVisible());
 
-  // We only show it when paused.
-  SimulateSessionPaused();
+  // Moving the mouse back over the notification should re-show it.
+  SimulateHoverOverContainer();
   EXPECT_TRUE(IsDismissButtonVisible());
 
   // Clicking it should inform observers that we've been dismissed.
   EXPECT_CALL(observer(), OnContainerDismissed(kTestNotificationId));
   SimulateDismissButtonClicked();
+  testing::Mock::VerifyAndClearExpectations(&observer());
+}
+
+TEST_F(MediaNotificationContainerImplViewTest, KeyboardToDismiss) {
+  // Ensure that the mouse is not over the container and that nothing is
+  // focused. The dismiss button should not be visible.
+  SimulateNotHoveringOverContainer();
+  ASSERT_EQ(nullptr, GetFocusManager()->GetFocusedView());
+  ASSERT_FALSE(notification_container()->IsMouseHovered());
+  EXPECT_FALSE(IsDismissButtonVisible());
+
+  // When the notification receives keyboard focus, the dismiss button should be
+  // visible.
+  GetFocusManager()->SetFocusedView(notification_container());
+  EXPECT_TRUE(IsDismissButtonVisible());
+
+  // When the notification loses keyboard focus, the dismiss button should be
+  // hidden.
+  GetFocusManager()->SetFocusedView(nullptr);
+  EXPECT_FALSE(IsDismissButtonVisible());
+
+  // If it gets focus again, it should re-show the dismiss button.
+  GetFocusManager()->SetFocusedView(notification_container());
+  EXPECT_TRUE(IsDismissButtonVisible());
+
+  // Clicking it should inform observers that we've been dismissed.
+  EXPECT_CALL(observer(), OnContainerDismissed(kTestNotificationId));
+  SimulatePressingDismissButtonWithKeyboard();
+  testing::Mock::VerifyAndClearExpectations(&observer());
 }
 
 TEST_F(MediaNotificationContainerImplViewTest, ForceExpandedState) {