Bring TabGroupHeader up to spec.

See bug for the iterations with UX, including screenshots.

I tried to get rid of as many magic numbers as I could, but a few
remain because they're just tweaked until things look right visually.

Bug: 1005333
Change-Id: I479668d5a2c8a5e018b17be23496092792a24ac3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1898615Reviewed-by: Collin Baker <collinbaker@chromium.org>
Reviewed-by: Taylor Bergquist <tbergquist@chromium.org>
Reviewed-by: Charlene Yan <cyan@chromium.org>
Commit-Queue: Connie Wan <connily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713093}

chrome/browser/ui/tabs/tab_style.h

@@ -143,13 +143,13 @@ class TabStyle {
   // or og:image images, etc.
   static gfx::Size GetPreviewImageSize();
 
+  // Returns the radius of the outer corners of the tab shape.
+  static int GetCornerRadius();
+
  protected:
   // Avoid implicitly-deleted constructor.
   TabStyle() = default;
 
-  // Returns the radius of the outer corners of the tab shape.
-  static int GetCornerRadius();
-
   // Returns how far from the leading and trailing edges of a tab the contents
   // should actually be laid out.
   static int GetContentsHorizontalInsetSize();

chrome/browser/ui/views/chrome_layout_provider.cc

@@ -72,8 +72,6 @@ gfx::Insets ChromeLayoutProvider::GetInsetsMetric(int metric) const {
       return touch_ui ? gfx::Insets(8, 10) : gfx::Insets(6);
     case INSETS_TOAST:
       return gfx::Insets(0, kHarmonyLayoutUnit);
-    case INSETS_TAB_GROUP_TITLE_CHIP:
-      return gfx::Insets(0, 12);
     default:
       return LayoutProvider::GetInsetsMetric(metric);
   }
@@ -145,8 +143,6 @@ int ChromeLayoutProvider::GetDistanceMetric(int metric) const {
       return kHarmonyLayoutUnit * 12;
     case DISTANCE_SUBSECTION_HORIZONTAL_INDENT:
       return 0;
-    case DISTANCE_TAB_GROUP_TITLE_CHIP_MARGIN:
-      return 4;
     case DISTANCE_TOAST_CONTROL_VERTICAL:
       return 8;
     case DISTANCE_TOAST_LABEL_VERTICAL:

chrome/browser/ui/views/chrome_layout_provider.h

@@ -19,9 +19,6 @@ enum ChromeInsetsMetric {
   INSETS_BOOKMARKS_BAR_BUTTON = views::VIEWS_INSETS_END,
   // Margins used by toasts.
   INSETS_TOAST,
-  // Margins around the title of a tab group header, that form the bounds of the
-  // title chip.
-  INSETS_TAB_GROUP_TITLE_CHIP,
 };
 
 enum ChromeDistanceMetric {
@@ -51,9 +48,6 @@ enum ChromeDistanceMetric {
   // Horizontal indent of a subsection relative to related items above, e.g.
   // checkboxes below explanatory text/headings.
   DISTANCE_SUBSECTION_HORIZONTAL_INDENT,
-  // The horizontal padding on each side of a tab group header's title chip that
-  // creates visual space between it and the adjacent tabs.
-  DISTANCE_TAB_GROUP_TITLE_CHIP_MARGIN,
   // Vertical margin for controls in a toast.
   DISTANCE_TOAST_CONTROL_VERTICAL,
   // Vertical margin for labels in a toast.

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

@@ -11,9 +11,9 @@
 #include "chrome/browser/ui/layout_constants.h"
 #include "chrome/browser/ui/tabs/tab_group_visual_data.h"
 #include "chrome/browser/ui/tabs/tab_style.h"
-#include "chrome/browser/ui/views/chrome_layout_provider.h"
 #include "chrome/browser/ui/views/tabs/tab_controller.h"
 #include "chrome/browser/ui/views/tabs/tab_group_editor_bubble_view.h"
+#include "chrome/browser/ui/views/tabs/tab_group_underline.h"
 #include "chrome/browser/ui/views/tabs/tab_slot_view.h"
 #include "chrome/browser/ui/views/tabs/tab_strip.h"
 #include "chrome/browser/ui/views/tabs/tab_strip_controller.h"
@@ -40,20 +40,14 @@ TabGroupHeader::TabGroupHeader(TabStrip* tab_strip, TabGroupId group)
 
   set_group(group);
 
-  views::FlexLayout* layout =
-      SetLayoutManager(std::make_unique<views::FlexLayout>());
-  layout->SetOrientation(views::LayoutOrientation::kHorizontal)
-      .SetMainAxisAlignment(views::LayoutAlignment::kCenter)
-      .SetCrossAxisAlignment(views::LayoutAlignment::kCenter);
-
+  // The size and color of the chip are set in VisualsChanged().
   title_chip_ = AddChildView(std::make_unique<views::View>());
-  title_chip_->SetLayoutManager(std::make_unique<views::FillLayout>());
-  // Color and border are set in VisualsChanged().
 
+  // The text and color of the title are set in VisualsChanged().
   title_ = title_chip_->AddChildView(std::make_unique<views::Label>());
   title_->SetCollapseWhenHidden(true);
   title_->SetAutoColorReadabilityEnabled(false);
-  title_->SetHorizontalAlignment(gfx::ALIGN_CENTER);
+  title_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   title_->SetElideBehavior(gfx::FADE_TAIL);
 
   VisualsChanged();
@@ -134,40 +128,73 @@ TabSizeInfo TabGroupHeader::GetTabSizeInfo() const {
 }
 
 int TabGroupHeader::CalculateWidth() const {
-  ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
-  const int title_chip_outside_margin =
-      provider->GetDistanceMetric(DISTANCE_TAB_GROUP_TITLE_CHIP_MARGIN);
   // We don't want tabs to visually overlap group headers, so we add that space
   // to the width to compensate. We don't want to actually remove the overlap
   // during layout however; that would cause an the margin to be visually uneven
   // when the header is in the first slot and thus wouldn't overlap anything to
   // the left.
   const int overlap_margin = TabStyle::GetTabOverlap() * 2;
-  return title_chip_->GetPreferredSize().width() + title_chip_outside_margin +
-         overlap_margin;
+
+  // The empty and non-empty chips have different sizes and corner radii, but
+  // both should look nestled against the group stroke of the tab to the right.
+  // This requires a +/- 2px adjustment to the width, which causes the tab to
+  // the right to be positioned in the right spot.
+  const TabGroupVisualData* data =
+      tab_strip_->controller()->GetVisualDataForGroup(group().value());
+  const int right_adjust = data->title().empty() ? 2 : -2;
+
+  return overlap_margin + title_chip_->width() + right_adjust;
 }
 
 void TabGroupHeader::VisualsChanged() {
-  const ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
   const TabGroupVisualData* data =
       tab_strip_->controller()->GetVisualDataForGroup(group().value());
+
   if (data->title().empty()) {
+    // If the title is empty, the chip is just a circle.
     title_->SetVisible(false);
-    constexpr gfx::Rect kEmptyTitleChipSize(12, 12);
-    title_chip_->SetBorder(
-        views::CreateEmptyBorder(kEmptyTitleChipSize.InsetsFrom(gfx::Rect())));
+
+    constexpr int kEmptyChipSize = 14;
+    const int y = (height() - kEmptyChipSize) / 2;
+
+    title_chip_->SetBounds(TabGroupUnderline::GetStrokeInset(), y,
+                           kEmptyChipSize, kEmptyChipSize);
+    title_chip_->SetBackground(
+        views::CreateRoundedRectBackground(data->color(), kEmptyChipSize / 2));
   } else {
+    // If the title is set, the chip is a rounded rect that matches the active
+    // tab shape, particularly the tab's corner radius.
     title_->SetVisible(true);
     title_->SetEnabledColor(
         color_utils::GetColorWithMaxContrast(data->color()));
     title_->SetText(data->title());
-    title_chip_->SetBorder(views::CreateEmptyBorder(
-        provider->GetInsetsMetric(INSETS_TAB_GROUP_TITLE_CHIP)));
+
+    // Set the radius such that the chip nestles snugly against the tab corner
+    // radius, taking into account the group underline stroke.
+    const int corner_radius =
+        TabStyle::GetCornerRadius() - TabGroupUnderline::kStrokeThickness;
+
+    // Clamp the width to a maximum of half the standard tab width (not counting
+    // overlap).
+    const int max_width =
+        (TabStyle::GetStandardWidth() - TabStyle::GetTabOverlap()) / 2;
+    const int text_width =
+        std::min(title_->GetPreferredSize().width(), max_width);
+    const int text_height = title_->GetPreferredSize().height();
+    const int text_vertical_inset = 1;
+    const int text_horizontal_inset = corner_radius + text_vertical_inset;
+
+    const int y = (height() - text_height) / 2 - text_vertical_inset;
+
+    title_chip_->SetBounds(TabGroupUnderline::GetStrokeInset(), y,
+                           text_width + 2 * text_horizontal_inset,
+                           text_height + 2 * text_vertical_inset);
+    title_chip_->SetBackground(
+        views::CreateRoundedRectBackground(data->color(), corner_radius));
+
+    title_->SetBounds(text_horizontal_inset, text_vertical_inset, text_width,
+                      text_height);
   }
-  title_chip_->SetBackground(views::CreateRoundedRectBackground(
-      data->color(),
-      provider->GetCornerRadiusMetric(views::EMPHASIS_MAXIMUM,
-                                      title_chip_->GetPreferredSize())));
 }
 
 void TabGroupHeader::RemoveObserverFromWidget(views::Widget* widget) {

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

@@ -56,19 +56,22 @@ void TabGroupUnderline::UpdateBounds() {
             kStrokeThickness);
 }
 
+// static
+int TabGroupUnderline::GetStrokeInset() {
+  return TabStyle::GetTabOverlap() + kStrokeThickness;
+}
+
 int TabGroupUnderline::GetStart() const {
   const TabGroupHeader* group_header = tab_strip_->group_header(group_);
 
-  constexpr int kInset = 20;
-  return group_header->bounds().x() + kInset;
+  return group_header->bounds().x() + GetStrokeInset();
 }
 
 int TabGroupUnderline::GetEnd() const {
   // Fall back to the group header end for any corner cases. This ensures
   // that the underline always has a positive width.
   const TabGroupHeader* group_header = tab_strip_->group_header(group_);
-  constexpr int kInset = 20;
-  const int header_end = group_header->bounds().right() - kInset;
+  const int header_end = group_header->bounds().right() - GetStrokeInset();
 
   const std::vector<int> tabs_in_group =
       tab_strip_->controller()->ListTabsInGroup(group_);
@@ -78,8 +81,9 @@ int TabGroupUnderline::GetEnd() const {
   const int last_tab_index = tabs_in_group[tabs_in_group.size() - 1];
   const Tab* last_tab = tab_strip_->tab_at(last_tab_index);
 
-  const int tab_end = last_tab->bounds().right() +
-                      (last_tab->IsActive() ? kStrokeThickness : -kInset);
+  const int tab_end =
+      last_tab->bounds().right() +
+      (last_tab->IsActive() ? kStrokeThickness : -GetStrokeInset());
   return std::max(tab_end, header_end);
 }
 

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

@@ -16,6 +16,7 @@ class TabStrip;
 class TabGroupUnderline : public views::View {
  public:
   static constexpr int kStrokeThickness = 3;
+  static int GetStrokeInset();
 
   TabGroupUnderline(TabStrip* tab_strip, TabGroupId group);
 

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

@@ -1196,12 +1196,11 @@ TEST_P(TabStripTest, GroupUnderlineBasics) {
   // Update underline manually in the absence of a real Paint cycle.
   underline->UpdateBounds();
 
-  constexpr int kInset = 20;
-
-  EXPECT_EQ(underline->x(), kInset);
+  EXPECT_EQ(underline->x(), TabGroupUnderline::GetStrokeInset());
   EXPECT_GT(underline->width(), 0);
   EXPECT_EQ(underline->bounds().right(),
-            tab_strip_->tab_at(0)->bounds().right() - kInset);
+            tab_strip_->tab_at(0)->bounds().right() -
+                TabGroupUnderline::GetStrokeInset());
   EXPECT_EQ(underline->height(), TabGroupUnderline::kStrokeThickness);
 
   // Endpoints are different if the last grouped tab is active.
@@ -1209,7 +1208,7 @@ TEST_P(TabStripTest, GroupUnderlineBasics) {
   controller_->MoveTabIntoGroup(1, group);
   underline->UpdateBounds();
 
-  EXPECT_EQ(underline->x(), kInset);
+  EXPECT_EQ(underline->x(), TabGroupUnderline::GetStrokeInset());
   EXPECT_EQ(underline->bounds().right(),
             tab_strip_->tab_at(1)->bounds().right() +
                 TabGroupUnderline::kStrokeThickness);