Add textfield internal padding from FocusableBorder.

chrome/browser/ui/views/find_bar_view.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/find_bar_view.h"
 
 #include <algorithm>
 
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
@@ skipping 19 matching lines @@
 #include "ui/resources/grit/ui_resources.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/ime/input_method.h"
 #include "ui/views/painter.h"
 #include "ui/views/widget/widget.h"
 
 namespace {
 
-// The margins around the search field, match count label, and the close button.
+// The margins around the UI controls, derived from assets and design specs.
 const int kMarginLeftOfCloseButton = 3;

[Peter Kasting, 2014/09/05 22:56:48]

Nit: Should we collapse this and kMarginLeftOfMatchCountLabel together, under
the theory that it's really "kMarginRightOfTextfield"?

[msw, 2014/09/05 23:46:55]

That might be reasonable for follow-up, but I'm not sure that
kMarginLeftOfCloseButton is immediately right of the textfield if the match
count label and the find next/prev buttons are all between the textfield and the
close button, so I'll leave it as-is for now.

 const int kMarginRightOfCloseButton = 7;
-const int kMarginLeftOfMatchCountLabel = 2;
+const int kMarginLeftOfMatchCountLabel = 3;
 const int kMarginRightOfMatchCountLabel = 1;
 const int kMarginLeftOfFindTextfield = 12;
+const int kMarginAboveFindTextfield = 6;

[Peter Kasting, 2014/09/05 22:56:48]

Nit: Maybe this and the next constant should just be one?  (I can't think of why
we'd want to make the vertical spacing uneven.)

[msw, 2014/09/05 23:46:55]

Done. I'm not entirely sure that these values are optimal, since the
IDR_TEXTFIELD assets have some nearly transparent pixels at the top and bottom
of the interior space, but they match the current layout.

+const int kMarginBelowFindTextfield = 6;
 
 // The margins around the match count label (We add extra space so that the
 // background highlight extends beyond just the text).
 const int kMatchCountExtraWidth = 9;
 
 // Minimum width for the match count label.
 const int kMatchCountMinWidth = 30;
 
 // The text color for the match count label.
 const SkColor kTextColorMatchCount = SkColorSetRGB(178, 178, 178);
@@ skipping 255 matching lines @@
                                    sz.height());
 
   // Then the label showing the match count number.
   sz = match_count_text_->GetPreferredSize();
   // We extend the label bounds a bit to give the background highlighting a bit
   // of breathing room (margins around the text).
   sz.Enlarge(kMatchCountExtraWidth, 0);
   sz.SetToMax(gfx::Size(kMatchCountMinWidth, 0));
   int match_count_x =
       find_previous_button_->x() - kMarginRightOfMatchCountLabel - sz.width();
-  int find_text_y = (height() - find_text_->GetPreferredSize().height()) / 2;
+  int find_text_y = kMarginAboveFindTextfield;

[Peter Kasting, 2014/09/05 22:56:49]

Interesting, is it no longer possible to use the preferred height to compute
this?

[msw, 2014/09/05 23:46:55]

This approach is preferable to the current code for several reasons:
(1) The textfield is given the entire vertical space available to it, which may
be needed if the fallback text exceeds the additional 6px that this change
provides. (unlikely, but possible)
(2) This lets the textfield center via the baseline technique, rather than
combining that with some additional naive control centering logic within the
vertical space.
(3) The find bar textfield currentlt exceeds the available height with
excessively tall system fonts (eg. Segoe UI 15pt), which causes the textfield
control's white background to spill over the find bar assets. This change fixes
that defect (but obviously we'll need to expand the control or clamp the font
height [further] in the long term).
(4) Avoiding potentially expensice GetPreferredSize calls is always good when
the preferred size isn't actually needed.

   match_count_text_->SetBounds(match_count_x,
                                find_text_y + find_text_->GetBaseline() -
                                    match_count_text_->GetBaseline(),
                                sz.width(), sz.height());
 
-  // And whatever space is left in between, gets filled up by the find edit box.
-  int find_text_width = std::max(0, match_count_x -
-      kMarginLeftOfMatchCountLabel - kMarginLeftOfFindTextfield);
-  find_text_->SetBounds(kMarginLeftOfFindTextfield, find_text_y,
-      find_text_width, find_text_->GetPreferredSize().height());
+  // Fill the remaining width and available height with the textfield.
+  int margin_a = kMarginLeftOfFindTextfield - views::Textfield::kTextPadding;
+  int margin_b = kMarginLeftOfMatchCountLabel - views::Textfield::kTextPadding;

[Peter Kasting, 2014/09/05 22:56:48]

Nit: I'm not in love with "margin_a" and "margin_b" as names... maybe
"left_margin" and roll the other temp into the width calculation below?

[msw, 2014/09/05 23:46:55]

Done.

+  int find_text_width = std::max(0, match_count_x - margin_a - margin_b);
+  find_text_->SetBounds(
+      margin_a,
+      find_text_y,
+      find_text_width,
+      height() - kMarginAboveFindTextfield - kMarginBelowFindTextfield);
 
   // 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.
   int find_text_edge = find_text_->x() + find_text_->width();
   focus_forwarder_view_->SetBounds(
       find_text_edge, find_previous_button_->y(),
       find_previous_button_->x() - find_text_edge,
       find_previous_button_->height());
 }
 
 gfx::Size FindBarView::GetPreferredSize() const {
   gfx::Size prefsize = find_text_->GetPreferredSize();
   prefsize.set_height(preferred_height_);
 
   // Add up all the preferred sizes and margins of the rest of the controls.
   prefsize.Enlarge(kMarginLeftOfCloseButton + kMarginRightOfCloseButton +
-                       kMarginLeftOfFindTextfield,
+                       kMarginLeftOfFindTextfield -
+                       2 * views::Textfield::kTextPadding,
                    0);
   prefsize.Enlarge(find_previous_button_->GetPreferredSize().width(), 0);
   prefsize.Enlarge(find_next_button_->GetPreferredSize().width(), 0);
   prefsize.Enlarge(close_button_->GetPreferredSize().width(), 0);
   return prefsize;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // FindBarView, views::ButtonListener implementation:
 
@@ skipping 132 matching lines @@
 
 void FindBarView::OnThemeChanged() {
   ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
   if (GetThemeProvider()) {
     close_button_->SetBackground(
         GetThemeProvider()->GetColor(ThemeProperties::COLOR_TAB_TEXT),
         rb.GetImageSkiaNamed(IDR_CLOSE_1),
         rb.GetImageSkiaNamed(IDR_CLOSE_1_MASK));
   }
 }