Removed reliance on HoverButton from CastDialogNoSinksView

Currently CastDialogNoSinksView is implemented using a HoverButton
and plays with the button's internal implementation in a way that
doesn't make sense from a UX/a11y standpoint.

Refactored the view to eliminate the need for HoverButtons, cleaned
up the code, updated its tests.

Bug: 1060448
Change-Id: Idf86635c5241367c6c9a4d1615a8c01de6ec4121
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2099104
Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
Reviewed-by: Takumi Fujimoto <takumif@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756883}

chrome/browser/ui/views/media_router/cast_dialog_no_sinks_view.cc

@@ -11,10 +11,12 @@
 #include "base/strings/string16.h"
 #include "base/task/post_task.h"
 #include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "chrome/app/vector_icons/vector_icons.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
+#include "chrome/browser/ui/views/chrome_layout_provider.h"
 #include "chrome/browser/ui/views/hover_button.h"
 #include "chrome/browser/ui/views/media_router/cast_dialog_helper.h"
 #include "chrome/common/url_constants.h"
@@ -26,81 +28,79 @@
 #include "ui/base/page_transition_types.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/paint_vector_icon.h"
-#include "ui/native_theme/native_theme.h"
 #include "ui/views/controls/button/image_button.h"
+#include "ui/views/controls/button/image_button_factory.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/throbber.h"
 #include "ui/views/layout/box_layout.h"
+#include "ui/views/view_class_properties.h"
 #include "url/gurl.h"
 
