Improve tab drag interaction with groups

The attached bug contains details on where the previous logic in GetTabGroupForTargetIndex was causing different behavior based on group size.

This CL updates the logic to focus more on current position than previous group membership. As noted in the inline comments:

  (If the tab is between two tabs with the same group membership, match that group.)

  // If the tabs on the left and right have different group memberships,
  // including if one is ungrouped or nonexistent, change the group of the
  // dragged tab based on whether it is "leaning" toward the left or the
  // right of the gap. If the tab is centered in the gap, make the tab
  // ungrouped.

  ("Leaning" is defined by being 1/4 the tab width toward either side of the gap.)

  // Extra polish: Prefer staying in an existing group, if any. This prevents
  // tabs at the edge of the group from flickering between grouped and
  // ungrouped. It also gives groups a slightly "sticky" feel while dragging.

  (There is also some special handling for the far left and far right of the tab strip.)

A screencast of this interaction is attached to the bug.

Bug: 1004946
Change-Id: Id3cf631354a47ffa5c3f1aeacdc82bd2af13ea24
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1823729
Commit-Queue: Connie Wan <connily@chromium.org>
Reviewed-by: Charlene Yan <cyan@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#700449}

chrome/browser/ui/tabs/tab_strip_model.cc

@@ -1708,7 +1708,6 @@ WebContents* TabStripModel::GetWebContentsAtImpl(int index) const {
   return contents_data_[index]->web_contents();
 }
 
