Limit tabstrip width to its preferred width.

Prior to this CL, the tabstrip always occupies the entire width of the
titlebar area (minus the caption buttons). Changing this allows us to
lay out the NTB just to the right of the tabstrip view itself instead
of having the tabstrip control the NTB's bounds. This CL does that
by simply changing the FlexRule governing the tabstrip's width (and
fixing the numerous resulting bugs).

This is a modified reland of https://crrev.com/c/2336296. Changes from
that version:
- PreferredSizeChanged() while dragging, since the dragged tabs may have
been moved past the previous extents of the tabstrip.
- Take tab closing mode into account when determining the tab area's
preferred width.
- Bypass ScrollView when observing the tabstrip's preferred size changes.
- PreferredSizeChanged() in SnapToIdealBounds, since the tabs may have
been snapped outside the previous extents of the tabstrip.

Known issues:
- Tab closing mode doesn't play nicely with scrolling in all cases. That
was true before this CL, but this moves around which cases are broken.

Bug: 1093972
Change-Id: I214534847bd07c53829005b7496a9b9bc63e6d5e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2432088
Commit-Queue: Taylor Bergquist <tbergquist@chromium.org>
Reviewed-by: Charlene Yan <cyan@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816778}

chrome/browser/ui/ash/tab_scrubber_browsertest.cc

@@ -250,6 +250,10 @@ class TabScrubberTest : public InProcessBrowserTest,
     ASSERT_EQ(num_tabs, browser->tab_strip_model()->active_index());
     tab_strip->StopAnimating(true);
     ASSERT_FALSE(tab_strip->IsAnimating());
+    // Perform any scheduled layouts so the tabstrip is in a steady state.
+    BrowserView::GetBrowserViewForBrowser(browser)
+        ->GetWidget()
+        ->LayoutRootViewIfNecessary();
   }
 
   // TabStripModelObserver overrides.

chrome/browser/ui/views/frame/tab_strip_region_view.cc

@@ -20,6 +20,22 @@
 #include "ui/views/layout/flex_layout.h"
 #include "ui/views/view_class_properties.h"
 
