Tab Strip: Fix caption area hit testing for TabStripRegionView

Currently if Tab Scrolling is enabled the TabStrip is allowed to grow
beyond the visible bounds of the TabStripRegionView with the help of
a ScrollView.

Existing caption hit testing uses the TabStrip view to determine
whether a rect or point belongs to the caption area. This results in
a bug where areas of the browser window are no longer recognized as
part of the caption area.

This is due to the non-visible parts of the TabStrip continuing to
contribute to the hit testing logic as it overflows.

This patch updates the TabStripRegionView to support the necessary
hit testing logic and ensure that only the visible portion of the
TabStrip is used.

Bug: 1129703

Change-Id: I49ccf9ba573afba84921a66249ef38723787cbba
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2411704
Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
Reviewed-by: Taylor Bergquist <tbergquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#808579}

chrome/browser/ui/views/frame/browser_non_client_frame_view.cc

@@ -321,24 +321,29 @@ bool BrowserNonClientFrameView::DoesIntersectRect(const views::View* target,
   if (!frame_->client_view()->HitTestRect(rect_in_client_view_coords))
     return true;
 
-  // Otherwise, claim |rect| only if it is above the bottom of the tabstrip in
-  // a non-tab portion.
-  TabStrip* tabstrip = browser_view_->tabstrip();
-  // The tabstrip may not be in a Widget (e.g. when switching into immersive
-  // reveal).
-  if (tabstrip->GetWidget()) {
-    gfx::RectF rect_in_tabstrip_coords_f(rect);
-    View::ConvertRectToTarget(this, tabstrip, &rect_in_tabstrip_coords_f);
-    gfx::Rect rect_in_tabstrip_coords =
-        gfx::ToEnclosingRect(rect_in_tabstrip_coords_f);
-    if (rect_in_tabstrip_coords.y() >= tabstrip->GetLocalBounds().bottom()) {
-      // |rect| is below the tabstrip.
+  // Otherwise, claim |rect| only if it is above the bottom of the tab strip
+  // region view in a non-tab portion.
+  TabStripRegionView* tab_strip_region_view =
+      browser_view_->tab_strip_region_view();
+
+  // The |tab_strip_region_view| may not be in a Widget (e.g. when switching
+  // into immersive reveal the BrowserView's TopContainerView is reparented).
+  if (tab_strip_region_view->GetWidget()) {
+    gfx::RectF rect_in_region_view_coords_f(rect);
+    View::ConvertRectToTarget(this, tab_strip_region_view,
+                              &rect_in_region_view_coords_f);
+    gfx::Rect rect_in_region_view_coords =
+        gfx::ToEnclosingRect(rect_in_region_view_coords_f);
+    if (rect_in_region_view_coords.y() >=
+        tab_strip_region_view->GetLocalBounds().bottom()) {
+      // |rect| is below the tab_strip_region_view.
       return false;
     }
 
-    if (tabstrip->HitTestRect(rect_in_tabstrip_coords)) {
+    if (tab_strip_region_view->HitTestRect(rect_in_region_view_coords)) {
       // Claim |rect| if it is in a non-tab portion of the tabstrip.
-      return tabstrip->IsRectInWindowCaption(rect_in_tabstrip_coords);
+      return tab_strip_region_view->IsRectInWindowCaption(
+          rect_in_region_view_coords);
     }
   }
 

chrome/browser/ui/views/frame/browser_view_layout.cc

@@ -135,7 +135,7 @@ BrowserViewLayout::BrowserViewLayout(
     gfx::NativeView host_view,
     BrowserView* browser_view,
     views::View* top_container,
-    views::View* tab_strip_region_view,
+    TabStripRegionView* tab_strip_region_view,
     TabStrip* tab_strip,
     views::View* toolbar,
     InfoBarContainerView* infobar_container,
@@ -233,10 +233,10 @@ int BrowserViewLayout::NonClientHitTest(const gfx::Point& point) {
   // Determine if the TabStrip exists and is capable of being clicked on. We
   // might be a popup window without a TabStrip.
   if (delegate_->IsTabStripVisible()) {
-    // See if the mouse pointer is within the bounds of the TabStrip.
+    // See if the mouse pointer is within the bounds of the TabStripRegionView.
     gfx::Point test_point(point);
-    if (ConvertedHitTest(parent, tab_strip_, &test_point)) {
-      if (tab_strip_->IsPositionInWindowCaption(test_point))
+    if (ConvertedHitTest(parent, tab_strip_region_view_, &test_point)) {
+      if (tab_strip_region_view_->IsPositionInWindowCaption(test_point))
         return HTCAPTION;
       return HTCLIENT;
     }
@@ -244,10 +244,12 @@ int BrowserViewLayout::NonClientHitTest(const gfx::Point& point) {
     // The top few pixels of the TabStrip are a drop-shadow - as we're pretty
     // starved of dragable area, let's give it to window dragging (this also
     // makes sense visually).
+    // TODO(tluk): Investigate the impact removing this has on draggable area
+    // given the tab strip no longer uses shadows.
     views::Widget* widget = browser_view_->GetWidget();
     if (!(widget->IsMaximized() || widget->IsFullscreen()) &&
         (point_in_browser_view_coords.y() <
-            (tab_strip_->y() + kTabShadowSize))) {
+         (tab_strip_region_view_->y() + kTabShadowSize))) {
       // We return HTNOWHERE as this is a signal to our containing
       // NonClientView that it should figure out what the correct hit-test
       // code is given the mouse position...

chrome/browser/ui/views/frame/browser_view_layout.h

@@ -21,6 +21,7 @@ class BrowserViewLayoutDelegate;
 class ImmersiveModeController;
 class InfoBarContainerView;
 class TabStrip;
+class TabStripRegionView;
 
 namespace gfx {
 class Point;
@@ -52,7 +53,7 @@ class BrowserViewLayout : public views::LayoutManager {
                     gfx::NativeView host_view,
                     BrowserView* browser_view,
                     views::View* top_container,
-                    views::View* tab_strip_region_view,
+                    TabStripRegionView* tab_strip_region_view,
                     TabStrip* tab_strip,
                     views::View* toolbar,
                     InfoBarContainerView* infobar_container,
@@ -146,7 +147,7 @@ class BrowserViewLayout : public views::LayoutManager {
   // NOTE: If you add a view, try to add it as a views::View, which makes
   // testing much easier.
   views::View* const top_container_;
-  views::View* const tab_strip_region_view_;
+  TabStripRegionView* const tab_strip_region_view_;
   views::View* const toolbar_;
   InfoBarContainerView* const infobar_container_;
   views::View* const contents_container_;

chrome/browser/ui/views/frame/browser_view_layout_unittest.cc

@@ -172,8 +172,8 @@ class BrowserViewLayoutTest : public ChromeViewsTestBase {
     auto tab_strip = std::make_unique<TabStrip>(
         std::make_unique<FakeBaseTabStripController>());
     tab_strip_ = tab_strip.get();
-    views::View* tab_strip_region_view =
-        top_container_->AddChildView(std::move(tab_strip));
+    TabStripRegionView* tab_strip_region_view = top_container_->AddChildView(
+        std::make_unique<TabStripRegionView>(std::move(tab_strip)));
     webui_tab_strip_ =
         top_container_->AddChildView(CreateFixedSizeView(gfx::Size(800, 200)));
     webui_tab_strip_->SetVisible(false);

chrome/browser/ui/views/frame/tab_strip_region_view.cc

@@ -38,6 +38,30 @@ TabStripRegionView::TabStripRegionView(std::unique_ptr<TabStrip> tab_strip) {
 
 TabStripRegionView::~TabStripRegionView() = default;
 
+bool TabStripRegionView::IsRectInWindowCaption(const gfx::Rect& rect) {
+  const auto get_target_rect = [&](views::View* target) {
+    gfx::RectF rect_in_target_coords_f(rect);
+    View::ConvertRectToTarget(this, target, &rect_in_target_coords_f);
+    return gfx::ToEnclosingRect(rect_in_target_coords_f);
+  };
+
+  // Perform a hit test against the |tab_strip_container_| to ensure that the
+  // rect is within the visible portion of the |tab_strip_| before calling the
+  // tab strip's |IsRectInWindowCaption()|.
+  // TODO(tluk): Address edge case where |rect| might partially intersect with
+  // the |tab_strip_container_| and the |tab_strip_| but not over the same
+  // pixels. This could lead to this returning false when it should be returning
+  // true.
+  if (tab_strip_container_->HitTestRect(get_target_rect(tab_strip_container_)))
+    return tab_strip_->IsRectInWindowCaption(get_target_rect(tab_strip_));
+
+  return true;
+}
+
+bool TabStripRegionView::IsPositionInWindowCaption(const gfx::Point& point) {
+  return IsRectInWindowCaption(gfx::Rect(point, gfx::Size(1, 1)));
+}
+
 const char* TabStripRegionView::GetClassName() const {
   return "TabStripRegionView";
 }

chrome/browser/ui/views/frame/tab_strip_region_view.h

@@ -15,6 +15,16 @@ class TabStripRegionView final : public views::View {
   explicit TabStripRegionView(std::unique_ptr<TabStrip> tab_strip);
   ~TabStripRegionView() override;
 
+  // Returns true if the specified rect intersects the window caption area of
+  // the browser window. |rect| is in the local coordinate space
+  // of |this|.
+  bool IsRectInWindowCaption(const gfx::Rect& rect);
+
+  // A convenience function which calls |IsRectInWindowCaption()| with a rect of
+  // size 1x1 and an origin of |point|. |point| is in the local coordinate space
+  // of |this|.
+  bool IsPositionInWindowCaption(const gfx::Point& point);
+
   // views::View overrides:
   const char* GetClassName() const override;
   void ChildPreferredSizeChanged(views::View* child) override;
@@ -28,7 +38,7 @@ class TabStripRegionView final : public views::View {
   int CalculateTabStripAvailableWidth();
 
   views::View* tab_strip_container_;
-  views::View* tab_strip_;
+  TabStrip* tab_strip_;
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_FRAME_TAB_STRIP_REGION_VIEW_H_