Fix slot index being ambiguous depending on the group of the tab.

If a tab wants to move to model index to the left of the leftmost tab
in a group, where it should end up in TabStripLayoutHelper::slots_ is
ambiguous. If it is in the same group as the grouped tab, it should go
to the right of the header so the group remains contiguous. But if not,
it should go to the left. To disambiguate this case,
GetSlotIndexForTabModelIndex now takes the group of the tab at
|model_index| as a parameter. It cannot look up the Tab* in
TabStrip::tabs_ in the general case, since it might be called in the
middle of processing a MoveTab or RemoveTab, where the model indices
have already been updated in that data structure.

Bug: 989964
Change-Id: I2f35218d01c8398e6e024096bd35244ca67ca330
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1764181
Commit-Queue: Bret Sepulveda <bsep@chromium.org>
Reviewed-by: Taylor Bergquist <tbergquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690053}

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

@@ -2170,7 +2170,7 @@ void TabStrip::StartRemoveTabAnimation(int model_index, bool was_active) {
                              UpdateIdealBoundsForPinnedTabs(nullptr), old_x);
   }
 
-  layout_helper_->RemoveTab(model_index);
+  layout_helper_->RemoveTab(model_index, tab);
 }
 
 void TabStrip::StartFallbackRemoveTabAnimation(int model_index,
@@ -2224,7 +2224,7 @@ void TabStrip::StartFallbackRemoveTabAnimation(int model_index,
                              UpdateIdealBoundsForPinnedTabs(nullptr), old_x);
   }
 
-  layout_helper_->RemoveTabNoAnimation(model_index);
+  layout_helper_->RemoveTabNoAnimation(model_index, tab);
   UpdateIdealBounds();
   AnimateToIdealBounds();
 

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

