Views: Fix tab controls to sit flush with the top of the tab strip

This CL updates the tab strip's UpdateNewTabButtonBorder() method to
apply the vertical border insets to the tab control buttons rather
than the tab controls container.

This is necessary as the NewTabButton will extend its targetable
area to the top of its bounds when the tab strip is in a condensed
state (i.e. the window is maximized). This is done to ensure the
tab strip controls comply with Fitt's law expectations.

Bug: 1136557
Change-Id: I8b312363bdabbef358ec2689321bb30d7680953b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2463994
Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815821}

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

@@ -3646,11 +3646,19 @@ void TabStrip::UpdateNewTabButtonBorder() {
   const int extra_vertical_space = GetLayoutConstant(TAB_HEIGHT) -
                                    GetLayoutConstant(TABSTRIP_TOOLBAR_OVERLAP) -
                                    NewTabButton::kButtonSize.height();
+
+  // Extend the border of the tab controls such that it extends to the top of
+  // the tabstrip bounds. This is essential to ensure they targetable on the
+  // edge of the screen when in fullscreen mode and ensures the buttons abide by
+  // the correct Fitt's Law behavior (https://crbug.com/1136557).
+  const gfx::Insets button_insets(extra_vertical_space / 2, 0, 0, 0);
+  new_tab_button_->SetBorder(views::CreateEmptyBorder(button_insets));
+  if (tab_search_button_)
+    tab_search_button_->SetBorder(views::CreateEmptyBorder(button_insets));
+
   constexpr int kHorizontalInset = 8;
-  // TODO(tluk): Investigate whether this should apply to the |new_tab_button_|
-  // or the |tab_controls_container_|.
-  tab_controls_container_->SetBorder(views::CreateEmptyBorder(gfx::Insets(
-      extra_vertical_space / 2, kHorizontalInset, 0, kHorizontalInset)));
+  tab_controls_container_->SetBorder(views::CreateEmptyBorder(
+      gfx::Insets(0, kHorizontalInset, 0, kHorizontalInset)));
 }
 
 void TabStrip::OnTabSlotAnimationProgressed(TabSlotView* view) {

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

@@ -138,6 +138,10 @@ class TabStripTest : public ChromeViewsTestBase,
     return tab->icon_->ShowingAttentionIndicator();
   }
 
+  views::View* tab_controls_container() {
+    return tab_strip_->tab_controls_container_;
+  }
+
   // Checks whether |tab| contains |point_in_tabstrip_coords|, where the point
   // is in |tab_strip_| coordinates.
   bool IsPointInTab(Tab* tab, const gfx::Point& point_in_tabstrip_coords) {
@@ -1336,4 +1340,31 @@ TEST_P(TabStripTest, ChangingLayoutTypeResizesTabs) {
   }
 }
 
+// We want to make sure that the new tab button sits flush with the top of the
+// tab strip. This is important in ensuring that we maximise the targetable area
+// of the new tab button and users are able to hit the new tab button when the
+// tab strip is flush with the top of the screen when the window is maximized
+// (https://crbug.com/1136557).
+TEST_P(TabStripTest, NewTabButtonFlushWithTopOfTabStrip) {
+  tab_strip_parent_->SetBounds(0, 0, 1000, 100);
+  controller_->AddTab(0, true);
+
+  AnimateToIdealBounds();
+
+  // |tab_controls_container_| should sit flush with the top of the tab strip.
+  EXPECT_EQ(0, tab_strip_->tab_controls_container_ideal_bounds().y());
+
+  // The new tab button should sit flush with the top of the
+  // |tab_controls_container_|.
+  EXPECT_EQ(0, tab_strip_->new_tab_button()->bounds().y());
+
+  // The new tab button should be positioned flush with the top of the tab
+  // strip.
+  gfx::RectF ntb_in_child_coords_f(tab_strip_->new_tab_button()->bounds());
+  views::View::ConvertRectToTarget(tab_controls_container(), tab_strip_,
+                                   &ntb_in_child_coords_f);
+  gfx::Rect ntb_in_child_coords = gfx::ToEnclosingRect(ntb_in_child_coords_f);
+  EXPECT_EQ(0, ntb_in_child_coords.y());
+}
+
 INSTANTIATE_TEST_SUITE_P(All, TabStripTest, ::testing::Values(false, true));