[omnibox] Put ellipsis before tail suggestion

This CL adds an ellipsis(...) before each of the subsequent tail
suggestions in the Omnibox tail suggestion list.

Bug: 726769
Change-Id: Ifba11bb5aca8474ee4a37dca652c9f9c676980c5
Reviewed-on: https://chromium-review.googlesource.com/c/1297726
Commit-Queue: Kevin Bailey <krb@chromium.org>
Reviewed-by: Christopher Grant <cjgrant@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604743}

chrome/browser/ui/views/omnibox/omnibox_match_cell_view.cc

@@ -25,6 +25,7 @@
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/image/canvas_image_source.h"
 #include "ui/gfx/paint_vector_icon.h"
+#include "ui/gfx/render_text.h"
 #include "ui/views/border.h"
 #include "ui/views/controls/image_view.h"
 
@@ -329,6 +330,11 @@ void OmniboxMatchCellView::OnMatchUpdate(const OmniboxResultView* result_view,
                                                                         color));
     }
   }
+  if (match.type == AutocompleteMatchType::SEARCH_SUGGEST_TAIL)
+    // Used for indent calculation.
+    SetTailSuggestCommonPrefixWidth(match.tail_suggest_common_prefix);
+  else
+    SetTailSuggestCommonPrefixWidth(base::string16());
 }
 
 const char* OmniboxMatchCellView::GetClassName() const {
@@ -352,7 +358,8 @@ void OmniboxMatchCellView::Layout() {
   } else if (is_old_style_answer_) {
     LayoutOldStyleAnswer(icon_view_width, text_indent);
   } else {
-    LayoutOneLineSuggestion(icon_view_width, text_indent);
+    LayoutOneLineSuggestion(icon_view_width,
+                            text_indent + tail_suggest_common_prefix_width_);
   }
 }
 
