Refine the layout of omnibox answer 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 30 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 lines in a top and bottom layout.

[Peter Kasting, 2017/04/24 22:16:05]

What is "a top and bottom layout"?  Do you mean "When a single result has more
than one line of text"?

Also, I wonder if instead of pulling this in multiple places below, it would
make sense to use RenderText::SetMinLineHeight() on the answer rendertext.  Of
course, that will result in spacing out every line of text, not just placing
space above "the whole answer"... maybe that's not what you want.

[Justin Donnelly, 2017/04/25 16:52:30]

"Top and bottom" is meant to distinguish a layout where the content and
description are laid out vertically rather than side by side or horizontally.
We're going to be experimenting with having other suggestions than answers use
this layout so I wanted to introduce more general terminology. Now that I say it
out loud "vertical layout" sounds much better. Switched to that and changed the
wording to hopefully make this clearer.
This is the spacing between the content and description rendertexts. It's
confusing because these have been referred to as "lines" in this class but now
the description can be multiline. Suggestions on making this clearer are
welcome.

+static const int kVerticalPadding = 3;
+
 // 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 },
   { NativeTheme::kColorId_ResultsTableHoveredBackground,
@@ skipping 226 matching lines @@
 
   // Notify assistive technology when results with answers attached are
   // 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_ALERT, true);
 }
 
 gfx::Size OmniboxResultView::GetPreferredSize() const {
-  if (!match_.answer)
-    return gfx::Size(0, GetContentLineHeight());
-  if (match_.answer->second_line().num_text_lines() == 1)
-    return gfx::Size(0, GetContentLineHeight() + GetAnswerLineHeight());
-  if (!description_rendertext_) {
-    description_rendertext_ =
-        CreateAnswerLine(match_.answer->second_line(), GetAnswerLineFont());
-  }
-  description_rendertext_->SetDisplayRect(
-      gfx::Rect(text_bounds_.width(), 0));
-  description_rendertext_->GetStringSize();
-  return gfx::Size(
-      0, GetContentLineHeight() +
-             GetAnswerLineHeight() * description_rendertext_->GetNumLines());
+  int height = GetTextHeight() + (2 * GetVerticalMargin());
+  if (match_.answer)
+    height += GetAnswerLineHeight() + kVerticalPadding;
+  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);
 }
 
@@ skipping 13 matching lines @@
 
 int OmniboxResultView::GetTextHeight() const {
   return font_height_;
 }
 
 void OmniboxResultView::PaintMatch(const AutocompleteMatch& match,
                                    gfx::RenderText* contents,
                                    gfx::RenderText* description,
                                    gfx::Canvas* canvas,
                                    int x) const {
-  int y = text_bounds_.y();
+  int y = text_bounds_.y() + GetVerticalMargin();
 
   if (!separator_rendertext_) {
     const base::string16& separator =
         l10n_util::GetStringUTF16(IDS_AUTOCOMPLETE_MATCH_DESCRIPTION_SEPARATOR);
     separator_rendertext_ = CreateRenderText(separator);
     separator_rendertext_->SetColor(GetColor(GetState(), DIMMED_TEXT));
     separator_width_ = separator_rendertext_->GetContentWidth();
   }
 
   contents->SetDisplayRect(gfx::Rect(gfx::Size(INT_MAX, 0)));
   if (description)
     description->SetDisplayRect(gfx::Rect(gfx::Size(INT_MAX, 0)));
   int contents_max_width, description_max_width;
   OmniboxPopupModel::ComputeMatchMaxWidths(
       contents->GetContentWidth(),
       separator_width_,
       description ? description->GetContentWidth() : 0,
       mirroring_context_->remaining_width(x),
       match.answer != nullptr,
       !AutocompleteMatch::IsSearchType(match.type),
       &contents_max_width,
       &description_max_width);
 
   int after_contents_x = DrawRenderText(match, contents, CONTENTS, canvas,
                                         x, y, contents_max_width);
 
   if (description_max_width != 0) {
     if (match.answer) {
-      y += GetContentLineHeight();
+      y += GetTextHeight() + kVerticalPadding;
       if (!answer_image_.isNull()) {
         int answer_icon_size = GetAnswerLineHeight();

[Peter Kasting, 2017/04/24 22:16:05]

This will vertically center the icon against (however many answer lines you
have) instead of against the center of the top line of the answer.  Is that
really what you want?

[Justin Donnelly, 2017/04/25 16:52:30]

Yes, that's what I want. With multiple lines of text, the line height will be
small.

         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;
@@ skipping 67 matching lines @@
       x += start_offset;
       prefix_x = x - prefix_width;
     }
     prefix_render_text->SetDirectionalityMode(is_match_contents_rtl ?
         gfx::DIRECTIONALITY_FORCE_RTL : gfx::DIRECTIONALITY_FORCE_LTR);
     prefix_render_text->SetHorizontalAlignment(
           is_match_contents_rtl ? gfx::ALIGN_RIGHT : gfx::ALIGN_LEFT);
     prefix_render_text->SetDisplayRect(
         gfx::Rect(mirroring_context_->mirrored_left_coord(
                       prefix_x, prefix_x + prefix_width),
-                  y, prefix_width, GetContentLineHeight()));
+                  y, prefix_width, GetTextHeight()));
     prefix_render_text->Draw(canvas);
   }
 
   // Set the display rect to trigger elision.
