Fix the count of activated corner buttons

ScrollableShelfView records the number of corner buttons whose ripple
ring is activated. However, the count does not work as expected in the
following two scenarios:

(1) After transition between clamshell and tablet, the shelf icon's
context menu should be closed. However, when the count of activated
corner buttons updates at the end of ripple ring fadeout animation, due
to change in scrollable shelf's layout, the button whose ripple ring is
deactivated is not at the corner of shelf anymore. As a result, the
count does not decrease as expected.

(2) The count fails to decrease when removing an icon from context menu.
Because the ripple ring animation ends after shelf icon removes itself
from the ink drop listener list.

This CL fixes the issue by creating a scoped count owned by
ShelfAppButton

Bug: 1086484
Change-Id: Ib31f8b52d4da845d4a7dc945d44c7f38f3a4662f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2213877
Commit-Queue: Andrew Xu <andrewxu@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772311}

ash/BUILD.gn

@@ -678,6 +678,7 @@ component("ash") {
     "shelf/shelf_bubble.h",
     "shelf/shelf_button.cc",
     "shelf/shelf_button.h",
+    "shelf/shelf_button_delegate.cc",
     "shelf/shelf_button_delegate.h",
     "shelf/shelf_button_pressed_metric_tracker.cc",
     "shelf/shelf_button_pressed_metric_tracker.h",

ash/shelf/scrollable_shelf_view.cc

@@ -310,6 +310,30 @@ class ScrollableShelfView::ScrollableShelfArrowView
   Shelf* const shelf_ = nullptr;
 };
 
+////////////////////////////////////////////////////////////////////////////////
+// ScopedActiveInkDropCountImpl
+
+class ScrollableShelfView::ScopedActiveInkDropCountImpl
+    : public ScrollableShelfView::ScopedActiveInkDropCount {
+ public:
+  explicit ScopedActiveInkDropCountImpl(ScrollableShelfView* owner)
+      : owner_(owner) {
+    owner_->OnActiveInkDropChange(/*increase=*/true);
+  }
+
+  ~ScopedActiveInkDropCountImpl() override {
+    owner_->OnActiveInkDropChange(/*increase=*/false);
+  }
+
+  ScopedActiveInkDropCountImpl(const ScopedActiveInkDropCountImpl& rhs) =
+      delete;
+  ScopedActiveInkDropCountImpl& operator=(
+      const ScopedActiveInkDropCountImpl& rhs) = delete;
+
+ private:
+  ScrollableShelfView* owner_ = nullptr;
+};
+
 ////////////////////////////////////////////////////////////////////////////////
 // ScrollableShelfAnimationMetricsReporter
 
@@ -700,6 +724,10 @@ void ScrollableShelfView::SetTestObserver(TestObserver* test_observer) {
   test_observer_ = test_observer;
 }
 
+bool ScrollableShelfView::IsAnyCornerButtonInkDropActivatedForTest() const {
+  return activated_corner_buttons_ > 0;
+}
+
 int ScrollableShelfView::GetSumOfButtonSizeAndSpacing() const {
   return shelf_view_->GetButtonSize() + ShelfConfig::Get()->button_spacing();
 }