@@ -398,7 +405,8 @@ void OmniboxMatchCellView::LayoutNewStyleTwoLineSuggestion() {
   if (description_view_->text().empty()) {
     // This vertically centers content in the rare case that no description is
     // provided.
-    content_view_->SetBounds(x + GetTextIndent(), y, text_width,
+    content_view_->SetBounds(
+        x + GetTextIndent() + tail_suggest_common_prefix_width_, y, text_width,
         child_area.height());
     description_view_->SetSize(gfx::Size());
   } else {
@@ -437,3 +445,23 @@ void OmniboxMatchCellView::LayoutOneLineSuggestion(int icon_view_width,
     separator_view_->SetSize(gfx::Size());
   }
 }
+
+void OmniboxMatchCellView::SetTailSuggestCommonPrefixWidth(
+    const base::string16& common_prefix) {
+  if (common_prefix.empty()) {
+    tail_suggest_common_prefix_width_ = 0;
+    return;
+  }
+  std::unique_ptr<gfx::RenderText> render_text =
+      content_view_->CreateRenderText(common_prefix);
+  auto size = render_text->GetStringSize();
+  tail_suggest_common_prefix_width_ = size.width();
+  // Only calculate fixed string width once.
+  if (!ellipsis_width_) {
+    render_text->SetText(base::ASCIIToUTF16(AutocompleteMatch::kEllipsis));
+    size = render_text->GetStringSize();
+    ellipsis_width_ = size.width();
+  }
+  // Indent text by prefix, but come back by width of ellipsis.
+  tail_suggest_common_prefix_width_ -= ellipsis_width_;
+}

chrome/browser/ui/views/omnibox/omnibox_match_cell_view.h

@@ -62,6 +62,18 @@ class OmniboxMatchCellView : public views::View {
   OmniboxTextView* separator_view_;
 
  private:
+  void SetTailSuggestCommonPrefixWidth(const base::string16& common_prefix);
+
+  // This (permanently) holds the rendered width of
+  // AutocompleteMatch::kEllipsis so that we don't have to keep calculating
+  // it.
+  int ellipsis_width_ = 0;
+
+  // This holds the rendered width of the common prefix of a set of tail
+  // suggestions so that it doesn't have to be re-calculated if the prefix
+  // doesn't change.
+  int tail_suggest_common_prefix_width_ = 0;
+
   DISALLOW_COPY_AND_ASSIGN(OmniboxMatchCellView);
 };
 

chrome/browser/ui/views/omnibox/omnibox_text_view.cc

@@ -354,8 +354,6 @@ void OmniboxTextView::ReapplyStyling() {
       render_text_->SetDirectionalityMode(gfx::DIRECTIONALITY_AS_URL);
     } else if (classifications[i].style & ACMatchClassification::DIM) {
       part = OmniboxPart::RESULTS_TEXT_DIMMED;
-    } else if (classifications[i].style & ACMatchClassification::INVISIBLE) {
-      part = OmniboxPart::RESULTS_TEXT_INVISIBLE;
     }
     render_text_->ApplyColor(result_view_->GetColor(part), current_range);
   }

chrome/browser/ui/views/omnibox/omnibox_text_view.h

@@ -67,10 +67,12 @@ class OmniboxTextView : public views::View {
   // parts.
   void ReapplyStyling();
 
- private:
+  // Creates a platform-approriate RenderText, sets its format to that of
+  // a suggestion and inserts (renders) the provided |text|.
   std::unique_ptr<gfx::RenderText> CreateRenderText(
       const base::string16& text) const;
 
+ private:
   // Adds text from an answer field to the render text using appropriate style.
   // A prefix (such as separating space) may also be prepended to field text.
   void AppendText(const SuggestionAnswer::TextField& field,

chrome/browser/vr/elements/omnibox_formatting.cc

@@ -44,9 +44,6 @@ TextFormatting ConvertClassification(
     if (classifications[i].style & ACMatchClassification::URL) {
       formatting.push_back(
           TextFormattingAttribute(color_scheme.hyperlink, current_range));
-    } else if (classifications[i].style & ACMatchClassification::INVISIBLE) {
-      formatting.push_back(
-          TextFormattingAttribute(SK_ColorTRANSPARENT, current_range));
     }
   }
   return formatting;

chrome/browser/vr/elements/omnibox_formatting_unittest.cc

@@ -31,7 +31,6 @@ TEST(OmniboxSuggestionFormatting, TextFormatting) {
       ACMatchClassification(0, ACMatchClassification::NONE),
       ACMatchClassification(1, ACMatchClassification::URL),
       ACMatchClassification(2, ACMatchClassification::MATCH),
-      ACMatchClassification(3, ACMatchClassification::INVISIBLE),
   };
   size_t string_length = classifications.size();
 
@@ -43,7 +42,7 @@ TEST(OmniboxSuggestionFormatting, TextFormatting) {
   TextFormatting formatting =
       ConvertClassification(classifications, string_length, color_scheme);
 
-  ASSERT_EQ(formatting.size(), 5u);
+  ASSERT_EQ(formatting.size(), 4u);
 
   EXPECT_EQ(formatting[0].type(), TextFormattingAttribute::COLOR);
   EXPECT_EQ(formatting[0].color(), kDefaultColor);
@@ -60,10 +59,6 @@ TEST(OmniboxSuggestionFormatting, TextFormatting) {
   EXPECT_EQ(formatting[3].type(), TextFormattingAttribute::WEIGHT);
   EXPECT_EQ(formatting[3].weight(), gfx::Font::Weight::BOLD);
   EXPECT_EQ(formatting[3].range(), gfx::Range(2, 3));
-
-  EXPECT_EQ(formatting[4].type(), TextFormattingAttribute::COLOR);
-  EXPECT_EQ(formatting[4].color(), SK_ColorTRANSPARENT);
-  EXPECT_EQ(formatting[4].range(), gfx::Range(3, 4));
 }
 
 struct ElisionTestcase {

chrome/browser/vr/testapp/vr_test_context.cc

@@ -686,14 +686,12 @@ void VrTestContext::StartAutocomplete(const AutocompleteRequest& request) {
   // Add a suggestion to exercise classification text styling.
   result->suggestions.emplace_back(OmniboxSuggestion(
       base::UTF8ToUTF16("Suggestion with classification"),
-      base::UTF8ToUTF16("none url match dim invsible"),
-      ACMatchClassifications(),
+      base::UTF8ToUTF16("none url match dim"), ACMatchClassifications(),
       {
           ACMatchClassification(0, ACMatchClassification::NONE),
           ACMatchClassification(5, ACMatchClassification::URL),
           ACMatchClassification(9, ACMatchClassification::MATCH),
           ACMatchClassification(15, ACMatchClassification::DIM),
-          ACMatchClassification(19, ACMatchClassification::INVISIBLE),
       },
       &vector_icons::kSearchIcon, GURL("http://www.test.com/"),
       base::string16(), base::string16()));

components/omnibox/browser/autocomplete_match.cc

@@ -81,6 +81,8 @@ const base::char16 AutocompleteMatch::kInvalidChars[] = {
   0
 };
 
+const char AutocompleteMatch::kEllipsis[] = "... ";
+
 AutocompleteMatch::AutocompleteMatch()
     : provider(nullptr),
       relevance(0),
@@ -125,6 +127,7 @@ AutocompleteMatch::AutocompleteMatch(const AutocompleteMatch& match)
       image_dominant_color(match.image_dominant_color),
       image_url(match.image_url),
       document_type(match.document_type),
+      tail_suggest_common_prefix(match.tail_suggest_common_prefix),
       contents(match.contents),
       contents_class(match.contents_class),
       description(match.description),
@@ -170,6 +173,7 @@ AutocompleteMatch& AutocompleteMatch::operator=(
   image_dominant_color = match.image_dominant_color;
   image_url = match.image_url;
   document_type = match.document_type;
+  tail_suggest_common_prefix = match.tail_suggest_common_prefix;
   contents = match.contents;
   contents_class = match.contents_class;
   description = match.description;
@@ -750,14 +754,15 @@ AutocompleteMatch::GetMatchWithContentsAndDescriptionPossiblySwapped() const {
 
 void AutocompleteMatch::InlineTailPrefix(const base::string16& common_prefix) {
   if (type == AutocompleteMatchType::SEARCH_SUGGEST_TAIL) {
-    contents = common_prefix + contents;
+    tail_suggest_common_prefix = common_prefix;
+    // Insert an ellipsis before uncommon part.
+    const auto ellipsis = base::ASCIIToUTF16(kEllipsis);
+    contents = ellipsis + contents;
     // Shift existing styles.
-    for (ACMatchClassification& classification : contents_class)
-      classification.offset += common_prefix.size();
-    // Prefix with invisible text.
-    contents_class.insert(
-        contents_class.begin(),
-        ACMatchClassification(0, ACMatchClassification::INVISIBLE));
+    for (ACMatchClassification& classification : contents_class) {
+      if (classification.offset > 0)
+        classification.offset += ellipsis.size();
+    }
   }
 }
 

components/omnibox/browser/autocomplete_match.h

@@ -79,7 +79,6 @@ struct AutocompleteMatch {
       URL       = 1 << 0,  // A URL
       MATCH     = 1 << 1,  // A match for the user's search term
       DIM       = 1 << 2,  // "Helper text"
-      INVISIBLE = 1 << 3,  // "Prefix" text we don't want to see
     };
     // clang-format on
 
@@ -434,6 +433,9 @@ struct AutocompleteMatch {
   // Optional override to use for types that specify an icon sub-type.
   DocumentType document_type;
 
+  // Holds the common part of tail suggestion.
+  base::string16 tail_suggest_common_prefix;
+
   // The main text displayed in the address bar dropdown.
   base::string16 contents;
   ACMatchClassifications contents_class;
@@ -517,6 +519,9 @@ struct AutocompleteMatch {
   // ensure if a match is deleted, the duplicates are deleted as well.
   std::vector<AutocompleteMatch> duplicate_matches;
 
+  // So users of AutocompleteMatch can use the same ellipsis that it uses.
+  static const char kEllipsis[];
+
 #if DCHECK_IS_ON()
   // Does a data integrity check on this match.
   void Validate() const;

components/omnibox/browser/autocomplete_match_unittest.cc

@@ -133,12 +133,10 @@ TEST(AutocompleteMatchTest, InlineTailPrefix) {
     ACMatchClassifications before_contents_class, after_contents_class;
   } cases[] = {
       {"90123456",
-       "1234567890123456",
-       // should prepend INVISIBLE and offset rest
+       "... 90123456",
+       // should prepend ellipsis, and offset remainder
        {{0, ACMatchClassification::NONE}, {2, ACMatchClassification::MATCH}},
-       {{0, ACMatchClassification::INVISIBLE},
-        {8, ACMatchClassification::NONE},
-        {10, ACMatchClassification::MATCH}}},
+       {{0, ACMatchClassification::NONE}, {6, ACMatchClassification::MATCH}}},
   };
   for (const auto& test_case : cases) {
     AutocompleteMatch match;

components/omnibox/browser/autocomplete_result_unittest.cc

@@ -942,19 +942,18 @@ TEST_F(AutocompleteResultTest, InlineTailPrefixes) {
       // It should not touch this, since it's not a tail suggestion.
       {
           AutocompleteMatchType::SEARCH_WHAT_YOU_TYPED,
-          "superman",
-          "superman",
-          {{0, ACMatchClassification::NONE}, {5, ACMatchClassification::MATCH}},
-          {{0, ACMatchClassification::NONE}, {5, ACMatchClassification::MATCH}},
+          "this is a test",
+          "this is a test",
+          {{0, ACMatchClassification::NONE}, {9, ACMatchClassification::MATCH}},
+          {{0, ACMatchClassification::NONE}, {9, ACMatchClassification::MATCH}},
       },
       // Make sure it finds this tail suggestion, and prepends appropriately.
       {
           AutocompleteMatchType::SEARCH_SUGGEST_TAIL,
-          "star",
-          "superstar",
+          "a recording",
+          "... a recording",
+          {{0, ACMatchClassification::MATCH}},
           {{0, ACMatchClassification::MATCH}},
-          {{0, ACMatchClassification::INVISIBLE},
-           {5, ACMatchClassification::MATCH}},
       },
   };
   ACMatches matches;
@@ -967,8 +966,9 @@ TEST_F(AutocompleteResultTest, InlineTailPrefixes) {
     matches.push_back(match);
   }
   // Tail suggestion needs one-off initialization.
-  matches[1].RecordAdditionalInfo(kACMatchPropertyContentsStartIndex, "5");
-  matches[1].RecordAdditionalInfo(kACMatchPropertySuggestionText, "superstar");
+  matches[1].RecordAdditionalInfo(kACMatchPropertyContentsStartIndex, "9");
+  matches[1].RecordAdditionalInfo(kACMatchPropertySuggestionText,
+                                  "this is a test");
   AutocompleteResult result;
   result.AppendMatches(AutocompleteInput(), matches);
   result.InlineTailPrefixes();