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

chrome/browser/ui/views/tabs/tab.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.h"
 
 #include <limits>
 
 #include "base/command_line.h"
 #include "base/debug/alias.h"
@@ skipping 401 matching lines @@
     // button is hidden behind another tab.
     gfx::Path tab_mask;
     tab_->GetHitTestMask(source, &tab_mask);
 
     gfx::Rect button_bounds(GetContentsBounds());
     button_bounds.set_x(GetMirroredXForRect(button_bounds));
     gfx::RectF tab_bounds_f(gfx::SkRectToRectF(tab_mask.getBounds()));
     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.

     }
   }
 
   virtual const char* GetClassName() const OVERRIDE {
     return kTabCloseButtonName;
   }
 
  private:
   Tab* tab_;
 
@@ skipping 1297 matching lines @@
                          const gfx::ImageSkia& image) {
   DCHECK_NE(scale_factor, ui::SCALE_FACTOR_NONE);
   ImageCacheEntry entry;
   entry.resource_id = resource_id;
   entry.scale_factor = scale_factor;
   entry.image = image;
   image_cache_->push_front(entry);
   if (image_cache_->size() > kMaxImageCacheSize)
     image_cache_->pop_back();
 }