Hide close buttons of inactive stacked tabs by default

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

Index: chrome/browser/ui/views/tabs/tab_strip_unittest.cc
diff --git a/chrome/browser/ui/views/tabs/tab_strip_unittest.cc b/chrome/browser/ui/views/tabs/tab_strip_unittest.cc
index 8219f60869338d5bdbbb032027ab662a7e52ce12..8b135308e49b031ac757724a8cb18de440b13827 100644
--- a/chrome/browser/ui/views/tabs/tab_strip_unittest.cc
+++ b/chrome/browser/ui/views/tabs/tab_strip_unittest.cc
@@ -316,8 +316,8 @@ TEST_F(TabStripTest, ImmersiveMode) {
 }
 
 // Creates a tab strip in stacked layout mode and verifies the correctness
-// of hit tests against the visible/occluded regions of a tab and
-// visible/occluded tab close buttons.
+// of hit tests against the visible/occluded regions of a tab and the tab
+// close button of the active tab.
 TEST_F(TabStripTest, TabHitTestMaskWhenStacked) {
   tab_strip_->SetBounds(0, 0, 300, 20);
 
@@ -346,22 +346,13 @@ TEST_F(TabStripTest, TabHitTestMaskWhenStacked) {
   tab_strip_->DoLayout();
 
 
-  // Tests involving |left_tab|, which has part of its bounds and its tab
-  // close button completely occluded by |active_tab|.
+  // Tests involving |left_tab|, which has part of its bounds occluded by
+  // |active_tab|.
 
   // Bounds of the tab's hit test mask.
   gfx::Rect tab_bounds = GetTabHitTestMask(left_tab);
   EXPECT_EQ(gfx::Rect(6, 2, 61, 27).ToString(), tab_bounds.ToString());
 
-  // Bounds of the tab close button (without padding) in the tab's
-  // coordinate space.
-  gfx::Rect contents_bounds = GetTabCloseHitTestMask(left_tab, false);
-  // TODO(tdanderson): Uncomment this line once crbug.com/311609 is resolved.
-  //EXPECT_EQ(gfx::Rect(84, 8, 18, 18).ToString(), contents_bounds.ToString());
-
-  // Verify that the tab close button is completely occluded.
-  EXPECT_FALSE(tab_bounds.Contains(contents_bounds));
-
   // Hit tests in the non-occuluded region of the tab.
   EXPECT_TRUE(left_tab->HitTestRect(gfx::Rect(6, 2, 2, 2)));
   EXPECT_TRUE(left_tab->HitTestRect(gfx::Rect(6, 2, 1, 1)));
@@ -378,22 +369,14 @@ TEST_F(TabStripTest, TabHitTestMaskWhenStacked) {
   EXPECT_FALSE(left_tab->HitTestRect(gfx::Rect(-20, -25, 1, 1)));
   EXPECT_FALSE(left_tab->HitTestRect(gfx::Rect(-20, -25, 3, 19)));
 
-  // All hit tests against the tab close button should fail because
-  // it is occluded by |active_tab|.
-  views::ImageButton* left_close = left_tab->close_button_;
-  EXPECT_FALSE(left_close->HitTestRect(gfx::Rect(1, 1, 1, 1)));
-  EXPECT_FALSE(left_close->HitTestRect(gfx::Rect(1, 1, 5, 10)));
-  EXPECT_FALSE(left_close->HitTestRect(gfx::Rect(10, 10, 1, 1)));
-  EXPECT_FALSE(left_close->HitTestRect(gfx::Rect(10, 10, 3, 4)));
-
 
   // Tests involving |active_tab|, which is completely visible.
 
   tab_bounds = GetTabHitTestMask(active_tab);
   EXPECT_EQ(gfx::Rect(6, 2, 108, 27).ToString(), tab_bounds.ToString());
-  contents_bounds = GetTabCloseHitTestMask(active_tab, false);
+  gfx::Rect contents_bounds = GetTabCloseHitTestMask(active_tab, false);
   // TODO(tdanderson): Uncomment this line once crbug.com/311609 is resolved.
-  //EXPECT_EQ(gfx::Rect(84, 8, 18, 18).ToString(), contents_bounds.ToString());
+  // EXPECT_EQ(gfx::Rect(84, 8, 18, 18).ToString(), contents_bounds.ToString());
 
   // Verify that the tab close button is not occluded.
   EXPECT_TRUE(tab_bounds.Contains(contents_bounds));
@@ -418,18 +401,10 @@ TEST_F(TabStripTest, TabHitTestMaskWhenStacked) {
 
 
   // Tests involving |most_right_tab|, which has part of its bounds occluded
-  // by |right_tab| but has its tab close button completely visible.
+  // by |right_tab|.
 
   tab_bounds = GetTabHitTestMask(most_right_tab);
   EXPECT_EQ(gfx::Rect(84, 2, 30, 27).ToString(), tab_bounds.ToString());
-  contents_bounds = GetTabCloseHitTestMask(active_tab, false);
-  // TODO(tdanderson): Uncomment this line once crbug.com/311609 is resolved.
-  //EXPECT_EQ(gfx::Rect(84, 8, 18, 18).ToString(), contents_bounds.ToString());
-  local_bounds = GetTabCloseHitTestMask(active_tab, true);
-  EXPECT_EQ(gfx::Rect(81, 0, 39, 29).ToString(), local_bounds.ToString());
-
-  // Verify that the tab close button is not occluded.
-  EXPECT_TRUE(tab_bounds.Contains(contents_bounds));
 
   // Hit tests in the occluded region of the tab.
   EXPECT_FALSE(most_right_tab->HitTestRect(gfx::Rect(20, 15, 1, 1)));
@@ -438,74 +413,50 @@ TEST_F(TabStripTest, TabHitTestMaskWhenStacked) {
   // Hit tests in the non-occluded region of the tab.
   EXPECT_TRUE(most_right_tab->HitTestRect(gfx::Rect(85, 15, 1, 1)));
   EXPECT_TRUE(most_right_tab->HitTestRect(gfx::Rect(85, 15, 2, 2)));
-
-  // Hit tests against the tab close button. Note that hit tests from either
-  // mouse or touch should both fail if they are strictly contained within
-  // the button's padding.
-  views::ImageButton* most_right_close = most_right_tab->close_button_;
-  EXPECT_FALSE(most_right_close->HitTestRect(gfx::Rect(1, 1, 1, 1)));
-  EXPECT_FALSE(most_right_close->HitTestRect(gfx::Rect(1, 1, 2, 2)));
-  EXPECT_TRUE(most_right_close->HitTestRect(gfx::Rect(10, 10, 1, 1)));
-  EXPECT_TRUE(most_right_close->HitTestRect(gfx::Rect(10, 10, 25, 35)));
-  EXPECT_TRUE(most_right_close->HitTestRect(gfx::Rect(-10, 10, 25, 35)));
 }
 
-// Creates a tab strip in stacked layout mode and verifies the correctness
-// of hit tests against the visible/occluded region of a partially-occluded
-// tab close button.
-TEST_F(TabStripTest, ClippedTabCloseButton) {
-  tab_strip_->SetBounds(0, 0, 220, 20);
-
+// Tests that the tab close buttons of non-active tabs are hidden when
+// the tabstrip is in stacked tab mode.
+TEST_F(TabStripTest, TabCloseButtonVisibilityWhenStacked) {

[tdanderson, 2015/03/06 19:59:21]

My test approach here feels like a bit of a hack, but this is the only way I
could find in order to trigger a relayout (and thus the new code path) for
stacked tab mode. I was unable to implement working tests that would let me
properly switch tabs and add tabs while in stacked mode. Can you offer any
suggestions here?

[sky, 2015/03/09 19:55:59]

Your change means we need to make sure SetStackedLayout() invokes Layout() on
all the tabs. That isn't necessarily the case right now. You'll have to update
SetStackedLayout() to do this.

Can you elaborate one what problems you had in adding/removing? Maybe you're
tripping over the bug I mentioned TabStrip::ShouldHideCloseButtonForInactiveTabs
?

[tdanderson, 2015/03/11 18:01:38]

I've made comments about this in the latest patchset.

   controller_->AddTab(0, false);
   controller_->AddTab(1, true);
-  ASSERT_EQ(2, tab_strip_->tab_count());
-
-  Tab* left_tab = tab_strip_->tab_at(0);
-  left_tab->SetBoundsRect(gfx::Rect(gfx::Point(0, 0), gfx::Size(200, 20)));
+  controller_->AddTab(2, false);
+  ASSERT_EQ(3, tab_strip_->tab_count());
 
-  Tab* active_tab = tab_strip_->tab_at(1);
-  active_tab->SetBoundsRect(gfx::Rect(gfx::Point(180, 0), gfx::Size(200, 20)));
-  ASSERT_TRUE(active_tab->IsActive());
+  Tab* tab0 = tab_strip_->tab_at(0);
+  tab0->SetBoundsRect(gfx::Rect(gfx::Point(0, 0), gfx::Size(100, 20)));
 
-  // Switch to stacked layout mode and force a layout to ensure tabs stack.
-  tab_strip_->SetStackedLayout(true);
-  tab_strip_->DoLayout();
+  Tab* tab1 = tab_strip_->tab_at(1);
+  tab1->SetBoundsRect(gfx::Rect(gfx::Point(100, 0), gfx::Size(100, 20)));
+  ASSERT_TRUE(tab1->IsActive());
 
+  Tab* tab2 = tab_strip_->tab_at(2);
+  tab2->SetBoundsRect(gfx::Rect(gfx::Point(200, 0), gfx::Size(100, 20)));
 
-  // Tests involving |left_tab|, which has part of its bounds and its tab
-  // close button partially occluded by |active_tab|.
+  tab_strip_->SetBounds(0, 0, 300, 20);
 
-  // Bounds of the tab's hit test mask.
-  gfx::Rect tab_bounds = GetTabHitTestMask(left_tab);
-  EXPECT_EQ(gfx::Rect(6, 2, 91, 27).ToString(), tab_bounds.ToString());
+  // Ensure that all tab close buttons are visible.
+  EXPECT_TRUE(tab0->showing_close_button_);
+  EXPECT_TRUE(tab1->showing_close_button_);
+  EXPECT_TRUE(tab2->showing_close_button_);
 
-  // Bounds of the tab close button (without padding) in the tab's
-  // coordinate space.
-  gfx::Rect contents_bounds = GetTabCloseHitTestMask(left_tab, false);
-  // TODO(tdanderson): Uncomment this line once crbug.com/311609 is resolved.
-  //EXPECT_EQ(gfx::Rect(84, 8, 18, 18).ToString(), contents_bounds.ToString());
-
-  // Verify that the tab close button is only partially occluded.
-  EXPECT_FALSE(tab_bounds.Contains(contents_bounds));
-  EXPECT_TRUE(tab_bounds.Intersects(contents_bounds));
-
-  views::ImageButton* left_close = left_tab->close_button_;
-
-  // Hit tests from mouse should return true if and only if the location
-  // is within a visible region.
-  EXPECT_FALSE(left_close->HitTestRect(gfx::Rect(2, 15, 1, 1)));
-  EXPECT_TRUE(left_close->HitTestRect(gfx::Rect(3, 15, 1, 1)));
-  EXPECT_TRUE(left_close->HitTestRect(gfx::Rect(10, 10, 1, 1)));
-  EXPECT_TRUE(left_close->HitTestRect(gfx::Rect(15, 12, 1, 1)));
-  EXPECT_FALSE(left_close->HitTestRect(gfx::Rect(16, 10, 1, 1)));
-
-  // All hit tests from touch should return false because the button is
-  // not fully visible.
-  EXPECT_FALSE(left_close->HitTestRect(gfx::Rect(2, 15, 2, 2)));
-  EXPECT_FALSE(left_close->HitTestRect(gfx::Rect(3, 15, 25, 25)));
-  EXPECT_FALSE(left_close->HitTestRect(gfx::Rect(10, 10, 4, 5)));
-  EXPECT_FALSE(left_close->HitTestRect(gfx::Rect(15, 12, 2, 2)));
-  EXPECT_FALSE(left_close->HitTestRect(gfx::Rect(16, 10, 20, 20)));
+  // Only the close button of the active tab should be visible
+  // in stacked tab mode. Setting the tab strip size triggers
+  // a relayout.
+  tab_strip_->SetStackedLayout(true);
+  tab_strip_->SetBounds(0, 0, 200, 20);
+  EXPECT_FALSE(tab0->showing_close_button_);
+  EXPECT_TRUE(tab1->showing_close_button_);
+  EXPECT_FALSE(tab2->showing_close_button_);
+
+  // When exiting stacked layout mode all tab close buttons should
+  // become visible again. Setting the tab strip size triggers
+  // a relayout.
+  tab_strip_->SetStackedLayout(false);
+  tab_strip_->SetBounds(0, 0, 300, 20);
+  EXPECT_TRUE(tab0->showing_close_button_);
+  EXPECT_TRUE(tab1->showing_close_button_);
+  EXPECT_TRUE(tab2->showing_close_button_);
 }
 
 TEST_F(TabStripTest, GetEventHandlerForOverlappingArea) {