Fixed layout of the find bar.

Fixed logic governing bounding box of the find bar in the browser
layout view. Removed unnecessary insetting and fixed comments to
better reflect its function.

Bug: 871419
Change-Id: I89a341825508b1e088b0f10f449eb2c3a91fcc38
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1866969
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#711355}

chrome/browser/ui/find_bar/find_bar.h

@@ -56,10 +56,10 @@ class FindBar {
       const gfx::Range& selected_range) = 0;
 
   // Gets the search string currently visible in the find box.
-  virtual base::string16 GetFindText() = 0;
+  virtual base::string16 GetFindText() const = 0;
 
   // Gets the selection.
-  virtual gfx::Range GetSelectedRange() = 0;
+  virtual gfx::Range GetSelectedRange() const = 0;
 
   // Updates the FindBar with the find result details contained within the
   // specified |result|.
@@ -69,21 +69,21 @@ class FindBar {
   // No match was found; play an audible alert.
   virtual void AudibleAlert() = 0;
 
-  virtual bool IsFindBarVisible() = 0;
+  virtual bool IsFindBarVisible() const = 0;
 
   // Upon dismissing the window, restore focus to the last focused view which is
   // not FindBarView or any of its children.
   virtual void RestoreSavedFocus() = 0;
 
   // Returns true if all tabs use a single find pasteboard.
-  virtual bool HasGlobalFindPasteboard() = 0;
+  virtual bool HasGlobalFindPasteboard() const = 0;
 
   // Called when the web contents associated with the find bar changes.
   virtual void UpdateFindBarForChangedWebContents() = 0;
 
   // Returns a pointer to the testing interface to the FindBar, or NULL
   // if there is none.
-  virtual FindBarTesting* GetFindBarTesting() = 0;
+  virtual const FindBarTesting* GetFindBarTesting() const = 0;
 };
 
 class FindBarTesting {
@@ -97,19 +97,19 @@ class FindBarTesting {
   // This is used for UI tests of the find bar. If the find bar is not currently
   // shown (return value of false), the out params will be {(0, 0), false}.
   virtual bool GetFindBarWindowInfo(gfx::Point* position,
-                                    bool* fully_visible) = 0;
+                                    bool* fully_visible) const = 0;
 
   // Gets the search string currently selected in the Find box.
-  virtual base::string16 GetFindSelectedText() = 0;
+  virtual base::string16 GetFindSelectedText() const = 0;
 
   // Gets the match count text (ie. 1 of 3) visible in the Find box.
-  virtual base::string16 GetMatchCountText() = 0;
+  virtual base::string16 GetMatchCountText() const = 0;
 
-  // Gets the pixel width of the FindBar.
-  virtual int GetWidth() = 0;
+  // Gets the pixel width of the FindBar contents.
+  virtual int GetContentsWidth() const = 0;
 
   // Gets the number of audible alerts that have been issued by this bar.
-  virtual size_t GetAudibleAlertCount() = 0;
+  virtual size_t GetAudibleAlertCount() const = 0;
 };
 
 #endif  // CHROME_BROWSER_UI_FIND_BAR_FIND_BAR_H_

chrome/browser/ui/find_bar/find_bar_host_browsertest.cc

