From 9000f5659d4a257d2c6ddf804c5ea3146fffb07b Mon Sep 17 00:00:00 2001
From: Kaan Alsan <[email protected]>
Date: Fri, 01 May 2026 15:36:30 -0700
Subject: [PATCH] Use tab drag data as source of truth when detaching a tab drag

This resolves an HBO issue that was caused by TabDragController using
the tab strip's selection model as the source of truth while detaching
tabs during a drag. This was faulty because the selection model may
change during a drag.

This CL fixes this by using the tab drag data as the source of truth.

Note, there's still some existing buggy behavior when the selection
model changes during a drag that will require additional work. This CL
addresses the immediate HBO concern though.

Fixed: 505371980
Change-Id: Id82af5de05983974cb8ff8557b0e49e05af03fc8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7801697
Reviewed-by: Vince Lugli <[email protected]>
Commit-Queue: Kaan Alsan <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1624124}
---

diff --git a/chrome/browser/ui/views/tabs/dragging/tab_drag_controller.cc b/chrome/browser/ui/views/tabs/dragging/tab_drag_controller.cc
index 29bc4e4..67e3b29 100644
--- a/chrome/browser/ui/views/tabs/dragging/tab_drag_controller.cc
+++ b/chrome/browser/ui/views/tabs/dragging/tab_drag_controller.cc
@@ -1370,11 +1370,13 @@
       const WebContents* web_contents = tab->get()->tab->GetContents();
       // If it's a tab - we add it to the tabstrip.
       int add_types = AddTabTypes::ADD_NONE;
-      TabDragData& tab_data = *std::find_if(
-          drag_data_.tab_drag_data_.begin(), drag_data_.tab_drag_data_.end(),
-          [web_contents](TabDragData& tab_data) {
-            return web_contents == tab_data.contents;
-          });
+      auto it = std::find_if(drag_data_.tab_drag_data_.begin(),
+                             drag_data_.tab_drag_data_.end(),
+                             [web_contents](TabDragData& tab_data) {
+                               return web_contents == tab_data.contents;
+                             });
+      CHECK(it != drag_data_.tab_drag_data_.end());
+      TabDragData& tab_data = *it;
       if (tab_data.pinned) {
         add_types |= AddTabTypes::ADD_PINNED;
       }
@@ -1488,13 +1490,15 @@
   }
 
   std::vector<int> dragged_indices;
-
-  // TODO(crbug.com/435178910) Remove this usage of ListSelectionModel.
-  for (int dragged_index : attached_model->selection_model()
-                               .GetListSelectionModel()
-                               .selected_indices()) {
-    dragged_indices.push_back(dragged_index);
+  for (const auto& data : drag_data_.tab_drag_data_) {
+    if (data.contents) {
+      const int index = attached_model->GetIndexOfWebContents(data.contents);
+      if (index != TabStripModel::kNoTab) {
+        dragged_indices.push_back(index);
+      }
+    }
   }
+  std::ranges::sort(dragged_indices);
   const std::vector<tab_groups::TabGroupId> groups_to_move =
       attached_model->GetGroupsDestroyedFromRemovingIndices(dragged_indices);
 
diff --git a/chrome/browser/ui/views/tabs/dragging/tab_drag_controller_interactive_uitest.cc b/chrome/browser/ui/views/tabs/dragging/tab_drag_controller_interactive_uitest.cc
index 9b43c08..b507b391 100644
--- a/chrome/browser/ui/views/tabs/dragging/tab_drag_controller_interactive_uitest.cc
+++ b/chrome/browser/ui/views/tabs/dragging/tab_drag_controller_interactive_uitest.cc
@@ -6267,6 +6267,72 @@
   ASSERT_TRUE(ReleaseInput());
 }
 
+// Regression test for http://crbug.com/505371980.
+// Verifies that the correct tab is detached even if the selection changes
+// mid-drag.
+#if BUILDFLAG(IS_WIN)
+#define MAYBE_SelectTabDuringDragAndDetach SelectTabDuringDragAndDetach
+#else
+#define MAYBE_SelectTabDuringDragAndDetach DISABLED_SelectTabDuringDragAndDetach
+#endif
+IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
+                       MAYBE_SelectTabDuringDragAndDetach) {
+  TabStripModel* model = browser()->tab_strip_model();
+  TabStrip* tab_strip = GetTabStripForBrowser(browser());
+
+  AddTabsAndResetBrowser(browser(), 1);
+  ASSERT_EQ(2, model->count());
+  // The second tab (index 1) is active.
+  ASSERT_EQ(1, model->active_index());
+
+  // Save the contents of the tabs to verify later.
+  content::WebContents* tab0_contents = model->GetWebContentsAt(0);
+  content::WebContents* tab1_contents = model->GetWebContentsAt(1);
+
+  // Use QuitDraggingObserver to wait for the drag to end safely.
+  test::QuitDraggingObserver observer(tab_strip);
+
+  // Start dragging tab 0.
+  ASSERT_TRUE(PressInputAtCenter(tab_strip->tab_at(0)));
+
+  // Drag slightly to start the drag, then change selection and detach.
+  // DragInputToCenterNotifyWhenDone is more robust as it handles the potential
+  // transition into a nested move loop.
+  ASSERT_TRUE(DragInputToCenterNotifyWhenDone(
+      tab_strip->tab_at(0), base::BindLambdaForTesting([&]() {
+        // 1. Change selection mid-drag.
+        ui::ListSelectionModel new_selection;
+        new_selection.SetSelectedIndex(1);
+        model->SetSelectionFromModel(std::move(new_selection));
+
+        // 2. Trigger detach with a larger offset to ensure it crosses the
+        // threshold on all bots.
+        DragInputToCenterAsync(tab_strip->tab_at(0),
+                               gfx::Vector2d(0, GetDetachY(tab_strip) + 20));
+
+        // 3. Use async release to avoid hanging in the move loop.
+        ReleaseInput(0, true);
+      }),
+      gfx::Vector2d(0, 5)));
+
+  // Wait for the drag to end.
+  observer.Wait();
+
+  // Verify that a new browser was created and it contains tab 0.
+  ASSERT_EQ(2u, GetAllBrowserWindowInterfaces().size());
+  BrowserWindowInterface* new_browser =
+      ui_test_utils::GetBrowserNotInSet({browser()});
+  ASSERT_TRUE(new_browser);
+
+  EXPECT_EQ(1, new_browser->GetTabStripModel()->count());
+  EXPECT_EQ(tab0_contents,
+            new_browser->GetTabStripModel()->GetWebContentsAt(0));
+
+  // Verify that the original browser still contains tab 1.
+  EXPECT_EQ(1, model->count());
+  EXPECT_EQ(tab1_contents, model->GetWebContentsAt(0));
+}
+
 #if BUILDFLAG(IS_CHROMEOS)
 // Runs tests with a tabbed system web app that is locked for OnTask. This is
 // not related to normal web browsers.

