Keep the active tab within a group when closing

Additionally, slightly refactor DetermineNewSelectedIndex() so that it feels less upside-down: handle the "closing inactive tab" case first. Judging by the tests and the way this is called in TabStripModel::DetachWebContentsImpl(), this is the intended order of behavior.

I wasn't able to manually reproduce a case where a grouped tab has an opener. I believe this is actually a different bug in groups: when a tab is opened by an opener, it doesn't get grouped. I'll log this separately (it's already in the Remaining Work doc for Tab Groups). For now, I kept the group logic after the opener logic, so that in most cases it will continue to behave as before.

From my experimentation, this works as expected when closing multiple tabs at once, including if they're non-consecutive or in different groups. The behavior just falls back to closing one tab at a time, so it will respect the group of the rightmost tab, which is the last to become active (see TabStripModel::DetachWebContentsImpl()).

Bug: 992512
Change-Id: I347fc52c0b0a2b5aca84a3ad2e4a6264bf8b11ee
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1834641
Commit-Queue: Connie Wan <connily@chromium.org>
Reviewed-by: Taylor Bergquist <tbergquist@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702625}

chrome/browser/ui/tabs/tab_strip_model.cc

@@ -477,9 +477,11 @@ std::unique_ptr<content::WebContents> TabStripModel::DetachWebContentsImpl(
   if (create_historical_tab)
     delegate_->CreateHistoricalTab(raw_web_contents);
 
+  base::Optional<int> next_selected_index =
+      order_controller_->DetermineNewSelectedIndex(index);
+
   NotifyGroupChange(index, UngroupTab(index), base::nullopt);
 
-  int next_selected_index = order_controller_->DetermineNewSelectedIndex(index);
   std::unique_ptr<WebContentsData> old_data = std::move(contents_data_[index]);
   contents_data_.erase(contents_data_.begin() + index);
 
@@ -497,9 +499,10 @@ std::unique_ptr<content::WebContents> TabStripModel::DetachWebContentsImpl(
         selection_model_.set_active(selection_model_.selected_indices()[0]);
         selection_model_.set_anchor(selection_model_.active());
       } else {
+        DCHECK(next_selected_index.has_value());
         // The active tab was removed and nothing is selected. Reset the
         // selection and send out notification.
-        selection_model_.SetSelectedIndex(next_selected_index);
+        selection_model_.SetSelectedIndex(next_selected_index.value());
       }
     }
   }

chrome/browser/ui/tabs/tab_strip_model_order_controller.cc

@@ -48,10 +48,20 @@ int TabStripModelOrderController::DetermineInsertionIndex(
   return tabstrip_->count();
 }
 
