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.
(cherry picked from commit 022d70c762cc57f7037f84719be1bca368c2d165)
(cherry picked from commit ca6b4380d9b765a7fd6e47d4863ab1f073cfc5c2)
Bug: 1136557
Change-Id: I8b312363bdabbef358ec2689321bb30d7680953b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2463994
Commit-Queue: Thomas Lukaszewicz <[email protected]>
Reviewed-by: Scott Violet <[email protected]>
Cr-Original-Original-Commit-Position: refs/heads/master@{#815821}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2472602
Reviewed-by: Thomas Lukaszewicz <[email protected]>
Cr-Original-Commit-Position: refs/branch-heads/4280@{#388}
Cr-Original-Branched-From: ea420fb963f9658c9969b6513c56b8f47efa1a2a-refs/heads/master@{#812852}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2476393
Cr-Commit-Position: refs/branch-heads/4240@{#1249}
Cr-Branched-From: f297677702651916bbf65e59c0d4bbd4ce57d1ee-refs/heads/master@{#800218}
diff --git a/chrome/browser/ui/views/tabs/tab_strip.cc b/chrome/browser/ui/views/tabs/tab_strip.cc
index eebe9a46..3b2351f 100644
--- a/chrome/browser/ui/views/tabs/tab_strip.cc
+++ b/chrome/browser/ui/views/tabs/tab_strip.cc
@@ -3547,11 +3547,19 @@
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) {
diff --git a/chrome/browser/ui/views/tabs/tab_strip_unittest.cc b/chrome/browser/ui/views/tabs/tab_strip_unittest.cc
index 5002832..bd78b045 100644
--- a/chrome/browser/ui/views/tabs/tab_strip_unittest.cc
+++ b/chrome/browser/ui/views/tabs/tab_strip_unittest.cc
@@ -170,6 +170,10 @@
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) {
@@ -1343,4 +1347,31 @@
}
}
+// 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));
