Activate notification only when the remote input is focused

Previously notification was activated when the notification is clicked. This patch changes this timing of focus change. With this CL, the notification is activated only when the notification's remote input is focused.

Bug: 844310
Bug: b/78604162
Test: manual (notification is not focused just on click, and is focused when the remote input is activated)

Change-Id: I30b91ca5d65b0e1089d63a062bdc9edc82de1f89
Reviewed-on: https://chromium-review.googlesource.com/1080502Reviewed-by: Eliot Courtney <edcourtney@chromium.org>
Commit-Queue: Yoshiki Iguchi <yoshiki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568738}

ash/system/message_center/arc/arc_notification_content_view.cc

@@ -89,21 +89,6 @@ class ArcNotificationContentView::EventForwarder : public ui::EventHandler {
       return;
     }
 
-    // TODO(sarakato): Use a better tigger (eg. focusing EditText on
-    // notification) than clicking (b/78604162).
-    if (event->type() == ui::ET_MOUSE_PRESSED ||
-        event->type() == ui::ET_GESTURE_TAP) {
-      // Remove the focus from the currently focused view-control in the message
-      // center before activating the window of ARC notification, so that
-      // unexpected key handling doesn't happen (b/74415372).
-      // Focusing notification surface window doesn't steal the focus from
-      // the focucued view control in the message center, so that input events
-      // handles on both side wrongly without this.
-      owner_->GetFocusManager()->ClearFocus();
-
-      owner_->Activate();
-    }
-
     views::Widget* widget = owner_->GetWidget();
     if (!widget)
       return;
