Polish TabGroupUnderline UI

This addresses #1, #2, #5, and #6 from the attached bug.

#3 and #4 are not addressed because there isn't a good way to do so without relying on TabStrip::GetTabAnimationTargetBounds, which in turn relies on the soon-to-be-deprecated BoundsAnimator. If there is a replacement in the new animation system we can revisit, but otherwise it's likely not worth it to fix. The issue is only visible during the brief period when a tab is animating across the tab strip, and it's not present when dragging.

See the screenshots on the attached bug for updated visuals.

Bug: 1008438
Change-Id: I013f3a8bd141ec8e159773522251f5fb4eadd8d6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1830400
Commit-Queue: Connie Wan <connily@chromium.org>
Reviewed-by: Taylor Bergquist <tbergquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#701362}

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

@@ -676,7 +676,6 @@ void Tab::SetGroup(base::Optional<TabGroupId> group) {
   if (group_ == group)
     return;
   group_ = group;
-  SchedulePaint();
 }
 
 base::Optional<SkColor> Tab::GetGroupColor() const {

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

@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "chrome/browser/ui/tabs/tab_group_visual_data.h"
+#include "chrome/browser/ui/tabs/tab_style.h"
 #include "chrome/browser/ui/views/tabs/tab.h"
 #include "chrome/browser/ui/views/tabs/tab_group_header.h"
 #include "chrome/browser/ui/views/tabs/tab_strip.h"
@@ -21,17 +22,27 @@ constexpr int TabGroupUnderline::kStrokeThickness;
 
 TabGroupUnderline::TabGroupUnderline(TabStrip* tab_strip, TabGroupId group)
     : tab_strip_(tab_strip), group_(group) {
-  UpdateVisuals();
+  UpdateBounds();
 }
 
 void TabGroupUnderline::OnPaint(gfx::Canvas* canvas) {
-  UpdateVisuals();
-  OnPaintBackground(canvas);
+  UpdateBounds();
+
+  SkPath path = GetPath();
+  cc::PaintFlags flags;
+  flags.setAntiAlias(true);
+  flags.setColor(GetColor());
+  flags.setStyle(cc::PaintFlags::kFill_Style);
+  canvas->DrawPath(path, flags);
+
+  // Ensure the active tab border stroke is repainted.
+  const int active_index = tab_strip_->controller()->GetActiveIndex();
+  if (active_index != ui::ListSelectionModel::kUnselectedIndex &&
+      tab_strip_->tab_at(active_index)->group() == group_)
+    tab_strip_->tab_at(active_index)->SchedulePaint();
 }
 
-void TabGroupUnderline::UpdateVisuals() {
-  SkColor color = GetColor();
-
+void TabGroupUnderline::UpdateBounds() {
   const int start_x = GetStart();
   const int end_x = GetEnd();
 
@@ -39,31 +50,52 @@ void TabGroupUnderline::UpdateVisuals() {
 
   SetBounds(start_x, start_y - kStrokeThickness, end_x - start_x,
             kStrokeThickness);
-  SetBackground(views::CreateSolidBackground(color));
-}
-
-SkColor TabGroupUnderline::GetColor() const {
-  const TabGroupVisualData* data =
-      tab_strip_->controller()->GetVisualDataForGroup(group_);
-
-  return data->color();
 }
 
 int TabGroupUnderline::GetStart() const {
-  const gfx::Rect group_header_bounds =
-      tab_strip_->group_header(group_)->bounds();
+  const TabGroupHeader* group_header = tab_strip_->group_header(group_);
 
   constexpr int kInset = 20;
-  return group_header_bounds.x() + kInset;
+  return group_header->bounds().x() + kInset;
 }
 
 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 std::vector<int> tabs_in_group =
       tab_strip_->controller()->ListTabsInGroup(group_);
+  if (tabs_in_group.size() <= 0)
+    return header_end;
+
   const int last_tab_index = tabs_in_group[tabs_in_group.size() - 1];
-  const gfx::Rect& last_tab_bounds =
-      tab_strip_->tab_at(last_tab_index)->bounds();
+  const Tab* last_tab = tab_strip_->tab_at(last_tab_index);
 
-  constexpr int kInset = 20;
-  return last_tab_bounds.right() - kInset;
+  const int tab_end = last_tab->bounds().right() +
+                      (last_tab->IsActive() ? kStrokeThickness : -kInset);
+  return std::max(tab_end, header_end);
+}
+
+SkPath TabGroupUnderline::GetPath() const {
+  SkPath path;
+
+  path.moveTo(0, kStrokeThickness);
+  path.arcTo(kStrokeThickness, kStrokeThickness, 0, SkPath::kSmall_ArcSize,
+             SkPath::kCW_Direction, kStrokeThickness, 0);
+  path.lineTo(width() - kStrokeThickness, 0);
+  path.arcTo(kStrokeThickness, kStrokeThickness, 0, SkPath::kSmall_ArcSize,
+             SkPath::kCW_Direction, width(), kStrokeThickness);
+  path.close();
+
+  return path;
+}
+
+SkColor TabGroupUnderline::GetColor() const {
+  const TabGroupVisualData* data =
+      tab_strip_->controller()->GetVisualDataForGroup(group_);
+
+  return data->color();
 }

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

@@ -15,29 +15,35 @@ class TabStrip;
 // strip flow and positioned across all tabs in the group.
 class TabGroupUnderline : public views::View {
  public:
-  static constexpr int kStrokeThickness = 2;
+  static constexpr int kStrokeThickness = 3;
 
   TabGroupUnderline(TabStrip* tab_strip, TabGroupId group);
 
   TabGroupId group() const { return group_; }
 
-  // Updates the bounds and color of the underline for painting.
-  void UpdateVisuals();
+  // Updates the bounds of the underline for painting.
+  void UpdateBounds();
 
   // views::View:
   void OnPaint(gfx::Canvas* canvas) override;
 
  private:
-  // The underline color is the group color.
-  SkColor GetColor() const;
-
   // The underline starts at the left edge of the header chip.
   int GetStart() const;
 
   // The underline ends at the right edge of the last grouped tab's close
-  // button.
+  // button. If the last grouped tab is active, the underline ends at the
+  // right edge of the active tab border stroke.
   int GetEnd() const;
 
+  // The underline is a straight line with half-rounded endcaps. Since this
+  // geometry is nontrivial to represent using primitives, it's instead
+  // represented using a fill path.
+  SkPath GetPath() const;
+
+  // The underline color is the group color.
+  SkColor GetColor() const;
+
   TabStrip* const tab_strip_;
   const TabGroupId group_;
 

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

@@ -1136,41 +1136,45 @@ void TabStrip::ChangeTabGroup(int model_index,
                               base::Optional<TabGroupId> old_group,
                               base::Optional<TabGroupId> new_group) {
   tab_at(model_index)->SetGroup(new_group);
-  if (new_group.has_value() && !group_headers_[new_group.value()]) {
-    auto header = std::make_unique<TabGroupHeader>(this, new_group.value());
-    header->set_owned_by_client();
-    AddChildView(header.get());
-    layout_helper_->InsertGroupHeader(
-        new_group.value(), header.get(),
-        base::BindOnce(&TabStrip::OnGroupCloseAnimationCompleted,
-                       base::Unretained(this), new_group.value()));
-    group_headers_[new_group.value()] = std::move(header);
-
-    auto underline =
-        std::make_unique<TabGroupUnderline>(this, new_group.value());
-    underline->set_owned_by_client();
-    AddChildView(underline.get());
-    group_underlines_[new_group.value()] = std::move(underline);
+  if (new_group.has_value()) {
+    if (!group_headers_[new_group.value()]) {
+      auto header = std::make_unique<TabGroupHeader>(this, new_group.value());
+      header->set_owned_by_client();
+      AddChildView(header.get());
+      layout_helper_->InsertGroupHeader(
+          new_group.value(), header.get(),
+          base::BindOnce(&TabStrip::OnGroupCloseAnimationCompleted,
+                         base::Unretained(this), new_group.value()));
+      group_headers_[new_group.value()] = std::move(header);
+    }
+    if (!group_underlines_[new_group.value()]) {
+      auto underline =
+          std::make_unique<TabGroupUnderline>(this, new_group.value());
+      underline->set_owned_by_client();
+      AddChildView(underline.get());
+      group_underlines_[new_group.value()] = std::move(underline);
+    }
+    // The group header may be in the wrong place if the tab didn't actually
+    // move in terms of model indices.
+    layout_helper_->UpdateGroupHeaderIndex(new_group.value());
+    group_underlines_[new_group.value()]->SchedulePaint();
   }
   if (old_group.has_value()) {
     if (controller_->ListTabsInGroup(old_group.value()).size() == 0) {
       layout_helper_->RemoveGroupHeader(old_group.value());
     } else {
-      // The group header may be in the wrong place if the tab didn't actually
-      // move in terms of model indices.
+      // As above, ensure the header is in the right place.
       layout_helper_->UpdateGroupHeaderIndex(old_group.value());
+      group_underlines_[old_group.value()]->SchedulePaint();
     }
   }
-  if (new_group.has_value()) {
-    // As above, ensure the header is in the right place.
-    layout_helper_->UpdateGroupHeaderIndex(new_group.value());
-  }
   UpdateIdealBounds();
   AnimateToIdealBounds();
 }
 
 void TabStrip::GroupVisualsChanged(TabGroupId group) {
   group_headers_[group]->VisualsChanged();
+  group_underlines_[group]->SchedulePaint();
   // The group title may have changed size.
   UpdateIdealBounds();
   AnimateToIdealBounds();

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

@@ -1184,8 +1184,7 @@ TEST_P(TabStripTest, DeleteTabGroupHeaderAndUnderlineWhenEmpty) {
 TEST_P(TabStripTest, GroupUnderlineBasics) {
   tab_strip_->SetBounds(0, 0, 1000, 100);
   bounds_animator()->SetAnimationDuration(base::TimeDelta());
-  tab_strip_->AddTabAt(0, TabRendererData(), false);
-  tab_strip_->AddTabAt(1, TabRendererData(), false);
+  controller_->AddTab(0, false);
 
   base::Optional<TabGroupId> group = TabGroupId::GenerateNew();
   controller_->MoveTabIntoGroup(0, group);
@@ -1195,13 +1194,25 @@ TEST_P(TabStripTest, GroupUnderlineBasics) {
   EXPECT_EQ(1u, underlines.size());
   TabGroupUnderline* underline = underlines[0];
   // Update underline manually in the absence of a real Paint cycle.
-  underline->UpdateVisuals();
+  underline->UpdateBounds();
+
   constexpr int kInset = 20;
+
   EXPECT_EQ(underline->x(), kInset);
   EXPECT_GT(underline->width(), 0);
   EXPECT_EQ(underline->bounds().right(),
             tab_strip_->tab_at(0)->bounds().right() - kInset);
   EXPECT_EQ(underline->height(), TabGroupUnderline::kStrokeThickness);
+
+  // Endpoints are different if the last grouped tab is active.
+  controller_->AddTab(1, true);
+  controller_->MoveTabIntoGroup(1, group);
+  underline->UpdateBounds();
+
+  EXPECT_EQ(underline->x(), kInset);
+  EXPECT_EQ(underline->bounds().right(),
+            tab_strip_->tab_at(1)->bounds().right() +
+                TabGroupUnderline::kStrokeThickness);
 }
 
 TEST_P(TabStripTest, ChangingLayoutTypeResizesTabs) {