diff --git a/ash/shelf/scrollable_shelf_view.cc b/ash/shelf/scrollable_shelf_view.cc index 277f2c7dd65f8e..997d45009bacbf 100644 --- a/ash/shelf/scrollable_shelf_view.cc +++ b/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::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); } diff --git a/ash/shelf/scrollable_shelf_view.h b/ash/shelf/scrollable_shelf_view.h index dc337025610554..5feba440e00fe2 100644 --- a/ash/shelf/scrollable_shelf_view.h +++ b/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 CreateScopedActiveInkDropCount( const ShelfButton* sender) override; + void OnButtonWillBeRemoved() override; // ContextMenuController: void ShowContextMenuForViewImpl(views::View* source, diff --git a/ash/shelf/scrollable_shelf_view_unittest.cc b/ash/shelf/scrollable_shelf_view_unittest.cc index 0b81901c9018ce..d16af9f64ed628 100644 --- a/ash/shelf/scrollable_shelf_view_unittest.cc +++ b/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) { diff --git a/ash/shelf/shelf_button_delegate.h b/ash/shelf/shelf_button_delegate.h index 76b6f2db2f4852..f8ccd4eede30c1 100644 --- a/ash/shelf/shelf_button_delegate.h +++ b/ash/shelf/shelf_button_delegate.h @@ -61,6 +61,9 @@ class ShelfButtonDelegate { // active. virtual std::unique_ptr CreateScopedActiveInkDropCount(const ShelfButton* button); + + // Notifies the host view that one button will be removed. + virtual void OnButtonWillBeRemoved() {} }; } // namespace ash diff --git a/ash/shelf/shelf_view.cc b/ash/shelf/shelf_view.cc index 47316ed4e43a41..60bae1d7717adc 100644 --- a/ash/shelf/shelf_view.cc +++ b/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 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_)