+namespace {
+
+// Define a custom FlexRule for |tabstrip_scroll_container_|. Equivalent to
+// using a (kScaleToMinimum, kPreferred) flex specification on the tabstrip
+// itself, bypassing the ScrollView.
+// TODO(1132488): Make ScrollView take on TabStrip's preferred size instead.
+gfx::Size TabScrollContainerFlexRule(const views::View* tab_strip,
+                                     const views::View* view,
+                                     const views::SizeBounds& size_bounds) {
+  const gfx::Size preferred_size = tab_strip->GetPreferredSize();
+  return gfx::Size(size_bounds.width().min_of(preferred_size.width()),
+                   preferred_size.height());
+}
+
+}  // namespace
+
 TabStripRegionView::TabStripRegionView(std::unique_ptr<TabStrip> tab_strip) {
   views::FlexLayout* layout_manager =
       SetLayoutManager(std::make_unique<views::FlexLayout>());
@@ -30,6 +46,10 @@ TabStripRegionView::TabStripRegionView(std::unique_ptr<TabStrip> tab_strip) {
       base::BindRepeating(&TabStripRegionView::CalculateTabStripAvailableWidth,
                           base::Unretained(this)));
   if (base::FeatureList::IsEnabled(features::kScrollableTabStrip)) {
+    // TODO(https://crbug.com/1132488): ScrollView doesn't propagate changes to
+    // the TabStrip's preferred size; observe that manually.
+    tab_strip->View::AddObserver(this);
+
     views::ScrollView* tab_strip_scroll_container =
         AddChildView(std::make_unique<views::ScrollView>(
             views::ScrollView::ScrollWithLayers::kEnabled));
@@ -39,6 +59,13 @@ TabStripRegionView::TabStripRegionView(std::unique_ptr<TabStrip> tab_strip) {
     tab_strip_scroll_container->SetTreatAllScrollEventsAsHorizontal(true);
     tab_strip_container_ = tab_strip_scroll_container;
     tab_strip_scroll_container->SetContents(std::move(tab_strip));
+    // This base::Unretained is safe because the callback is called by the
+    // layout manager, which is cleaned up before view children like
+    // |tab_strip_scroll_container| (which owns |tab_strip_|).
+    tab_strip_scroll_container->SetProperty(
+        views::kFlexBehaviorKey,
+        views::FlexSpecification(base::BindRepeating(
+            &TabScrollContainerFlexRule, base::Unretained(tab_strip_))));
 
     auto left_scroll = std::make_unique<views::ImageButton>(base::BindRepeating(
         &TabStripRegionView::ScrollLeft, base::Unretained(this)));
@@ -49,16 +76,16 @@ TabStripRegionView::TabStripRegionView(std::unique_ptr<TabStrip> tab_strip) {
     right_scroll_ = AddChildView(std::move(right_scroll));
   } else {
     tab_strip_container_ = AddChildView(std::move(tab_strip));
-  }
 
-  // Allow the |tab_strip_container_| to grow into the free space available in
-  // the TabStripRegionView.
-  const views::FlexSpecification tab_strip_container_flex_spec =
-      views::FlexSpecification(views::LayoutOrientation::kHorizontal,
-                               views::MinimumFlexSizeRule::kScaleToZero,
-                               views::MaximumFlexSizeRule::kUnbounded);
-  tab_strip_container_->SetProperty(views::kFlexBehaviorKey,
-                                    tab_strip_container_flex_spec);
+    // Allow the |tab_strip_container_| to grow into the free space available in
+    // the TabStripRegionView.
+    const views::FlexSpecification tab_strip_container_flex_spec =
+        views::FlexSpecification(views::LayoutOrientation::kHorizontal,
+                                 views::MinimumFlexSizeRule::kScaleToZero,
+                                 views::MaximumFlexSizeRule::kPreferred);
+    tab_strip_container_->SetProperty(views::kFlexBehaviorKey,
+                                      tab_strip_container_flex_spec);
+  }
 
   if (base::FeatureList::IsEnabled(features::kTabSearch) &&
       base::FeatureList::IsEnabled(features::kTabSearchFixedEntrypoint) &&
@@ -172,6 +199,17 @@ void TabStripRegionView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   node_data->role = ax::mojom::Role::kTabList;
 }
 
+void TabStripRegionView::OnViewPreferredSizeChanged(View* view) {
+  DCHECK_EQ(view, tab_strip_);
+
+  // The |tab_strip_|'s preferred size changing can change our own preferred
+  // size; however, with scrolling enabled, the ScrollView does not propagate
+  // ChildPreferredSizeChanged up the view hierarchy, instead assuming that its
+  // own preferred size is independent of its childrens'.
+  // TODO(https://crbug.com/1132488): Make ScrollView not be like that.
+  PreferredSizeChanged();
+}
+
 int TabStripRegionView::CalculateTabStripAvailableWidth() {
   // The tab strip can occupy the space not currently taken by its fixed-width
   // sibling views.
@@ -193,6 +231,7 @@ void TabStripRegionView::ScrollLeft() {
                    visible_content.height());
   scroll_view_container->contents()->ScrollRectToVisible(scroll);
 }
+
 void TabStripRegionView::ScrollRight() {
   views::ScrollView* scroll_view_container =
       static_cast<views::ScrollView*>(tab_strip_container_);

chrome/browser/ui/views/frame/tab_strip_region_view.h

@@ -13,7 +13,8 @@ class TabStrip;
 
 // Container for the tabstrip, new tab button, and reserved grab handle space.
 // TODO (https://crbug.com/949660) Under construction.
-class TabStripRegionView final : public views::AccessiblePaneView {
+class TabStripRegionView final : public views::AccessiblePaneView,
+                                 views::ViewObserver {
  public:
   explicit TabStripRegionView(std::unique_ptr<TabStrip> tab_strip);
   ~TabStripRegionView() override;
@@ -41,6 +42,9 @@ class TabStripRegionView final : public views::AccessiblePaneView {
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
   views::View* GetDefaultFocusableChild() override;
 
+  // views::ViewObserver:
+  void OnViewPreferredSizeChanged(View* view) override;
+
   // TODO(958173): Override OnBoundsChanged to cancel tabstrip animations.
 
  private:

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

@@ -255,6 +255,10 @@ void TabDragControllerTest::AddTabsAndResetBrowser(Browser* browser,
   }
   browser->window()->Show();
   StopAnimating(GetTabStripForBrowser(browser));
+  // Perform any scheduled layouts so the tabstrip is in a steady state.
+  BrowserView::GetBrowserViewForBrowser(browser)
+      ->GetWidget()
+      ->LayoutRootViewIfNecessary();
   ResetIDs(browser->tab_strip_model(), 0);
 }
 
@@ -1299,6 +1303,8 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
 
   ASSERT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_TAB, false,
                                               false, false, false));
+  StopAnimating(tab_strip);
+
   EXPECT_EQ("1 0", IDString(browser()->tab_strip_model()));
   EXPECT_FALSE(TabDragController::IsActive());
   EXPECT_FALSE(tab_strip->GetDragContext()->IsDragSessionActive());
@@ -1347,6 +1353,8 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest,
   ASSERT_TRUE(PressInput(GetCenterInScreenCoordinates(tab_strip->tab_at(1))));
   ASSERT_TRUE(DragInputTo(GetCenterInScreenCoordinates(tab_strip->tab_at(0))));
   ASSERT_TRUE(ReleaseInput());
+  StopAnimating(tab_strip);
+
   EXPECT_EQ("1 0", IDString(model));
   EXPECT_FALSE(TabDragController::IsActive());
   EXPECT_FALSE(tab_strip->GetDragContext()->IsDragSessionActive());
@@ -1946,6 +1954,8 @@ IN_PROC_BROWSER_TEST_P(DetachToBrowserTabDragControllerTest, DragInSameWindow) {
   // Test that the dragging info is correctly set on |tab_strip|.
   EXPECT_TRUE(IsTabDraggingInfoSet(tab_strip, tab_strip));
   ASSERT_TRUE(ReleaseInput());
+  StopAnimating(tab_strip);
+
   EXPECT_EQ("1 0", IDString(model));
   EXPECT_FALSE(TabDragController::IsActive());
   EXPECT_FALSE(tab_strip->GetDragContext()->IsDragSessionActive());

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

@@ -584,7 +584,16 @@ class TabStrip::TabDragContextImpl : public TabDragContext {
   int GetTabAreaWidth() const override { return tab_strip_->GetTabAreaWidth(); }
 
   int GetTabDragAreaWidth() const override {
-    return tab_strip_->width() - tab_strip_->FrameGrabWidth();
+    // There are two cases here (with tab scrolling enabled):
+    // 1) If the tab strip is not wider than the tab strip region (and thus
+    // not scrollable), returning the available width for tabs rather than the
+    // actual width for tabs allows tabs to be dragged past the current bounds
+    // of the tabstrip, anywhere along the tab strip region.
+    // 2) If the tabstrip is wider than the tab strip region (and thus is
+    // scrollable), returning the tab area width allows tabs to be dragged
+    // anywhere within the tabstrip, not just in the leftmost region of it.
+    return std::max(tab_strip_->CalculateAvailableWidthForTabs(),
+                    tab_strip_->GetTabAreaWidth());
   }
 
   int TabDragAreaBeginX() const override {
@@ -819,6 +828,9 @@ class TabStrip::TabDragContextImpl : public TabDragContext {
       }
     }
     tab_strip_->SetTabSlotVisibility();
+    // The rightmost tab may have moved, which would change the tabstrip's
+    // preferred width.
+    tab_strip_->PreferredSizeChanged();
   }
 
   // Forces the entire tabstrip to lay out.
@@ -2215,7 +2227,7 @@ void TabStrip::Layout() {
     // width.
     // It should never be larger than its preferred width.
     const int max_width =
-        layout_helper_->CalculatePreferredWidth() + GetRightSideReservedWidth();
+        CalculatePreferredWidthForTabs() + GetRightSideReservedWidth();
     // It should never be smaller than its minimum width.
     const int min_width = GetMinimumSize().width();
     // If it can, it should fit within the tab strip region.
@@ -2408,7 +2420,7 @@ gfx::Size TabStrip::CalculatePreferredSize() const {
     // tab strip is sized to exactly fit the current position of the tabs.
     preferred_tab_area_width = max_x;
   } else {
-    preferred_tab_area_width = layout_helper_->CalculatePreferredWidth();
+    preferred_tab_area_width = CalculatePreferredWidthForTabs();
   }
   return gfx::Size(preferred_tab_area_width + GetRightSideReservedWidth(),
                    GetLayoutConstant(TAB_HEIGHT));
@@ -2775,6 +2787,8 @@ void TabStrip::SnapToIdealBounds() {
   }
 
   tab_controls_container_->SetBoundsRect(tab_controls_container_ideal_bounds_);
+
+  PreferredSizeChanged();
 }
 
 void TabStrip::ExitTabClosingMode() {
@@ -3488,6 +3502,12 @@ int TabStrip::CalculateAvailableWidthForTabs() {
       GetAvailableWidthForTabStrip() - GetRightSideReservedWidth());
 }
 
+int TabStrip::CalculatePreferredWidthForTabs() const {
+  return override_available_width_for_tabs_
+             ? override_available_width_for_tabs_.value()
+             : layout_helper_->CalculatePreferredWidth();
+}
+
 int TabStrip::GetAvailableWidthForTabStrip() {
   return available_width_callback_ ? available_width_callback_.Run() : width();
 }

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

@@ -599,6 +599,9 @@ class TabStrip : public views::View,
   // Calculates the width that can be occupied by the tabs in the strip.
   int CalculateAvailableWidthForTabs();
 
+  // Calculates the width that tabs would like to occupy.
+  int CalculatePreferredWidthForTabs() const;
+
   // Returns the total width available for the TabStrip's use.
   int GetAvailableWidthForTabStrip();