Harmonize the find in page dialog.

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/i18n/number_formatting.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_util.h"
 #include "chrome/app/vector_icons/vector_icons.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/ui/find_bar/find_bar_controller.h"
 #include "chrome/browser/ui/find_bar/find_bar_state.h"
 #include "chrome/browser/ui/find_bar/find_bar_state_factory.h"
 #include "chrome/browser/ui/find_bar/find_notification_details.h"
 #include "chrome/browser/ui/find_bar/find_tab_helper.h"
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/find_bar_host.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/ui/views/harmony/chrome_layout_provider.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/strings/grit/components_strings.h"
 #include "ui/base/ime/input_method.h"
 #include "ui/base/ime/text_input_flags.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/theme_provider.h"
 #include "ui/events/event.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/native_theme/native_theme.h"
 #include "ui/vector_icons/vector_icons.h"
 #include "ui/views/background.h"
 #include "ui/views/border.h"
 #include "ui/views/bubble/bubble_border.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/button/image_button_factory.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/separator.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/painter.h"
+#include "ui/views/view_properties.h"
 #include "ui/views/view_targeter.h"
 #include "ui/views/widget/widget.h"
 
 namespace {
 
-// These layout constants are all in dp.
-// The horizontal and vertical insets for the bar.
-const int kInteriorPadding = 8;
-// Default spacing between child views.
-const int kInterChildSpacing = 4;
-// Additional spacing around the separator.
-const int kSeparatorLeftSpacing = 12 - kInterChildSpacing;
-const int kSeparatorRightSpacing = 8 - kInterChildSpacing;
-
 // The default number of average characters that the text box will be.
 const int kDefaultCharWidth = 30;
 
 // 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 MatchCountLabel : public views::Label {
  public:
   MatchCountLabel() {}
   ~MatchCountLabel() override {}
@@ skipping 85 matching lines @@
   AddChildView(close_button_);
 
   AddChildView(focus_forwarder_view_);
 
   EnableCanvasFlippingForRTLUI(true);
 
   match_count_text_->SetEventTargeter(
       base::MakeUnique<views::ViewTargeter>(this));
   AddChildViewAt(match_count_text_, 1);
 
-  separator_->SetBorder(views::CreateEmptyBorder(0, kSeparatorLeftSpacing, 0,
-                                                 kSeparatorRightSpacing));
   AddChildViewAt(separator_, 2);
 
+  find_text_->SetProperty(views::kMarginsKey, new gfx::Insets(12, 0));

[Peter Kasting, 2017/07/10 20:13:08]

I think you want 8 here since this is a textfield control and not a label.

[ananta, 2017/07/12 19:19:48]

The mocks are a touch confusing. They show the insets from the top to be
16 at places and 12 at the others. Looked at this with kylixrd. We thought it
would be better to just specify the insets as 16 through out for horizontal and
vertical and then revisit this with the UI folks.

[Peter Kasting, 2017/07/12 20:33:01]

I'm not sure what's confusing.  Use 12 pt margins above labels and 8 pt above
everything else, including all controls (such as textfields).  What did I miss?

[ananta, 2017/07/13 04:04:34]

It looks like 12 pt is not a harmony unit?. Some areas arond the text field show
a 16 px margin from the top. Other places show 12 from the top to the border of
the text control and 4 px to be the width of the border. Based on that it seemed
like 16 px for all controls in the bar could be acceptable?

[Peter Kasting, 2017/07/13 04:11:24]

Not everything in Harmony is 16s.
Text fields != labels.

Labels always use 12 pt vertical margins in toasts, other controls use 8, and
the total margin is set (automatically by the layout manager) based on the
largest height + margin.
I don't know what you're referring to here.

The source of the 8 and 12 values is the "toasts with" images on
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(...
.

+  match_count_text_->SetProperty(views::kMarginsKey,

[Peter Kasting, 2017/07/10 20:13:08]

Whereas this is a label, so you'd want 12.

[ananta, 2017/07/12 19:19:48]

Not needed anymore.

+      new gfx::Insets(8, 0));
+  separator_->SetProperty(views::kMarginsKey, new gfx::Insets(8, 16, 8, 16));

[Peter Kasting, 2017/07/10 20:13:08]

Don't add horizontal margins here.

[ananta, 2017/07/12 19:19:48]

We need the horizontal margins for the separator to ensure that it has the
correct space on its left and right.

[Peter Kasting, 2017/07/12 20:33:01]

Your separator has 2x too much space with these (as visible on your screenshot).
 It should be correct without them.

[ananta, 2017/07/13 04:04:34]

If we don't specify the margin on both sides of the separator it shows up too
close to the match count field in the non harmony case. From the mock it appears
to be equidistant from the button on the right and the text match field on the
left. 

It seems like the distance between controls in the box layout is not spacing the
separator correctly?

[Peter Kasting, 2017/07/13 04:11:24]

As long as non-Harmony isn't completely broken, we don't care how it looks.
I think you're seeing the problems of built-in padding on the objects to either
side (some small amount in the label, a larger amount in the button equivalent
to the vector image button inset, which is why I keep talking about trying to
lay out ignoring that value).

[ananta, 2017/07/13 04:44:43]

ok. thanks. I updated the patchset with distance metrics added as per your
original suggestion. I also added an image for the find bar with harmony on with
these values.

+  find_previous_button_->SetProperty(views::kMarginsKey, new gfx::Insets(8, 0));
+  find_next_button_->SetProperty(views::kMarginsKey, new gfx::Insets(8, 0));
+  close_button_->SetProperty(views::kMarginsKey, new gfx::Insets(8, 0));
+
   find_text_->SetBorder(views::NullBorder());
 
   views::BoxLayout* manager =
       new views::BoxLayout(views::BoxLayout::kHorizontal,
-                           gfx::Insets(kInteriorPadding), kInterChildSpacing);
+                           gfx::Insets(0, 16),
+                           ChromeLayoutProvider::Get()->GetDistanceMetric(
+                              views::DISTANCE_RELATED_CONTROL_HORIZONTAL));
+
   SetLayoutManager(manager);
   manager->SetFlexForView(find_text_, 1);
 }
 
 FindBarView::~FindBarView() {
 }
 
 void FindBarView::SetFindTextAndSelectedRange(
     const base::string16& find_text,
     const gfx::Range& selected_range) {
@@ skipping 233 matching lines @@
   separator_->SetColor(SkColorSetA(text_color, 0x26));
 
   const SkColor base_icon_color = GetNativeTheme()->GetSystemColor(
       ui::NativeTheme::kColorId_TextfieldDefaultColor);
   views::SetImageFromVectorIcon(find_previous_button_, kCaretUpIcon,
                                 base_icon_color);
   views::SetImageFromVectorIcon(find_next_button_, kCaretDownIcon,
                                 base_icon_color);
   views::SetImageFromVectorIcon(close_button_, ui::kCloseIcon, base_icon_color);
 }