Provide vertical padding to the RenderTexts used in suggestions.

chrome/browser/ui/views/omnibox/omnibox_result_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.
 
 // For WinDDK ATL compatibility, these ATL headers must come first.
 #include "build/build_config.h"
 
 #if defined(OS_WIN)
 #include <atlbase.h>  // NOLINT
 #include <atlwin.h>  // NOLINT
@@ skipping 32 matching lines @@
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/gfx/range/range.h"
 #include "ui/gfx/render_text.h"
 #include "ui/gfx/text_utils.h"
 #include "ui/native_theme/native_theme.h"
 
 using ui::NativeTheme;
 
 namespace {
 
-// The padding that should be placed between content and description in a
-// vertical layout.
-static const int kVerticalPadding = 3;
+// The vertical margin that should be used above and below each suggestion.
+static const int kVerticalMargin = 1;
+
+// The vertical padding to provide each RenderText in addition to the height of
+// the font. Where possible, RenderText uses this additional space to vertically
+// center the cap height of the font instead of centering the entire font.
+static const int kVerticalPadding = 4;
 
 // A mapping from OmniboxResultView's ResultViewState/ColorKind types to
 // NativeTheme colors.
 struct TranslationTable {
   ui::NativeTheme::ColorId id;
   OmniboxResultView::ResultViewState state;
   OmniboxResultView::ColorKind kind;
 } static const kTranslationTable[] = {
   { NativeTheme::kColorId_ResultsTableNormalBackground,
     OmniboxResultView::NORMAL, OmniboxResultView::BACKGROUND },
@@ skipping 230 matching lines @@
   // selected. The non-answer text is already accessible as a consequence of
   // updating the text in the omnibox but this alert and GetAccessibleNodeData
   // below make the answer contents accessible.
   if (match_.answer)
     NotifyAccessibilityEvent(ui::AX_EVENT_SELECTION, true);
 }
 
 gfx::Size OmniboxResultView::CalculatePreferredSize() const {
   int height = GetTextHeight() + (2 * GetVerticalMargin());
   if (match_.answer)
-    height += GetAnswerHeight() + kVerticalPadding;
+    height += GetAnswerHeight();
   else if (base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout))
-    height += GetTextHeight() + kVerticalPadding;
+    height += GetTextHeight();
   return gfx::Size(0, height);
 }
 
 void OmniboxResultView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
   node_data->SetName(match_.answer
                          ? l10n_util::GetStringFUTF16(
                                IDS_OMNIBOX_ACCESSIBLE_ANSWER, match_.contents,
                                match_.answer->second_line().AccessibleText())
                          : match_.contents);
 }
 
 void OmniboxResultView::OnNativeThemeChanged(const ui::NativeTheme* theme) {
   Invalidate();
   SchedulePaint();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // OmniboxResultView, protected:
 
 OmniboxResultView::ResultViewState OmniboxResultView::GetState() const {
   if (model_->IsSelectedIndex(model_index_))
     return SELECTED;
   return model_->IsHoveredIndex(model_index_) ? HOVERED : NORMAL;
 }
 
 int OmniboxResultView::GetTextHeight() const {
-  return font_height_;
+  return font_height_ + kVerticalPadding;
 }
 
 void OmniboxResultView::PaintMatch(const AutocompleteMatch& match,
                                    gfx::RenderText* contents,
                                    gfx::RenderText* description,
                                    gfx::Canvas* canvas,
                                    int x) const {
   int y = text_bounds_.y() + GetVerticalMargin();
 
   if (!separator_rendertext_) {
@@ skipping 14 matching lines @@
   OmniboxPopupModel::ComputeMatchMaxWidths(
       contents->GetContentWidth(), separator_width_,
       description ? description->GetContentWidth() : 0,
       mirroring_context_->remaining_width(x), description_on_separate_line,
       !AutocompleteMatch::IsSearchType(match.type), &contents_max_width,
       &description_max_width);
 
   // Answers in Suggest results.
   if (match.answer && description_max_width != 0) {
     DrawRenderText(match, contents, CONTENTS, canvas, x, y, contents_max_width);
-    y += GetTextHeight() + kVerticalPadding;
+    y += GetTextHeight();
     if (!answer_image_.isNull()) {
-      int answer_icon_size = GetAnswerHeight();
+      int answer_icon_size = GetAnswerFont().GetHeight();

[Peter Kasting, 2017/06/15 22:43:34]

Nit: Is it worth commenting here why using GetAnswerHeight() is wrong?

[Justin Donnelly, 2017/06/16 14:14:31]

Done.

       canvas->DrawImageInt(answer_image_, 0, 0, answer_image_.width(),
-                           answer_image_.height(), GetMirroredXInView(x), y,
-                           answer_icon_size, answer_icon_size, true);
+                           answer_image_.height(), GetMirroredXInView(x),
+                           y + (kVerticalPadding / 2), answer_icon_size,
+                           answer_icon_size, true);
       // TODO(dschuyler): Perhaps this should be based on the font size
       // instead of hardcoded to 2 dp (e.g. by adding a space in an
       // appropriate font to the beginning of the description, then reducing
       // the additional padding here to zero).
       const int kAnswerIconToTextPadding = 2;
       x += answer_icon_size + kAnswerIconToTextPadding;
     }
     DrawRenderText(match, description, DESCRIPTION, canvas, x, y,
                    description_max_width);
     return;
   }
 
   // Regular results.
   if (base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout)) {
     // For no description, shift down halfways to draw contents in middle.
     if (description_max_width == 0)
-      y += (GetTextHeight() + kVerticalPadding) / 2;
+      y += GetTextHeight() / 2;
 
     DrawRenderText(match, contents, CONTENTS, canvas, x, y, contents_max_width);
 
     if (description_max_width != 0) {
-      y += GetTextHeight() + kVerticalPadding;
+      y += GetTextHeight();
       DrawRenderText(match, description, DESCRIPTION, canvas, x, y,
                      description_max_width);
     }
   } else {
     x = DrawRenderText(match, contents, CONTENTS, canvas, x, y,
                        contents_max_width);
     if (description_max_width != 0) {
       x = DrawRenderText(match, separator_rendertext_.get(), SEPARATOR, canvas,
                          x, y, separator_width_);
       DrawRenderText(match, description, DESCRIPTION, canvas, x, y,
@@ skipping 232 matching lines @@
   // the omnibox border outline shape exactly in OnPaint().  We have to inset
   // here to keep the icons lined up.
   const int start_x = BackgroundWith1PxBorder::kLocationBarBorderThicknessDip +
                       horizontal_padding;
   const int end_x = width() - start_x;
 
   const gfx::ImageSkia icon = GetIcon();
 
   int row_height = GetTextHeight();
   if (base::FeatureList::IsEnabled(omnibox::kUIExperimentVerticalLayout))
-    row_height += kVerticalPadding + GetTextHeight();
+    row_height += match_.answer ? GetAnswerHeight() : GetTextHeight();
 
   const int icon_y = GetVerticalMargin() + (row_height - icon.height()) / 2;
   icon_bounds_.SetRect(start_x, icon_y, icon.width(), icon.height());
 
   const int text_x = start_x + LocationBarView::kIconWidth + horizontal_padding;
   int text_width = end_x - text_x;
 
   if (match_.associated_keyword.get()) {
     const int max_kw_x = end_x - keyword_icon_->width();
     const int kw_x = animation_->CurrentValueBetween(max_kw_x, start_x);
@@ skipping 79 matching lines @@
           ? match_.answer->second_line().text_fields()[0].type()
           : SuggestionAnswer::SUGGESTION;
   return ui::ResourceBundle::GetSharedInstance().GetFontList(
       GetTextStyle(text_type).font);
 }
 
 int OmniboxResultView::GetAnswerHeight() const {
   // If the answer specifies a maximum of 1 line we can simply return the answer
   // font height.
   if (match_.answer->second_line().num_text_lines() == 1)
-    return GetAnswerFont().GetHeight();
+    return GetAnswerFont().GetHeight() + kVerticalPadding;
 
   // Multi-line answers require layout in order to determine the number of lines
   // the RenderText will use.
   if (!description_rendertext_) {
     description_rendertext_ =
         CreateAnswerText(match_.answer->second_line(), GetAnswerFont());
   }
   description_rendertext_->SetDisplayRect(gfx::Rect(text_bounds_.width(), 0));
   description_rendertext_->GetStringSize();
-  return GetAnswerFont().GetHeight() * description_rendertext_->GetNumLines();
+  return (GetAnswerFont().GetHeight() *
+          description_rendertext_->GetNumLines()) +
+         kVerticalPadding;
 }
 
 int OmniboxResultView::GetVerticalMargin() const {
   // Regardless of the text size, we ensure a minimum size for the content line
   // here. This minimum is larger for hybrid mouse/touch devices to ensure an
   // adequately sized touch target.
   using Md = ui::MaterialDesignController;
   const int kIconVerticalPad = base::GetFieldTrialParamByFeatureAsInt(
       omnibox::kUIExperimentVerticalMargin,
       OmniboxFieldTrial::kUIVerticalMarginParam,
       Md::GetMode() == Md::MATERIAL_HYBRID ? 8 : 4);
   const int min_height = LocationBarView::kIconWidth + 2 * kIconVerticalPad;
 
-  return std::max(kVerticalPadding, (min_height - GetTextHeight()) / 2);
+  return std::max(kVerticalMargin, (min_height - GetTextHeight()) / 2);
 }
 
 std::unique_ptr<gfx::RenderText> OmniboxResultView::CreateAnswerText(
     const SuggestionAnswer::ImageLine& line,
     const gfx::FontList& font_list) const {
   std::unique_ptr<gfx::RenderText> destination =
       CreateRenderText(base::string16());
   destination->SetFontList(font_list);
 
   for (const SuggestionAnswer::TextField& text_field : line.text_fields())
@@ skipping 61 matching lines @@
   destination->AppendText(text);
   const TextStyle& text_style = GetTextStyle(text_type);
   // TODO(dschuyler): follow up on the problem of different font sizes within
   // one RenderText.  Maybe with destination->SetFontList(...).
   destination->ApplyWeight(
       is_bold ? gfx::Font::Weight::BOLD : gfx::Font::Weight::NORMAL, range);
   destination->ApplyColor(
       GetNativeTheme()->GetSystemColor(text_style.colors[GetState()]), range);
   destination->ApplyBaselineStyle(text_style.baseline, range);
 }