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;

[Peter Kasting, 2017/06/13 23:42:50]

So why a margin of 1 and padding of 4 instead of a margin of 0 and padding of 6?

[Justin Donnelly, 2017/06/15 18:56:53]

Because that won't work in the multi-part case (answers and/or vertical layout).
It would introduce more padding between the two parts than we want. (That's what
caused this bug in the first place, was me changing the layout so that we could
have a different amount of space between the two parts than we have between
suggestions.)

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

OK, but using 1 and 4 is sufficient for your needs?

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

Yes. I've tested this extensively on Linux and Windows. (And I've discovered a
couple other Windows issues as a result. I guess I really need to be testing on
Windows more.)

+
+// 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;

[Peter Kasting, 2017/06/13 23:42:50]

Nit: kAdditionalTextLineHeight?

Should this just be set as part of the line height for the RenderText used to
draw this, so it doesn't have to be added in manually?

[Justin Donnelly, 2017/06/15 18:56:53]

That would add additional, undesired space between lines in multi-line answers.

 
 // 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();
       canvas->DrawImageInt(answer_image_, 0, 0, answer_image_.width(),
                            answer_image_.height(), GetMirroredXInView(x), y,
                            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 ? GetTextHeight() : GetAnswerHeight();
 
   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);
 }