Move selection clearing from section views to child bubbles.

Child bubbles have padding and child spacing such that there are gaps
between holding space sections and child bubble edges as well as gaps
between holding space sections within the same child bubble.

Moving selection clearing to child bubbles instead of section views
eliminates what the user might otherwise perceive as dead spots.

Bug: 1167248
Change-Id: I561f2862258189fa9960d6c25de7bd6da124771a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2632808Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#844186}

ash/system/holding_space/holding_space_item_view_delegate.cc

@@ -227,7 +227,7 @@ void HoldingSpaceItemViewDelegate::OnHoldingSpaceItemViewMouseReleased(
     OpenItems(GetSelection());
 }
 
-bool HoldingSpaceItemViewDelegate::OnHoldingSpaceTrayKeyPressed(
+bool HoldingSpaceItemViewDelegate::OnHoldingSpaceTrayBubbleKeyPressed(
     const ui::KeyEvent& event) {
   // The ENTER key should open all selected holding space items.
   if (event.key_code() == ui::KeyboardCode::VKEY_RETURN) {
@@ -239,16 +239,14 @@ bool HoldingSpaceItemViewDelegate::OnHoldingSpaceTrayKeyPressed(
   return false;
 }
 
-void HoldingSpaceItemViewDelegate::OnHoldingSpaceItemViewsSectionMousePressed(
-    const ui::MouseEvent& event) {
-  SetSelection({});
-}
-
-void HoldingSpaceItemViewDelegate::OnHoldingSpaceItemViewsSectionGestureEvent(
+void HoldingSpaceItemViewDelegate::OnHoldingSpaceTrayChildBubbleGestureEvent(
     const ui::GestureEvent& event) {
-  if (event.type() != ui::ET_GESTURE_TAP)
-    return;
+  if (event.type() == ui::ET_GESTURE_TAP)
+    SetSelection({});
+}
 
+void HoldingSpaceItemViewDelegate::OnHoldingSpaceTrayChildBubbleMousePressed(
+    const ui::MouseEvent& event) {
   SetSelection({});
 }
 

ash/system/holding_space/holding_space_item_view_delegate.h

@@ -86,16 +86,14 @@ class ASH_EXPORT HoldingSpaceItemViewDelegate
   void OnHoldingSpaceItemViewMouseReleased(HoldingSpaceItemView* view,
                                            const ui::MouseEvent& event);
 
-  // Invoked when the tray receives the specified key pressed `event`.
-  bool OnHoldingSpaceTrayKeyPressed(const ui::KeyEvent& event);
+  // Invoked when the tray bubble receives the specified key pressed `event`.
+  bool OnHoldingSpaceTrayBubbleKeyPressed(const ui::KeyEvent& event);
 
-  // Invoked when the item views section receives the specified mouse pressed
-  // `event`.
-  void OnHoldingSpaceItemViewsSectionMousePressed(const ui::MouseEvent& event);
+  // Invoked when a tray child bubble receives the specified gesture `event`.
+  void OnHoldingSpaceTrayChildBubbleGestureEvent(const ui::GestureEvent& event);
 
-  // Invoked when the item views section receives the specified gesture `event`.
-  void OnHoldingSpaceItemViewsSectionGestureEvent(
-      const ui::GestureEvent& event);
+  // Invoked when a tray child bubble receives the given mouse pressed `event`.
+  void OnHoldingSpaceTrayChildBubbleMousePressed(const ui::MouseEvent& event);
 
  private:
   // views::ContextMenuController:

ash/system/holding_space/holding_space_item_views_section.cc

@@ -252,16 +252,6 @@ void HoldingSpaceItemViewsSection::ViewHierarchyChanged(
   PreferredSizeChanged();
 }
 
-bool HoldingSpaceItemViewsSection::OnMousePressed(const ui::MouseEvent& event) {
-  delegate_->OnHoldingSpaceItemViewsSectionMousePressed(event);
-  return true;
-}
-
-void HoldingSpaceItemViewsSection::OnGestureEvent(ui::GestureEvent* event) {
-  delegate_->OnHoldingSpaceItemViewsSectionGestureEvent(*event);
-  views::View::OnGestureEvent(event);
-}
-
 void HoldingSpaceItemViewsSection::OnHoldingSpaceItemsAdded(
     const std::vector<const HoldingSpaceItem*>& items) {
   const bool needs_update = std::any_of(

ash/system/holding_space/holding_space_item_views_section.h

@@ -53,8 +53,6 @@ class HoldingSpaceItemViewsSection : public views::View {
   void ChildVisibilityChanged(views::View* child) override;
   void PreferredSizeChanged() override;
   void ViewHierarchyChanged(const views::ViewHierarchyChangedDetails&) override;
-  void OnGestureEvent(ui::GestureEvent* event) override;
-  bool OnMousePressed(const ui::MouseEvent& event) override;
 
   // `HoldingSpaceModelObserver` events forwarded from the parent
   // `HoldingSpaceTrayChildBubble`. Note that events may be withheld from this

ash/system/holding_space/holding_space_tray_bubble.cc

@@ -62,34 +62,39 @@ void RecordTimeFromFirstAvailabilityToFirstEntry(PrefService* prefs) {
       time_of_first_entry - time_of_first_availability);
 }
 
-// HoldingSpaceEventFilter -----------------------------------------------------
-class HoldingSpaceEventFilter : public ui::EventHandler {
+// HoldingSpaceTrayBubbleEventHandler ------------------------------------------
+class HoldingSpaceTrayBubbleEventHandler : public ui::EventHandler {
  public:
-  HoldingSpaceEventFilter(HoldingSpaceItemViewDelegate* delegate)
+  explicit HoldingSpaceTrayBubbleEventHandler(
+      HoldingSpaceItemViewDelegate* delegate)
       : delegate_(delegate) {
     aura::Env::GetInstance()->AddPreTargetHandler(
         this, ui::EventTarget::Priority::kSystem);
   }
-  ~HoldingSpaceEventFilter() override {
+
+  HoldingSpaceTrayBubbleEventHandler(
+      const HoldingSpaceTrayBubbleEventHandler&) = delete;
+  HoldingSpaceTrayBubbleEventHandler& operator=(
+      const HoldingSpaceTrayBubbleEventHandler&) = delete;
+
+  ~HoldingSpaceTrayBubbleEventHandler() override {
     aura::Env::GetInstance()->RemovePreTargetHandler(this);
   }
-  HoldingSpaceEventFilter(const HoldingSpaceEventFilter&) = delete;
-  HoldingSpaceEventFilter& operator=(const HoldingSpaceEventFilter&) = delete;
 
  private:
   // ui::EventHandler:
   void OnKeyEvent(ui::KeyEvent* event) override {
     if (event->type() == ui::ET_KEY_PRESSED &&
-        delegate_->OnHoldingSpaceTrayKeyPressed(*event)) {
+        delegate_->OnHoldingSpaceTrayBubbleKeyPressed(*event)) {
       event->StopPropagation();
     }
-    return;
   }
 
-  HoldingSpaceItemViewDelegate* delegate_;
+  HoldingSpaceItemViewDelegate* const delegate_;
 };
 
 // ChildBubbleContainerLayout --------------------------------------------------
+
 // A class similar to a `views::LayoutManager` which supports calculating and
 // applying `views::ProposedLayout`s. Views are laid out similar to a vertical
 // `views::BoxLayout` with the first child flexing to cede layout space if the
@@ -320,7 +325,8 @@ HoldingSpaceTrayBubble::HoldingSpaceTrayBubble(
       ->frame_view()
       ->SetVisible(false);
 
-  event_filter_ = std::make_unique<HoldingSpaceEventFilter>(&delegate_);
+  event_handler_ =
+      std::make_unique<HoldingSpaceTrayBubbleEventHandler>(&delegate_);
 
   PrefService* const prefs =
       Shell::Get()->session_controller()->GetLastActiveUserPrefService();
@@ -341,7 +347,6 @@ HoldingSpaceTrayBubble::HoldingSpaceTrayBubble(
 }
 
 HoldingSpaceTrayBubble::~HoldingSpaceTrayBubble() {
-  event_filter_.reset();
   bubble_wrapper_->bubble_view()->ResetDelegate();
 
   // Explicitly reset child bubbles so that they will stop observing the holding

ash/system/holding_space/holding_space_tray_bubble.h

@@ -66,9 +66,9 @@ class ASH_EXPORT HoldingSpaceTrayBubble : public ScreenLayoutObserver,
   // Views owned by view hierarchy.
   ChildBubbleContainer* child_bubble_container_;
   std::vector<HoldingSpaceTrayChildBubble*> child_bubbles_;
-  std::unique_ptr<ui::EventHandler> event_filter_;
 
   std::unique_ptr<TrayBubbleWrapper> bubble_wrapper_;
+  std::unique_ptr<ui::EventHandler> event_handler_;
 
   ScopedObserver<Shelf, ShelfObserver> shelf_observer_{this};
   ScopedObserver<TabletModeController, TabletModeObserver>

ash/system/holding_space/holding_space_tray_child_bubble.cc

@@ -8,6 +8,7 @@
 
 #include "ash/public/cpp/holding_space/holding_space_constants.h"
 #include "ash/style/ash_color_provider.h"
+#include "ash/system/holding_space/holding_space_item_view_delegate.h"
 #include "ash/system/holding_space/holding_space_item_views_section.h"
 #include "ash/system/holding_space/holding_space_util.h"
 #include "ash/system/tray/tray_constants.h"
@@ -252,6 +253,16 @@ void HoldingSpaceTrayChildBubble::ChildVisibilityChanged(views::View* child) {
   PreferredSizeChanged();
 }
 
+void HoldingSpaceTrayChildBubble::OnGestureEvent(ui::GestureEvent* event) {
+  delegate_->OnHoldingSpaceTrayChildBubbleGestureEvent(*event);
+  views::View::OnGestureEvent(event);
+}
+
+bool HoldingSpaceTrayChildBubble::OnMousePressed(const ui::MouseEvent& event) {
+  delegate_->OnHoldingSpaceTrayChildBubbleMousePressed(event);
+  return true;
+}
+
 void HoldingSpaceTrayChildBubble::MaybeAnimateIn() {
   // Don't preempt an out animation as new content will populate and be animated
   // in, if any exists, once the out animation completes.

ash/system/holding_space/holding_space_tray_child_bubble.h

@@ -67,6 +67,8 @@ class HoldingSpaceTrayChildBubble : public views::View,
   const char* GetClassName() const override;
   void ChildPreferredSizeChanged(views::View* child) override;
   void ChildVisibilityChanged(views::View* child) override;
+  void OnGestureEvent(ui::GestureEvent* event) override;
+  bool OnMousePressed(const ui::MouseEvent& event) override;
 
   // Invoked to animate in/out this view if necessary.
   void MaybeAnimateIn();

ash/system/holding_space/holding_space_tray_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "ash/system/holding_space/holding_space_tray.h"
 
+#include <array>
 #include <deque>
 #include <vector>
 
@@ -1678,35 +1679,34 @@ TEST_P(HoldingSpaceTrayTest, ClickBackgroundToDeselectItems) {
   AddItem(HoldingSpaceItem::Type::kDownload, base::FilePath("/tmp/fake2"));
   EXPECT_TRUE(test_api()->IsShowingInShelf());
 
-  // Show the bubble.
+  // Show the bubble and cache holding space item views.
   test_api()->Show();
   std::vector<views::View*> download_chips = test_api()->GetDownloadChips();
-  HoldingSpaceItemView* holding_space_item =
-      HoldingSpaceItemView::Cast(download_chips[0]);
+  ASSERT_EQ(2u, download_chips.size());
+  std::array<HoldingSpaceItemView*, 2> item_views = {
+      HoldingSpaceItemView::Cast(download_chips[0]),
+      HoldingSpaceItemView::Cast(download_chips[1])};
 
   // Click an item chip. The view should be selected.
   Click(download_chips[0], 0);
-  ASSERT_TRUE(holding_space_item->selected());
+  ASSERT_TRUE(item_views[0]->selected());
+  ASSERT_FALSE(item_views[1]->selected());
+
   // Clicking on the parent view should deselect item.
   Click(download_chips[0]->parent(), 0);
-  ASSERT_FALSE(holding_space_item->selected());
-
-  test_api()->Show();
-
-  download_chips = test_api()->GetDownloadChips();
-  holding_space_item = HoldingSpaceItemView::Cast(download_chips[0]);
-  HoldingSpaceItemView* holding_space_item_2 =
-      HoldingSpaceItemView::Cast(download_chips[1]);
+  ASSERT_FALSE(item_views[0]->selected());
+  ASSERT_FALSE(item_views[1]->selected());
 
   // Click on both items to select them both.
   Click(download_chips[0], ui::EF_SHIFT_DOWN);
   Click(download_chips[1], ui::EF_SHIFT_DOWN);
-  ASSERT_TRUE(holding_space_item->selected());
-  ASSERT_TRUE(holding_space_item_2->selected());
+  ASSERT_TRUE(item_views[0]->selected());
+  ASSERT_TRUE(item_views[1]->selected());
+
   // Clicking on the parent view should deselect both items.
   Click(download_chips[0]->parent(), 0);
-  ASSERT_FALSE(holding_space_item->selected());
-  ASSERT_FALSE(holding_space_item_2->selected());
+  ASSERT_FALSE(item_views[0]->selected());
+  ASSERT_FALSE(item_views[1]->selected());
 }
 
 INSTANTIATE_TEST_SUITE_P(All, HoldingSpaceTrayTest, testing::Bool());