Eliminate FindBarView::Layout().

FindBarView was already using a LayoutManager, but also overrode Layout() to do
two things: position an invisible FocusForwarderView atop some children, and set
highlight paths on the buttons.  Each of these tasks is better achieved via
another route: buttons can set their own highlight paths, and setting textfield
focus can be done by FindBarView itself as long as we exclude the relevant
children from processing events.

Bug: 1005568
Change-Id: I32c102abeea7fbedf7dbfc57a22ce01e2c976904
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1817290
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#698714}

chrome/browser/ui/views/find_bar_view.cc

@@ -47,55 +47,33 @@
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/painter.h"
 #include "ui/views/view_class_properties.h"
-#include "ui/views/view_targeter.h"
 #include "ui/views/widget/widget.h"
 
-namespace {
-
-// The default number of average characters that the text box will be.
-constexpr int kDefaultCharWidth = 30;
-
-// The minimum allowable width in chars for the find_text_ view. This ensures
-// the view can at least display the caret and some number of characters.
-constexpr int kMinimumCharWidth = 1;
-
-// We use a hidden view to grab mouse clicks and bring focus to the find
-// text box. This is because although the find text box may look like it
-// extends all the way to the find button, it only goes as far as to the
-// match_count label. The user, however, expects being able to click anywhere
-// inside what looks like the find text box (including on or around the
-// match_count label) and have focus brought to the find box.
-class FocusForwarderView : public views::View {
+// An ImageButton that has a centered circular highlight.
+class FindBarView::FindBarButton : public views::ImageButton {
  public:
-  explicit FocusForwarderView(
-      views::Textfield* view_to_focus_on_mousedown)
-    : view_to_focus_on_mousedown_(view_to_focus_on_mousedown) {}
-
- private:
-  bool OnMousePressed(const ui::MouseEvent& event) override {
-    if (view_to_focus_on_mousedown_)
-      view_to_focus_on_mousedown_->RequestFocus();
-    return true;
-  }
-
-  views::Textfield* view_to_focus_on_mousedown_;
+  using ImageButton::ImageButton;
 
-  DISALLOW_COPY_AND_ASSIGN(FocusForwarderView);
+ protected:
+  void OnBoundsChanged(const gfx::Rect& previous_bounds) override;
 };
 
-}  // namespace
+void FindBarView::FindBarButton::OnBoundsChanged(
+    const gfx::Rect& previous_bounds) {
+  const gfx::Rect bounds = GetLocalBounds();
+  auto highlight_path = std::make_unique<SkPath>();
+  const gfx::Point center = bounds.CenterPoint();
+  const int radius = views::LayoutProvider::Get()->GetCornerRadiusMetric(
+      views::EMPHASIS_MAXIMUM, bounds.size());
+  highlight_path->addCircle(center.x(), center.y(), radius);
+  SetProperty(views::kHighlightPathKey, highlight_path.release());
+}
 
-// The match count label is like a normal label, but can process events (which
-// makes it easier to forward events to the text input --- see
-// FindBarView::TargetForRect).
 class FindBarView::MatchCountLabel : public views::Label {
  public:
   MatchCountLabel() {}
   ~MatchCountLabel() override {}
 
-  // views::Label overrides:
-  bool CanProcessEventsWithinSubtree() const override { return true; }
-
   gfx::Size CalculatePreferredSize() const override {
     // We need to return at least 1dip so that box layout adds padding on either
     // side (otherwise there will be a jump when our size changes between empty
@@ -138,10 +116,6 @@ class FindBarView::MatchCountLabel : public views::Label {
     SetText(base::string16());
   }
 
- protected:
-  // views::Label:
-  void SetText(const base::string16& text) override { Label::SetText(text); }
-
  private:
   base::Optional<FindNotificationDetails> last_result_;
 
@@ -154,21 +128,23 @@ class FindBarView::MatchCountLabel : public views::Label {
 FindBarView::FindBarView(FindBarHost* host) : find_bar_host_(host) {
   auto find_text = std::make_unique<views::Textfield>();
   find_text->SetID(VIEW_ID_FIND_IN_PAGE_TEXT_FIELD);
-  find_text->SetDefaultWidthInChars(kDefaultCharWidth);
-  find_text->SetMinimumWidthInChars(kMinimumCharWidth);
+  find_text->SetDefaultWidthInChars(30);
+  find_text->SetMinimumWidthInChars(1);
   find_text->set_controller(this);
   find_text->SetAccessibleName(l10n_util::GetStringUTF16(IDS_ACCNAME_FIND));
   find_text->SetTextInputFlags(ui::TEXT_INPUT_FLAG_AUTOCORRECT_OFF);
   find_text_ = AddChildView(std::move(find_text));
 
   auto match_count_text = std::make_unique<MatchCountLabel>();
-  match_count_text->SetEventTargeter(
-      std::make_unique<views::ViewTargeter>(this));
+  match_count_text->set_can_process_events_within_subtree(false);
   match_count_text_ = AddChildView(std::move(match_count_text));
 
-  separator_ = AddChildView(std::make_unique<views::Separator>());
+  auto separator = std::make_unique<views::Separator>();
+  separator->set_can_process_events_within_subtree(false);
+  separator_ = AddChildView(std::move(separator));
 
-  auto find_previous_button = views::CreateVectorImageButton(this);
+  auto find_previous_button = std::make_unique<FindBarButton>(this);
+  views::ConfigureVectorImageButton(find_previous_button.get());
   find_previous_button->SetID(VIEW_ID_FIND_IN_PAGE_PREVIOUS_BUTTON);
   find_previous_button->SetFocusForPlatform();
   find_previous_button->SetTooltipText(
@@ -177,7 +153,8 @@ FindBarView::FindBarView(FindBarHost* host) : find_bar_host_(host) {
       l10n_util::GetStringUTF16(IDS_ACCNAME_PREVIOUS));
   find_previous_button_ = AddChildView(std::move(find_previous_button));
 
-  auto find_next_button = views::CreateVectorImageButton(this);
+  auto find_next_button = std::make_unique<FindBarButton>(this);
+  views::ConfigureVectorImageButton(find_next_button.get());
   find_next_button->SetID(VIEW_ID_FIND_IN_PAGE_NEXT_BUTTON);
   find_next_button->SetFocusForPlatform();
   find_next_button->SetTooltipText(
@@ -186,7 +163,8 @@ FindBarView::FindBarView(FindBarHost* host) : find_bar_host_(host) {
       l10n_util::GetStringUTF16(IDS_ACCNAME_NEXT));
   find_next_button_ = AddChildView(std::move(find_next_button));
 
-  auto close_button = views::CreateVectorImageButton(this);
+  auto close_button = std::make_unique<FindBarButton>(this);
+  views::ConfigureVectorImageButton(close_button.get());
   close_button->SetID(VIEW_ID_FIND_IN_PAGE_CLOSE_BUTTON);
   close_button->SetFocusForPlatform();
   close_button->SetTooltipText(
@@ -194,9 +172,6 @@ FindBarView::FindBarView(FindBarHost* host) : find_bar_host_(host) {
   close_button->SetAnimationDuration(base::TimeDelta());
   close_button_ = AddChildView(std::move(close_button));
 
-  auto focus_forwarder_view = std::make_unique<FocusForwarderView>(find_text_);
-  focus_forwarder_view_ = AddChildView(std::move(focus_forwarder_view));
-
   EnableCanvasFlippingForRTLUI(true);
 
   // Normally we could space objects horizontally by simply passing a constant
@@ -322,29 +297,20 @@ void FindBarView::ClearMatchCount() {
 ///////////////////////////////////////////////////////////////////////////////
 // FindBarView, views::View overrides:
 
-void FindBarView::Layout() {
-  views::View::Layout();
-
-  // The focus forwarder view is a hidden view that should cover the area
-  // between the find text box and the find button so that when the user clicks
-  // in that area we focus on the find text box.
-  const int find_text_edge = find_text_->x() + find_text_->width();
-  focus_forwarder_view_->SetBounds(
-      find_text_edge, find_previous_button_->y(),
+bool FindBarView::OnMousePressed(const ui::MouseEvent& event) {
+  // The find text box only extends to the match count label.  However, users
+  // expect to be able to click anywhere inside what looks like the find text
+  // box (including on or around the match_count label) and have focus brought
+  // to the find box.  Cause clicks between the textfield and the find previous
+  // button to focus the textfield.
+  const int find_text_edge = find_text_->bounds().right();
+  const gfx::Rect focus_area(find_text_edge, find_previous_button_->y(),
                              find_previous_button_->x() - find_text_edge,
                              find_previous_button_->height());
-
-  for (auto* button :
-       {find_previous_button_, find_next_button_, close_button_}) {
-    constexpr int kCircleDiameterDp = 24;
-    auto highlight_path = std::make_unique<SkPath>();
-    // Use a centered circular shape for inkdrops and focus rings.
-    gfx::Rect circle_rect(button->GetLocalBounds());
-    circle_rect.ClampToCenteredSize(
-        gfx::Size(kCircleDiameterDp, kCircleDiameterDp));
-    highlight_path->addOval(gfx::RectToSkRect(circle_rect));
-    button->SetProperty(views::kHighlightPathKey, highlight_path.release());
-  }
+  if (!GetMirroredRect(focus_area).Contains(event.location()))
+    return false;
+  find_text_->RequestFocus();
+  return true;
 }
 
 gfx::Size FindBarView::CalculatePreferredSize() const {
@@ -450,11 +416,6 @@ void FindBarView::OnAfterPaste() {
   last_searched_text_.clear();
 }
 
-views::View* FindBarView::TargetForRect(View* root, const gfx::Rect& rect) {
-  DCHECK_EQ(match_count_text_, root);
-  return find_text_;
-}
-
 void FindBarView::Find(const base::string16& search_text) {
   FindBarController* controller = find_bar_host_->GetFindBarController();
   DCHECK(controller);

chrome/browser/ui/views/find_bar_view.h

@@ -14,7 +14,6 @@
 #include "ui/views/controls/button/button.h"
 #include "ui/views/controls/textfield/textfield_controller.h"
 #include "ui/views/view.h"
-#include "ui/views/view_targeter_delegate.h"
 
 class FindBarHost;
 class FindNotificationDetails;
@@ -24,7 +23,6 @@ class Range;
 }
 
 namespace views {
-class ImageButton;
 class Painter;
 class Separator;
 class Textfield;
@@ -40,8 +38,7 @@ class Textfield;
 class FindBarView : public views::View,
                     public DropdownBarHostDelegate,
                     public views::ButtonListener,
-                    public views::TextfieldController,
-                    public views::ViewTargeterDelegate {
+                    public views::TextfieldController {
  public:
   explicit FindBarView(FindBarHost* host);
   ~FindBarView() override;
@@ -68,7 +65,7 @@ class FindBarView : public views::View,
 
   // views::View:
   const char* GetClassName() const override;
-  void Layout() override;
+  bool OnMousePressed(const ui::MouseEvent& event) override;
   gfx::Size CalculatePreferredSize() const override;
   void OnThemeChanged() override;
 
@@ -84,14 +81,12 @@ class FindBarView : public views::View,
   void OnAfterUserAction(views::Textfield* sender) override;
   void OnAfterPaste() override;
 
-  // views::ViewTargeterDelegate:
-  views::View* TargetForRect(View* root, const gfx::Rect& rect) override;
-
  protected:
   // views::View overrides:
   void AddedToWidget() override;
 
  private:
+  class FindBarButton;
   class MatchCountLabel;
 
   // Starts finding |search_text|.  If the text is empty, stops finding.
@@ -115,11 +110,10 @@ class FindBarView : public views::View,
   views::Textfield* find_text_;
   std::unique_ptr<views::Painter> find_text_border_;
   MatchCountLabel* match_count_text_;
-  views::View* focus_forwarder_view_;
   views::Separator* separator_;
-  views::ImageButton* find_previous_button_;
-  views::ImageButton* find_next_button_;
-  views::ImageButton* close_button_;
+  FindBarButton* find_previous_button_;
+  FindBarButton* find_next_button_;
+  FindBarButton* close_button_;
 
   // The preferred height of the find bar.
   int preferred_height_;

ui/views/controls/button/image_button_factory.cc

@@ -15,19 +15,6 @@
 
 namespace views {
 
-namespace {
-
-void ConfigureVectorImageButton(ImageButton* button) {
-  button->SetInkDropMode(Button::InkDropMode::ON);
-  button->set_has_ink_drop_action_on_click(true);
-  button->SetImageHorizontalAlignment(ImageButton::ALIGN_CENTER);
-  button->SetImageVerticalAlignment(ImageButton::ALIGN_MIDDLE);
-  button->SetBorder(CreateEmptyBorder(
-      LayoutProvider::Get()->GetInsetsMetric(INSETS_VECTOR_IMAGE_BUTTON)));
-}
-
-}  // namespace
-
 std::unique_ptr<ImageButton> CreateVectorImageButton(ButtonListener* listener) {
   auto button = std::make_unique<ImageButton>(listener);
   ConfigureVectorImageButton(button.get());
@@ -41,6 +28,15 @@ std::unique_ptr<ToggleImageButton> CreateVectorToggleImageButton(
   return button;
 }
 
+void ConfigureVectorImageButton(ImageButton* button) {
+  button->SetInkDropMode(Button::InkDropMode::ON);
+  button->set_has_ink_drop_action_on_click(true);
+  button->SetImageHorizontalAlignment(ImageButton::ALIGN_CENTER);
+  button->SetImageVerticalAlignment(ImageButton::ALIGN_MIDDLE);
+  button->SetBorder(CreateEmptyBorder(
+      LayoutProvider::Get()->GetInsetsMetric(INSETS_VECTOR_IMAGE_BUTTON)));
+}
+
 void SetImageFromVectorIcon(ImageButton* button, const gfx::VectorIcon& icon) {
   SetImageFromVectorIconWithColor(
       button, icon,

ui/views/controls/button/image_button_factory.h

@@ -30,6 +30,10 @@ VIEWS_EXPORT std::unique_ptr<ImageButton> CreateVectorImageButton(
 VIEWS_EXPORT std::unique_ptr<ToggleImageButton> CreateVectorToggleImageButton(
     ButtonListener* listener);
 
+// Configures an existing ImageButton with an ink drop and a centered image in
+// preparation for applying a vector icon with SetImageFromVectorIcon below.
+VIEWS_EXPORT void ConfigureVectorImageButton(ImageButton* button);
+
 // Sets images on |button| for STATE_NORMAL and STATE_DISABLED from the given
 // vector icon using the default color from the current NativeTheme.
 VIEWS_EXPORT void SetImageFromVectorIcon(ImageButton* button,