Harmonize the find in page dialog.

This patch changes the insets and control spacing and the match count text

We space the controls in the find bar using margins to ensure that the
distances are computed to the vector image glyphs and not to the border.

Next steps would be to tackle the rest of the items in this dialog for harmony

BUG=651643
TBR=blundell

Review-Url: https://codereview.chromium.org/2968713003
Cr-Commit-Position: refs/heads/master@{#487316}

chrome/browser/ui/find_bar/find_bar_host_browsertest.cc

@@ -1181,7 +1181,7 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, PrepopulateSameTab) {
   EnsureFindBoxOpen();
 
   EXPECT_EQ(ASCIIToUTF16("page"), GetFindBarText());
-  EXPECT_EQ(ASCIIToUTF16("1 of 1"), GetMatchCountText());
+  EXPECT_EQ(ASCIIToUTF16("1/1"), GetMatchCountText());
 
   // Close the Find box.
   browser()->GetFindBarController()->EndFindSession(
@@ -1194,7 +1194,7 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, PrepopulateSameTab) {
   // After the Find box has been reopened, it should have been prepopulated with
   // the word "page" again.
   EXPECT_EQ(ASCIIToUTF16("page"), GetFindBarText());
-  EXPECT_EQ(ASCIIToUTF16("1 of 1"), GetMatchCountText());
+  EXPECT_EQ(ASCIIToUTF16("1/1"), GetMatchCountText());
 }
 
 // This tests that whenever you open Find in a new tab it should prepopulate
@@ -1211,7 +1211,7 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, PrepopulateInNewTab) {
       browser()->tab_strip_model()->GetActiveWebContents();
   EXPECT_EQ(1, FindInPageASCII(web_contents_1, "page",
                                kFwd, kIgnoreCase, &ordinal));
-  EXPECT_EQ(ASCIIToUTF16("1 of 1"), GetMatchCountText());
+  EXPECT_EQ(ASCIIToUTF16("1/1"), GetMatchCountText());
 
   // Now create a second tab and load the same page.
   chrome::AddSelectedTabWithURL(browser(), url, ui::PAGE_TRANSITION_TYPED);
@@ -1474,14 +1474,14 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest,
   GURL url = GetURL(kSimple);
   ui_test_utils::NavigateToURL(browser(), url);
 
-  // Change the match count on the first tab to "1 of 1".
+  // Change the match count on the first tab to "1/1".
   int ordinal = 0;
   WebContents* web_contents_1 =
       browser()->tab_strip_model()->GetActiveWebContents();
   EXPECT_EQ(1, FindInPageASCII(web_contents_1, "page",
                                kFwd, kIgnoreCase, &ordinal));
   EnsureFindBoxOpen();
-  EXPECT_EQ(ASCIIToUTF16("1 of 1"), GetMatchCountText());
+  EXPECT_EQ(ASCIIToUTF16("1/1"), GetMatchCountText());
 
   // Next, do a search in a second tab.
   chrome::AddTabAt(browser(), GURL(), -1, true);
@@ -1489,7 +1489,7 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest,
   WebContents* web_contents_2 =
       browser()->tab_strip_model()->GetActiveWebContents();
   FindInPageASCII(web_contents_2, "text", kFwd, kIgnoreCase, &ordinal);
-  EXPECT_EQ(ASCIIToUTF16("1 of 1"), GetMatchCountText());
+  EXPECT_EQ(ASCIIToUTF16("1/1"), GetMatchCountText());
 
   // Go back to the first tab and verify that the match text is cleared.
   // text to "text".
@@ -1528,7 +1528,7 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, IncognitoFindNextSecret) {
   FindInPageASCII(web_contents_incognito, "foo", true, kIgnoreCase, NULL);
   EXPECT_EQ(ASCIIToUTF16("foo"),
       GetFindBarTextForBrowser(browser_incognito));
-  EXPECT_EQ(ASCIIToUTF16("1 of 2"),
+  EXPECT_EQ(ASCIIToUTF16("1/2"),
             GetFindBarMatchCountTextForBrowser(browser_incognito));
 
   // Close the find bar.
@@ -1544,7 +1544,7 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, IncognitoFindNextSecret) {
   observer.Wait();
   EXPECT_EQ(ASCIIToUTF16("foo"),
             GetFindBarTextForBrowser(browser_incognito));
-  EXPECT_EQ(ASCIIToUTF16("2 of 2"),
+  EXPECT_EQ(ASCIIToUTF16("2/2"),
             GetFindBarMatchCountTextForBrowser(browser_incognito));
 }
 

chrome/browser/ui/views/find_bar_view.cc

@@ -21,6 +21,7 @@
 #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 "components/vector_icons/vector_icons.h"
@@ -42,20 +43,12 @@
 #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;
 
@@ -161,15 +154,56 @@ FindBarView::FindBarView(FindBarHost* host)
       base::MakeUnique<views::ViewTargeter>(this));
   AddChildViewAt(match_count_text_, 1);
 
-  separator_->SetBorder(views::CreateEmptyBorder(0, kSeparatorLeftSpacing, 0,
-                                                 kSeparatorRightSpacing));
+  ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
+
   AddChildViewAt(separator_, 2);
 
+  // Normally we could space objects horizontally by simply passing a constant
+  // value to BoxLayout for between-child spacing.  But for the vector image
+  // buttons, we want the spacing to apply between the inner "glyph" portions
+  // of the buttons, ignoring the surrounding borders.  BoxLayout has no way
+  // to dynamically adjust for this, so instead of using between-child spacing,
+  // we place views directly adjacent, with horizontal margins on each view
+  // that will add up to the right spacing amounts.
+
+  const gfx::Insets horizontal_margin(
+      0,
+      provider->GetDistanceMetric(DISTANCE_UNRELATED_CONTROL_HORIZONTAL) / 2);
+  const gfx::Insets vector_button =
+      provider->GetInsetsMetric(views::INSETS_VECTOR_IMAGE_BUTTON);
+  const gfx::Insets vector_button_horizontal_margin(
+      0, horizontal_margin.left() - vector_button.left(), 0,
+      horizontal_margin.right() - vector_button.right());
+  const gfx::Insets toast_control_vertical_margin(
+      provider->GetDistanceMetric(DISTANCE_TOAST_CONTROL_VERTICAL), 0);
+  const gfx::Insets toast_label_vertical_margin(
+      provider->GetDistanceMetric(DISTANCE_TOAST_LABEL_VERTICAL), 0);
+  find_previous_button_->SetProperty(
+      views::kMarginsKey, new gfx::Insets(toast_control_vertical_margin +
+                                          vector_button_horizontal_margin));
+  find_next_button_->SetProperty(
+      views::kMarginsKey, new gfx::Insets(toast_control_vertical_margin +
+                                          vector_button_horizontal_margin));
+  close_button_->SetProperty(views::kMarginsKey,
+                             new gfx::Insets(toast_control_vertical_margin +
+                                             vector_button_horizontal_margin));
+  separator_->SetProperty(
+      views::kMarginsKey,
+      new gfx::Insets(toast_control_vertical_margin + horizontal_margin));
+  find_text_->SetProperty(
+      views::kMarginsKey,
+      new gfx::Insets(toast_control_vertical_margin + horizontal_margin));
+  match_count_text_->SetProperty(
+      views::kMarginsKey,
+      new gfx::Insets(toast_label_vertical_margin + horizontal_margin));
+
   find_text_->SetBorder(views::NullBorder());
 
-  views::BoxLayout* manager =
-      new views::BoxLayout(views::BoxLayout::kHorizontal,
-                           gfx::Insets(kInteriorPadding), kInterChildSpacing);
+  views::BoxLayout* manager = new views::BoxLayout(
+      views::BoxLayout::kHorizontal,
+      gfx::Insets(provider->GetInsetsMetric(INSETS_TOAST) - horizontal_margin),
+      0);
+
   SetLayoutManager(manager);
   manager->SetFlexForView(find_text_, 1);
 }

