Skip to content

Commit

Permalink
Prevent crash after moving the last item from a folder
Browse files Browse the repository at this point in the history
Existing logic of unfoldering the last item to the right:
1) calculate target position as "folder index + 1";
2) move item at that position;
3) delete the folder;
4) reparented item moves one position back.

The issue was that code continues using position from 1) after 4).
This leads to:
1) focusing the wrong (+ 1) item, if the folder was not at the end;
2) crash, if the folder was at the end of the grid.

(cherry picked from commit efe4528)

Bug: b/262715563
Change-Id: I3208635b8b612057440fc5be993bd8abb5e40b1b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4117547
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Commit-Queue: Artsiom Mitrokhin <amitrokhin@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1086060}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4135539
Cr-Commit-Position: refs/branch-heads/5481@{#120}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
  • Loading branch information
Artsiom Mitrokhin authored and Chromium LUCI CQ committed Jan 4, 2023
1 parent 8b1140e commit 55bc7f1
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 3 deletions.
19 changes: 16 additions & 3 deletions ash/app_list/views/apps_grid_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2045,21 +2045,34 @@ void AppsGridView::HandleKeyboardReparent(
DCHECK(!folder_delegate_);
DCHECK(view_model_.GetIndexOfView(original_parent_item_view).has_value());

const std::string reparented_item_id = reparented_view->item()->id();

// Set |original_parent_item_view| selected so |target_index| will be
// computed relative to the open folder.
SetSelectedView(original_parent_item_view);
const GridIndex target_index = GetTargetGridIndexForKeyboardReparent(
GetIndexOfView(original_parent_item_view), key_code);
ReparentItemForReorder(reparented_view->item(), target_index);

// `target_index` could point to an invalid/wrong position after reparenting.
// This happens after trying to move the last item from the folder
// to the right (`target_index` is "folder index + 1", but after reparenting
// it actually moves one position back).
const AppListItem* const item_after_reparent =
item_list_->FindItem(reparented_item_id);
DCHECK(item_after_reparent);
const int final_model_index = GetModelIndexOfItem(item_after_reparent);
const GridIndex final_grid_index =
GetGridIndexFromIndexInViewModel(final_model_index);

// Update paging because the move could have resulted in a
// page getting created.
UpdatePaging();

Layout();
EnsureViewVisible(target_index);
GetViewAtIndex(target_index)->RequestFocus();
AnnounceReorder(target_index);
EnsureViewVisible(final_grid_index);
GetViewAtIndex(final_grid_index)->RequestFocus();
AnnounceReorder(final_grid_index);

RecordAppMovingTypeMetrics(kMoveByKeyboardOutOfFolder);
}
Expand Down
33 changes: 33 additions & 0 deletions ash/app_list/views/apps_grid_view_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3250,6 +3250,39 @@ TEST_P(AppsGridViewClamshellAndTabletTest,
EXPECT_TRUE(apps_grid_view_->IsSelectedView(new_folder));
}

TEST_P(AppsGridViewClamshellAndTabletTest,
MoveLastItemFromFolderToRightDoesNotCrash) {
ui::test::EventGenerator* const event_generator = GetEventGenerator();

// Create a folder with two items in it.
model_->CreateAndPopulateFolderWithApps(2);
AppListItemView* folder_view = test_api_->GetViewAtIndex(GridIndex(0, 0));

// Open the folder.
folder_view->RequestFocus();
event_generator->PressAndReleaseKey(ui::VKEY_RETURN);
EXPECT_TRUE(GetAppListTestHelper()->IsInFolderView());

// An item inside the folder should be in focus. Move it to the left to put it
// before the folder icon.
event_generator->PressAndReleaseKey(ui::VKEY_LEFT,
ui::EF_CONTROL_DOWN | ui::EF_SHIFT_DOWN);
EXPECT_FALSE(GetAppListTestHelper()->IsInFolderView());
EXPECT_TRUE(test_api_->GetViewAtIndex(GridIndex(0, 0))->HasFocus());

// Open the folder again.
folder_view->RequestFocus();
event_generator->PressAndReleaseKey(ui::VKEY_RETURN);
EXPECT_TRUE(GetAppListTestHelper()->IsInFolderView());

// An item inside the folder should be in focus. Move it to the right to put
// it after/instead the folder icon. No crash happens.
event_generator->PressAndReleaseKey(ui::VKEY_RIGHT,
ui::EF_CONTROL_DOWN | ui::EF_SHIFT_DOWN);
EXPECT_FALSE(GetAppListTestHelper()->IsInFolderView());
EXPECT_TRUE(test_api_->GetViewAtIndex(GridIndex(0, 1))->HasFocus());
}

TEST_P(AppsGridViewTabletTest, TouchDragFlipToNextPage) {
ASSERT_TRUE(paged_apps_grid_view_);

Expand Down

0 comments on commit 55bc7f1

Please sign in to comment.