diff --git a/chrome/browser/ui/views/tabs/dragging/tab_drag_controller_interactive_uitest.cc b/chrome/browser/ui/views/tabs/dragging/tab_drag_controller_interactive_uitest.cc
index 9b43c08..b507b391 100644
--- a/chrome/browser/ui/views/tabs/dragging/tab_drag_controller_interactive_uitest.cc
+++ b/chrome/browser/ui/views/tabs/dragging/tab_drag_controller_interactive_uitest.cc
@@ -6267,6 +6267,72 @@
   ASSERT_TRUE(ReleaseInput());
 }
 
+// Regression test for http://crbug.com/505371980.
+// Verifies that the correct tab is detached even if the selection changes
+// mid-drag.
+#if BUILDFLAG(IS_WIN)
+#define MAYBE_SelectTabDuringDragAndDetach SelectTabDuringDragAndDetach
+#else
+#define MAYBE_SelectTabDuringDragAndDetach DISABLED_SelectTabDuringDragAndDetach
+#endif
+IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
+                       MAYBE_SelectTabDuringDragAndDetach) {
+  TabStripModel* model = browser()->tab_strip_model();
+  TabStrip* tab_strip = GetTabStripForBrowser(browser());
+
+  AddTabsAndResetBrowser(browser(), 1);
+  ASSERT_EQ(2, model->count());
+  // The second tab (index 1) is active.
+  ASSERT_EQ(1, model->active_index());
+
+  // Save the contents of the tabs to verify later.
+  content::WebContents* tab0_contents = model->GetWebContentsAt(0);
+  content::WebContents* tab1_contents = model->GetWebContentsAt(1);
+
+  // Use QuitDraggingObserver to wait for the drag to end safely.
+  test::QuitDraggingObserver observer(tab_strip);
+
+  // Start dragging tab 0.
+  ASSERT_TRUE(PressInputAtCenter(tab_strip->tab_at(0)));
+
+  // Drag slightly to start the drag, then change selection and detach.
+  // DragInputToCenterNotifyWhenDone is more robust as it handles the potential
+  // transition into a nested move loop.
+  ASSERT_TRUE(DragInputToCenterNotifyWhenDone(
+      tab_strip->tab_at(0), base::BindLambdaForTesting([&]() {
+        // 1. Change selection mid-drag.
+        ui::ListSelectionModel new_selection;
+        new_selection.SetSelectedIndex(1);
+        model->SetSelectionFromModel(std::move(new_selection));
+
+        // 2. Trigger detach with a larger offset to ensure it crosses the
+        // threshold on all bots.
+        DragInputToCenterAsync(tab_strip->tab_at(0),
+                               gfx::Vector2d(0, GetDetachY(tab_strip) + 20));
+
+        // 3. Use async release to avoid hanging in the move loop.
+        ReleaseInput(0, true);
+      }),
+      gfx::Vector2d(0, 5)));
+
+  // Wait for the drag to end.
+  observer.Wait();
+
+  // Verify that a new browser was created and it contains tab 0.
+  ASSERT_EQ(2u, GetAllBrowserWindowInterfaces().size());
+  BrowserWindowInterface* new_browser =
+      ui_test_utils::GetBrowserNotInSet({browser()});
+  ASSERT_TRUE(new_browser);
+
+  EXPECT_EQ(1, new_browser->GetTabStripModel()->count());
+  EXPECT_EQ(tab0_contents,
+            new_browser->GetTabStripModel()->GetWebContentsAt(0));
+
+  // Verify that the original browser still contains tab 1.
+  EXPECT_EQ(1, model->count());
+  EXPECT_EQ(tab1_contents, model->GetWebContentsAt(0));
+}
+
 #if BUILDFLAG(IS_CHROMEOS)
 // Runs tests with a tabbed system web app that is locked for OnTask. This is
 // not related to normal web browsers.