@@ -754,7 +782,7 @@ void ScrollableShelfView::StartShelfScrollAnimation(float scroll_distance) {
   const bool one_arrow_in_target_state =
       (layout_strategy_ == LayoutStrategy::kShowLeftArrowButton ||
        layout_strategy_ == LayoutStrategy::kShowRightArrowButton);
-  if (one_arrow_in_target_state && Shell::Get()->IsInTabletMode())
+  if (one_arrow_in_target_state)
     EnableShelfRoundedCorners(/*enable=*/true);
 
   ui::ScopedLayerAnimationSettings animation_settings(
@@ -1120,25 +1148,12 @@ void ScrollableShelfView::HandleAccessibleActionScrollToMakeVisible(
   }
 }
 
-void ScrollableShelfView::NotifyInkDropActivity(bool activated,
-                                                views::Button* sender) {
-  // When scrolling shelf by gestures, the shelf icon's ink drop ripple may be
-  // activated accidentally. So ignore the ink drop activity during animation.
-  if (during_scroll_animation_)
-    return;
+std::unique_ptr<ScrollableShelfView::ScopedActiveInkDropCount>
+ScrollableShelfView::CreateScopedActiveInkDropCount(const ShelfButton* sender) {
+  if (!ShouldCountActivatedInkDrop(sender))
+    return nullptr;
 
-  // When long pressing icons, sometimes there are more ripple animations
-  // pending over others buttons. Only activate rounded corners when at least
-  // one button needs them.
-  if (InkDropNeedsClipping(sender)) {
-    if (activated)
-      ++activated_corner_buttons_;
-    else
-      --activated_corner_buttons_;
-  }
-  DCHECK_GE(activated_corner_buttons_, 0);
-  DCHECK_LE(activated_corner_buttons_, 2);
-  EnableShelfRoundedCorners(activated_corner_buttons_ > 0);
+  return std::make_unique<ScopedActiveInkDropCountImpl>(this);
 }
 
 void ScrollableShelfView::ShowContextMenuForViewImpl(
@@ -2171,8 +2186,8 @@ ScrollableShelfView::CalculateShelfContainerRoundedCorners() const {
   // This function may access TabletModeController during destruction of
   // Hotseat. However, TabletModeController is destructed before Hotseat. So
   // check the pointer explicitly here.
-  // TODO(andrewxu): reorder the destruction order in Shell::~Shell then remove
-  // the explicit check.
+  // TODO(https://crbug.com/1067490): reorder the destruction order in
+  // Shell::~Shell then remove the explicit check.
   const bool is_in_tablet_mode =
       Shell::Get()->tablet_mode_controller() && Shell::Get()->IsInTabletMode();
 
@@ -2332,22 +2347,41 @@ int ScrollableShelfView::CalculateScrollOffsetForTargetAvailableSpace(
   return target_scroll_offset;
 }
 
-bool ScrollableShelfView::InkDropNeedsClipping(views::Button* sender) const {
+bool ScrollableShelfView::ShouldCountActivatedInkDrop(
+    const views::View* sender) const {
+  bool should_count = false;
+
+  // When scrolling shelf by gestures, the shelf icon's ink drop ripple may be
+  // activated accidentally. So ignore the ink drop activity during animation.
+  if (during_scroll_animation_)
+    return should_count;
+
   // The ink drop needs to be clipped only if |sender| is the app at one of the
   // corners of the shelf. This happens if it is either the first or the last
   // tappable app and no arrow is showing on its side.
   if (shelf_view_->view_model()->view_at(first_tappable_app_index_) == sender) {
-    return !(layout_strategy_ == kShowButtons ||
-             layout_strategy_ == kShowLeftArrowButton);
-  }
-  if (shelf_view_->view_model()->view_at(last_tappable_app_index_) == sender) {
-    return !(layout_strategy_ == kShowButtons ||
-             layout_strategy_ == kShowRightArrowButton);
+    should_count = !(layout_strategy_ == kShowButtons ||
+                     layout_strategy_ == kShowLeftArrowButton);
+  } else if (shelf_view_->view_model()->view_at(last_tappable_app_index_) ==
+             sender) {
+    should_count = !(layout_strategy_ == kShowButtons ||
+                     layout_strategy_ == kShowRightArrowButton);
   }
-  return false;
+
+  return should_count;
 }
 
 void ScrollableShelfView::EnableShelfRoundedCorners(bool enable) {
+  // Only enable shelf rounded corners in tablet mode. Note that we allow
+  // disabling rounded corners in clamshell. Because when switching to clamshell
+  // from tablet, this method may be called after tablet mode ends.
+  // TODO(https://crbug.com/1067490): reorder the destruction order in
+  // Shell::~Shell then remove the explicit check.
+  const bool is_in_tablet_mode =
+      Shell::Get()->tablet_mode_controller() && Shell::Get()->IsInTabletMode();
+  if (enable && !is_in_tablet_mode)
+    return;
+
   ui::Layer* layer = shelf_container_view_->layer();
 
   const bool has_rounded_corners = !(layer->rounded_corner_radii().IsEmpty());
@@ -2361,4 +2395,18 @@ void ScrollableShelfView::EnableShelfRoundedCorners(bool enable) {
     layer->SetIsFastRoundedCorner(/*enable=*/true);
 }
 
+void ScrollableShelfView::OnActiveInkDropChange(bool increase) {
+  if (increase)
+    ++activated_corner_buttons_;
+  else
+    --activated_corner_buttons_;
+
+  // When long pressing icons, sometimes there are more ripple animations
+  // pending over others buttons. Only activate rounded corners when at least
+  // one button needs them.
+  CHECK_GE(activated_corner_buttons_, 0);
+  CHECK_LE(activated_corner_buttons_, 2);
+  EnableShelfRoundedCorners(activated_corner_buttons_ > 0);
+}
+
 }  // namespace ash

ash/shelf/scrollable_shelf_view.h

@@ -106,6 +106,9 @@ class ASH_EXPORT ScrollableShelfView : public views::AccessiblePaneView,
 
   void SetTestObserver(TestObserver* test_observer);
 
+  // Returns true if any shelf corner button has ripple ring activated.
+  bool IsAnyCornerButtonInkDropActivatedForTest() const;
+
   ShelfView* shelf_view() { return shelf_view_; }
   ShelfContainerView* shelf_container_view() { return shelf_container_view_; }
   const ShelfContainerView* shelf_container_view() const {
@@ -150,6 +153,7 @@ class ASH_EXPORT ScrollableShelfView : public views::AccessiblePaneView,
   class GradientLayerDelegate;
   class ScrollableShelfArrowView;
   class DragIconDropAnimationDelegate;
+  class ScopedActiveInkDropCountImpl;
 
   struct FadeZone {
     // Bounds of the fade in/out zone.
@@ -227,7 +231,8 @@ class ASH_EXPORT ScrollableShelfView : public views::AccessiblePaneView,
                      const ui::Event& event,
                      views::InkDrop* ink_drop) override;
   void HandleAccessibleActionScrollToMakeVisible(ShelfButton* button) override;
-  void NotifyInkDropActivity(bool activated, views::Button* sender) override;
+  std::unique_ptr<ScopedActiveInkDropCount> CreateScopedActiveInkDropCount(
+      const ShelfButton* sender) override;
 
   // ContextMenuController:
   void ShowContextMenuForViewImpl(views::View* source,
@@ -450,12 +455,16 @@ class ASH_EXPORT ScrollableShelfView : public views::AccessiblePaneView,
   int CalculateScrollOffsetForTargetAvailableSpace(
       const gfx::Rect& target_space) const;
 
-  // Returns whether the ink drop for the |sender| needs to be clipped.
-  bool InkDropNeedsClipping(views::Button* sender) const;
+  // Returns whether |sender|'s activated ink drop should be counted.
+  bool ShouldCountActivatedInkDrop(const views::View* sender) const;
 
   // Enable/disable the rounded corners of the shelf container.
   void EnableShelfRoundedCorners(bool enable);
 
+  // Update the number of corner buttons with ripple ring activated. |increase|
+  // indicates whether the number increases or decreases.
+  void OnActiveInkDropChange(bool increase);
+
   LayoutStrategy layout_strategy_ = kNotShowArrowButtons;
 
   // Child views Owned by views hierarchy.
@@ -503,8 +512,7 @@ class ASH_EXPORT ScrollableShelfView : public views::AccessiblePaneView,
   int first_tappable_app_index_ = -1;
   int last_tappable_app_index_ = -1;
 
-  // The number of activated buttons that need to have an ink drop clipping at
-  // the edges.
+  // The number of corner buttons whose ink drop is activated.
   int activated_corner_buttons_ = 0;
 
   // Whether this view should focus its last focusable child (instead of its

ash/shelf/scrollable_shelf_view_unittest.cc

@@ -684,6 +684,90 @@ TEST_F(HotseatScrollableShelfViewTest, CheckRoundedCornersSetForInkDrop) {
   EXPECT_FALSE(HasRoundedCornersOnAppButtonAfterMouseRightClick(first_icon));
 }
 
+// Verify that the rounded corners work as expected after transition from
+// clamshell mode to tablet mode (https://crbug.com/1086484).
+TEST_F(ScrollableShelfViewTest, CheckRoundedCornersAfterTabletStateTransition) {
+  ui::ScopedAnimationDurationScaleMode regular_animations(
+      ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
+  PopulateAppShortcut(1);
+
+  ShelfAppButton* icon = test_api_->GetButton(0);
+
+  // Right click on the app button to activate the ripple ring.
+  GetEventGenerator()->MoveMouseTo(icon->GetBoundsInScreen().CenterPoint());
+  GetEventGenerator()->ClickRightButton();
+  {
+    InkDropAnimationWaiter waiter(icon);
+    waiter.Wait();
+  }
+  ASSERT_EQ(views::InkDropState::ACTIVATED,
+            icon->GetInkDrop()->GetTargetInkDropState());
+
+  // Verify that in clamshell when the ripple ring is activated, the rounded
+  // corners should not be applied.
+  EXPECT_TRUE(
+      scrollable_shelf_view_->IsAnyCornerButtonInkDropActivatedForTest());
+  EXPECT_TRUE(scrollable_shelf_view_->shelf_container_view()
+                  ->layer()
+                  ->rounded_corner_radii()
+                  .IsEmpty());
+
+  // Switch to tablet mode. The ripple ring should be hidden.
+  Shell::Get()->tablet_mode_controller()->SetEnabledForTest(true);
+  {
+    InkDropAnimationWaiter waiter(icon);
+    waiter.Wait();
+  }
+  EXPECT_EQ(views::InkDropState::HIDDEN,
+            icon->GetInkDrop()->GetTargetInkDropState());
+
+  // Verify that the rounded corners should not be applied when the ripple ring
+  // is hidden.
+  ASSERT_TRUE(scrollable_shelf_view_->shelf_container_view()
+                  ->layer()
+                  ->rounded_corner_radii()
+                  .IsEmpty());
+  EXPECT_FALSE(
+      scrollable_shelf_view_->IsAnyCornerButtonInkDropActivatedForTest());
+}
+
+// Verify that the count of activated corner buttons is expected after removing
+// an app icon from context menu (https://crbug.com/1086484).
+TEST_F(ScrollableShelfViewTest,
+       CheckRoundedCornersAfterUnpinningFromContextMenu) {
+  ui::ScopedAnimationDurationScaleMode regular_animations(
+      ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
+  Shell::Get()->tablet_mode_controller()->SetEnabledForTest(true);
+
+  AddAppShortcut();
+  const ShelfID app_id = AddAppShortcut();
+  ASSERT_EQ(2, test_api_->GetButtonCount());
+
+  ShelfModel* shelf_model = ShelfModel::Get();
+  const int index = shelf_model->ItemIndexByID(app_id);
+
+  ShelfAppButton* icon = test_api_->GetButton(index);
+
+  // Right click on the app button to activate the ripple ring.
+  GetEventGenerator()->MoveMouseTo(icon->GetBoundsInScreen().CenterPoint());
+  GetEventGenerator()->ClickRightButton();
+  {
+    InkDropAnimationWaiter waiter(icon);
+    waiter.Wait();
+  }
+  EXPECT_EQ(views::InkDropState::ACTIVATED,
+            icon->GetInkDrop()->GetTargetInkDropState());
+
+  // Emulate to remove a shelf icon from context menu.
+  shelf_model->RemoveItemAt(index);
+  test_api_->RunMessageLoopUntilAnimationsDone();
+  ASSERT_EQ(1, test_api_->GetButtonCount());
+
+  // Verify the count of activated corner buttons.
+  EXPECT_FALSE(
+      scrollable_shelf_view_->IsAnyCornerButtonInkDropActivatedForTest());
+}
+
 // Verifies that when two shelf app buttons are animating at the same time,
 // rounded corners are being kept if needed. (see https://crbug.com/1079330)
 TEST_F(ScrollableShelfViewTest, CheckRoundedCornersAfterLongPress) {

ash/shelf/shelf_app_button.cc

@@ -887,7 +887,12 @@ void ShelfAppButton::SetInkDropAnimationStarted(bool started) {
     return;
 
   ink_drop_animation_started_ = started;
-  shelf_button_delegate()->NotifyInkDropActivity(started, /*sender=*/this);
+  if (started) {
+    ink_drop_count_ = shelf_button_delegate()->CreateScopedActiveInkDropCount(
+        /*sender=*/this);
+  } else {
+    ink_drop_count_.reset(nullptr);
+  }
 }
 
 }  // namespace ash

ash/shelf/shelf_app_button.h

@@ -7,6 +7,7 @@
 
 #include "ash/ash_export.h"
 #include "ash/shelf/shelf_button.h"
+#include "ash/shelf/shelf_button_delegate.h"
 #include "base/macros.h"
 #include "base/timer/timer.h"
 #include "ui/compositor/layer_animation_observer.h"
@@ -195,6 +196,9 @@ class ASH_EXPORT ShelfAppButton : public ShelfButton,
   // A timer to activate the ink drop ripple during a long press.
   base::OneShotTimer ripple_activation_timer_;
 
+  std::unique_ptr<ShelfButtonDelegate::ScopedActiveInkDropCount>
+      ink_drop_count_;
+
   DISALLOW_COPY_AND_ASSIGN(ShelfAppButton);
 };
 

ash/shelf/shelf_button_delegate.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ash/shelf/shelf_button_delegate.h"
+
+namespace ash {
+
+std::unique_ptr<ShelfButtonDelegate::ScopedActiveInkDropCount>
+ShelfButtonDelegate::CreateScopedActiveInkDropCount(const ShelfButton* sender) {
+  return nullptr;
+}
+
+}  // namespace ash

ash/shelf/shelf_button_delegate.h

@@ -28,6 +28,11 @@ class ShelfButton;
 // return value can be merged into ButtonListener.
 class ShelfButtonDelegate {
  public:
+  class ScopedActiveInkDropCount {
+   public:
+    virtual ~ScopedActiveInkDropCount() = default;
+  };
+
   ShelfButtonDelegate() {}
   ~ShelfButtonDelegate() = default;
 
@@ -47,8 +52,12 @@ class ShelfButtonDelegate {
   // focus.
   virtual void HandleAccessibleActionScrollToMakeVisible(ShelfButton* button) {}
 
-  // Notify the host view of the change in |sender|'s ink drop view.
-  virtual void NotifyInkDropActivity(bool activated, views::Button* sender) {}
+  // Returns a scoped count that indicates whether |button| has an active ink
+  // drop. |button| calls this to get the scoped count when its ink drop is
+  // activated. It holds on to the scoped count until the ink drop is no longer
+  // active.
+  virtual std::unique_ptr<ScopedActiveInkDropCount>
+  CreateScopedActiveInkDropCount(const ShelfButton* button);
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ShelfButtonDelegate);