[Tab Groups Collapse] Fix drag for collapsed groups.

The bug that makes it not possible to drag a tab to the right of a
collapsed group header is due to TabStrip::GetLastVisibleTab() clamping
the insertion index.

TabStrip::ShouldTabBeVisible is used for determining if a tab is visible
in the tabstrip due to space constraints. Moving the logic for collapsed
tabs into TabStrip::SetTabVisibility().

This should also make is slightly nicer and show less headers in the case
where the tabstrip is full of group headers and not visible tabs.

Bug: 1099367
Change-Id: If31bf23b9d2fc069481f092e38310635b89d3165
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2277381
Commit-Queue: Charlene Yan <cyan@chromium.org>
Reviewed-by: Connie Wan <connily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#786498}

chrome/browser/ui/tabs/tab_strip_model.cc

@@ -469,17 +469,7 @@ std::unique_ptr<content::WebContents> TabStripModel::DetachWebContentsImpl(
   base::Optional<int> next_selected_index =
       order_controller_->DetermineNewSelectedIndex(index);
 
-  // Here we update the group model manually instead of calling UngroupTab(), so
-  // that the tab isn't prematurely ungrouped. Ungrouping the tab can change its
-  // position while it's still animating closed, which can result in the wrong
-  // view getting removed.
-  base::Optional<tab_groups::TabGroupId> group = GetTabGroupForTab(index);
-  if (group.has_value()) {
-    TabGroup* tab_group = group_model_->GetTabGroup(group.value());
-    tab_group->RemoveTab();
-    if (tab_group->IsEmpty())
-      group_model_->RemoveTabGroup(group.value());
-  }
+  UngroupTab(index);
 
   std::unique_ptr<WebContentsData> old_data = std::move(contents_data_[index]);
   contents_data_.erase(contents_data_.begin() + index);

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

@@ -715,7 +715,7 @@ class TabStrip::TabDragContextImpl : public TabDragContext {
       DCHECK_NE(TabStripModel::kNoTab, tab_data_index);
       views[i]->SetBoundsRect(ideal_bounds(tab_data_index));
     }
-    tab_strip_->SetTabVisibility();
+    tab_strip_->SetTabSlotVisibility();
     tab_strip_->SchedulePaint();
   }
 
@@ -776,7 +776,7 @@ class TabStrip::TabDragContextImpl : public TabDragContext {
         view->SetBoundsRect(new_bounds);
       }
     }
-    tab_strip_->SetTabVisibility();
+    tab_strip_->SetTabSlotVisibility();
   }
 
   // Forces the entire tabstrip to lay out.