@@ -124,7 +124,8 @@ void TabStripLayoutHelper::InsertTabAtNoAnimation(
     Tab* tab,
     base::OnceClosure tab_removed_callback,
     TabAnimationState::TabPinnedness pinned) {
-  const int slot_index = GetSlotIndexForTabModelIndex(model_index);
+  const int slot_index =
+      GetSlotIndexForTabModelIndex(model_index, tab->group());
   slots_.insert(
       slots_.begin() + slot_index,
       TabSlot::CreateForTab(tab, TabAnimationState::TabOpenness::kOpen, pinned,
@@ -136,7 +137,8 @@ void TabStripLayoutHelper::InsertTabAt(
     Tab* tab,
     base::OnceClosure tab_removed_callback,
     TabAnimationState::TabPinnedness pinned) {
-  const int slot_index = GetSlotIndexForTabModelIndex(model_index);
+  const int slot_index =
+      GetSlotIndexForTabModelIndex(model_index, tab->group());
   slots_.insert(
       slots_.begin() + slot_index,
       TabSlot::CreateForTab(tab, TabAnimationState::TabOpenness::kClosed,
@@ -146,16 +148,17 @@ void TabStripLayoutHelper::InsertTabAt(
                   TabAnimationState::TabOpenness::kOpen));
 }
 
-void TabStripLayoutHelper::RemoveTabNoAnimation(int model_index) {
+void TabStripLayoutHelper::RemoveTabNoAnimation(int model_index, Tab* tab) {
   TabAnimation* animation =
-      slots_[GetSlotIndexForTabModelIndex(model_index)].animation.get();
+      slots_[GetSlotIndexForTabModelIndex(model_index, tab->group())]
+          .animation.get();
   animation->AnimateTo(animation->target_state().WithOpenness(
       TabAnimationState::TabOpenness::kClosed));
   animation->CompleteAnimation();
 }
 
-void TabStripLayoutHelper::RemoveTab(int model_index) {
-  int slot_index = GetSlotIndexForTabModelIndex(model_index);
+void TabStripLayoutHelper::RemoveTab(int model_index, Tab* tab) {
+  int slot_index = GetSlotIndexForTabModelIndex(model_index, tab->group());
   AnimateSlot(slot_index,
               slots_[slot_index].animation->target_state().WithOpenness(
                   TabAnimationState::TabOpenness::kClosed));
@@ -172,27 +175,31 @@ void TabStripLayoutHelper::OnTabDestroyed(Tab* tab) {
     slots_.erase(it);
 }
 
-void TabStripLayoutHelper::MoveTab(
-    base::Optional<TabGroupId> group_at_prev_index,
-    int prev_index,
-    int new_index) {
-  const int prev_slot_index = GetSlotIndexForTabModelIndex(prev_index);
+void TabStripLayoutHelper::MoveTab(base::Optional<TabGroupId> moving_tab_group,
+                                   int prev_index,
+                                   int new_index) {
+  const int prev_slot_index =
+      GetSlotIndexForTabModelIndex(prev_index, moving_tab_group);
   TabSlot moving_tab = std::move(slots_[prev_slot_index]);
   slots_.erase(slots_.begin() + prev_slot_index);
 
-  const int new_slot_index = GetSlotIndexForTabModelIndex(new_index);
+  const int new_slot_index =
+      GetSlotIndexForTabModelIndex(new_index, moving_tab_group);
   slots_.insert(slots_.begin() + new_slot_index, std::move(moving_tab));
 
-  if (group_at_prev_index.has_value())
-    UpdateGroupHeaderIndex(group_at_prev_index.value());
+  if (moving_tab_group.has_value())
+    UpdateGroupHeaderIndex(moving_tab_group.value());
 }
 
 void TabStripLayoutHelper::SetTabPinnedness(
     int model_index,
     TabAnimationState::TabPinnedness pinnedness) {
   // TODO(958173): Animate state change.
+  views::ViewModelT<Tab>* tabs = get_tabs_callback_.Run();
   TabAnimation* animation =
-      slots_[GetSlotIndexForTabModelIndex(model_index)].animation.get();
+      slots_[GetSlotIndexForTabModelIndex(model_index,
+                                          tabs->view_at(model_index)->group())]
+          .animation.get();
   animation->AnimateTo(animation->target_state().WithPinnedness(pinnedness));
   animation->CompleteAnimation();
 }
@@ -203,7 +210,8 @@ void TabStripLayoutHelper::InsertGroupHeader(
     base::OnceClosure header_removed_callback) {
   // TODO(958173): Animate open.
   std::vector<int> tabs_in_group = controller_->ListTabsInGroup(group);
-  const int header_slot_index = GetSlotIndexForTabModelIndex(tabs_in_group[0]);
+  const int header_slot_index =
+      GetSlotIndexForTabModelIndex(tabs_in_group[0], group);
   slots_.insert(slots_.begin() + header_slot_index,
                 TabSlot::CreateForGroupHeader(
                     group, header, TabAnimationState::TabPinnedness::kUnpinned,
@@ -224,21 +232,24 @@ void TabStripLayoutHelper::UpdateGroupHeaderIndex(TabGroupId group) {
   slots_.erase(slots_.begin() + slot_index);
   std::vector<int> tabs_in_group = controller_->ListTabsInGroup(group);
   const int first_tab_slot_index =
-      GetSlotIndexForTabModelIndex(tabs_in_group[0]);
+      GetSlotIndexForTabModelIndex(tabs_in_group[0], group);
   slots_.insert(slots_.begin() + first_tab_slot_index, std::move(header_slot));
 }
 
 void TabStripLayoutHelper::SetActiveTab(int prev_active_index,
                                         int new_active_index) {
+  views::ViewModelT<Tab>* tabs = get_tabs_callback_.Run();
   // Set activeness without animating by retargeting the existing animation.
   if (prev_active_index >= 0) {
-    const int prev_slot_index = GetSlotIndexForTabModelIndex(prev_active_index);
+    const int prev_slot_index = GetSlotIndexForTabModelIndex(
+        prev_active_index, tabs->view_at(prev_active_index)->group());
     TabAnimation* animation = slots_[prev_slot_index].animation.get();
     animation->RetargetTo(animation->target_state().WithActiveness(
         TabAnimationState::TabActiveness::kInactive));
   }
   if (new_active_index >= 0) {
-    const int new_slot_index = GetSlotIndexForTabModelIndex(new_active_index);
+    const int new_slot_index = GetSlotIndexForTabModelIndex(
+        new_active_index, tabs->view_at(new_active_index)->group());
     TabAnimation* animation = slots_[new_slot_index].animation.get();
     animation->RetargetTo(animation->target_state().WithActiveness(
         TabAnimationState::TabActiveness::kActive));
@@ -257,14 +268,23 @@ void TabStripLayoutHelper::CompleteAnimationsWithoutDestroyingTabs() {
 }
 
 void TabStripLayoutHelper::UpdateIdealBounds(int available_width) {
+  views::ViewModelT<Tab>* tabs = get_tabs_callback_.Run();
+  std::map<TabGroupId, TabGroupHeader*> group_headers =
+      get_group_headers_callback_.Run();
+
   const int active_tab_model_index = controller_->GetActiveIndex();
   const int active_tab_slot_index =
       controller_->IsValidIndex(active_tab_model_index)
-          ? GetSlotIndexForTabModelIndex(active_tab_model_index)
+          ? GetSlotIndexForTabModelIndex(
+                active_tab_model_index,
+                tabs->view_at(active_tab_model_index)->group())
           : TabStripModel::kNoTab;
   const int pinned_tab_count = GetPinnedTabCount();
+  const int last_pinned_tab_index = pinned_tab_count - 1;
   const int last_pinned_tab_slot_index =
-      pinned_tab_count > 0 ? GetSlotIndexForTabModelIndex(pinned_tab_count - 1)
+      pinned_tab_count > 0 ? GetSlotIndexForTabModelIndex(
+                                 last_pinned_tab_index,
+                                 tabs->view_at(last_pinned_tab_index)->group())
                            : TabStripModel::kNoTab;
 
   TabLayoutConstants layout_constants = GetTabLayoutConstants();
@@ -290,10 +310,6 @@ void TabStripLayoutHelper::UpdateIdealBounds(int available_width) {
       CalculateTabBounds(layout_constants, tab_widths, available_width);
   DCHECK_EQ(slots_.size(), bounds.size());
 
-  views::ViewModelT<Tab>* tabs = get_tabs_callback_.Run();
-  std::map<TabGroupId, TabGroupHeader*> group_headers =
-      get_group_headers_callback_.Run();
-
   int current_tab_model_index = 0;
   for (int i = 0; i < int{bounds.size()}; ++i) {
     const TabSlot& slot = slots_[i];
@@ -395,14 +411,26 @@ int TabStripLayoutHelper::LayoutTabs(int available_width) {
   return trailing_x;
 }
 
-int TabStripLayoutHelper::GetSlotIndexForTabModelIndex(int model_index) const {
+int TabStripLayoutHelper::GetSlotIndexForTabModelIndex(
+    int model_index,
+    base::Optional<TabGroupId> group) const {
   int current_model_index = 0;
   for (size_t i = 0; i < slots_.size(); i++) {
-    if (slots_[i].type == ViewType::kTab && !slots_[i].animation->IsClosing()) {
-      if (current_model_index == model_index)
+    const bool model_space_index =
+        slots_[i].type == ViewType::kTab && !slots_[i].animation->IsClosing();
+    if (current_model_index == model_index) {
+      if (model_space_index) {
         return i;
-      ++current_model_index;
+      } else if (slots_[i].type == ViewType::kGroupHeader) {
+        // If the tab is in the header's group, then it should be to its right,
+        // so as to be contiguous with the group. If not, it goes to its left.
+        return static_cast<TabGroupHeader*>(slots_[i].view)->group() == group
+                   ? i + 1
+                   : i;
+      }
     }
+    if (model_space_index)
+      ++current_model_index;
   }
   DCHECK_EQ(model_index, current_model_index);
   return slots_.size();

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

@@ -70,18 +70,18 @@ class TabStripLayoutHelper {
   // the tab has been removed from the model but the old animation style owns
   // animating it.
   // TODO(958173): Remove this when the old animation style is removed.
-  void RemoveTabNoAnimation(int model_index);
+  void RemoveTabNoAnimation(int model_index, Tab* tab);
 
   // Marks the tab at |model_index| as closing and animates it closed.
-  void RemoveTab(int model_index);
+  void RemoveTab(int model_index, Tab* tab);
 
   // Invoked when |tab| has been destroyed by TabStrip (i.e. the remove
   // animation has completed).
   void OnTabDestroyed(Tab* tab);
 
-  // Moves the tab at |prev_index| with group |group_at_prev_index| to
-  // |new_index|. Also updates the group header's location if necessary.
-  void MoveTab(base::Optional<TabGroupId> group_at_prev_index,
+  // Moves the tab at |prev_index| with group |moving_tab_group| to |new_index|.
+  // Also updates the group header's location if necessary.
+  void MoveTab(base::Optional<TabGroupId> moving_tab_group,
                int prev_index,
                int new_index);
 
@@ -135,9 +135,10 @@ class TabStripLayoutHelper {
  private:
   struct TabSlot;
 
-  // Given a tab's |model_index|, returns the index of its corresponding TabSlot
-  // in |slots_|.
-  int GetSlotIndexForTabModelIndex(int model_index) const;
+  // Given a tab's |model_index| and |group|, returns the index of its
+  // corresponding TabSlot in |slots_|.
+  int GetSlotIndexForTabModelIndex(int model_index,
+                                   base::Optional<TabGroupId> group) const;
 
   // Given a group ID, returns the index of its header's corresponding TabSlot
   // in |slots_|.

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

@@ -1121,6 +1121,23 @@ TEST_P(TabStripTest, GroupHeaderMovesOnRegrouping) {
   EXPECT_LT(header1->x(), tab_strip_->tab_at(2)->x());
 }
 
+TEST_P(TabStripTest, UngroupedTabMovesLeftOfHeader) {
+  tab_strip_->SetBounds(0, 0, 2000, 100);
+  for (int i = 0; i < 2; i++)
+    tab_strip_->AddTabAt(i, TabRendererData(), false);
+  TabGroupId group = TabGroupId::GenerateNew();
+  controller_->MoveTabIntoGroup(0, group);
+  CompleteAnimationAndLayout();
+
+  controller_->MoveTab(1, 0);
+  CompleteAnimationAndLayout();
+
+  // Header is right of tab 0.
+  TabGroupHeader* header = ListGroupHeaders()[0];
+  EXPECT_LT(tab_strip_->tab_at(0)->x(), header->x());
+  EXPECT_LT(header->x(), tab_strip_->tab_at(1)->x());
+}
+
 // This can happen when a tab in the middle of a group starts to close.
 TEST_P(TabStripTest, DiscontinuousGroup) {
   tab_strip_->SetBounds(0, 0, 1000, 100);