[Merge to M-101][Shelf] Fix temporarily incorrect index value during …

…item fade out

When a shelf item is removed, `ScrollableShelfView::last_tappable_app-
_index_` updates when the shelf button view is removed from the view
hierarchy. However, removing view from view hierarchy is performed
at the end of the fade out animation while the view is removed from
the view model at the beginning of animation. This discrepancy could
cause subtle issues when users interact with shelf during the fade
out animation.

Ideally, removing the view from view model should be postponed to
the end of the fade out animation as well. But this change will bring
the side effect that the item view to be removed is interactive during
the animation. See `ShelfItemForView` used in `ShelfView::OnMenuClosed`
and other places. This side effect may be hard to eliminate (for
example, in `ShelfView`, `view_model_` is used in many places).

This CL performs a quick fixing by notifying `ScrollableShelfView`
of shelf item removal before the fade out animation starts. Then
`ScrollableShelfView` can decrease `last_tappable_app_index_` instantly.

(cherry picked from commit 7412ebc)

Bug: 1300561
Change-Id: I812d919185fbac4e9447904cce6f59d54df3f476
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3532928
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Commit-Queue: Andrew Xu <andrewxu@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#983601}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3584271
Cr-Commit-Position: refs/branch-heads/4951@{#705}
Cr-Branched-From: 27de622-refs/heads/main@{#982481}

ash/shelf/scrollable_shelf_view.cc

@@ -1013,6 +1013,26 @@ void ScrollableShelfView::HandleAccessibleActionScrollToMakeVisible(
   GetShelf()->shelf_widget()->shelf_layout_manager()->UpdateVisibilityState();
 }
 
+void ScrollableShelfView::OnButtonWillBeRemoved() {
+  const int view_size_before_removal = shelf_view_->view_model()->view_size();
+  DCHECK_GT(view_size_before_removal, 0);
+
+  // Ensure `last_tappable_app_index_` to be valid after removal. Normally
+  // `last_tappable_app_index_` updates when the shelf button is removed. But
+  // button removal could be performed at the end of the button fade out
+  // animation, which means that incorrect `last_tappable_app_index_` could be
+  // accessed during the animation. To handle this issue, update
+  // `last_tappable_app_index_` before removal finishes.
+  // The code block also covers the edge case that the only shelf item is going
+  // to be removed, i.e. `view_size_before_removal_` is one. In this case,
+  // both `first_tappable_app_index_` and `last_tappable_app_index_` are reset
+  // to invalid values (see https://crbug.com/1300561).
+  last_tappable_app_index_ =
+      std::min(last_tappable_app_index_, view_size_before_removal - 2);
+  first_tappable_app_index_ =
+      std::min(first_tappable_app_index_, last_tappable_app_index_);
+}
+
 std::unique_ptr<ScrollableShelfView::ScopedActiveInkDropCount>
 ScrollableShelfView::CreateScopedActiveInkDropCount(const ShelfButton* sender) {
   if (!ShouldCountActivatedInkDrop(sender))
@@ -2184,8 +2204,11 @@ void ScrollableShelfView::OnActiveInkDropChange(bool increase) {
   // 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.
+  // NOTE: `last_tappable_app_index_` is used to compute whether a button is
+  // at the corner or not. Meanwhile, `last_tappable_app_index_` could update
+  // before the button fade out animation ends. As a result, in edge cases
+  // `activated_corner_buttons_` could be greater than 2.
   CHECK_GE(activated_corner_buttons_, 0);
-  CHECK_LE(activated_corner_buttons_, 2);
   EnableShelfRoundedCorners(activated_corner_buttons_ > 0);
 }
 

ash/shelf/scrollable_shelf_view.h

@@ -248,6 +248,7 @@ class ASH_EXPORT ScrollableShelfView : public views::AccessiblePaneView,
   void HandleAccessibleActionScrollToMakeVisible(ShelfButton* button) override;
   std::unique_ptr<ScopedActiveInkDropCount> CreateScopedActiveInkDropCount(
       const ShelfButton* sender) override;
+  void OnButtonWillBeRemoved() override;
 
   // ContextMenuController:
   void ShowContextMenuForViewImpl(views::View* source,

ash/shelf/scrollable_shelf_view_unittest.cc

@@ -1214,6 +1214,34 @@ TEST_P(ScrollableShelfViewRTLTest, ClickAtLastIcon) {
   EXPECT_FALSE(shelf_view_->IsShowingMenuForView(last_icon));
 }
 
+// Verifies that mouse click at the second last shelf item during the last item
+// removal animation does not lead to crash (see https://crbug.com/1300561).
+TEST_F(ScrollableShelfViewTest, RemoveLastItemWhileClickingSeoncdLastOne) {
+  PopulateAppShortcut(3);
+  ASSERT_EQ(ScrollableShelfView::kNotShowArrowButtons,
+            scrollable_shelf_view_->layout_strategy_for_test());
+
+  const int view_size_before_removal =
+      shelf_view_->view_model_for_test()->view_size();
+  {
+    // Remove the last shelf item with animation enabled.
+    ui::ScopedAnimationDurationScaleMode regular_animations(
+        ui::ScopedAnimationDurationScaleMode::SLOW_DURATION);
+    ShelfModel::Get()->RemoveItemAt(view_size_before_removal - 1);
+    EXPECT_TRUE(shelf_view_->IsAnimating());
+  }
+
+  // Mouse right click at the second last item and wait for the ink drop
+  // animation to complete.
+  ShelfAppButton* second_last_item =
+      ShelfViewTestAPI(shelf_view_).GetButton(view_size_before_removal - 2);
+  GetEventGenerator()->MoveMouseTo(
+      second_last_item->GetBoundsInScreen().CenterPoint());
+  InkDropAnimationWaiter waiter(second_last_item);
+  GetEventGenerator()->ClickRightButton();
+  waiter.Wait();
+}
+
 // Verifies that presentation time for shelf gesture scroll is recorded as
 // expected (https://crbug.com/1095259).
 TEST_F(ScrollableShelfViewTest, PresentationTimeMetricsForGestureScroll) {

ash/shelf/shelf_button_delegate.h

@@ -61,6 +61,9 @@ class ShelfButtonDelegate {
   // active.
   virtual std::unique_ptr<ScopedActiveInkDropCount>
   CreateScopedActiveInkDropCount(const ShelfButton* button);
+
+  // Notifies the host view that one button will be removed.
+  virtual void OnButtonWillBeRemoved() {}
 };
 
 }  // namespace ash

ash/shelf/shelf_view.cc

@@ -2112,6 +2112,8 @@ void ShelfView::ShelfItemRemoved(int model_index, const ShelfItem& old_item) {
   // If std::move is not called on |view|, |view| will be deleted once out of
   // scope.
   std::unique_ptr<views::View> view(view_model_->view_at(model_index));
+
+  shelf_button_delegate_->OnButtonWillBeRemoved();
   view_model_->Remove(model_index);
 
   if (old_item.id == context_menu_id_ && shelf_menu_model_adapter_)