[Phone Hub] Replace Phone Hub onboarding dismiss page with context menu

This CL deletes Dismiss button and Dismiss prompt page of onboarding
step and add a context menu to the Phone Hub icon when right-clicked to
hide the button.

Screenshot: https://screenshot.googleplex.com/9ruqgBRc2qxJNW7.png
Screen recording: https://screencast.googleplex.com/cast/NDYyODQwNjcxNjA3MTkzNnw5M2ZlMWVlZi1jNw

a right-click and Phone Hub is at onboarding state. See screenshot and
screen recording.

Test: deployed to DUT and verified that context menu only appears by
Change-Id: I9610be3e32add580d0def5a4a2e1c4c809e7e5e8
Fixed: b/307811923
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5105041
Reviewed-by: Jon Mann <jonmann@chromium.org>
Commit-Queue: Pu Shi <pushi@google.com>
Cr-Commit-Position: refs/heads/main@{#1237224}

ash/ash_strings.grd

@@ -2042,6 +2042,9 @@ Style notes:
       <message name="IDS_ASH_PHONE_HUB_NOTIFICATION_ACCESSIBLE_NAME" desc="Accessible name for notification received in Phone Hub from paired phone, containing app_name, notification_title, message, phone_name.">
         <ph name="APP_NAME">$1<ex>Chat</ex></ph>, <ph name="NOTIFICATION_TITLE">$2<ex>+1-111-111-1111</ex></ph>: <ph name="MESSAGE">$3<ex>Hello</ex></ph>, <ph name="PHONE_NAME">$4<ex>Pixel</ex></ph>
       </message>
+      <message name="IDS_ASH_PHONE_HUB_TRAY_ICON_DISMISS_TEXT" desc="The text that appears in the context menu when right click Phone Hub icon in the tray to dismiss the icon if feature is not set up.">
+        Remove Phone Hub from shelf
+      </message>
       <message name="IDS_ASH_STYLUS_BATTERY_LOW_LABEL" desc="The label next to the stylus battery indicator when the battery level is below 24%">
         Low
       </message>

ash/ash_strings_grd/IDS_ASH_PHONE_HUB_TRAY_ICON_DISMISS_TEXT.png.sha1

@@ -0,0 +1 @@
+799b55918ba242cd0ca425b12e13f7687d78772f

ash/system/phonehub/onboarding_view.cc

@@ -78,19 +78,6 @@ class OnboardingMainView : public PhoneHubInterstitialView {
     SetDescription(l10n_util::GetStringUTF16(
         IDS_ASH_PHONE_HUB_ONBOARDING_DIALOG_DESCRIPTION));
 
-    // Add "Dismiss" and "Get started" buttons.
-    // TODO(b/281844561): Migrate the "Dismiss" button to use
-    // |PillButton::Type::kSecondaryWithoutIcon| when the PillButton colors
-    // are updated with better contrast-ratios.
-    auto dismiss = std::make_unique<PillButton>(
-        base::BindRepeating(&OnboardingMainView::DismissButtonPressed,
-                            base::Unretained(this)),
-        l10n_util::GetStringUTF16(
-            IDS_ASH_PHONE_HUB_ONBOARDING_DIALOG_DISMISS_BUTTON),
-        PillButton::Type::kFloatingWithoutIcon, /*icon=*/nullptr);
-    dismiss->SetID(PhoneHubViewID::kOnboardingDismissButton);
-    AddButton(std::move(dismiss));
-
     auto get_started = std::make_unique<PillButton>(
         base::BindRepeating(&OnboardingMainView::GetStartedButtonPressed,
                             base::Unretained(this)),
@@ -110,11 +97,6 @@ class OnboardingMainView : public PhoneHubInterstitialView {
         parent_view_->IsOnboardingViewStartedFromNudge());
   }
 
-  void DismissButtonPressed() {
-    LogInterstitialScreenEvent(InterstitialScreenEvent::kDismiss);
-    parent_view_->ShowDismissPrompt();
-  }
-
   raw_ptr<phonehub::OnboardingUiTracker, ExperimentalAsh>
       onboarding_ui_tracker_ = nullptr;
   raw_ptr<OnboardingView, ExperimentalAsh> parent_view_ = nullptr;
@@ -124,78 +106,6 @@ class OnboardingMainView : public PhoneHubInterstitialView {
 BEGIN_METADATA(OnboardingMainView)
 END_METADATA
 
-// OnboardingDismissPromptView ------------------------------------------------
-// A follow-up prompt screen that pops up when the user has chosen to dismiss
-// the main onboarding screen. It should not be shown again after being
-// dismissed manually by either clicking the ack button or outside the bubble.
-class OnboardingDismissPromptView : public PhoneHubInterstitialView {
-  METADATA_HEADER(OnboardingDismissPromptView, PhoneHubInterstitialView)
-
- public:
-  explicit OnboardingDismissPromptView(
-      phonehub::OnboardingUiTracker* onboarding_ui_tracker)
-      : PhoneHubInterstitialView(/*show_progress=*/false, /*show_image=*/false),
-        onboarding_ui_tracker_(onboarding_ui_tracker) {
-    SetID(PhoneHubViewID::kOnboardingDismissPromptView);
-    InitLayout();
-  }
-
- private:
-  void InitLayout() {
-    // Adds title and description.
-    SetTitle(l10n_util::GetStringUTF16(
-        IDS_ASH_PHONE_HUB_ONBOARDING_DISMISS_DIALOG_TITLE));
-    std::u16string part1 = l10n_util::GetStringUTF16(
-        IDS_ASH_PHONE_HUB_ONBOARDING_DISMISS_DIALOG_DESCRIPTION_PART_1);
-    std::u16string part2 = l10n_util::GetStringUTF16(
-        IDS_ASH_PHONE_HUB_ONBOARDING_DISMISS_DIALOG_DESCRIPTION_PART_2);
-    // Uses "\n" to create a newline separator between two text paragraphs.
-    SetDescription(base::StrCat({part1, u"\n\n", part2}));
-
-    // Adds "Ok, got it" button.
-    auto ack_button = std::make_unique<PillButton>(
-        base::BindRepeating(&OnboardingDismissPromptView::ButtonPressed,
-                            base::Unretained(this)),
-        l10n_util::GetStringUTF16(
-            IDS_ASH_PHONE_HUB_ONBOARDING_DISMISS_DIALOG_OK_BUTTON),
-        PillButton::Type::kDefaultWithoutIcon, /*icon=*/nullptr);
-    ack_button->SetID(PhoneHubViewID::kOnboardingDismissAckButton);
-    AddButton(std::move(ack_button));
-  }
-
-  void ButtonPressed() {
-    LogInterstitialScreenEvent(InterstitialScreenEvent::kConfirm);
-
-    // Close Phone Hub bubble in current display.
-    views::Widget* const widget = GetWidget();
-    // |widget| is null when this function is called before the view is added to
-    // a widget (in unit tests).
-    if (!widget)
-      return;
-    int64_t current_display_id =
-        display::Screen::GetScreen()
-            ->GetDisplayNearestWindow(widget->GetNativeWindow())
-            .id();
-    Shell::GetRootWindowControllerWithDisplayId(current_display_id)
-        ->GetStatusAreaWidget()
-        ->phone_hub_tray()
-        ->CloseBubble();
-  }
-
-  // PhoneHubInterstitialView:
-  void OnBubbleClose() override { onboarding_ui_tracker_->DismissSetupUi(); }
-
-  Screen GetScreenForMetrics() const override {
-    return Screen::kOnboardingDismissPrompt;
-  }
-
-  raw_ptr<phonehub::OnboardingUiTracker, ExperimentalAsh>
-      onboarding_ui_tracker_ = nullptr;
-};
-
-BEGIN_METADATA(OnboardingDismissPromptView)
-END_METADATA
-
 // OnboardingView -------------------------------------------------------------
 OnboardingView::OnboardingView(
     phonehub::OnboardingUiTracker* onboarding_ui_tracker,
@@ -221,20 +131,6 @@ Screen OnboardingView::GetScreenForMetrics() const {
   return main_view_->GetScreenForMetrics();
 }
 
-void OnboardingView::ShowDismissPrompt() {
-  DCHECK(main_view_);
-
-  LogInterstitialScreenEvent(InterstitialScreenEvent::kShown);
-
-  RemoveChildViewT(main_view_.get());
-  main_view_ = AddChildView(
-      std::make_unique<OnboardingDismissPromptView>(onboarding_ui_tracker_));
-
-  // We don't show status header view on top for the dismiss prompt.
-  DCHECK(delegate_);
-  delegate_->HideStatusHeaderView();
-}
-
 bool OnboardingView::IsOnboardingViewStartedFromNudge() {
   if (!delegate_) {
     return false;

ash/system/phonehub/phone_hub_tray.cc

@@ -39,9 +39,11 @@
 #include "ash/system/tray/tray_utils.h"
 #include "base/functional/bind.h"
 #include "base/functional/callback_helpers.h"
+#include "base/notreached.h"
 #include "base/power_monitor/power_monitor.h"
 #include "base/task/sequenced_task_runner.h"
 #include "base/time/default_clock.h"
+#include "chromeos/ash/components/multidevice/logging/logging.h"
 #include "chromeos/ash/components/phonehub/icon_decoder.h"
 #include "chromeos/ash/components/phonehub/phone_hub_manager.h"
 #include "chromeos/ash/components/phonehub/phone_model.h"
@@ -51,6 +53,7 @@
 #include "ui/base/models/image_model.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/chromeos/styles/cros_tokens_color_mappings.h"
+#include "ui/color/color_id.h"
 #include "ui/events/event.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/paint_vector_icon.h"
@@ -62,12 +65,17 @@ namespace ash {
 
 namespace {
 
+// Command ID for Phone Hub context menu
+constexpr int kHidePhoneHubIconCommandId = 1;
+
 // Padding for tray icons (dp; the button that shows the phone_hub menu).
 constexpr int kTrayIconMainAxisInset = 6;
 constexpr int kTrayIconCrossAxisInset = 0;
 constexpr int kEcheIconMinSize = 24;
 constexpr int kIconSpacing = 12;
 
+constexpr int kHidePhoneHubContexMenuIconSize = 20;
+
 constexpr auto kBubblePadding =
     gfx::Insets::TLBR(0, 0, kBubbleBottomPaddingDip, 0);
 
@@ -473,10 +481,37 @@ void PhoneHubTray::UpdateVisibility() {
       !IsInsideUnlockWindow()) {
     return;
   }
+  icon_->set_context_menu_controller(
+      ui_state == PhoneHubUiController::UiState::kOnboardingWithPhone ||
+              ui_state == PhoneHubUiController::UiState::kOnboardingWithoutPhone
+          ? this
+          : nullptr);
+
   SetVisiblePreferred(ui_state != PhoneHubUiController::UiState::kHidden &&
                       IsInUserSession());
 }
 
+std::unique_ptr<ui::SimpleMenuModel> PhoneHubTray::CreateContextMenuModel() {
+  auto context_menu_model = std::make_unique<ui::SimpleMenuModel>(this);
+
+  context_menu_model->AddItemWithIcon(
+      kHidePhoneHubIconCommandId,
+      l10n_util::GetStringUTF16(IDS_ASH_PHONE_HUB_TRAY_ICON_DISMISS_TEXT),
+      ui::ImageModel::FromVectorIcon(kVisibilityOffIcon,
+                                     ui::kColorAshSystemUIMenuIcon,
+                                     kHidePhoneHubContexMenuIconSize));
+
+  return context_menu_model;
+}
+
+void PhoneHubTray::ExecuteCommand(int command_id, int event_flags) {
+  if (command_id == kHidePhoneHubIconCommandId) {
+    phone_hub_manager_->GetOnboardingUiTracker()->DismissSetupUi();
+    return;
+  }
+  NOTREACHED();
+}
+
 void PhoneHubTray::UpdateHeaderVisibility() {
   if (!features::IsEcheLauncherEnabled())
     return;

ash/system/phonehub/phone_hub_tray.h

@@ -25,6 +25,7 @@
 #include "chromeos/ash/components/phonehub/icon_decoder.h"
 #include "chromeos/ash/components/phonehub/phone_hub_manager.h"
 #include "ui/base/metadata/metadata_header_macros.h"
+#include "ui/base/models/simple_menu_model.h"
 #include "ui/events/event.h"
 #include "ui/views/controls/button/image_button.h"
 
@@ -52,6 +53,7 @@ class ASH_EXPORT PhoneHubTray : public TrayBackgroundView,
                                 public OnboardingView::Delegate,
                                 public PhoneStatusView::Delegate,
                                 public PhoneHubUiController::Observer,
+                                public ui::SimpleMenuModel::Delegate,
                                 public SessionObserver,
                                 public WindowTreeHostManager::Observer,
                                 public phonehub::AppStreamManager::Observer {
@@ -76,6 +78,7 @@ class ASH_EXPORT PhoneHubTray : public TrayBackgroundView,
   void Initialize() override;
   void CloseBubble() override;
   void ShowBubble() override;
+  std::unique_ptr<ui::SimpleMenuModel> CreateContextMenuModel() override;
   TrayBubbleView* GetBubbleView() override;
   views::Widget* GetBubbleWidget() const override;
   const char* GetClassName() const override;
@@ -143,6 +146,9 @@ class ASH_EXPORT PhoneHubTray : public TrayBackgroundView,
   void OnSessionStateChanged(session_manager::SessionState state) override;
   void OnActiveUserSessionChanged(const AccountId& account_id) override;
 
+  // Ui::SimpleMenuModel::Delegate:
+  void ExecuteCommand(int command_id, int event_flags) override;
+
   // Updates the visibility of the tray in the shelf based on the feature is
   // enabled.
   void UpdateVisibility();

ash/system/phonehub/phone_hub_tray_unittest.cc

@@ -35,6 +35,8 @@
 #include "ui/events/event.h"
 #include "ui/events/keycodes/keyboard_codes_posix.h"
 #include "ui/views/controls/button/button.h"
+#include "ui/views/controls/menu/menu_controller.h"
+#include "ui/views/controls/menu/menu_item_view.h"
 
 namespace ash {
 
@@ -153,26 +155,11 @@ class PhoneHubTrayTest : public AshTestBase {
     return bubble_view()->GetViewByID(PhoneHubViewID::kOnboardingMainView);
   }
 
-  views::View* onboarding_dismiss_prompt_view() {
-    return bubble_view()->GetViewByID(
-        PhoneHubViewID::kOnboardingDismissPromptView);
-  }
-
   views::Button* onboarding_get_started_button() {
     return static_cast<views::Button*>(bubble_view()->GetViewByID(
         PhoneHubViewID::kOnboardingGetStartedButton));
   }
 
-  views::Button* onboarding_dismiss_button() {
-    return static_cast<views::Button*>(
-        bubble_view()->GetViewByID(PhoneHubViewID::kOnboardingDismissButton));
-  }
-
-  views::Button* onboarding_dismiss_ack_button() {
-    return static_cast<views::Button*>(bubble_view()->GetViewByID(
-        PhoneHubViewID::kOnboardingDismissAckButton));
-  }
-
   views::Button* disconnected_refresh_button() {
     return static_cast<views::Button*>(
         bubble_view()->GetViewByID(PhoneHubViewID::kDisconnectedRefreshButton));
@@ -560,27 +547,19 @@ TEST_F(PhoneHubTrayTest, StartOnboardingFlow) {
   EXPECT_EQ(1u, GetOnboardingUiTracker()->handle_get_started_call_count());
 }
 
-TEST_F(PhoneHubTrayTest, DismissOnboardingFlowByClickingAckButton) {
+TEST_F(PhoneHubTrayTest, DismissOnboardingFlowByRightClickIcon) {
   // Simulate a pending setup state to show the onboarding screen.
   GetFeatureStatusProvider()->SetStatus(
       phonehub::FeatureStatus::kEligiblePhoneButNotSetUp);
   GetOnboardingUiTracker()->SetShouldShowOnboardingUi(true);
 
-  ClickTrayButton();
-  EXPECT_TRUE(phone_hub_tray_->is_active());
-  EXPECT_EQ(PhoneHubViewID::kOnboardingView, content_view()->GetID());
-  // It should display the onboarding main view at first.
-  EXPECT_TRUE(onboarding_main_view());
-
-  // Simulate a click on the "Dismiss" button.
-  LeftClickOn(onboarding_dismiss_button());
-
-  // It should transit to show the dismiss prompt.
-  EXPECT_TRUE(onboarding_dismiss_prompt_view());
-  EXPECT_TRUE(onboarding_dismiss_prompt_view()->GetVisible());
-
-  // Simulate a click on the "OK, got it" button to ack.
-  LeftClickOn(onboarding_dismiss_ack_button());
+  RightClickOn(phone_hub_tray_);
+  EXPECT_TRUE(views::MenuController::GetActiveInstance());
+  views::MenuItemView* menu_item_view =
+      views::MenuController::GetActiveInstance()
+          ->GetSelectedMenuItem()
+          ->GetMenuItemByID(/*kHidePhoneHubIconCommandId*/ 1);
+  LeftClickOn(menu_item_view);
 
   // Clicking "Ok, got it" button should dismiss the bubble, hide the tray icon,
   // and disable the ability to show onboarding UI again.
@@ -589,35 +568,6 @@ TEST_F(PhoneHubTrayTest, DismissOnboardingFlowByClickingAckButton) {
   EXPECT_FALSE(GetOnboardingUiTracker()->ShouldShowOnboardingUi());
 }
 
-TEST_F(PhoneHubTrayTest, DismissOnboardingFlowByClickingOutside) {
-  // Simulate a pending setup state to show the onboarding screen.
-  GetFeatureStatusProvider()->SetStatus(
-      phonehub::FeatureStatus::kEligiblePhoneButNotSetUp);
-  GetOnboardingUiTracker()->SetShouldShowOnboardingUi(true);
-
-  ClickTrayButton();
-  EXPECT_TRUE(phone_hub_tray_->is_active());
-  EXPECT_EQ(PhoneHubViewID::kOnboardingView, content_view()->GetID());
-  // It should display the onboarding main view at first.
-  EXPECT_TRUE(onboarding_main_view());
-
-  // Simulate a click on the "Dismiss" button.
-  LeftClickOn(onboarding_dismiss_button());
-
-  // It should transit to show the dismiss prompt.
-  EXPECT_TRUE(onboarding_dismiss_prompt_view());
-  EXPECT_TRUE(onboarding_dismiss_prompt_view()->GetVisible());
-
-  // Simulate a click outside the bubble.
-  phone_hub_tray_->ClickedOutsideBubble();
-
-  // Clicking outside should dismiss the bubble, hide the tray icon, and disable
-  // the ability to show onboarding UI again.
-  EXPECT_FALSE(phone_hub_tray_->GetBubbleView());
-  EXPECT_FALSE(phone_hub_tray_->GetVisible());
-  EXPECT_FALSE(GetOnboardingUiTracker()->ShouldShowOnboardingUi());
-}
-
 TEST_F(PhoneHubTrayTest, ShouldNotShowMiniLauncherOnCloseBubble) {
   GetFeatureStatusProvider()->SetStatus(
       phonehub::FeatureStatus::kEnabledAndConnected);

ash/system/phonehub/phone_hub_view_ids.h

@@ -28,9 +28,6 @@ enum PhoneHubViewID {
   kOnboardingView,
   kOnboardingMainView,
   kOnboardingGetStartedButton,
-  kOnboardingDismissButton,
-  kOnboardingDismissPromptView,
-  kOnboardingDismissAckButton,
 
   // Phone disconnected view and its components.
   kDisconnectedView,