Tab close buttons can only be targeted with touch if the whole button is visible

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

Index: chrome/browser/ui/views/tabs/tab.cc
diff --git a/chrome/browser/ui/views/tabs/tab.cc b/chrome/browser/ui/views/tabs/tab.cc
index 7c36320d8f30050043f05e8a064d6cbdd53f73fa..6b177d0fa16079786f8c23e8d366a2dd6e25d45c 100644
--- a/chrome/browser/ui/views/tabs/tab.cc
+++ b/chrome/browser/ui/views/tabs/tab.cc
@@ -419,16 +419,34 @@ class Tab::TabCloseButton : public views::ImageButton {
     views::View::ConvertRectToTarget(tab_, this, &tab_bounds_f);
     gfx::Rect tab_bounds = gfx::ToEnclosingRect(tab_bounds_f);
 
-    // If the button is hidden behind another tab, the hit test mask is empty.
-    // Otherwise set the hit test mask to be the contents bounds.
+    // If either the top or bottom of the tab close button is clipped,
+    // do not consider these regions to be part of the button's bounds.
+    int top_overflow = tab_bounds.y() - button_bounds.y();
+    int bottom_overflow = button_bounds.bottom() - tab_bounds.bottom();
+    if (top_overflow > 0)
+      button_bounds.set_y(tab_bounds.y());
+    else if (bottom_overflow > 0)
+      button_bounds.set_height(button_bounds.height() - bottom_overflow);
+
+    // TODO(tdanderson): Consider always returning the intersection if
+    // the non-rectangular shape of the tabs can be accounted for.
     path->reset();
-    if (tab_bounds.Intersects(button_bounds)) {
-      // Include the padding in the hit test mask for touch events.
-      if (source == HIT_TEST_SOURCE_TOUCH)
-        button_bounds = GetLocalBounds();
+    gfx::Rect intersection(gfx::IntersectRects(tab_bounds, button_bounds));
+    if (intersection.IsEmpty()) {
+      // Do not set a hit test mask if the button is completely hidden.
+      return;
+    } else if (source == HIT_TEST_SOURCE_TOUCH) {

[sky, 2014/02/06 00:24:10]

nit: style guide says no else after a return.

[tdanderson, 2014/02/06 01:19:55]

Done.

+      // If the hit test request is from touch, only return a hit test mask
+      // if the button is completely visible to the user.
+      if (!tab_bounds.Contains(button_bounds))

[sky, 2014/02/06 00:24:10]

Because you adjust button_bounds, can this ever be false? Wouldn't the
intersection be empty in this case?

[tdanderson, 2014/02/06 01:19:55]

The adjustments to |button_bounds| on lines 427 and 429
only change vertical clipping, so this can still be
false if the close button is partially or fully covered
along its right side by another tab.
Not necessarily, since the button can still be 
partially visible.

I think I may be misreading your questions, can you
please clarify?

+        return;
 
-      // TODO: this isn't quite right, really need to intersect the two regions.
-      path->addRect(RectToSkRect(button_bounds));
+      // Includes padding.
+      path->addRect(RectToSkRect(GetLocalBounds()));
+    } else {
+      // If the hit test request is from mouse, set the hit test mask to be
+      // the region of the button that is visible to the user.
+      path->addRect(RectToSkRect(intersection));

[sky, 2014/02/06 00:24:10]

Why don't you want to do this for both cases?

[tdanderson, 2014/02/06 01:19:55]

I was including the extra padding in the hit test
mask when the source was from touch to maintain
the same behaviour from before. But it looks as though
the only difference would be if a touch region was
entirely within the button padding, which in
practice is not possible.

Changed this in the next patch set and updated
the tests accordingly.

     }
   }