-
 TabStripSelectionChange TabStripModel::SetSelection(
     ui::ListSelectionModel new_model,
     TabStripModelObserver::ChangeReason reason,

chrome/browser/ui/views/tabs/tab_drag_controller.cc

@@ -2091,21 +2091,14 @@ void TabDragController::UpdateGroupForDraggedTabs(int to_index) {
 
   const int current_index = selected[0];
 
-  // If the tab hasn't moved, there is no need to update tab group membership.
-  if (current_index == to_index)
-    return;
-  base::Optional<TabGroupId> updated_group =
+  const base::Optional<TabGroupId> updated_group =
       GetTabGroupForTargetIndex(current_index, to_index);
-  base::Optional<TabGroupId> current_selected_group =
+  const base::Optional<TabGroupId> current_selected_group =
       attached_model->GetTabGroupForTab(current_index);
 
-  // If the dragging all tabs in the group, we will not do anything since the
-  // group membership will not change.
-  // TODO(crbug.com/978609): Support multi-select drag case.
-  if (current_selected_group.has_value() && updated_group.has_value() &&
-      current_selected_group.value() == updated_group.value()) {
+  if (updated_group == current_selected_group)
     return;
-  }
+
   if (updated_group.has_value()) {
     attached_model->MoveTabsIntoGroup({current_index}, to_index,
                                       updated_group.value());
@@ -2117,45 +2110,57 @@ void TabDragController::UpdateGroupForDraggedTabs(int to_index) {
 base::Optional<TabGroupId> TabDragController::GetTabGroupForTargetIndex(
     int current_index,
     int to_index) {
-  TabStripModel* attached_model = attached_context_->GetTabStripModel();
-  base::Optional<TabGroupId> current_group =
-      attached_model->GetTabGroupForTab(current_index);
+  const TabStripModel* attached_model = attached_context_->GetTabStripModel();
 
   // For the currently dragged tab, find the tab index of the tab to the left
   // (|left_tab_index|) and right (|right_tab_index|)) after this current move
-  // has finished.
-  auto left_tab_index = [](int current_index, int to_index) {
-    return current_index < to_index ? to_index : to_index - 1;
-  };
-  auto right_tab_index = [](int current_index, int to_index) {
-    return current_index < to_index ? to_index + 1 : to_index;
-  };
-
-  base::Optional<TabGroupId> left_group = attached_model->GetTabGroupForTab(
-      left_tab_index(current_index, to_index));
-  base::Optional<TabGroupId> right_group = attached_model->GetTabGroupForTab(
-      right_tab_index(current_index, to_index));
-
-  // First check if the tab bring dragged should join another group to ensure
-  // tab contiguity. This should happen if tabs to the immediate left and right
-  // of the tab are the same, the tab being dragged should match that. Next
-  // check if the tab being dragged contains all tabs of a particular group. If
-  // so, those tabs should remain in their tab group. If exactly one of the
-  // neighboring tabs is in the same group as the tab being dragged, the tab
-  // will remain in the group. Otherwise, the dragged tab will be ungrouped.
-  base::Optional<TabGroupId> updated_group = base::nullopt;
-  if (left_group == right_group && left_group.has_value()) {
-    updated_group = left_group;
-  } else if (current_group.has_value() &&
-             attached_model->ListTabsInGroup(current_group.value()).size() ==
-                 1) {
-    // Keep tab in tab group if dragging all tabs in the tab group.
-    // TODO(crbug.com/978609): Handle multi-select drag case.
-    return attached_model->GetTabGroupForTab(current_index);
-  } else if (left_group == current_group || right_group == current_group) {
-    updated_group = current_group;
-  }
-  return updated_group;
+  // has finished. If this is a left-drag or a right-drag, grab two adjacent
+  // indices near to_index. Otherwise, if there is no drag direction (i.e. if
+  // current_index == to_index), grab the indices to the left and the right.
+  const int left_tab_index = current_index < to_index ? to_index : to_index - 1;
+  const int right_tab_index =
+      current_index <= to_index ? to_index + 1 : to_index;
+
+  const base::Optional<TabGroupId> left_group =
+      attached_model->GetTabGroupForTab(left_tab_index);
+  const base::Optional<TabGroupId> right_group =
+      attached_model->GetTabGroupForTab(right_tab_index);
+  const base::Optional<TabGroupId> current_group =
+      attached_model->GetTabGroupForTab(current_index);
+
+  if (left_group == right_group)
+    return left_group;
+
+  // If the tabs on the left and right have different group memberships,
+  // including if one is ungrouped or nonexistent, change the group of the
+  // dragged tab based on whether it is "leaning" toward the left or the
+  // right of the gap. If the tab is centered in the gap, make the tab
+  // ungrouped.
+
+  // TODO(crbug.com/978609): Adjust this for multi-select case.
+  const Tab* current_tab = source_context_->GetTabAt(current_index);
+  const int buffer = current_tab->width() / 4;
+
+  // Use the left edge for a reliable fallback, e.g. if this is the leftmost
+  // tab or there is a group header to the immediate left.
+  int left_edge =
+      attached_model->ContainsIndex(left_tab_index)
+          ? source_context_->GetTabAt(left_tab_index)->bounds().right()
+          : 0;
+
+  // Extra polish: Prefer staying in an existing group, if any. This prevents
+  // tabs at the edge of the group from flickering between grouped and
+  // ungrouped. It also gives groups a slightly "sticky" feel while dragging.
+  if (left_group.has_value() && left_group == current_group)
+    left_edge += buffer;
+  if (right_group.has_value() && right_group == current_group && left_edge > 0)
+    left_edge -= buffer;
+
+  if (current_tab->x() <= left_edge - buffer)
+    return left_group;
+  if (current_tab->x() >= left_edge + buffer)
+    return right_group;
+  return base::nullopt;
 }
 
 bool TabDragController::ShouldDisallowDrag(gfx::NativeWindow window) {

chrome/browser/ui/views/tabs/tab_drag_controller.h

@@ -505,14 +505,13 @@ class TabDragController : public views::WidgetObserver {
 
   // Helper method for TabDragController::MoveAttached to update the tab group
   // membership of selected tabs.
-  // TODO (cyan): Make this work for dragging into a tab group.
   void UpdateGroupForDraggedTabs(int to_index);
 
   // Helper method for TabDragController::UpdateGroupForDraggedTabs to decide if
   // a dragged tab should stay in the tab group. Returns base::nullopt if the
   // tab should not be in a group. Otherwise returns TabGroupId of the group
   // being selected.
-  base::Optional<TabGroupId> GetTabGroupForTargetIndex(int index_of_selected,
+  base::Optional<TabGroupId> GetTabGroupForTargetIndex(int current_index,
                                                        int to_index);
 
   EventSource event_source_;

chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc

@@ -748,9 +748,9 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
 }
 
 // Creates a browser with four tabs. The first tab is in a Tab Group. Dragging
-// all tabs in a tab group (1 tab) will not modify the group of the dragged tab.
+// the only tab in that group will remove the group.
 IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
-                       DragSingleTabInGroupDoesNotModifyGroup) {
+                       DragOnlyTabInGroupRemovesGroup) {
   scoped_feature_list_.InitAndEnableFeature(features::kTabGroups);
 
   TabStrip* tab_strip = GetTabStripForBrowser(browser());
@@ -764,30 +764,22 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
   EXPECT_THAT(model->ListTabsInGroup(group), testing::ElementsAre(0));
 
   // Dragging the tab in the zero-th index to the tab in the first index
-  // switches the tabs but group membership does not change.
+  // switches the tabs and removes the group of the zero-th tab.
   ASSERT_TRUE(PressInput(GetCenterInScreenCoordinates(tab_strip->tab_at(0))));
   ASSERT_TRUE(DragInputTo(GetCenterInScreenCoordinates(tab_strip->tab_at(1))));
   ASSERT_TRUE(ReleaseInput());
   StopAnimating(tab_strip);
 
   EXPECT_EQ("1 0 2 3", IDString(model));
-  EXPECT_THAT(model->ListTabsInGroup(group), testing::ElementsAre(1));
-
-  ASSERT_TRUE(PressInput(GetCenterInScreenCoordinates(tab_strip->tab_at(1))));
-  ASSERT_TRUE(DragInputTo(GetCenterInScreenCoordinates(tab_strip->tab_at(0))));
-  ASSERT_TRUE(ReleaseInput());
-  StopAnimating(tab_strip);
-
-  // Dragging the tab in the first index to the tab in the zero-th index
-  // switches the tabs but group membership does not change.
-  EXPECT_EQ("0 1 2 3", IDString(model));
-  EXPECT_THAT(model->ListTabsInGroup(group), testing::ElementsAre(0));
+  EXPECT_THAT(model->ListTabsInGroup(group), testing::ElementsAre());
 }
 
-// Creates a browser with four tabs. The first two tabs are in Tab Group 1. The
-// third tab is in Tab Group 2. Dragging the third tab over one to the left will
-// result in the tab joining Tab Group 1. Then dragging the fourth tab over one
-// to the left will result in the tab joining Tab Group 1 as well.
+// Creates a browser with four tabs. The first tab is in Tab Group 1. The
+// third tab is in Tab Group 2. Dragging the second tab over one to the left
+// will result in the second tab joining Tab Group 1. Then dragging the third
+// tab over one to the left will result in the tab joining Tab Group 1. Then
+// dragging the fourth tab over one to the left will result in the tab joining
+// Tab Group 1 as well.
 IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
                        DragSingleTabLeftIntoGroup) {
   scoped_feature_list_.InitAndEnableFeature(features::kTabGroups);
@@ -796,10 +788,27 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
   TabStripModel* model = browser()->tab_strip_model();
 
   AddTabsAndResetBrowser(browser(), 3);
-  TabGroupId group1 = model->AddToNewGroup({0, 1});
+  TabGroupId group1 = model->AddToNewGroup({0});
   model->AddToNewGroup({2});
   StopAnimating(tab_strip);
 
+  // Dragging the tab in the first index toward the tab in the zero-th index
+  // switches the tabs and adds the dragged tab to the group.
+  // Drag only half the tab length, to ensure that the tab in the first index
+  // lands in the slot between the tab in the zero-th index and the group
+  // header to the left of the zero-th index.
+  const gfx::Point drag_origin =
+      GetCenterInScreenCoordinates(tab_strip->tab_at(1));
+  const gfx::Vector2d drag_amount =
+      gfx::Vector2d(-tab_strip->tab_at(0)->width() / 2, 0);
+  ASSERT_TRUE(PressInput(drag_origin));
+  ASSERT_TRUE(DragInputTo(drag_origin + drag_amount));
+  ASSERT_TRUE(ReleaseInput());
+  StopAnimating(tab_strip);
+
+  EXPECT_EQ("1 0 2 3", IDString(model));
+  EXPECT_THAT(model->ListTabsInGroup(group1), testing::ElementsAre(0, 1));
+
   // Dragging the tab in the second index to the tab in the first index switches
   // the tabs and adds the dragged tab to the group.
   ASSERT_TRUE(PressInput(GetCenterInScreenCoordinates(tab_strip->tab_at(2))));
@@ -807,7 +816,7 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
   ASSERT_TRUE(ReleaseInput());
   StopAnimating(tab_strip);
 
-  EXPECT_EQ("0 2 1 3", IDString(model));
+  EXPECT_EQ("1 2 0 3", IDString(model));
   EXPECT_THAT(model->ListTabsInGroup(group1), testing::ElementsAre(0, 1, 2));
 
   // Dragging the tab in the third index to the tab in the second index switches
@@ -817,14 +826,16 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
   ASSERT_TRUE(ReleaseInput());
   StopAnimating(tab_strip);
 
-  EXPECT_EQ("0 2 3 1", IDString(model));
+  EXPECT_EQ("1 2 3 0", IDString(model));
   EXPECT_THAT(model->ListTabsInGroup(group1), testing::ElementsAre(0, 1, 2, 3));
 }
 
-// Creates a browser with four tabs. The last two tabs are in Tab Group 1. The
-// second tab is in Tab Group 2. Dragging the second tab over one to the right
-// will result in the tab joining Tab Group 1. Then dragging the first tab over
-// one to the right will result in the tab joining Tab Group 1 as well.
+// Creates a browser with four tabs. The last tab is in Tab Group 1. The
+// second tab is in Tab Group 2. Dragging the third tab over one to the right
+// will result in the tab joining Tab Group 1. Then dragging the second tab
+// over one to the right will result in the tab joining Tab Group 1. Then
+// dragging the first tab over one to the right will result in the tab joining
+// Tab Group 1 as well.
 IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
                        DragSingleTabRightIntoGroup) {
   scoped_feature_list_.InitAndEnableFeature(features::kTabGroups);
@@ -833,10 +844,20 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
   TabStripModel* model = browser()->tab_strip_model();
 
   AddTabsAndResetBrowser(browser(), 3);
-  TabGroupId group1 = model->AddToNewGroup({2, 3});
+  TabGroupId group1 = model->AddToNewGroup({3});
   model->AddToNewGroup({1});
   StopAnimating(tab_strip);
 
+  // Dragging the tab in the second index to the tab in the third index switches
+  // the tabs and adds the dragged tab to the group.
+  ASSERT_TRUE(PressInput(GetCenterInScreenCoordinates(tab_strip->tab_at(2))));
+  ASSERT_TRUE(DragInputTo(GetCenterInScreenCoordinates(tab_strip->tab_at(3))));
+  ASSERT_TRUE(ReleaseInput());
+  StopAnimating(tab_strip);
+
+  EXPECT_EQ("0 1 3 2", IDString(model));
+  EXPECT_THAT(model->ListTabsInGroup(group1), testing::ElementsAre(2, 3));
+
   // Dragging the tab in the first index to the tab in the second index switches
   // the tabs and adds the dragged tab to the group.
   ASSERT_TRUE(PressInput(GetCenterInScreenCoordinates(tab_strip->tab_at(1))));
@@ -844,7 +865,7 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
   ASSERT_TRUE(ReleaseInput());
   StopAnimating(tab_strip);
 
-  EXPECT_EQ("0 2 1 3", IDString(model));
+  EXPECT_EQ("0 3 1 2", IDString(model));
   EXPECT_THAT(model->ListTabsInGroup(group1), testing::ElementsAre(1, 2, 3));
 
   // Dragging the tab in the zero-th index to the tab in the first index
@@ -854,7 +875,7 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
   ASSERT_TRUE(ReleaseInput());
   StopAnimating(tab_strip);
 
-  EXPECT_EQ("2 0 1 3", IDString(model));
+  EXPECT_EQ("3 0 1 2", IDString(model));
   EXPECT_THAT(model->ListTabsInGroup(group1), testing::ElementsAre(0, 1, 2, 3));
 }
 

chrome/browser/ui/views/tabs/tab_strip.cc

@@ -1161,6 +1161,10 @@ void TabStrip::ChangeTabGroup(int model_index,
       layout_helper_->UpdateGroupHeaderIndex(old_group.value());
     }
   }
+  if (new_group.has_value()) {
+    // As above, ensure the header is in the right place.
+    layout_helper_->UpdateGroupHeaderIndex(new_group.value());
+  }
   UpdateIdealBounds();
   AnimateToIdealBounds();
 }