@@ -90,7 +90,7 @@ class FindInPageControllerTest : public InProcessBrowserTest {
  protected:
   bool GetFindBarWindowInfoForBrowser(
       Browser* browser, gfx::Point* position, bool* fully_visible) {
-    FindBarTesting* find_bar =
+    const FindBarTesting* find_bar =
         browser->GetFindBarController()->find_bar()->GetFindBarTesting();
     return find_bar->GetFindBarWindowInfo(position, fully_visible);
   }
@@ -109,7 +109,7 @@ class FindInPageControllerTest : public InProcessBrowserTest {
   }
 
   base::string16 GetFindBarMatchCountTextForBrowser(Browser* browser) {
-    FindBarTesting* find_bar =
+    const FindBarTesting* find_bar =
         browser->GetFindBarController()->find_bar()->GetFindBarTesting();
     return find_bar->GetMatchCountText();
   }
@@ -119,13 +119,13 @@ class FindInPageControllerTest : public InProcessBrowserTest {
   }
 
   int GetFindBarWidthForBrowser(Browser* browser) {
-    FindBarTesting* find_bar =
+    const FindBarTesting* find_bar =
         browser->GetFindBarController()->find_bar()->GetFindBarTesting();
-    return find_bar->GetWidth();
+    return find_bar->GetContentsWidth();
   }
 
   size_t GetFindBarAudibleAlertsForBrowser(Browser* browser) {
-    FindBarTesting* find_bar =
+    const FindBarTesting* find_bar =
         browser->GetFindBarController()->find_bar()->GetFindBarTesting();
     return find_bar->GetAudibleAlertCount();
   }

chrome/browser/ui/views/dropdown_bar_host.h

@@ -115,7 +115,8 @@ class DropdownBarHost : public ui::AcceleratorTarget,
   virtual void OnVisibilityChanged();
 
   // Returns the dropdown bar view.
-  views::View* view() const { return view_; }
+  views::View* view() { return view_; }
+  const views::View* view() const { return view_; }
 
   // Returns the focus tracker.
   views::ExternalFocusTracker* focus_tracker() const {

chrome/browser/ui/views/find_bar_host.cc

@@ -112,11 +112,11 @@ void FindBarHost::SetFindTextAndSelectedRange(
   find_bar_view()->SetFindTextAndSelectedRange(find_text, selected_range);
 }
 
-base::string16 FindBarHost::GetFindText() {
+base::string16 FindBarHost::GetFindText() const {
   return find_bar_view()->GetFindText();
 }
 
-gfx::Range FindBarHost::GetSelectedRange() {
+gfx::Range FindBarHost::GetSelectedRange() const {
   return find_bar_view()->GetSelectedRange();
 }
 
@@ -143,7 +143,7 @@ void FindBarHost::AudibleAlert() {
 #endif
 }
 
-bool FindBarHost::IsFindBarVisible() {
+bool FindBarHost::IsFindBarVisible() const {
   return DropdownBarHost::IsVisible();
 }
 
@@ -156,7 +156,7 @@ void FindBarHost::RestoreSavedFocus() {
   }
 }
 
-bool FindBarHost::HasGlobalFindPasteboard() {
+bool FindBarHost::HasGlobalFindPasteboard() const {
 #if defined(OS_MACOSX)
   return true;
 #else
@@ -167,7 +167,7 @@ bool FindBarHost::HasGlobalFindPasteboard() {
 void FindBarHost::UpdateFindBarForChangedWebContents() {
 }
 
-FindBarTesting* FindBarHost::GetFindBarTesting() {
+const FindBarTesting* FindBarHost::GetFindBarTesting() const {
   return this;
 }
 
@@ -203,7 +203,7 @@ bool FindBarHost::CanHandleAccelerators() const {
 // FindBarTesting implementation:
 
 bool FindBarHost::GetFindBarWindowInfo(gfx::Point* position,
-                                      bool* fully_visible) {
+                                       bool* fully_visible) const {
   if (!find_bar_controller_ ||
 #if defined(OS_WIN) && !defined(USE_AURA)
       !::IsWindow(host()->GetNativeView())) {
@@ -228,19 +228,19 @@ bool FindBarHost::GetFindBarWindowInfo(gfx::Point* position,
   return true;
 }
 
-base::string16 FindBarHost::GetFindSelectedText() {
+base::string16 FindBarHost::GetFindSelectedText() const {
   return find_bar_view()->GetFindSelectedText();
 }
 
-base::string16 FindBarHost::GetMatchCountText() {
+base::string16 FindBarHost::GetMatchCountText() const {
   return find_bar_view()->GetMatchCountText();
 }
 
-int FindBarHost::GetWidth() {
-  return view()->width();
+int FindBarHost::GetContentsWidth() const {
+  return view()->GetContentsBounds().width();
 }
 
-size_t FindBarHost::GetAudibleAlertCount() {
+size_t FindBarHost::GetAudibleAlertCount() const {
   return audible_alerts_;
 }
 
@@ -258,17 +258,17 @@ gfx::Rect FindBarHost::GetDialogPosition(gfx::Rect avoid_overlapping_rect) {
   gfx::Size prefsize = view()->GetPreferredSize();
 
   // Limit width to the available area.
-  if (widget_bounds.width() < prefsize.width())
-    prefsize.set_width(widget_bounds.width());
+  gfx::Insets insets = view()->GetInsets();
+  prefsize.set_width(
+      std::min(prefsize.width(), widget_bounds.width() + insets.width()));
 
-  // Don't show the find bar if |widget_bounds| is not tall enough.
-  if (widget_bounds.height() < prefsize.height())
+  // Don't show the find bar if |widget_bounds| is not tall enough to fit.
+  if (widget_bounds.height() < prefsize.height() - insets.height())
     return gfx::Rect();
 
   // Place the view in the top right corner of the widget boundaries (top left
   // for RTL languages). Adjust for the view insets to ensure the border lines
   // up with the location bar.
-  gfx::Insets insets = view()->border()->GetInsets();
   int x = widget_bounds.x() - insets.left();
   if (!base::i18n::IsRTL())
     x += widget_bounds.width() - prefsize.width() + insets.width();

chrome/browser/ui/views/find_bar_host.h

@@ -57,27 +57,28 @@ class FindBarHost : public DropdownBarHost,
   void MoveWindowIfNecessary() override;
   void SetFindTextAndSelectedRange(const base::string16& find_text,
                                    const gfx::Range& selected_range) override;
-  base::string16 GetFindText() override;
-  gfx::Range GetSelectedRange() override;
+  base::string16 GetFindText() const override;
+  gfx::Range GetSelectedRange() const override;
   void UpdateUIForFindResult(const FindNotificationDetails& result,
                              const base::string16& find_text) override;
   void AudibleAlert() override;
-  bool IsFindBarVisible() override;
+  bool IsFindBarVisible() const override;
   void RestoreSavedFocus() override;
-  bool HasGlobalFindPasteboard() override;
+  bool HasGlobalFindPasteboard() const override;
   void UpdateFindBarForChangedWebContents() override;
-  FindBarTesting* GetFindBarTesting() override;
+  const FindBarTesting* GetFindBarTesting() const override;
 
   // Overridden from ui::AcceleratorTarget in DropdownBarHost class:
   bool AcceleratorPressed(const ui::Accelerator& accelerator) override;
   bool CanHandleAccelerators() const override;
 
   // FindBarTesting implementation:
-  bool GetFindBarWindowInfo(gfx::Point* position, bool* fully_visible) override;
-  base::string16 GetFindSelectedText() override;
-  base::string16 GetMatchCountText() override;
-  int GetWidth() override;
-  size_t GetAudibleAlertCount() override;
+  bool GetFindBarWindowInfo(gfx::Point* position,
+                            bool* fully_visible) const override;
+  base::string16 GetFindSelectedText() const override;
+  base::string16 GetMatchCountText() const override;
+  int GetContentsWidth() const override;
+  size_t GetAudibleAlertCount() const override;
 
   // Overridden from DropdownBarHost:
   // Returns the rectangle representing where to position the find bar. It uses
@@ -139,6 +140,9 @@ class FindBarHost : public DropdownBarHost,
 
   // Returns the FindBarView.
   FindBarView* find_bar_view() { return static_cast<FindBarView*>(view()); }
+  const FindBarView* find_bar_view() const {
+    return static_cast<const FindBarView*>(view());
+  }
 
   // A pointer back to the owning controller.
   FindBarController* find_bar_controller_;

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

@@ -164,6 +164,7 @@
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/geometry/rect_conversions.h"
 #include "ui/gfx/scoped_canvas.h"
+#include "ui/gfx/scrollbar_size.h"
 #include "ui/native_theme/native_theme_dark_aura.h"
 #include "ui/views/accessibility/view_accessibility_utils.h"
 #include "ui/views/controls/button/menu_button.h"
@@ -515,7 +516,22 @@ void BrowserView::InitStatusBubble() {
 }
 
 gfx::Rect BrowserView::GetFindBarBoundingBox() const {
-  return GetBrowserViewLayout()->GetFindBarBoundingBox();
+  gfx::Rect contents_bounds = contents_container_->ConvertRectToWidget(
+      contents_container_->GetLocalBounds());
+
+  // If the location bar is visible use it to position the bounding box,
+  // otherwise use the contents container.
+  if (!immersive_mode_controller_->IsEnabled() ||
+      immersive_mode_controller_->IsRevealed()) {
+    const gfx::Rect bounding_box =
+        toolbar_button_provider_->GetFindBarBoundingBox(
+            contents_bounds.bottom());
+    if (!bounding_box.IsEmpty())
+      return bounding_box;
+  }
+
+  contents_bounds.Inset(0, 0, gfx::scrollbar_size(), 0);
+  return contents_container_->GetMirroredRect(contents_bounds);
 }
 
 int BrowserView::GetTabStripHeight() const {

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

@@ -31,7 +31,6 @@
 #include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
-#include "ui/gfx/scrollbar_size.h"
 #include "ui/views/controls/webview/webview.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/window/client_view.h"
@@ -206,46 +205,6 @@ gfx::Size BrowserViewLayout::GetMinimumSize(const views::View* host) const {
   return gfx::Size(min_width, min_height);
 }
 
-gfx::Rect BrowserViewLayout::GetFindBarBoundingBox() const {
-  // This function returns the area the Find Bar can be laid out within. When
-  // the location bar/OmniBox is visible, the bounding box is the area extending
-  // from the bottom edge of the location bar/OmniBox to the bottom of the
-  // "user-perceived content area" of the browser window. The width matches the
-  // width of the location bar/OmniBox. If the location bar/OmniBox is not
-  // visible, the returned area is the full "user-perceived content area",
-  // excluding any vertical scrollbar.
-  // The "user-perceived content area" excludes the detached bookmark bar (in
-  // the New Tab case) and any infobars since they are not _visually_ connected
-  // to the Toolbar.
-  gfx::Rect bounding_box;
-  if (!immersive_mode_controller_->IsEnabled() ||
-      immersive_mode_controller_->IsRevealed()) {
-    bounding_box =
-        browser_view_->toolbar_button_provider()->GetFindBarBoundingBox(
-            contents_container_->height());
-  }
-  if (!bounding_box.IsEmpty())
-    return bounding_box;
-
-  // Otherwise, use the contents container minus any infobars and detached
-  // bookmark bar from the top and a scrollbar width from the appropriate edge.
-  bounding_box = contents_container_->ConvertRectToWidget(
-      contents_container_->GetLocalBounds());
-  // Under ChromeOS, the top_container_ may include the title bar for hosted
-  // apps. Just make sure something of consequence is visible before it's height
-  // is used.
-  const int top_container_height = (browser_view_->tabstrip()->GetVisible() ||
-                                    browser_view_->toolbar()->GetVisible() ||
-                                    browser_view_->IsBookmarkBarVisible())
-                                       ? top_container_->height()
-                                       : 0;
-  if (base::i18n::IsRTL())
-    bounding_box.Inset(gfx::scrollbar_size(), top_container_height, 0, 0);
-  else
-    bounding_box.Inset(0, top_container_height, gfx::scrollbar_size(), 0);
-  return bounding_box;
-}
-
 gfx::NativeView BrowserViewLayout::GetHostView() {
   return delegate_->GetHostView();
 }

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

@@ -78,9 +78,6 @@ class BrowserViewLayout : public views::LayoutManager {
 
   web_modal::WebContentsModalDialogHost* GetWebContentsModalDialogHost();
 
-  // Returns the bounding box, in widget coordinates,  for the find bar.
-  gfx::Rect GetFindBarBoundingBox() const;
-
   // Returns the view against which the dialog is positioned and parented.
   gfx::NativeView GetHostView();
 

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

@@ -26,6 +26,7 @@
 #include "content/public/test/test_service_manager_context.h"
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/base/l10n/l10n_util.h"
+#include "ui/gfx/scrollbar_size.h"
 #include "ui/views/controls/webview/webview.h"
 
 #if defined(OS_MACOSX)
@@ -189,6 +190,46 @@ TEST_F(BrowserViewTest, DISABLED_BrowserViewLayout) {
   BookmarkBarView::DisableAnimationsForTesting(false);
 }
 
+// Test the find bar's bounding box when the location bar is visible.
+TEST_F(BrowserViewTest, FindBarBoundingBoxLocationBar) {
+  ASSERT_FALSE(base::i18n::IsRTL());
+  const views::View* location_bar = browser_view()->GetLocationBarView();
+  const views::View* contents_container =
+      browser_view()->GetContentsContainerForTest();
+
+  // Make sure we are testing the case where the location bar is visible.
+  EXPECT_TRUE(location_bar->GetVisible());
+  const gfx::Rect find_bar_bounds = browser_view()->GetFindBarBoundingBox();
+  const gfx::Rect location_bar_bounds =
+      location_bar->ConvertRectToWidget(location_bar->GetLocalBounds());
+  const gfx::Rect contents_bounds = contents_container->ConvertRectToWidget(
+      contents_container->GetLocalBounds());
+
+  const gfx::Rect target(
+      location_bar_bounds.x(), location_bar_bounds.bottom(),
+      location_bar_bounds.width(),
+      contents_bounds.bottom() - location_bar_bounds.bottom());
+  EXPECT_EQ(target.ToString(), find_bar_bounds.ToString());
+}
+
+// Test the find bar's bounding box when the location bar is not visible.
+TEST_F(BrowserViewTest, FindBarBoundingBoxNoLocationBar) {
+  ASSERT_FALSE(base::i18n::IsRTL());
+  const views::View* location_bar = browser_view()->GetLocationBarView();
+  const views::View* contents_container =
+      browser_view()->GetContentsContainerForTest();
+
+  // Make sure we are testing the case where the location bar is absent.
+  browser_view()->GetLocationBarView()->SetVisible(false);
+  EXPECT_FALSE(location_bar->GetVisible());
+  const gfx::Rect find_bar_bounds = browser_view()->GetFindBarBoundingBox();
+  gfx::Rect contents_bounds = contents_container->ConvertRectToWidget(
+      contents_container->GetLocalBounds());
+  contents_bounds.Inset(0, 0, gfx::scrollbar_size(), 0);
+
+  EXPECT_EQ(contents_bounds.ToString(), find_bar_bounds.ToString());
+}
+
 // On macOS, most accelerators are handled by CommandDispatcher.
 #if !defined(OS_MACOSX)
 // Test that repeated accelerators are processed or ignored depending on the

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

@@ -47,10 +47,9 @@ class ToolbarButtonProvider {
   // Gets the app menu button.
   virtual AppMenuButton* GetAppMenuButton() = 0;
 
-  // Gets the area available for the find bar in widget space where
-  // |contents_height| is the amount of vertical space available, otherwise if
-  // there is no appropriate anchor point returns empty gfx::Rect.
-  virtual gfx::Rect GetFindBarBoundingBox(int contents_height) const = 0;
+  // Returns a bounding box for the find bar in widget coordinates given the
+  // bottom of the contents container.
+  virtual gfx::Rect GetFindBarBoundingBox(int contents_bottom) const = 0;
 
   // Gives the toolbar focus.
   virtual void FocusToolbar() = 0;

chrome/browser/ui/views/toolbar/toolbar_view.cc

@@ -782,7 +782,7 @@ AppMenuButton* ToolbarView::GetAppMenuButton() {
   return app_menu_button_;
 }
 
-gfx::Rect ToolbarView::GetFindBarBoundingBox(int contents_height) const {
+gfx::Rect ToolbarView::GetFindBarBoundingBox(int contents_bottom) const {
   if (!browser_->SupportsWindowFeature(Browser::FEATURE_LOCATIONBAR))
     return gfx::Rect();
 
@@ -792,7 +792,7 @@ gfx::Rect ToolbarView::GetFindBarBoundingBox(int contents_height) const {
   gfx::Rect bounds =
       location_bar_->ConvertRectToWidget(location_bar_->GetLocalBounds());
   return gfx::Rect(bounds.x(), bounds.bottom(), bounds.width(),
-                   contents_height);
+                   contents_bottom - bounds.bottom());
 }
 
 void ToolbarView::FocusToolbar() {

chrome/browser/ui/views/toolbar/toolbar_view.h

@@ -232,7 +232,7 @@ class ToolbarView : public views::AccessiblePaneView,
   views::View* GetDefaultExtensionDialogAnchorView() override;
   PageActionIconView* GetPageActionIconView(PageActionIconType type) override;
   AppMenuButton* GetAppMenuButton() override;
-  gfx::Rect GetFindBarBoundingBox(int contents_height) const override;
+  gfx::Rect GetFindBarBoundingBox(int contents_bottom) const override;
   void FocusToolbar() override;
   views::AccessiblePaneView* GetAsAccessiblePaneView() override;
   views::View* GetAnchorView(PageActionIconType type) override;

chrome/browser/ui/views/web_apps/web_app_frame_toolbar_view.cc

@@ -480,21 +480,23 @@ AppMenuButton* WebAppFrameToolbarView::GetAppMenuButton() {
 }
 
 gfx::Rect WebAppFrameToolbarView::GetFindBarBoundingBox(
-    int contents_height) const {
+    int contents_bottom) const {
   if (!IsDrawn())
     return gfx::Rect();
 
+  // If LTR find bar will be right aligned so align to right edge of app menu
+  // button. Otherwise it will be left aligned so align to the left edge of the
+  // app menu button.
   gfx::Rect anchor_bounds = web_app_menu_button_->ConvertRectToWidget(
       web_app_menu_button_->GetLocalBounds());
+  int x_pos = 0;
+  int width = anchor_bounds.right();
   if (base::i18n::IsRTL()) {
-    // Find bar will be left aligned so align to left edge of app menu button.
-    int widget_width = GetWidget()->GetRootView()->width();
-    return gfx::Rect(anchor_bounds.x(), anchor_bounds.bottom(),
-                     widget_width - anchor_bounds.x(), contents_height);
+    x_pos = anchor_bounds.x();
+    width = GetWidget()->GetRootView()->width() - anchor_bounds.x();
   }
-  // Find bar will be right aligned so align to right edge of app menu button.
-  return gfx::Rect(0, anchor_bounds.bottom(),
-                   anchor_bounds.x() + anchor_bounds.width(), contents_height);
+  return gfx::Rect(x_pos, anchor_bounds.bottom(), width,
+                   contents_bottom - anchor_bounds.bottom());
 }
 
 void WebAppFrameToolbarView::FocusToolbar() {

chrome/browser/ui/views/web_apps/web_app_frame_toolbar_view.h

@@ -129,7 +129,7 @@ class WebAppFrameToolbarView : public views::AccessiblePaneView,
   views::View* GetDefaultExtensionDialogAnchorView() override;
   PageActionIconView* GetPageActionIconView(PageActionIconType type) override;
   AppMenuButton* GetAppMenuButton() override;
-  gfx::Rect GetFindBarBoundingBox(int contents_height) const override;
+  gfx::Rect GetFindBarBoundingBox(int contents_bottom) const override;
   void FocusToolbar() override;
   views::AccessiblePaneView* GetAsAccessiblePaneView() override;
   views::View* GetAnchorView(PageActionIconType type) override;