-  const int final_width = right_x - x;
-  int height = GetContentLineHeight();
-  if (render_text_type == DESCRIPTION && match.answer) {
-    render_text->SetDisplayRect(gfx::Rect(gfx::Size(final_width, 0)));
-    render_text->GetStringSize();
-    height = GetAnswerLineHeight() * render_text->GetNumLines();
-  }
+  int height = (render_text_type == DESCRIPTION && match.answer)
+                   ? GetAnswerLineHeight()
+                   : GetTextHeight();
   render_text->SetDisplayRect(
       gfx::Rect(mirroring_context_->mirrored_left_coord(x, right_x), y,
-                final_width, height));
+                right_x - x, height));
   render_text->Draw(canvas);
   return right_x;
 }
 
 std::unique_ptr<gfx::RenderText> OmniboxResultView::CreateRenderText(
     const base::string16& text) const {
   std::unique_ptr<gfx::RenderText> render_text(
       gfx::RenderText::CreateInstance());
   render_text->SetDisplayRect(gfx::Rect(gfx::Size(INT_MAX, 0)));
   render_text->SetCursorEnabled(false);
@@ skipping 130 matching lines @@
       GetLayoutConstant(LOCATION_BAR_ELEMENT_PADDING) +
       LocationBarView::kIconInteriorPadding;
   // The horizontal bounds we're given are the outside bounds, so we can match
   // 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();
-  icon_bounds_.SetRect(start_x, (GetContentLineHeight() - icon.height()) / 2,
-                       icon.width(), icon.height());
+  const int icon_y =
+      GetVerticalMargin() + (GetTextHeight() - 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);
     const int kw_text_x = kw_x + keyword_icon_->width() + horizontal_padding;
 
     text_width = kw_x - text_x - horizontal_padding;
@@ skipping 18 matching lines @@
   if (!ShowOnlyKeywordMatch()) {
     canvas->DrawImageInt(GetIcon(), GetMirroredXForRect(icon_bounds_),
                          icon_bounds_.y());
     int x = GetMirroredXForRect(text_bounds_);
     mirroring_context_->Initialize(x, text_bounds_.width());
     InitContentsRenderTextIfNecessary();
 
     if (!description_rendertext_) {
       if (match_.answer) {
         description_rendertext_ =
-            CreateAnswerLine(match_.answer->second_line(), GetAnswerLineFont());
+            CreateAnswerLine(match_.answer->second_line(), GetAnswerFont());
       } else if (!match_.description.empty()) {
         description_rendertext_ = CreateClassifiedRenderText(
             match_.description, match_.description_class, true);
       }
     }
     PaintMatch(match_, contents_rendertext_.get(),
                description_rendertext_.get(), canvas, x);
   }
 
   AutocompleteMatch* keyword_match = match_.associated_keyword.get();
@@ skipping 12 matching lines @@
     PaintMatch(*keyword_match, keyword_contents_rendertext_.get(),
                keyword_description_rendertext_.get(), canvas, x);
   }
 }
 
 void OmniboxResultView::AnimationProgressed(const gfx::Animation* animation) {
   Layout();
   SchedulePaint();
 }
 
-const gfx::FontList& OmniboxResultView::GetAnswerLineFont() const {
+const gfx::FontList& OmniboxResultView::GetAnswerFont() const {
   // This assumes that the first text type in the second answer line can be used
   // to specify the font for all the text fields in the line. For now this works
   // but eventually it will be necessary to get RenderText to support multiple
   // font sizes or use multiple RenderTexts.
   int text_type =
       match_.answer && !match_.answer->second_line().text_fields().empty()
           ? match_.answer->second_line().text_fields()[0].type()
           : SuggestionAnswer::SUGGESTION;
   return ui::ResourceBundle::GetSharedInstance().GetFontList(
       GetTextStyle(text_type).font);
 }
 
 int OmniboxResultView::GetAnswerLineHeight() const {

[Peter Kasting, 2017/04/24 22:16:05]

Nit: Seems like GetAnswerHeight() would be better both from "parallel structure"
and "it can be more than one line" perspectives.

[Justin Donnelly, 2017/04/25 16:52:30]

Good point, done. Also changed CreateAnswerLine() to CreateAnswerText() to try
to eliminate the old "line" terminology.

-  return GetAnswerLineFont().GetHeight();
+  // 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();
+
+  // Multi-line answers require layout in order to determine the number of lines
+  // the RenderText will use.
+  if (!description_rendertext_) {
+    description_rendertext_ =
+        CreateAnswerLine(match_.answer->second_line(), GetAnswerFont());
+  }
+  description_rendertext_->SetDisplayRect(gfx::Rect(text_bounds_.width(), 0));
+  description_rendertext_->GetStringSize();
+  return GetAnswerFont().GetHeight() * description_rendertext_->GetNumLines();
 }
 
-int OmniboxResultView::GetContentLineHeight() const {
+int OmniboxResultView::GetVerticalMargin() const {
+  // Regardless of the text size, we ensure a minimum size for the content line

[Justin Donnelly, 2017/04/21 16:57:08]

This is my understanding of what this logic is trying to accomplish. Please let
me know if I'm missing some subtlety.

+  // here. This minimum is larger for hybrid mouse/touch devices to ensure an
+  // adequately sized touch target.
   using Md = ui::MaterialDesignController;
   const int kIconVerticalPad = Md::GetMode() == Md::MATERIAL_HYBRID ? 8 : 4;
-  const int kTextVerticalPad = 3;
-  return std::max(
-      LocationBarView::kIconWidth + 2 * kIconVerticalPad,
-      GetTextHeight() + 2 * kTextVerticalPad);
+  const int min_height = LocationBarView::kIconWidth + 2 * kIconVerticalPad;
+
+  return std::max(kVerticalPadding, (min_height - GetTextHeight()) / 2);
 }
 
 std::unique_ptr<gfx::RenderText> OmniboxResultView::CreateAnswerLine(
     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);
 }