@@ -615,7 +600,7 @@ void ArcNotificationContentView::OnFocus() {
   notification_view->OnContentFocused();
 
   if (surface_ && surface_->GetAXTreeId() != -1)
-    Activate();
+    ActivateWidget(true);
 }
 
 void ArcNotificationContentView::OnBlur() {
@@ -631,18 +616,34 @@ void ArcNotificationContentView::OnBlur() {
   notification_view->OnContentBlurred();
 }
 
-void ArcNotificationContentView::Activate() {
+void ArcNotificationContentView::OnRemoteInputActivationChanged(
+    bool activated) {
+  // Remove the focus from the currently focused view-control in the message
+  // center before activating the window of ARC notification, so that unexpected
+  // key handling doesn't happen (b/74415372).
+  // Focusing notification surface window doesn't steal the focus from the
+  // focused view control in the message center, so that input events handles
+  // on both side wrongly without this.
+  GetFocusManager()->ClearFocus();
+
+  ActivateWidget(activated);
+}
+
+void ArcNotificationContentView::ActivateWidget(bool activate) {
   if (!GetWidget())
     return;
 
   // Make the widget active.
-  if (!GetWidget()->IsActive()) {
-    GetWidget()->widget_delegate()->set_can_activate(true);
-    GetWidget()->Activate();
-  }
+  if (activate) {
+    if (!GetWidget()->IsActive()) {
+      GetWidget()->widget_delegate()->set_can_activate(true);
+      GetWidget()->Activate();
+    }
 
-  // Focus the surface window.
-  surface_->FocusSurfaceWindow();
+    surface_->FocusSurfaceWindow();
+  } else {
+    GetWidget()->widget_delegate()->set_can_activate(false);
+  }
 }
 
 views::FocusTraversable* ArcNotificationContentView::GetFocusTraversable() {
@@ -676,7 +677,7 @@ void ArcNotificationContentView::OnAccessibilityEvent(ax::mojom::Event event) {
     // not activated by default. We need to activate the widget. If other view
     // in message center has focus, it can consume key event. We need to request
     // focus to move it to this content view.
-    Activate();
+    ActivateWidget(true);
     RequestFocus();
   }
 }

ash/system/message_center/arc/arc_notification_content_view.h

@@ -58,10 +58,13 @@ class ArcNotificationContentView
   void OnSlideChanged();
   void OnContainerAnimationStarted();
   void OnContainerAnimationEnded();
+  void ActivateWidget(bool activate);
 
  private:
   friend class ArcNotificationViewTest;
   friend class ArcNotificationContentViewTest;
+  FRIEND_TEST_ALL_PREFIXES(ArcNotificationContentViewTest,
+                           ActivateWhenRemoteInputOpens);
 
   class EventForwarder;
   class MouseEnterExitHandler;
@@ -75,7 +78,6 @@ class ArcNotificationContentView
   void UpdatePreferredSize();
   void UpdateSnapshot();
   void AttachSurface();
-  void Activate();
   void SetExpanded(bool expanded);
   bool IsExpanded() const;
   void SetManuallyExpandedOrCollapsed(bool value);
@@ -106,6 +108,7 @@ class ArcNotificationContentView
 
   // ArcNotificationItem::Observer
   void OnItemDestroying() override;
+  void OnRemoteInputActivationChanged(bool activated) override;
 
   // ArcNotificationSurfaceManager::Observer:
   void OnNotificationSurfaceAdded(ArcNotificationSurface* surface) override;

ash/system/message_center/arc/arc_notification_content_view_unittest.cc

@@ -238,8 +238,9 @@ class ArcNotificationContentViewTest : public AshTestBase {
   ArcNotificationContentView* GetArcNotificationContentView() const {
     return notification_view_->content_view_;
   }
+
   void ActivateArcNotification() {
-    GetArcNotificationContentView()->Activate();
+    GetArcNotificationContentView()->ActivateWidget(true);
   }
 
  private:
@@ -474,7 +475,7 @@ TEST_F(ArcNotificationContentViewTest, Activate) {
   CloseNotificationView();
 }
 
-TEST_F(ArcNotificationContentViewTest, ActivateOnClick) {
+TEST_F(ArcNotificationContentViewTest, NotActivateOnClick) {
   std::string key("notification id");
   auto notification_item = std::make_unique<MockArcNotificationItem>(key);
   auto notification = CreateNotification(notification_item.get());
@@ -486,7 +487,24 @@ TEST_F(ArcNotificationContentViewTest, ActivateOnClick) {
   ui::test::EventGenerator generator(Shell::GetPrimaryRootWindow(),
                                      kNotificationSurfaceBounds.CenterPoint());
   generator.PressLeftButton();
+  EXPECT_EQ(nullptr, GetFocusedWindow());
+
+  CloseNotificationView();
+}
+
+TEST_F(ArcNotificationContentViewTest, ActivateWhenRemoteInputOpens) {
+  std::string key("notification id");
+  auto notification_item = std::make_unique<MockArcNotificationItem>(key);
+  auto notification = CreateNotification(notification_item.get());
+
+  PrepareSurface(key);
+  CreateAndShowNotificationView(notification);
+
+  EXPECT_EQ(nullptr, GetFocusedWindow());
+  GetArcNotificationContentView()->OnRemoteInputActivationChanged(true);
   EXPECT_EQ(surface()->window(), GetFocusedWindow());
+  GetArcNotificationContentView()->OnRemoteInputActivationChanged(false);
+  EXPECT_NE(surface()->window(), GetFocusedWindow());
 
   CloseNotificationView();
 }

ash/system/message_center/arc/arc_notification_item.h

@@ -18,6 +18,10 @@ class ArcNotificationItem {
     // Invoked when the notification data for this item has changed.
     virtual void OnItemDestroying() = 0;
 
+    // Invoked when the remote input textbox on notification is activated or
+    // deactivated.
+    virtual void OnRemoteInputActivationChanged(bool activated) {}
+
    protected:
     virtual ~Observer() = default;
   };
@@ -49,6 +53,10 @@ class ArcNotificationItem {
   // called from ArcNotificationContentView.
   virtual void ToggleExpansion() = 0;
 
+  // Called from ArcNotificationManager when the remote input textbox on
+  // notification is activated or deactivated.
+  virtual void OnRemoteInputActivationChanged(bool activate) = 0;
+
   // Adds an observer.
   virtual void AddObserver(Observer* observer) = 0;
   // Removes the observer.

ash/system/message_center/arc/arc_notification_item_impl.cc

@@ -8,7 +8,9 @@
 #include <vector>
 
 #include "ash/system/message_center/arc/arc_notification_constants.h"
+#include "ash/system/message_center/arc/arc_notification_content_view.h"
 #include "ash/system/message_center/arc/arc_notification_delegate.h"
+#include "ash/system/message_center/arc/arc_notification_view.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "ui/gfx/geometry/size.h"
@@ -193,6 +195,11 @@ void ArcNotificationItemImpl::ToggleExpansion() {
   manager_->SendNotificationToggleExpansionOnChrome(notification_key_);
 }
 
+void ArcNotificationItemImpl::OnRemoteInputActivationChanged(bool activated) {
+  for (auto& observer : observers_)
+    observer.OnRemoteInputActivationChanged(activated);
+}
+
 void ArcNotificationItemImpl::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }

ash/system/message_center/arc/arc_notification_item_impl.h

@@ -39,6 +39,7 @@ class ArcNotificationItemImpl : public ArcNotificationItem {
   void OpenSettings() override;
   void OpenSnooze() override;
   void ToggleExpansion() override;
+  void OnRemoteInputActivationChanged(bool activated) override;
   void AddObserver(Observer* observer) override;
   void RemoveObserver(Observer* observer) override;
   void IncrementWindowRefCount() override;

ash/system/message_center/arc/arc_notification_manager.cc

@@ -166,6 +166,32 @@ void ArcNotificationManager::OnNotificationUpdated(
   if (it == items_.end())
     return;
 
+  bool is_remote_view_focused =
+      (data->remote_input_state ==
+       arc::mojom::ArcNotificationRemoteInputState::OPENED);
+  if (is_remote_view_focused && (previously_focused_notification_key_ != key)) {
+    if (!previously_focused_notification_key_.empty()) {
+      auto prev_it = items_.find(previously_focused_notification_key_);
+      // The case that another remote input is focused. Notify the previously-
+      // focused notification (if any).
+      if (prev_it != items_.end())
+        prev_it->second->OnRemoteInputActivationChanged(false);
+    }
+
+    // Notify the newly-focused notification.
+    previously_focused_notification_key_ = key;
+    it->second->OnRemoteInputActivationChanged(true);
+  } else if (!is_remote_view_focused &&
+             (previously_focused_notification_key_ == key)) {
+    // The case that the previously-focused notification gets unfocused. Notify
+    // the previously-focused notification if the notification still exists.
+    auto it = items_.find(previously_focused_notification_key_);
+    if (it != items_.end())
+      it->second->OnRemoteInputActivationChanged(false);
+
+    previously_focused_notification_key_.clear();
+  }
+
   delegate_->GetAppIdByPackageName(
       data->package_name.value_or(std::string()),
       base::BindOnce(&ArcNotificationManager::OnGotAppId,

ash/system/message_center/arc/arc_notification_manager.h

@@ -84,6 +84,9 @@ class ArcNotificationManager
 
   bool ready_ = false;
 
+  // If any remote input is focused, its key is stored. Otherwise, empty.
+  std::string previously_focused_notification_key_;
+
   std::unique_ptr<InstanceOwner> instance_owner_;
 
   base::WeakPtrFactory<ArcNotificationManager> weak_ptr_factory_{this};

ash/system/message_center/arc/mock_arc_notification_item.h

@@ -43,6 +43,8 @@ class MockArcNotificationItem : public ArcNotificationItem {
   void OpenSnooze() override {}
   void IncrementWindowRefCount() override {}
   void DecrementWindowRefCount() override {}
+  void OnRemoteInputActivationChanged(bool activate) override {}
+
   arc::mojom::ArcNotificationType GetNotificationType() const override;
   arc::mojom::ArcNotificationExpandState GetExpandState() const override;
   arc::mojom::ArcNotificationShownContents GetShownContents() const override;