-int TabStripModelOrderController::DetermineNewSelectedIndex(
+base::Optional<int> TabStripModelOrderController::DetermineNewSelectedIndex(
     int removing_index) const {
-  int tab_count = tabstrip_->count();
-  DCHECK(removing_index >= 0 && removing_index < tab_count);
+  DCHECK(tabstrip_->ContainsIndex(removing_index));
+
+  // The case where the closed tab is inactive is handled directly in
+  // TabStripModel.
+  if (removing_index != tabstrip_->active_index())
+    return base::nullopt;
+
+  // The case where multiple tabs are selected is handled directly in
+  // TabStripModel.
+  if (tabstrip_->selection_model().size() > 1)
+    return base::nullopt;
+
   content::WebContents* parent_opener =
       tabstrip_->GetOpenerOfWebContentsAt(removing_index);
   // First see if the index being removed has any "child" tabs. If it does, we
@@ -81,12 +91,27 @@ int TabStripModelOrderController::DetermineNewSelectedIndex(
       return GetValidIndex(index, removing_index);
   }
 
-  // No opener set, fall through to the default handler...
-  int selected_index = tabstrip_->active_index();
-  if (selected_index >= (tab_count - 1))
-    return selected_index - 1;
+  // If closing a grouped tab, return a tab that is still in the group, if any.
+  const base::Optional<TabGroupId> current_group =
+      tabstrip_->GetTabGroupForTab(removing_index);
+  if (current_group.has_value()) {
+    // Match the default behavior below: prefer the tab to the right.
+    const base::Optional<TabGroupId> right_group =
+        tabstrip_->GetTabGroupForTab(removing_index + 1);
+    if (current_group == right_group)
+      return removing_index;
+
+    const base::Optional<TabGroupId> left_group =
+        tabstrip_->GetTabGroupForTab(removing_index - 1);
+    if (current_group == left_group)
+      return removing_index - 1;
+  }
+
+  // By default, return the tab on the right, unless this is the last tab.
+  if (removing_index >= (tabstrip_->count() - 1))
+    return removing_index - 1;
 
-  return selected_index;
+  return removing_index;
 }
 
 void TabStripModelOrderController::OnTabStripModelChanged(
@@ -128,8 +153,8 @@ void TabStripModelOrderController::OnTabStripModelChanged(
 ///////////////////////////////////////////////////////////////////////////////
 // TabStripModelOrderController, private:
 
-int TabStripModelOrderController::GetValidIndex(
-    int index, int removing_index) const {
+int TabStripModelOrderController::GetValidIndex(int index,
+                                                int removing_index) const {
   if (removing_index < index)
     index = std::max(0, index - 1);
   return index;

chrome/browser/ui/tabs/tab_strip_model_order_controller.h

@@ -28,7 +28,7 @@ class TabStripModelOrderController : public TabStripModelObserver {
                               bool foreground);
 
   // Determine where to shift selection after a tab is closed.
-  int DetermineNewSelectedIndex(int removed_index) const;
+  base::Optional<int> DetermineNewSelectedIndex(int removed_index) const;
 
   // Overridden from TabStripModelObserver:
   void OnTabStripModelChanged(

chrome/browser/ui/tabs/tab_strip_model_unittest.cc

@@ -1011,89 +1011,141 @@ TEST_F(TabStripModelTest, TestInsertionIndexDeterminationNestedOpener) {
 }
 
 // Tests that selection is shifted to the correct tab when a tab is closed.
-// If a tab is in the background when it is closed, the selection does not
+// If the tab is in the background when it is closed, the selection does not
 // change.
-// If a tab is in the foreground (selected),
-//   If that tab does not have an opener, selection shifts to the right.
-//   If the tab has an opener,
-//     The next tab (scanning LTR) in the entire strip that has the same opener
-//     is selected
-//     If there are no other tabs that have the same opener,
-//       The opener is selected
-TEST_F(TabStripModelTest, TestSelectOnClose) {
+TEST_F(TabStripModelTest, CloseInactiveTabKeepsSelection) {
   TestTabStripModelDelegate delegate;
   TabStripModel tabstrip(&delegate, profile());
-  EXPECT_TRUE(tabstrip.empty());
+  ASSERT_TRUE(tabstrip.empty());
 
   std::unique_ptr<WebContents> opener = CreateWebContents();
   tabstrip.AppendWebContents(std::move(opener), true);
+  InsertWebContentses(&tabstrip, CreateWebContents(), CreateWebContents(),
+                      CreateWebContents());
+  ASSERT_EQ(0, tabstrip.active_index());
 
-  std::unique_ptr<WebContents> contentses[3];
-  for (int i = 0; i < 3; ++i)
-    contentses[i] = CreateWebContents();
-
-  // Note that we use Detach instead of Close throughout this test to avoid
-  // having to keep reconstructing these WebContentses.
-
-  // First test that closing tabs that are in the background doesn't adjust the
-  // current selection.
-  InsertWebContentses(&tabstrip, std::move(contentses[0]),
-                      std::move(contentses[1]), std::move(contentses[2]));
+  tabstrip.CloseWebContentsAt(1, TabStripModel::CLOSE_NONE);
   EXPECT_EQ(0, tabstrip.active_index());
-
-  contentses[0] = tabstrip.DetachWebContentsAt(1);
+  tabstrip.CloseWebContentsAt(1, TabStripModel::CLOSE_NONE);
+  EXPECT_EQ(0, tabstrip.active_index());
+  tabstrip.CloseWebContentsAt(1, TabStripModel::CLOSE_NONE);
   EXPECT_EQ(0, tabstrip.active_index());
 
-  for (int i = tabstrip.count() - 1; i >= 1; --i)
-    contentses[i] = tabstrip.DetachWebContentsAt(i);
+  tabstrip.CloseAllTabs();
+  ASSERT_TRUE(tabstrip.empty());
+}
 
-  // Now test that when a tab doesn't have an opener, selection shifts to the
-  // right when the tab is closed.
-  InsertWebContentses(&tabstrip, std::move(contentses[0]),
-                      std::move(contentses[1]), std::move(contentses[2]));
-  EXPECT_EQ(0, tabstrip.active_index());
+// Tests that selection is shifted to the correct tab when a tab is closed.
+// If the tab doesn't have an opener or a group, selection shifts to the right.
+TEST_F(TabStripModelTest, CloseActiveTabShiftsSelectionRight) {
+  TestTabStripModelDelegate delegate;
+  TabStripModel tabstrip(&delegate, profile());
+  ASSERT_TRUE(tabstrip.empty());
+
+  std::unique_ptr<WebContents> opener = CreateWebContents();
+  tabstrip.AppendWebContents(std::move(opener), true);
+  InsertWebContentses(&tabstrip, CreateWebContents(), CreateWebContents(),
+                      CreateWebContents());
+  ASSERT_EQ(0, tabstrip.active_index());
 
   tabstrip.ForgetAllOpeners();
   tabstrip.ActivateTabAt(1, {TabStripModel::GestureType::kOther});
+  ASSERT_EQ(1, tabstrip.active_index());
+
+  tabstrip.CloseWebContentsAt(1, TabStripModel::CLOSE_NONE);
   EXPECT_EQ(1, tabstrip.active_index());
-  contentses[0] = tabstrip.DetachWebContentsAt(1);
-  EXPECT_EQ(1, tabstrip.active_index());
-  contentses[1] = tabstrip.DetachWebContentsAt(1);
+  tabstrip.CloseWebContentsAt(1, TabStripModel::CLOSE_NONE);
   EXPECT_EQ(1, tabstrip.active_index());
-  contentses[2] = tabstrip.DetachWebContentsAt(1);
+  tabstrip.CloseWebContentsAt(1, TabStripModel::CLOSE_NONE);
   EXPECT_EQ(0, tabstrip.active_index());
 
-  EXPECT_EQ(1, tabstrip.count());
+  tabstrip.CloseAllTabs();
+  ASSERT_TRUE(tabstrip.empty());
+}
+
+// Tests that selection is shifted to the correct tab when a tab is closed.
+// If the tab doesn't have an opener but is in a group, selection shifts to
+// another tab in the same group.
+TEST_F(TabStripModelTest, CloseGroupedTabShiftsSelectionWithinGroup) {
+  TestTabStripModelDelegate delegate;
+  TabStripModel tabstrip(&delegate, profile());
+  ASSERT_TRUE(tabstrip.empty());
+
+  std::unique_ptr<WebContents> opener = CreateWebContents();
+  tabstrip.AppendWebContents(std::move(opener), true);
+  InsertWebContentses(&tabstrip, CreateWebContents(), CreateWebContents(),
+                      CreateWebContents());
+  ASSERT_EQ(0, tabstrip.active_index());
+
+  tabstrip.ForgetAllOpeners();
+  tabstrip.AddToNewGroup({0, 1, 2});
+  tabstrip.ActivateTabAt(1, {TabStripModel::GestureType::kOther});
+  ASSERT_EQ(1, tabstrip.active_index());
 
-  // Now test that when a tab does have an opener, it selects the next tab
-  // opened by the same opener scanning LTR when it is closed.
-  InsertWebContentses(&tabstrip, std::move(contentses[0]),
-                      std::move(contentses[1]), std::move(contentses[2]));
+  tabstrip.CloseWebContentsAt(1, TabStripModel::CLOSE_NONE);
+  EXPECT_EQ(1, tabstrip.active_index());
+  tabstrip.CloseWebContentsAt(1, TabStripModel::CLOSE_NONE);
   EXPECT_EQ(0, tabstrip.active_index());
+  tabstrip.CloseWebContentsAt(1, TabStripModel::CLOSE_NONE);
+  EXPECT_EQ(0, tabstrip.active_index());
+
+  tabstrip.CloseAllTabs();
+  ASSERT_TRUE(tabstrip.empty());
+}
+
+// Tests that selection is shifted to the correct tab when a tab is closed.
+// If the tab does have an opener, selection shifts to the next tab opened by
+// the same opener scanning LTR.
+TEST_F(TabStripModelTest, CloseTabWithOpenerShiftsSelectionWithinOpened) {
+  TestTabStripModelDelegate delegate;
+  TabStripModel tabstrip(&delegate, profile());
+  ASSERT_TRUE(tabstrip.empty());
+
+  std::unique_ptr<WebContents> opener = CreateWebContents();
+  tabstrip.AppendWebContents(std::move(opener), true);
+  InsertWebContentses(&tabstrip, CreateWebContents(), CreateWebContents(),
+                      CreateWebContents());
+  ASSERT_EQ(0, tabstrip.active_index());
+
   tabstrip.ActivateTabAt(2, {TabStripModel::GestureType::kOther});
-  EXPECT_EQ(2, tabstrip.active_index());
+  ASSERT_EQ(2, tabstrip.active_index());
+
   tabstrip.CloseWebContentsAt(2, TabStripModel::CLOSE_NONE);
   EXPECT_EQ(2, tabstrip.active_index());
   tabstrip.CloseWebContentsAt(2, TabStripModel::CLOSE_NONE);
   EXPECT_EQ(1, tabstrip.active_index());
   tabstrip.CloseWebContentsAt(1, TabStripModel::CLOSE_NONE);
   EXPECT_EQ(0, tabstrip.active_index());
-  // Finally test that when a tab has no "siblings" that the opener is
-  // selected.
+
+  tabstrip.CloseAllTabs();
+  ASSERT_TRUE(tabstrip.empty());
+}
+
+// Tests that selection is shifted to the correct tab when a tab is closed.
+// If the tab has an opener but no "siblings" opened by the same opener,
+// selection shifts to the opener itself.
+TEST_F(TabStripModelTest, CloseTabWithOpenerShiftsSelectionToOpener) {
+  TestTabStripModelDelegate delegate;
+  TabStripModel tabstrip(&delegate, profile());
+  ASSERT_TRUE(tabstrip.empty());
+
+  std::unique_ptr<WebContents> opener = CreateWebContents();
+  tabstrip.AppendWebContents(std::move(opener), true);
   std::unique_ptr<WebContents> other_contents = CreateWebContents();
   tabstrip.InsertWebContentsAt(1, std::move(other_contents),
                                TabStripModel::ADD_NONE);
-  EXPECT_EQ(2, tabstrip.count());
+  ASSERT_EQ(2, tabstrip.count());
   std::unique_ptr<WebContents> opened_contents = CreateWebContents();
   tabstrip.InsertWebContentsAt(
       2, std::move(opened_contents),
       TabStripModel::ADD_ACTIVE | TabStripModel::ADD_INHERIT_OPENER);
-  EXPECT_EQ(2, tabstrip.active_index());
+  ASSERT_EQ(2, tabstrip.active_index());
+
   tabstrip.CloseWebContentsAt(2, TabStripModel::CLOSE_NONE);
   EXPECT_EQ(0, tabstrip.active_index());
 
   tabstrip.CloseAllTabs();
-  EXPECT_TRUE(tabstrip.empty());
+  ASSERT_TRUE(tabstrip.empty());
 }
 
 // Tests IsContextMenuCommandEnabled and ExecuteContextMenuCommand with