Hide close buttons of inactive stacked tabs by default

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/tabs/tab_strip.h"
 
 #include "base/message_loop/message_loop.h"
 #include "chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.h"
 #include "chrome/browser/ui/views/tabs/tab.h"
 #include "chrome/browser/ui/views/tabs/tab_strip.h"
@@ skipping 298 matching lines @@
 
   // Now tabs have the immersive height.
   int immersive_height = Tab::GetImmersiveHeight();
   EXPECT_EQ(immersive_height, tab_strip_->GetPreferredSize().height());
 
   // Sanity-check immersive tabs are shorter than normal tabs.
   EXPECT_LT(immersive_height, normal_height);
 }
 
 // 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);
 
   controller_->AddTab(0, false);
   controller_->AddTab(1, true);
   controller_->AddTab(2, false);
   controller_->AddTab(3, false);
   ASSERT_EQ(4, 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)));
 
   Tab* active_tab = tab_strip_->tab_at(1);
   active_tab->SetBoundsRect(gfx::Rect(gfx::Point(150, 0), gfx::Size(200, 20)));
   ASSERT_TRUE(active_tab->IsActive());
 
   Tab* right_tab = tab_strip_->tab_at(2);
   right_tab->SetBoundsRect(gfx::Rect(gfx::Point(300, 0), gfx::Size(200, 20)));
 
   Tab* most_right_tab = tab_strip_->tab_at(3);
   most_right_tab->SetBoundsRect(gfx::Rect(gfx::Point(450, 0),
                                           gfx::Size(200, 20)));
 
   // Switch to stacked layout mode and force a layout to ensure tabs stack.
   tab_strip_->SetStackedLayout(true);
   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)));
   EXPECT_TRUE(left_tab->HitTestRect(gfx::Rect(30, 15, 1, 1)));
   EXPECT_TRUE(left_tab->HitTestRect(gfx::Rect(30, 15, 25, 35)));
   EXPECT_TRUE(left_tab->HitTestRect(gfx::Rect(-10, -5, 20, 30)));
 
   // Hit tests in the occluded region of the tab.
   EXPECT_FALSE(left_tab->HitTestRect(gfx::Rect(70, 15, 2, 2)));
   EXPECT_FALSE(left_tab->HitTestRect(gfx::Rect(70, -15, 30, 40)));
   EXPECT_FALSE(left_tab->HitTestRect(gfx::Rect(87, 20, 5, 3)));
 
   // Hit tests completely outside of the tab.
   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));
 
   // Bounds of the tab close button (without padding) in the tab's
   // coordinate space.
   gfx::Rect local_bounds = GetTabCloseHitTestMask(active_tab, true);
   EXPECT_EQ(gfx::Rect(81, 0, 39, 29).ToString(), local_bounds.ToString());
 
   // Hit tests within the tab.
   EXPECT_TRUE(active_tab->HitTestRect(gfx::Rect(30, 15, 1, 1)));
   EXPECT_TRUE(active_tab->HitTestRect(gfx::Rect(30, 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* active_close = active_tab->close_button_;
   EXPECT_FALSE(active_close->HitTestRect(gfx::Rect(1, 1, 1, 1)));
   EXPECT_FALSE(active_close->HitTestRect(gfx::Rect(1, 1, 2, 2)));
   EXPECT_TRUE(active_close->HitTestRect(gfx::Rect(10, 10, 1, 1)));
   EXPECT_TRUE(active_close->HitTestRect(gfx::Rect(10, 10, 25, 35)));
 
 
   // 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)));
   EXPECT_FALSE(most_right_tab->HitTestRect(gfx::Rect(20, 15, 5, 6)));
 
   // 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());
+  controller_->AddTab(2, false);
+  ASSERT_EQ(3, 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)));
+  Tab* tab0 = tab_strip_->tab_at(0);
+  tab0->SetBoundsRect(gfx::Rect(gfx::Point(0, 0), gfx::Size(100, 20)));
 
-  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* tab1 = tab_strip_->tab_at(1);
+  tab1->SetBoundsRect(gfx::Rect(gfx::Point(100, 0), gfx::Size(100, 20)));
+  ASSERT_TRUE(tab1->IsActive());
 
-  // Switch to stacked layout mode and force a layout to ensure tabs stack.
+  Tab* tab2 = tab_strip_->tab_at(2);
+  tab2->SetBoundsRect(gfx::Rect(gfx::Point(200, 0), gfx::Size(100, 20)));
+
+  tab_strip_->SetBounds(0, 0, 300, 20);
+
+  // 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_);
+
+  // 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_->DoLayout();
+  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_);
 
-
-  // Tests involving |left_tab|, which has part of its bounds and its tab
-  // close button partially 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, 91, 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 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)));
+  // 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) {
   tab_strip_->SetBounds(0, 0, 1000, 20);
 
   controller_->AddTab(0, false);
   controller_->AddTab(1, true);
   controller_->AddTab(2, false);
   controller_->AddTab(3, false);
   ASSERT_EQ(4, tab_strip_->tab_count());
@@ skipping 112 matching lines @@
   ASSERT_TRUE(IsPointInTab(most_right_tab, unactive_overlap));
 
   EXPECT_EQ(
       right_tab,
       FindTabView(tab_strip_->GetTooltipHandlerForPoint(unactive_overlap)));
 
   // Confirm that tab strip doe not return tooltip handler for points that
   // don't hit it.
   EXPECT_FALSE(tab_strip_->GetTooltipHandlerForPoint(gfx::Point(-1, 2)));
 }