snap-group: Implement feedback button callback and cursor update

This cl does the following:
1. Implemented the callback to open feedback form;
2. Updated the cursor update logic so that the cursor will change
to default when hovering on the feedback button;
3. Apply the correct color on the feedback button.

Demo: http://b/328636577#comment2
Fixed: b/328636577
Test: Manual + added new
Change-Id: I7d02d10864c1dc1ba7169add2e5913f2419d8de2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5360075
Commit-Queue: Michele Fan <michelefan@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1271669}

ash/shell_delegate.h

@@ -59,6 +59,7 @@ class ASH_EXPORT ShellDelegate {
     kFocusMode,
     kGameDashboard,
     kOverview,
+    kSnapGroups,
     kWindowLayoutMenu,
   };
 

ash/test_shell_delegate.cc

@@ -180,6 +180,13 @@ base::FilePath TestShellDelegate::GetPrimaryUserDownloadsFolder() const {
   return base::FilePath();
 }
 
+void TestShellDelegate::OpenFeedbackDialog(
+    ShellDelegate::FeedbackSource source,
+    const std::string& description_template,
+    const std::string& category_tag) {
+  ++open_feedback_dialog_call_count_;
+}
+
 const GURL& TestShellDelegate::GetLastCommittedURLForWindowIfAny(
     aura::Window* window) {
   return last_committed_url_;

ash/test_shell_delegate.h

@@ -29,6 +29,10 @@ class TestShellDelegate : public ShellDelegate {
 
   ~TestShellDelegate() override;
 
+  int open_feedback_dialog_call_count() const {
+    return open_feedback_dialog_call_count_;
+  }
+
   // Allows tests to override the MultiDeviceSetup binding behavior for this
   // TestShellDelegate.
   using MultiDeviceSetupBinder = base::RepeatingCallback<void(
@@ -102,7 +106,7 @@ class TestShellDelegate : public ShellDelegate {
   base::FilePath GetPrimaryUserDownloadsFolder() const override;
   void OpenFeedbackDialog(FeedbackSource source,
                           const std::string& description_template,
-                          const std::string& category_tag) override {}
+                          const std::string& category_tag) override;
   void OpenProfileManager() override {}
   void SetLastCommittedURLForWindow(const GURL& url);
   version_info::Channel GetChannel() override;
@@ -143,6 +147,8 @@ class TestShellDelegate : public ShellDelegate {
   version_info::Channel channel_ = version_info::Channel::UNKNOWN;
 
   std::string version_string_;
+
+  int open_feedback_dialog_call_count_ = 0;
 };
 
 }  // namespace ash

ash/wm/snap_group/snap_group_unittest.cc

@@ -19,11 +19,13 @@
 #include "ash/root_window_controller.h"
 #include "ash/screen_util.h"
 #include "ash/shell.h"
+#include "ash/shell_delegate.h"
 #include "ash/style/close_button.h"
 #include "ash/style/system_toast_style.h"
 #include "ash/system/toast/toast_manager_impl.h"
 #include "ash/test/ash_test_base.h"
 #include "ash/test/ash_test_util.h"
+#include "ash/test_shell_delegate.h"
 #include "ash/wm/desks/desks_controller.h"
 #include "ash/wm/desks/desks_test_util.h"
 #include "ash/wm/desks/desks_util.h"
@@ -1414,7 +1416,7 @@ TEST_F(FasterSplitScreenTest, NoCrashWhenDoubleTapAfterTransition) {
   SwitchToTabletMode();
   EXPECT_TRUE(split_view_divider()->divider_widget());
 
-  // Double tap on the divider-> This will start a drag and notify
+  // Double tap on the divider. This will start a drag and notify
   // SplitViewOverviewSession.
   const gfx::Point divider_center =
       split_view_divider()
@@ -2211,7 +2213,7 @@ TEST_F(SnapGroupTest, RemoveDisplay) {
 }
 
 // Tests the snap ratio is updated correctly when resizing the windows in a snap
-// group with the split view divider->
+// group with the split view divider.
 TEST_F(SnapGroupTest, SnapRatioTest) {
   std::unique_ptr<aura::Window> w1(CreateTestWindow());
   std::unique_ptr<aura::Window> w2(CreateTestWindow());
@@ -2234,7 +2236,7 @@ TEST_F(SnapGroupTest, SnapRatioTest) {
 }
 
 // Tests that the windows in a snap group can be resized to an arbitrary
-// location with the split view divider->
+// location with the split view divider.
 TEST_F(SnapGroupTest, ResizeWithSplitViewDividerToArbitraryLocations) {
   std::unique_ptr<aura::Window> w1(CreateTestWindow());
   std::unique_ptr<aura::Window> w2(CreateTestWindow());
@@ -3956,11 +3958,41 @@ TEST_F(SnapGroupTest, ClamshellTabletTransitionGetClosestFixedRatio) {
   }
 }
 
+TEST_F(SnapGroupTest, FeedbackButtonTest) {
+  std::unique_ptr<aura::Window> w1(CreateAppWindow());
+  std::unique_ptr<aura::Window> w2(CreateAppWindow());
+  SnapTwoTestWindows(w1.get(), w2.get(), /*horizontal=*/true);
+
+  SplitViewDividerView* divider_view =
+      split_view_divider()->divider_view_for_testing();
+  auto* feedback_button = divider_view->feedback_button_for_testing();
+  EXPECT_TRUE(feedback_button);
+
+  // Verify that the feedback button is insivible by default.
+  EXPECT_FALSE(feedback_button->GetVisible());
+
+  // Test that the feedback button becomes visible upon hover on the divider.
+  gfx::Point hover_location =
+      split_view_divider_bounds_in_screen().CenterPoint();
+  hover_location.Offset(0, -10);
+
+  auto* event_generator = GetEventGenerator();
+  event_generator->MoveMouseTo(hover_location);
+  EXPECT_TRUE(feedback_button->GetVisible());
+
+  // Test that open feedback dialog callback will be triggered.
+  event_generator->MoveMouseTo(
+      feedback_button->GetBoundsInScreen().CenterPoint());
+  event_generator->ClickLeftButton();
+  EXPECT_EQ(1, static_cast<TestShellDelegate*>(Shell::Get()->shell_delegate())
+                   ->open_feedback_dialog_call_count());
+}
+
 // Tests that the cursor type gets updated to be resize cursor on mouse hovering
-// on the split view divider->
+// on the split view divider excluding the feedback button.
 TEST_F(SnapGroupTest, CursorUpdateTest) {
-  std::unique_ptr<aura::Window> w1(CreateTestWindow());
-  std::unique_ptr<aura::Window> w2(CreateTestWindow());
+  std::unique_ptr<aura::Window> w1(CreateAppWindow());
+  std::unique_ptr<aura::Window> w2(CreateAppWindow());
   SnapTwoTestWindows(w1.get(), w2.get(), /*horizontal=*/true);
   auto* divider = split_view_divider();
   ASSERT_TRUE(divider->divider_widget());
@@ -3974,7 +4006,7 @@ TEST_F(SnapGroupTest, CursorUpdateTest) {
   cursor_manager->SetCursor(CursorType::kPointer);
 
   // Test that the default cursor type when mouse is not hovered over the split
-  // view divider->
+  // view divider.
   auto* event_generator = GetEventGenerator();
   event_generator->MoveMouseTo(outside_point);
   EXPECT_TRUE(cursor_manager->IsCursorVisible());
@@ -3982,7 +4014,7 @@ TEST_F(SnapGroupTest, CursorUpdateTest) {
   EXPECT_EQ(CursorType::kNull, cursor_manager->GetCursor().type());
 
   // Test that the cursor changed to resize cursor while hovering over the split
-  // view divider->
+  // view divider.
   const auto delta_vector = gfx::Vector2d(0, -10);
   const gfx::Point cached_hover_point =
       divider_bounds.CenterPoint() + delta_vector;
@@ -3997,6 +4029,17 @@ TEST_F(SnapGroupTest, CursorUpdateTest) {
   EXPECT_EQ(CursorType::kColumnResize, cursor_manager->GetCursor().type());
   EXPECT_EQ(split_view_divider_bounds_in_screen().CenterPoint() + delta_vector,
             cached_hover_point + move_vector);
+
+  // Test that when hovering over the feedback button, the cursor type changed
+  // back to the default type.
+  SplitViewDividerView* divider_view =
+      split_view_divider()->divider_view_for_testing();
+  auto* feedback_button = divider_view->feedback_button_for_testing();
+  EXPECT_TRUE(feedback_button);
+  event_generator->MoveMouseTo(divider_view->feedback_button_for_testing()
+                                   ->GetBoundsInScreen()
+                                   .CenterPoint());
+  EXPECT_EQ(CursorType::kNull, cursor_manager->GetCursor().type());
 }
 
 // -----------------------------------------------------------------------------

ash/wm/splitview/split_view_divider.h

@@ -133,6 +133,8 @@ class ASH_EXPORT SplitViewDivider : public aura::WindowObserver,
   void OnTransientChildRemoved(aura::Window* window,
                                aura::Window* transient) override;
 
+  SplitViewDividerView* divider_view_for_testing() { return divider_view_; }
+
  private:
   friend class SplitViewController;
 

ash/wm/splitview/split_view_divider_view.cc

@@ -8,6 +8,7 @@
 #include "ash/public/cpp/window_properties.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/shell.h"
+#include "ash/shell_delegate.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/style/icon_button.h"
 #include "ash/utility/cursor_setter.h"
@@ -133,12 +134,14 @@ void SplitViewDividerView::OnMouseEntered(const ui::MouseEvent& event) {
   gfx::Point screen_location = event.location();
   ConvertPointToScreen(this, &screen_location);
 
-  // Set cursor type as the resize cursor when it's on the split view divider.
-  cursor_setter_.UpdateCursor(GetWidget()->GetNativeWindow()->GetRootWindow(),
-                              ui::mojom::CursorType::kColumnResize);
-
-  // Show `feedback_button_` on mouse entered.
-  RefreshFeedbackButton(/*visible=*/true);
+  if (!feedback_button_ ||
+      !feedback_button_->GetBoundsInScreen().Contains(screen_location)) {
+    // Set cursor type as the resize cursor when it's on the split view divider.
+    cursor_setter_.UpdateCursor(GetWidget()->GetNativeWindow()->GetRootWindow(),
+                                ui::mojom::CursorType::kColumnResize);
+    // Show `feedback_button_` on mouse entered.
+    RefreshFeedbackButton(/*visible=*/true);
+  }
 }
 
 void SplitViewDividerView::OnMouseExited(const ui::MouseEvent& event) {
@@ -146,8 +149,13 @@ void SplitViewDividerView::OnMouseExited(const ui::MouseEvent& event) {
   // exit `this` the cursor will be reset.
   cursor_setter_.ResetCursor();
 
+  gfx::Point screen_location = event.location();
+  ConvertPointToScreen(this, &screen_location);
   // Hide `feedback_button_` on mouse exited.
-  RefreshFeedbackButton(/*visible=*/false);
+  if (feedback_button_ &&
+      !feedback_button_->GetBoundsInScreen().Contains(screen_location)) {
+    RefreshFeedbackButton(/*visible=*/false);
+  }
 }
 
 bool SplitViewDividerView::OnMousePressed(const ui::MouseEvent& event) {
@@ -284,8 +292,7 @@ void SplitViewDividerView::RefreshFeedbackButton(bool visible) {
     feedback_button_->SetPaintToLayer();
     feedback_button_->layer()->SetFillsBoundsOpaquely(false);
     feedback_button_->SetPreferredSize(kFeedbackButtonSize);
-    // TODO(michelefan): Apply `cros.sys.inverse-whiteblack` as the icon color
-    // currently it's not available in the system yet.
+    feedback_button_->SetIconColor(cros_tokens::kCrosSysInverseWhiteblack);
     feedback_button_->SetVisible(true);
     feedback_button_->SetBackground(views::CreateThemedRoundedRectBackground(
         cros_tokens::kCrosSysSystemBaseElevated,
@@ -311,8 +318,10 @@ void SplitViewDividerView::EndResizing(gfx::Point location, bool swap_windows) {
 }
 
 void SplitViewDividerView::OnFeedbackButtonPressed() {
-  // TODO(michelefan): Implement the callback for the feedback button.
-  base::DoNothing();
+  Shell::Get()->shell_delegate()->OpenFeedbackDialog(
+      /*source=*/ShellDelegate::FeedbackSource::kSnapGroups,
+      /*description_template=*/std::string(),
+      /*category_tag=*/"FromSnapGroups");
 }
 
 BEGIN_METADATA(SplitViewDividerView)

ash/wm/splitview/split_view_divider_view.h

@@ -49,6 +49,8 @@ class SplitViewDividerView : public views::View,
   bool DoesIntersectRect(const views::View* target,
                          const gfx::Rect& rect) const override;
 
+  IconButton* feedback_button_for_testing() const { return feedback_button_; }
+
  private:
   void SwapWindows();
 
@@ -79,6 +81,7 @@ class SplitViewDividerView : public views::View,
   raw_ptr<SplitViewDivider, DanglingUntriaged> divider_;
 
   // Securely updates the cursor.
+  // TODO(michelefan): Consider overriding `View::GetCursor`.
   CursorSetter cursor_setter_;
 
   raw_ptr<IconButton> feedback_button_ = nullptr;

chrome/browser/ui/ash/chrome_shell_delegate.cc

@@ -130,6 +130,8 @@ chrome::FeedbackSource ToChromeFeedbackSource(
       return chrome::FeedbackSource::kFeedbackSourceGameDashboard;
     case ash::ShellDelegate::FeedbackSource::kOverview:
       return chrome::FeedbackSource::kFeedbackSourceOverview;
+    case ash::ShellDelegate::FeedbackSource::kSnapGroups:
+      return chrome::FeedbackSource::kFeedbackSourceSnapGroups;
     case ash::ShellDelegate::FeedbackSource::kWindowLayoutMenu:
       return chrome::FeedbackSource::kFeedbackSourceWindowLayoutMenu;
   }

chrome/browser/ui/chrome_pages.h

@@ -128,6 +128,7 @@ enum FeedbackSource {
   kFeedbackSourceAI,
   kFeedbackSourceFocusMode,
   kFeedbackSourceOverview,
+  kFeedbackSourceSnapGroups,
 
   // ATTENTION: Before making any changes or adding to feedback collection,
   // please ensure the teams that operationalize feedback are aware and

tools/metrics/histograms/enums.xml

@@ -11694,6 +11694,7 @@ Called by update_permissions_policy_enum.py.-->
   <int value="37" label="AI"/>
   <int value="38" label="Focus Mode"/>
   <int value="39" label="Overview Grid"/>
+  <int value="40" label="Snap Groups"/>
 </enum>
 
 <!-- Keep elements in sync with components/feed/core/feed_logging_metrics.cc and