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_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));
+  ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
+  const int related_control_horizontal = provider->GetDistanceMetric(
+      views::DISTANCE_RELATED_CONTROL_HORIZONTAL);
+
+  separator_->SetBorder(views::CreateEmptyBorder(
+      0, related_control_horizontal, 0, related_control_horizontal));

[Peter Kasting, 2017/07/06 06:29:16]

It seems like this will result in 2x "related control spacing" around the
separator, since we'll get this border + the standard boxlayout spacing below.

[ananta, 2017/07/07 22:51:02]

Thanks. With the margins as suggested incorporated, we can do away with the
border. 

+
   AddChildViewAt(separator_, 2);
 
   find_text_->SetBorder(views::NullBorder());
 
   views::BoxLayout* manager =
       new views::BoxLayout(views::BoxLayout::kHorizontal,
-                           gfx::Insets(kInteriorPadding), kInterChildSpacing);
+                           gfx::Insets(provider->GetInsetsMetric(
+                              views::INSETS_BUBBLE_CONTENTS)),

[Peter Kasting, 2017/07/06 06:29:16]

The bubble insets aren't correct for a toast like this.  Per the spec (
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(...
), we'd want to use 8 DIP on the top and bottom, whereas this uses 16.

You could address this by adding yet more inset types for "toasts with text" and
"toasts with buttons", but I was going to accomplish this differently: use the
newly-added margin functionality on BoxLayout, and give each subcomponent of the
find bar vertical margins.  8 DIP on the buttons, 12 DIP on the text.  (These
constants would, for now, be done by adding layout provider distance metrics for
"button/text vertical margin in a toast.)  Then if it made sense I'd find a way
to subsequently refactor the button/text creation to set these margins
automatically, so that if you e.g. created a label whose context was "toast", it
would auto-set that margin on the label, and we wouldn't need any code in the
find bar at all.

[ananta, 2017/07/07 22:51:02]

Thanks. I added some code to set the margins explicitly on the controls.

+                           related_control_horizontal);

[Peter Kasting, 2017/07/06 06:29:16]

This is sort of tricky, because on the current spec at least (
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(...
) we want the "related control" spacing between the 16 DIP inner parts of the
buttons, but each button is really 4 DIP bigger on every side.  You should
probably confirm with bettes@ that we want to end up with 8 DIP between these
24-DIP-per-side touchable buttons (or do we want to go ahead with 16 from the
outer edges of them?  That'd be easier but I think it'd look really bad.)

Then we should find a way to make it so the layout provider spaces these buttons
based on the inner region and not the outer one, without having to manually
compute spacing offsets between each child.  The buttons currently use a border
that's INSETS_VECTOR_IMAGE_BUTTON in size to outdent from 16 to 24.  Maybe
kylixrd@ has an idea of how you could tell the layout provider "space these
things 16 DIP apart not including their borders"?

[ananta, 2017/07/07 22:51:02]

The next patch uses an inset of 0, 16. Each control has its own margin
specified. This seemed to look reasonable to me.

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

The buttons get really far apart doing this, because of the issue I described
here.  Basically, you implemented what I said about "16 from the outer edges of
them?  That'd be easier but I think it'd look really bad", and it definitely
looks like the buttons are too spaced-out to me.

Maybe we could make this outer touchable expansion region be margin rather than
border and have the layout manager collapse margins here, which I think would do
what you want, but it's not clear to me that it makes logical sense for that to
be margin rather than border, since it's really "inside" the button and not
outside.

   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);
 }