Allow device chooser table to handle clicks.

Originally, when the table was visible, the device chooser had three children: a
scroll view containiner the table, and two invisible centered labels.  In
https://chromium-review.googlesource.com/c/chromium/src/+/1837079 I reworked
this to center the labels using anonymous views with layout managers.  This
meant that the device chooser now contained three visible full-size children
(which, in two cases, just contained an invisible centered child).  As a result
the last child (which was highest in Z order) became the event handler at all
times, and while it would bubble events to its parent, there was no way for them
to get over to the sibling scroll view.

The solution is to toggle the visibility of these centering views, rather than
toggling the visibility of their contained labels.  This causes event targeting
to select the scroll view again.

Also does various additional cleanup I noticed while working on this.

Bug: 1011536
Change-Id: I27c496ebf4899b94bd442a1eaeb5e6ddbca387a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1843294
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#703497}

chrome/browser/ui/views/device_chooser_content_view.cc

@@ -37,7 +37,24 @@ constexpr int kReScanButtonTag = 2;
 
 }  // namespace
 
-DeviceChooserContentView::BluetoothStatusContainer::BluetoothStatusContainer(
+class BluetoothStatusContainer : public views::View {
+ public:
+  explicit BluetoothStatusContainer(views::ButtonListener* listener);
+
+  void ShowScanningLabelAndThrobber();
+  void ShowReScanButton(bool enabled);
+
+ private:
+  friend class DeviceChooserContentView;
+
+  views::LabelButton* re_scan_button_;
+  views::Throbber* throbber_;
+  views::Label* scanning_label_;
+
+  DISALLOW_COPY_AND_ASSIGN(BluetoothStatusContainer);
+};
+
+BluetoothStatusContainer::BluetoothStatusContainer(
     views::ButtonListener* listener) {
   SetLayoutManager(std::make_unique<views::FillLayout>());
 
@@ -76,16 +93,14 @@ DeviceChooserContentView::BluetoothStatusContainer::BluetoothStatusContainer(
   scanning_label_ = scan_container->AddChildView(std::move(scanning_label));
 }
 
-void DeviceChooserContentView::BluetoothStatusContainer::
-    ShowScanningLabelAndThrobber() {
+void BluetoothStatusContainer::ShowScanningLabelAndThrobber() {
   re_scan_button_->SetVisible(false);
   throbber_->SetVisible(true);
   scanning_label_->SetVisible(true);
   throbber_->Start();
 }
 
-void DeviceChooserContentView::BluetoothStatusContainer::ShowReScanButton(
-    bool enabled) {
+void BluetoothStatusContainer::ShowReScanButton(bool enabled) {
   re_scan_button_->SetVisible(true);
   re_scan_button_->SetEnabled(enabled);
   throbber_->Stop();
@@ -115,7 +130,7 @@ DeviceChooserContentView::DeviceChooserContentView(
   table_parent_ = AddChildView(
       views::TableView::CreateScrollViewWithTable(std::move(table_view)));
 
-  const auto add_centered_view = [this](auto view) {
+  const auto add_centering_view = [this](auto view) {
     auto* container = AddChildView(std::make_unique<views::View>());
     auto* layout =
         container->SetLayoutManager(std::make_unique<views::BoxLayout>(
@@ -124,24 +139,26 @@ DeviceChooserContentView::DeviceChooserContentView(
         views::BoxLayout::MainAxisAlignment::kCenter);
     layout->set_cross_axis_alignment(
         views::BoxLayout::CrossAxisAlignment::kCenter);
-    return container->AddChildView(std::move(view));
+    layout->set_inside_border_insets(gfx::Insets(0, 6));
+    container->AddChildView(std::move(view));
+    return container;
   };
 
-  no_options_help_ = add_centered_view(
-      std::make_unique<views::Label>(chooser_controller_->GetNoOptionsText()));
-  no_options_help_->SetMultiLine(true);
+  auto no_options_help =
+      std::make_unique<views::Label>(chooser_controller_->GetNoOptionsText());
+  no_options_help->SetMultiLine(true);
+  no_options_view_ = add_centering_view(std::move(no_options_help));
 
   base::string16 link_text = l10n_util::GetStringUTF16(
       IDS_BLUETOOTH_DEVICE_CHOOSER_TURN_ON_BLUETOOTH_LINK_TEXT);
   size_t offset = 0;
   base::string16 text = l10n_util::GetStringFUTF16(
       IDS_BLUETOOTH_DEVICE_CHOOSER_TURN_ADAPTER_OFF, link_text, &offset);
-  adapter_off_help_ =
-      add_centered_view(std::make_unique<views::StyledLabel>(text, this));
-  adapter_off_help_->AddStyleRange(
+  auto adapter_off_help = std::make_unique<views::StyledLabel>(text, this);
+  adapter_off_help->AddStyleRange(
       gfx::Range(0, link_text.size()),
       views::StyledLabel::RangeStyleInfo::CreateForLink());
-  adapter_off_help_->SetVisible(false);
+  adapter_off_view_ = add_centering_view(std::move(adapter_off_help));
 
   UpdateTableView();
 }
@@ -157,13 +174,6 @@ gfx::Size DeviceChooserContentView::GetMinimumSize() const {
   return gfx::Size();
 }
 
-void DeviceChooserContentView::OnBoundsChanged(
-    const gfx::Rect& previous_bounds) {
-  constexpr int kMessagePadding = 6;
-  no_options_help_->SizeToFit(width() - 2 * kMessagePadding);
-  adapter_off_help_->SizeToFit(width() - 2 * kMessagePadding);
-}
-
 int DeviceChooserContentView::RowCount() {
   return base::checked_cast<int>(chooser_controller_->NumOptions());
 }
@@ -258,7 +268,6 @@ void DeviceChooserContentView::OnRefreshStateChanged(bool refreshing) {
 void DeviceChooserContentView::StyledLabelLinkClicked(views::StyledLabel* label,
                                                       const gfx::Range& range,
                                                       int event_flags) {
-  DCHECK_EQ(adapter_off_help_, label);
   chooser_controller_->OpenAdapterOffHelpUrl();
 }
 
@@ -349,10 +358,20 @@ void DeviceChooserContentView::Close() {
 void DeviceChooserContentView::UpdateTableView() {
   bool has_options = adapter_enabled_ && RowCount() > 0;
   table_parent_->SetVisible(has_options);
-  if (chooser_controller_->TableViewAlwaysDisabled())
-    table_view_->SetEnabled(false);
-  else
-    table_view_->SetEnabled(has_options);
-  no_options_help_->SetVisible(!has_options && adapter_enabled_);
-  adapter_off_help_->SetVisible(!adapter_enabled_);
+  table_view_->SetEnabled(has_options &&
+                          !chooser_controller_->TableViewAlwaysDisabled());
+  no_options_view_->SetVisible(!has_options && adapter_enabled_);
+  adapter_off_view_->SetVisible(!adapter_enabled_);
+}
+
+views::LabelButton* DeviceChooserContentView::ReScanButtonForTesting() {
+  return bluetooth_status_container_->re_scan_button_;
+}
+
+views::Throbber* DeviceChooserContentView::ThrobberForTesting() {
+  return bluetooth_status_container_->throbber_;
+}
+
+views::Label* DeviceChooserContentView::ScanningLabelForTesting() {
+  return bluetooth_status_container_->scanning_label_;
 }

chrome/browser/ui/views/device_chooser_content_view.h

@@ -13,13 +13,13 @@
 #include "ui/base/models/table_model.h"
 #include "ui/gfx/range/range.h"
 #include "ui/views/controls/button/button.h"
-#include "ui/views/controls/label.h"
 #include "ui/views/controls/styled_label_listener.h"
 #include "ui/views/view.h"
 
+class BluetoothStatusContainer;
 namespace views {
+class Label;
 class LabelButton;
-class StyledLabel;
 class TableView;
 class TableViewObserver;
 class Throbber;
@@ -40,7 +40,6 @@ class DeviceChooserContentView : public views::View,
 
   // views::View:
   gfx::Size GetMinimumSize() const override;
-  void OnBoundsChanged(const gfx::Rect& previous_bounds) override;
 
   // ui::TableModel:
   int RowCount() override;
@@ -73,39 +72,23 @@ class DeviceChooserContentView : public views::View,
   void Close();
   void UpdateTableView();
 
- private:
-  friend class ChooserDialogViewTest;
-  friend class DeviceChooserContentViewTest;
-  FRIEND_TEST_ALL_PREFIXES(DeviceChooserContentViewTest, ClickRescanLink);
-  FRIEND_TEST_ALL_PREFIXES(DeviceChooserContentViewTest, ClickGetHelpLink);
-
-  class BluetoothStatusContainer : public views::View {
-   public:
-    explicit BluetoothStatusContainer(views::ButtonListener* listener);
-
-    void ShowScanningLabelAndThrobber();
-    void ShowReScanButton(bool enabled);
-
-    views::LabelButton* re_scan_button() { return re_scan_button_; }
-    views::Throbber* throbber() { return throbber_; }
-    views::Label* scanning_label() { return scanning_label_; }
+  // Test-only accessors to children.
+  views::TableView* table_view_for_testing() { return table_view_; }
+  views::LabelButton* ReScanButtonForTesting();
+  views::Throbber* ThrobberForTesting();
+  views::Label* ScanningLabelForTesting();
 
  private:
-    views::LabelButton* re_scan_button_;
-    views::Throbber* throbber_;
-    views::Label* scanning_label_;
-
-    DISALLOW_COPY_AND_ASSIGN(BluetoothStatusContainer);
-  };
+  friend class DeviceChooserContentViewTest;
 
   std::unique_ptr<ChooserController> chooser_controller_;
 
   bool adapter_enabled_ = true;
 
-  views::TableView* table_view_ = nullptr;
   views::View* table_parent_ = nullptr;
-  views::Label* no_options_help_ = nullptr;
-  views::StyledLabel* adapter_off_help_ = nullptr;
+  views::TableView* table_view_ = nullptr;
+  views::View* no_options_view_ = nullptr;
+  views::View* adapter_off_view_ = nullptr;
   BluetoothStatusContainer* bluetooth_status_container_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceChooserContentView);

chrome/browser/ui/views/device_chooser_content_view_unittest.cc

@@ -48,7 +48,8 @@ class DeviceChooserContentViewTest : public ChromeViewsTestBase {
     extra_views_container_ = content_view().CreateExtraView();
 
     ASSERT_NE(nullptr, table_view());
-    ASSERT_NE(nullptr, adapter_off_help_link());
+    ASSERT_NE(nullptr, no_options_view());
+    ASSERT_NE(nullptr, adapter_off_view());
     ASSERT_NE(nullptr, re_scan_button());
     ASSERT_NE(nullptr, throbber());
     ASSERT_NE(nullptr, scanning_label());
@@ -61,21 +62,19 @@ class DeviceChooserContentViewTest : public ChromeViewsTestBase {
   MockTableViewObserver& table_observer() { return *table_observer_; }
   DeviceChooserContentView& content_view() { return *content_view_; }
 
-  views::TableView* table_view() { return content_view().table_view_; }
+  views::TableView* table_view() {
+    return content_view().table_view_for_testing();
+  }
   views::View* table_parent() { return content_view().table_parent_; }
   ui::TableModel* table_model() { return table_view()->model(); }
-  views::Label* no_options_label() { return content_view().no_options_help_; }
-  views::StyledLabel* adapter_off_help_link() {
-    return content_view().adapter_off_help_;
-  }
+  views::View* no_options_view() { return content_view().no_options_view_; }
+  views::View* adapter_off_view() { return content_view().adapter_off_view_; }
   views::LabelButton* re_scan_button() {
-    return content_view().bluetooth_status_container_->re_scan_button();
-  }
-  views::Throbber* throbber() {
-    return content_view().bluetooth_status_container_->throbber();
+    return content_view().ReScanButtonForTesting();
   }
+  views::Throbber* throbber() { return content_view().ThrobberForTesting(); }
   views::Label* scanning_label() {
-    return content_view().bluetooth_status_container_->scanning_label();
+    return content_view().ScanningLabelForTesting();
   }
 
   void AddUnpairedDevice() {
@@ -105,7 +104,7 @@ class DeviceChooserContentViewTest : public ChromeViewsTestBase {
   bool IsDeviceSelected() { return !table_view()->selection_model().empty(); }
 
   void ExpectNoDevices() {
-    EXPECT_TRUE(no_options_label()->GetVisible());
+    EXPECT_TRUE(no_options_view()->GetVisible());
     EXPECT_EQ(0, table_view()->GetRowCount());
     // The table should be disabled since there are no (real) options.
     EXPECT_FALSE(table_parent()->GetVisible());
@@ -128,7 +127,7 @@ TEST_F(DeviceChooserContentViewTest, InitialState) {
   EXPECT_CALL(table_observer(), OnSelectionChanged()).Times(0);
 
   ExpectNoDevices();
-  EXPECT_FALSE(adapter_off_help_link()->GetVisible());
+  EXPECT_FALSE(adapter_off_view()->GetVisible());
   EXPECT_FALSE(throbber()->GetVisible());
   EXPECT_FALSE(scanning_label()->GetVisible());
   EXPECT_TRUE(re_scan_button()->GetVisible());
@@ -210,8 +209,8 @@ TEST_F(DeviceChooserContentViewTest, TurnBluetoothOffAndOn) {
       FakeBluetoothChooserController::BluetoothStatus::UNAVAILABLE);
 
   EXPECT_FALSE(table_parent()->GetVisible());
-  EXPECT_FALSE(no_options_label()->GetVisible());
-  EXPECT_TRUE(adapter_off_help_link()->GetVisible());
+  EXPECT_FALSE(no_options_view()->GetVisible());
+  EXPECT_TRUE(adapter_off_view()->GetVisible());
   EXPECT_FALSE(throbber()->GetVisible());
   EXPECT_FALSE(scanning_label()->GetVisible());
   EXPECT_TRUE(re_scan_button()->GetVisible());
@@ -221,7 +220,7 @@ TEST_F(DeviceChooserContentViewTest, TurnBluetoothOffAndOn) {
   controller()->SetBluetoothStatus(
       FakeBluetoothChooserController::BluetoothStatus::IDLE);
   ExpectNoDevices();
-  EXPECT_FALSE(adapter_off_help_link()->GetVisible());
+  EXPECT_FALSE(adapter_off_view()->GetVisible());
   EXPECT_FALSE(throbber()->GetVisible());
   EXPECT_FALSE(scanning_label()->GetVisible());
   EXPECT_TRUE(re_scan_button()->GetVisible());
@@ -233,7 +232,7 @@ TEST_F(DeviceChooserContentViewTest, ScanForDevices) {
       FakeBluetoothChooserController::BluetoothStatus::SCANNING);
   EXPECT_EQ(0, table_view()->GetRowCount());
   EXPECT_FALSE(table_view()->GetEnabled());
-  EXPECT_FALSE(adapter_off_help_link()->GetVisible());
+  EXPECT_FALSE(adapter_off_view()->GetVisible());
   EXPECT_TRUE(throbber()->GetVisible());
   EXPECT_TRUE(scanning_label()->GetVisible());
   EXPECT_FALSE(re_scan_button()->GetVisible());
@@ -241,7 +240,7 @@ TEST_F(DeviceChooserContentViewTest, ScanForDevices) {
   AddUnpairedDevice();
   EXPECT_EQ(1, table_view()->GetRowCount());
   EXPECT_TRUE(table_view()->GetEnabled());
-  EXPECT_FALSE(adapter_off_help_link()->GetVisible());
+  EXPECT_FALSE(adapter_off_view()->GetVisible());
   EXPECT_TRUE(throbber()->GetVisible());
   EXPECT_TRUE(scanning_label()->GetVisible());
   EXPECT_FALSE(IsDeviceSelected());
@@ -250,7 +249,8 @@ TEST_F(DeviceChooserContentViewTest, ScanForDevices) {
 
 TEST_F(DeviceChooserContentViewTest, ClickAdapterOffHelpLink) {
   EXPECT_CALL(*controller(), OpenAdapterOffHelpUrl()).Times(1);
-  adapter_off_help_link()->LinkClicked(nullptr, 0);
+  static_cast<views::StyledLabel*>(adapter_off_view()->children().front())
+      ->LinkClicked(nullptr, 0);
 }
 
 TEST_F(DeviceChooserContentViewTest, ClickRescanButton) {

chrome/browser/ui/views/extensions/chooser_dialog_view_unittest.cc

@@ -63,12 +63,13 @@ class ChooserDialogViewTest : public ChromeViewsTestBase {
   }
 
   views::TableView* table_view() {
-    return dialog_->device_chooser_content_view_for_test()->table_view_;
+    return dialog_->device_chooser_content_view_for_test()
+        ->table_view_for_testing();
   }
 
   views::LabelButton* re_scan_button() {
     return dialog_->device_chooser_content_view_for_test()
-        ->bluetooth_status_container_->re_scan_button();
+        ->ReScanButtonForTesting();
   }
 
   void AddDevice() {