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 fa9e9d88a2bf886462a3959999e8debd57b0c552..818ad43bb8c2fe6e6d154ba656daede029a1c270 100644
--- a/chrome/browser/ui/views/tabs/tab.cc
+++ b/chrome/browser/ui/views/tabs/tab.cc
@@ -422,16 +422,24 @@ 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 the hit test request is in response to a gesture, |path| should be
+    // empty unless the entire tab close button is visible to the user. Hit
+    // test requests in response to a mouse event should always set |path|
+    // to be the visible portion of the tab close button, even if it is
+    // partially hidden behind another tab.
     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();
-
-      // TODO: this isn't quite right, really need to intersect the two regions.
-      path->addRect(RectToSkRect(button_bounds));
+    gfx::Rect intersection(gfx::IntersectRects(tab_bounds, button_bounds));
+    if (!intersection.IsEmpty()) {
+#if !defined(OS_WIN)

[sky, 2014/02/03 20:55:47]

Why the ifdef?

I also don't understand why we can't always return the intersection?

[tdanderson, 2014/02/03 23:08:13]

If we always return the intersection, then tabs will be too easy to close by
accident with touch in stacked tab mode when there is even the slightest amount
of the tab close button visible. Due to its size, this small intersection is
given priority in rect-based targeting, and I feel that more often than not the
user would not have intended on closing the tab in such cases. That is why I
originally checked for Contains() instead of Intersects() here. (*)

With the clipping of the tab close button on Windows if the tab's font is scaled
up, Contains() will always return false, so returning the intersection is
necessary for Windows only. The code inside the ifdef makes sure that the user
experience is the same on ChromeOS as it was before r246686.

(*) Side note: Also, the fact that we treat everything as a rect also makes it a
bit easier to close things by accident. See the image attached to
crbug.com/338516; the tab close button may not be visible, but part of its
rectangular bounding box still is. Ideally we would use tightly-bounding SkPaths
for everything as you originally suggested, but I have yet to find a nice way to
work with these.

+      // TODO(tdanderson): Enforce this constraint on Windows once the issue
+      //                   described in crbug.com/337850 is resolved; the
+      //                   improper layout/clipping of the tab close button
+      //                   causes Contains() to always return false.
+      if (source == HIT_TEST_SOURCE_TOUCH &&
+          !tab_bounds.Contains(button_bounds))
+        return;
+#endif
+      path->addRect(RectToSkRect(intersection));
     }
   }