@@ -1382,10 +1382,10 @@ void TabStrip::OnGroupMoved(const tab_groups::TabGroupId& group) {
 void TabStrip::OnGroupClosed(const tab_groups::TabGroupId& group) {
   bounds_animator_.StopAnimatingView(group_header(group));
   layout_helper_->RemoveGroupHeader(group);
-  group_views_.erase(group);
 
   UpdateIdealBounds();
   AnimateToIdealBounds();
+  group_views_.erase(group);
 }
 
 void TabStrip::ShiftGroupLeft(const tab_groups::TabGroupId& group) {
@@ -1401,14 +1401,6 @@ bool TabStrip::ShouldTabBeVisible(const Tab* tab) const {
   if (tab->detached())
     return false;
 
-  // Collapsed tabs disappear once they've reached their minimum size. This is
-  // different than very small non-collapsed tabs, because in that case the tab
-  // (and its favicon) must still be visible.
-  if (tab->group() && controller()->IsGroupCollapsed(*tab->group()) &&
-      tab->bounds().width() <= tab->tab_style()->GetMinimumInactiveWidth()) {
-    return false;
-  }
-
   // When stacking tabs, all tabs should always be visible.
   if (stacked_layout_)
     return true;
@@ -2111,7 +2103,7 @@ void TabStrip::Layout() {
 
   if (IsAnimating()) {
     // Hide tabs that have animated at least partially out of the clip region.
-    SetTabVisibility();
+    SetTabSlotVisibility();
     return;
   }
 
@@ -2128,7 +2120,7 @@ void TabStrip::Layout() {
     // It should be as wide as possible subject to the above constraints.
     const int width = std::min(max_width, std::max(min_width, available_width));
     SetBounds(0, 0, width, height());
-    SetTabVisibility();
+    SetTabSlotVisibility();
   }
 
   // Only do a layout if our size changed.
@@ -2478,7 +2470,6 @@ void TabStrip::StartRemoveTabAnimation(int model_index, bool was_active) {
     Tab* next_active_tab = tab_at(next_active_index);
     Tab* tab_being_removed = tab_at(model_index);
 
-    UpdateIdealBounds();
     int size_delta = tab_being_removed->width();
     if (!tab_being_removed->data().pinned && was_active &&
         GetActiveTabWidth() > GetInactiveTabWidth()) {
@@ -2666,18 +2657,32 @@ void TabStrip::CompleteAnimationAndLayout() {
   UpdateIdealBounds();
   SnapToIdealBounds();
 
-  SetTabVisibility();
+  SetTabSlotVisibility();
   SchedulePaint();
 }
 
-void TabStrip::SetTabVisibility() {
-  // We could probably be more efficient here by making use of the fact that the
-  // tabstrip will always have any visible tabs, and then any invisible tabs, so
-  // we could e.g. binary-search for the changeover point.  But since we have to
-  // iterate through all the tabs to call SetVisible() anyway, it doesn't seem
-  // worth it.
-  for (Tab* tab : layout_helper_->GetTabs())
-    tab->SetVisible(ShouldTabBeVisible(tab));
+void TabStrip::SetTabSlotVisibility() {
+  bool last_tab_visible = false;
+  base::Optional<tab_groups::TabGroupId> last_tab_group = base::nullopt;
+  std::vector<Tab*> tabs = layout_helper_->GetTabs();
+  for (std::vector<Tab*>::reverse_iterator tab = tabs.rbegin();
+       tab != tabs.rend(); ++tab) {
+    base::Optional<tab_groups::TabGroupId> current_group = (*tab)->group();
+    if (current_group != last_tab_group && last_tab_group.has_value())
+      group_header(last_tab_group.value())->SetVisible(last_tab_visible);
+    last_tab_visible = ShouldTabBeVisible(*tab);
+    last_tab_group = (*tab)->closing() ? base::nullopt : current_group;
+
+    // Collapsed tabs disappear once they've reached their minimum size. This
+    // is different than very small non-collapsed tabs, because in that case
+    // the tab (and its favicon) must still be visible.
+    bool is_collapsed =
+        (current_group.has_value() &&
+         controller()->IsGroupCollapsed(current_group.value()) &&
+         (*tab)->bounds().width() <=
+             (*tab)->tab_style()->GetMinimumInactiveWidth());
+    (*tab)->SetVisible(is_collapsed ? false : last_tab_visible);
+  }
 }
 
 void TabStrip::UpdateAccessibleTabIndices() {
@@ -2706,8 +2711,14 @@ int TabStrip::GetRightSideReservedWidth() const {
 const Tab* TabStrip::GetLastVisibleTab() const {
   for (int i = tab_count() - 1; i >= 0; --i) {
     const Tab* tab = tab_at(i);
-    if (tab->GetVisible())
+
+    // The tab is marked not visible in a collapsed group, but is "visible" in
+    // the tabstrip if the header is visible.
+    if (tab->GetVisible() ||
+        (tab->group().has_value() &&
+         group_header(tab->group().value())->GetVisible())) {
       return tab;
+    }
   }
   // While in normal use the tabstrip should always be wide enough to have at
   // least one visible tab, it can be zero-width in tests, meaning we get here.
@@ -3415,7 +3426,7 @@ void TabStrip::SwapLayoutIfNecessary() {
   PrepareForAnimation();
   UpdateIdealBounds();
   AnimateToIdealBounds();
-  SetTabVisibility();
+  SetTabSlotVisibility();
 }
 
 bool TabStrip::NeedsTouchLayout() const {

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

@@ -461,8 +461,9 @@ class TabStrip : public views::AccessiblePaneView,
   // Invoked from Layout if the size changes or layout is really needed.
   void CompleteAnimationAndLayout();
 
-  // Sets the visibility state of all tabs based on ShouldTabBeVisible().
-  void SetTabVisibility();
+  // Sets the visibility state of all tabs and group headers (if any) based on
+  // ShouldTabBeVisible().
+  void SetTabSlotVisibility();
 
   // Updates the indexes and count for AX data on all tabs. Used by some screen
   // readers (e.g. ChromeVox).

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

@@ -499,6 +499,48 @@ TEST_P(TabStripTest, VisibilityInOverflow) {
   EXPECT_TRUE(tab_strip_->tab_at(tab_strip_->tab_count() - 1)->GetVisible());
 }
 
+TEST_P(TabStripTest, GroupedTabSlotVisibility) {
+  constexpr int kInitialWidth = 250;
+  tab_strip_parent_->SetBounds(0, 0, kInitialWidth, 20);
+
+  // The first tab added to a reasonable-width strip should be visible.  If we
+  // add enough additional tabs, eventually one should be invisible due to
+  // overflow.
+  int invisible_tab_index = 0;
+  for (; invisible_tab_index < 100; ++invisible_tab_index) {
+    controller_->AddTab(invisible_tab_index, false);
+    CompleteAnimationAndLayout();
+    if (!tab_strip_->tab_at(invisible_tab_index)->GetVisible())
+      break;
+  }
+  ASSERT_GT(invisible_tab_index, 0);
+  ASSERT_LT(invisible_tab_index, 100);
+
+  // The tabs before the invisible tab should still be visible.
+  for (int i = 0; i < invisible_tab_index; ++i)
+    ASSERT_TRUE(tab_strip_->tab_at(i)->GetVisible());
+
+  // The group header of an invisible tab should not be visible.
+  base::Optional<tab_groups::TabGroupId> group1 =
+      tab_groups::TabGroupId::GenerateNew();
+  controller_->MoveTabIntoGroup(invisible_tab_index, group1);
+  CompleteAnimationAndLayout();
+  ASSERT_FALSE(tab_strip_->tab_at(invisible_tab_index)->GetVisible());
+  EXPECT_FALSE(tab_strip_->group_header(group1.value())->GetVisible());
+
+  // The group header of a visible tab should be visible when the group is
+  // expanded and collapsed.
+  base::Optional<tab_groups::TabGroupId> group2 =
+      tab_groups::TabGroupId::GenerateNew();
+  controller_->MoveTabIntoGroup(0, group2);
+  CompleteAnimationAndLayout();
+  ASSERT_FALSE(controller_->IsGroupCollapsed(group2.value()));
+  EXPECT_TRUE(tab_strip_->group_header(group2.value())->GetVisible());
+  controller_->ToggleTabGroupCollapsedState(group2.value(), false);
+  ASSERT_TRUE(controller_->IsGroupCollapsed(group2.value()));
+  EXPECT_TRUE(tab_strip_->group_header(group2.value())->GetVisible());
+}
+
 // Creates a tab strip in stacked layout mode and verifies that as we move
 // across the strip at the top, middle, and bottom, events will target each tab
 // in order.