chrome/browser/ui/views/harmony/chrome_layout_provider.cc

@@ -34,6 +34,15 @@ int ChromeLayoutProvider::GetControlHeightForFont(const gfx::FontList& font) {
          Get()->GetDistanceMetric(DISTANCE_CONTROL_TOTAL_VERTICAL_TEXT_PADDING);
 }
 
+gfx::Insets ChromeLayoutProvider::GetInsetsMetric(int metric) const {
+  switch (metric) {
+    case ChromeInsetsMetric::INSETS_TOAST:
+      return gfx::Insets(0, 8);
+    default:
+      return views::LayoutProvider::GetInsetsMetric(metric);
+  }
+}
+
 int ChromeLayoutProvider::GetDistanceMetric(int metric) const {
   switch (metric) {
     case DISTANCE_BUTTON_MINIMUM_WIDTH:
@@ -58,6 +67,10 @@ int ChromeLayoutProvider::GetDistanceMetric(int metric) const {
       return 20;
     case DISTANCE_UNRELATED_CONTROL_VERTICAL_LARGE:
       return 30;
+    case DISTANCE_TOAST_CONTROL_VERTICAL:
+      return 8;
+    case DISTANCE_TOAST_LABEL_VERTICAL:
+      return 12;
     default:
       return views::LayoutProvider::GetDistanceMetric(metric);
   }

chrome/browser/ui/views/harmony/chrome_layout_provider.h

@@ -12,6 +12,11 @@
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/layout/layout_provider.h"
 
+enum ChromeInsetsMetric {
+  // Margins used by toasts.
+  INSETS_TOAST = views::VIEWS_INSETS_END,
+};
+
 enum ChromeDistanceMetric {
   // Default minimum width of a button.
   DISTANCE_BUTTON_MINIMUM_WIDTH = views::VIEWS_DISTANCE_END,
@@ -33,6 +38,10 @@ enum ChromeDistanceMetric {
   // Horizontal indent of a subsection relative to related items above, e.g.
   // checkboxes below explanatory text/headings.
   DISTANCE_SUBSECTION_HORIZONTAL_INDENT,
+  // Vertical margin for controls in a toast.
+  DISTANCE_TOAST_CONTROL_VERTICAL,
+  // Vertical margin for labels in a toast.
+  DISTANCE_TOAST_LABEL_VERTICAL,
   // Horizontal spacing between controls that are logically unrelated.
   DISTANCE_UNRELATED_CONTROL_HORIZONTAL,
   // Larger horizontal spacing between unrelated controls.
@@ -54,6 +63,7 @@ class ChromeLayoutProvider : public views::LayoutProvider {
   static int GetControlHeightForFont(const gfx::FontList& font);
 
   // views::LayoutProvider:
+  gfx::Insets GetInsetsMetric(int metric) const override;
   int GetDistanceMetric(int metric) const override;
   const views::TypographyProvider& GetTypographyProvider() const override;
 

chrome/browser/ui/views/harmony/harmony_layout_provider.cc

@@ -18,6 +18,8 @@ gfx::Insets HarmonyLayoutProvider::GetInsetsMetric(int metric) const {
     }
     case views::INSETS_VECTOR_IMAGE_BUTTON:
       return gfx::Insets(kHarmonyLayoutUnit / 4);
+    case INSETS_TOAST:
+      return gfx::Insets(0, kHarmonyLayoutUnit);
     default:
       return ChromeLayoutProvider::GetInsetsMetric(metric);
   }
@@ -73,9 +75,9 @@ int HarmonyLayoutProvider::GetDistanceMetric(int metric) const {
       return kHarmonyLayoutUnit;
     case DISTANCE_UNRELATED_CONTROL_VERTICAL_LARGE:
       return kHarmonyLayoutUnit;
+    default:
+      return ChromeLayoutProvider::GetDistanceMetric(metric);
   }
-  NOTREACHED();
-  return 0;
 }
 
 views::GridLayout::Alignment

components/find_in_page_strings.grdp

@@ -2,7 +2,7 @@
 <grit-part>
 
   <message name="IDS_FIND_IN_PAGE_COUNT" desc="How many matches found and what item we are showing">
-    <ph name="ACTIVE_MATCH">$1<ex>1</ex></ph> of <ph name="TOTAL_MATCHCOUNT">$2<ex>5</ex></ph>
+    <ph name="ACTIVE_MATCH">$1</ph>/<ph name="TOTAL_MATCHCOUNT">$2</ph>
   </message>
   <message name="IDS_FIND_IN_PAGE_PREVIOUS_TOOLTIP" desc="The tooltip for the previous button">
     Previous