+namespace {
+
+auto CreateHelpIcon(views::ButtonListener* listener) {
+  auto help_icon = views::CreateVectorImageButtonWithNativeTheme(
+      listener, vector_icons::kHelpOutlineIcon);
+  help_icon->SetInstallFocusRingOnFocus(true);
+  help_icon->SetFocusForPlatform();
+  help_icon->SetBorder(
+      views::CreateEmptyBorder(media_router::kPrimaryIconBorder));
+  help_icon->SetAccessibleName(
+      l10n_util::GetStringUTF16(IDS_MEDIA_ROUTER_NO_DEVICES_FOUND_BUTTON));
+  help_icon->SetInkDropMode(views::InkDropHostView::InkDropMode::OFF);
+  return help_icon;
+}
+
+}  // namespace
+
 namespace media_router {
 
+constexpr base::TimeDelta CastDialogNoSinksView::kSearchWaitTime;
+
 CastDialogNoSinksView::CastDialogNoSinksView(Profile* profile)
     : profile_(profile) {
-  SetLayoutManager(std::make_unique<views::BoxLayout>(
-      views::BoxLayout::Orientation::kVertical));
-  looking_for_sinks_view_ = CreateLookingForSinksView();
-  AddChildView(looking_for_sinks_view_);
-
-  base::PostDelayedTask(FROM_HERE, {content::BrowserThread::UI},
-                        base::BindOnce(&CastDialogNoSinksView::ShowHelpIconView,
-                                       weak_factory_.GetWeakPtr()),
-                        base::TimeDelta::FromSeconds(3));
+  // Use horizontal button padding to ensure consistent spacing with the
+  // CastDialogView and its sink views that are implemented as Buttons.
+  const int horizontal_padding = ChromeLayoutProvider::Get()->GetDistanceMetric(
+      views::DISTANCE_BUTTON_HORIZONTAL_PADDING);
+
+  // Maintain required padding between the throbber / icon and the label.
+  const int icon_label_spacing = ChromeLayoutProvider::Get()->GetDistanceMetric(
+      views::DISTANCE_RELATED_LABEL_HORIZONTAL);
+
+  auto* layout_manager = SetLayoutManager(std::make_unique<views::BoxLayout>(
+      views::BoxLayout::Orientation::kHorizontal,
+      gfx::Insets(0, horizontal_padding), icon_label_spacing));
+  layout_manager->set_cross_axis_alignment(
+      views::BoxLayout::CrossAxisAlignment::kCenter);
+
+  icon_ = AddChildView(CreateThrobber());
+  label_ = AddChildView(std::make_unique<views::Label>(
+      l10n_util::GetStringUTF16(IDS_MEDIA_ROUTER_STATUS_LOOKING_FOR_DEVICES)));
+
+  timer_.Start(FROM_HERE, kSearchWaitTime,
+               base::BindOnce(&CastDialogNoSinksView::SetHelpIconView,
+                              base::Unretained(this)));
 }
 
 CastDialogNoSinksView::~CastDialogNoSinksView() = default;
 
 void CastDialogNoSinksView::ButtonPressed(views::Button* sender,
                                           const ui::Event& event) {
-  // This is called when |help_icon_| is clicked.
-  ShowHelpCenterArticle();
-}
-
-void CastDialogNoSinksView::ShowHelpIconView() {
-  delete looking_for_sinks_view_;
-  looking_for_sinks_view_ = nullptr;
-  help_icon_view_ = CreateHelpIconView();
-  AddChildView(help_icon_view_);
-  Layout();
-}
-
-void CastDialogNoSinksView::ShowHelpCenterArticle() {
-  const GURL url = GURL(chrome::kCastNoDestinationFoundURL);
-  NavigateParams params(profile_, url, ui::PAGE_TRANSITION_LINK);
+  // Opens the help center article for troubleshooting sinks not found in a
+  // new tab. Called when |help_icon| is clicked.
+  NavigateParams params(profile_, GURL(chrome::kCastNoDestinationFoundURL),
+                        ui::PAGE_TRANSITION_LINK);
   Navigate(&params);
 }
 
-views::View* CastDialogNoSinksView::CreateLookingForSinksView() {
-  base::string16 title =
-      l10n_util::GetStringUTF16(IDS_MEDIA_ROUTER_STATUS_LOOKING_FOR_DEVICES);
-  HoverButton* view = new HoverButton(
-      /* button_listener */ nullptr, CreateThrobber(), title, base::string16());
-  view->SetEnabled(false);
-  return view;
-}
+void CastDialogNoSinksView::SetHelpIconView() {
+  // Replace the throbber with the help icon.
+  RemoveChildViewT(icon_);
+  icon_ = AddChildViewAt(CreateHelpIcon(this), 0);
 
-views::View* CastDialogNoSinksView::CreateHelpIconView() {
-  base::string16 title =
-      l10n_util::GetStringUTF16(IDS_MEDIA_ROUTER_STATUS_NO_DEVICES_FOUND);
-  auto help_icon = std::make_unique<views::ImageButton>(this);
-  views::ImageButton* help_icon_ptr = help_icon.get();
-  const SkColor icon_color = help_icon->GetNativeTheme()->GetSystemColor(
-      ui::NativeTheme::kColorId_DefaultIconColor);
-  help_icon->SetInstallFocusRingOnFocus(true);
-  help_icon->SetImage(views::Button::STATE_NORMAL,
-                      gfx::CreateVectorIcon(::vector_icons::kHelpOutlineIcon,
-                                            kPrimaryIconSize, icon_color));
-  help_icon->SetFocusForPlatform();
-  help_icon->SetBorder(views::CreateEmptyBorder(kPrimaryIconBorder));
-  help_icon->SetAccessibleName(
-      l10n_util::GetStringUTF16(IDS_MEDIA_ROUTER_NO_DEVICES_FOUND_BUTTON));
-  HoverButton* view =
-      new HoverButton(/* button_listener */ nullptr, std::move(help_icon),
-                      title, base::string16());
-  view->SetEnabled(false);
-  // HoverButton disables event handling by its icons, so enable it again.
-  help_icon_ptr->set_can_process_events_within_subtree(true);
-  return view;
+  label_->SetText(
+      l10n_util::GetStringUTF16(IDS_MEDIA_ROUTER_STATUS_NO_DEVICES_FOUND));
 }
 
 }  // namespace media_router

chrome/browser/ui/views/media_router/cast_dialog_no_sinks_view.h

@@ -6,8 +6,8 @@
 #define CHROME_BROWSER_UI_VIEWS_MEDIA_ROUTER_CAST_DIALOG_NO_SINKS_VIEW_H_
 
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
 #include "ui/views/controls/button/button.h"
+#include "ui/views/controls/label.h"
 #include "ui/views/view.h"
 
 class Profile;
@@ -19,39 +19,28 @@ namespace media_router {
 // after that it shows an icon that links to a help center article.
 class CastDialogNoSinksView : public views::View, public views::ButtonListener {
  public:
+  static constexpr base::TimeDelta kSearchWaitTime =
+      base::TimeDelta::FromSeconds(3);
+
   explicit CastDialogNoSinksView(Profile* profile);
   ~CastDialogNoSinksView() override;
 
   // views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override;
 
-  // Called by tests.
-  views::View* looking_for_sinks_view_for_test() {
-    return looking_for_sinks_view_;
+  const base::OneShotTimer& timer_for_testing() const { return timer_; }
+  const views::View* icon_for_testing() const { return icon_; }
+  const base::string16& label_text_for_testing() const {
+    return label_->GetText();
   }
-  views::View* help_icon_view_for_test() { return help_icon_view_; }
 
  private:
-  // Hides |looking_for_sinks_view_| and shows |help_icon_view_|.
-  void ShowHelpIconView();
-
-  // Opens the help center article for troubleshooting sinks not found in a
-  // new tab.
-  void ShowHelpCenterArticle();
-
-  views::View* CreateLookingForSinksView();
-  views::View* CreateHelpIconView();
-
-  // View temporarily shown that indicates sink discovery is ongoing.
-  views::View* looking_for_sinks_view_ = nullptr;
-
-  // View indicating no sinks were found and containing an icon that links to
-  // a help center article.
-  views::View* help_icon_view_ = nullptr;
+  void SetHelpIconView();
 
   Profile* const profile_;
-
-  base::WeakPtrFactory<CastDialogNoSinksView> weak_factory_{this};
+  base::OneShotTimer timer_;
+  views::View* icon_ = nullptr;
+  views::Label* label_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(CastDialogNoSinksView);
 };

chrome/browser/ui/views/media_router/cast_dialog_no_sinks_view_unittest.cc

@@ -7,7 +7,6 @@
 #include <memory>
 
 #include "base/run_loop.h"
-#include "base/time/time.h"
 #include "chrome/test/views/chrome_test_views_delegate.h"
 #include "chrome/test/views/chrome_views_test_base.h"
 #include "content/public/test/browser_task_environment.h"
@@ -26,11 +25,14 @@ class CastDialogNoSinksViewTest : public ChromeViewsTestBase {
   }
 
  protected:
-  views::View* looking_for_sinks_view() {
-    return no_sinks_view_->looking_for_sinks_view_for_test();
+  bool running() const {
+    return no_sinks_view_->timer_for_testing().IsRunning();
   }
-  views::View* help_icon_view() {
-    return no_sinks_view_->help_icon_view_for_test();
+  const views::View* get_icon() const {
+    return no_sinks_view_->icon_for_testing();
+  }
+  const base::string16& get_label_text() const {
+    return no_sinks_view_->label_text_for_testing();
   }
 
  private:
@@ -40,14 +42,20 @@ class CastDialogNoSinksViewTest : public ChromeViewsTestBase {
 };
 
 TEST_F(CastDialogNoSinksViewTest, SwitchViews) {
-  // Initially, only the throbber view should be shown.
-  EXPECT_TRUE(looking_for_sinks_view()->GetVisible());
-  EXPECT_FALSE(help_icon_view());
-
-  task_environment()->FastForwardBy(base::TimeDelta::FromSeconds(3));
-  // After three seconds, only the help icon view should be shown.
-  EXPECT_FALSE(looking_for_sinks_view());
-  EXPECT_TRUE(help_icon_view()->GetVisible());
+  // Initially the search timer should be running and the icon and label should
+  // indicate we are searching for sinks. Icon should never be null.
+  EXPECT_TRUE(running());
+  const auto* initial_icon = get_icon();
+  auto initial_title = get_label_text();
+  EXPECT_NE(initial_icon, nullptr);
+
+  // After |kSearchWaitTime| the search timer should have stopped and the icon
+  // and label should have changed to indicate no sinks were found.
+  task_environment()->FastForwardBy(
+      media_router::CastDialogNoSinksView::kSearchWaitTime);
+  EXPECT_FALSE(running());
+  EXPECT_NE(initial_icon, get_icon());
+  EXPECT_NE(initial_title, get_label_text());
 }
 
 }  // namespace media_router