Adjust elision of omnibox keyword search label.

chrome/browser/ui/views/location_bar/selected_keyword_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.
 
 #include "chrome/browser/ui/views/location_bar/selected_keyword_view.h"
 
 #include "base/logging.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/ui/location_bar/location_bar_util.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/omnibox/browser/vector_icons.h"
 #include "components/search_engines/template_url_service.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/theme_provider.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/gfx/vector_icons_public.h"
 #include "ui/native_theme/native_theme.h"
 
 SelectedKeywordView::SelectedKeywordView(const gfx::FontList& font_list,
                                          Profile* profile)
     : IconLabelBubbleView(font_list, false), profile_(profile) {
+  set_can_process_events_within_subtree(false);

[Peter Kasting, 2017/03/07 02:28:45]

Don't do this (it looks like an artifact of the "prevent clicks on this" idea)

   full_label_.SetFontList(font_list);
   full_label_.SetVisible(false);
   partial_label_.SetFontList(font_list);
   partial_label_.SetVisible(false);
 }
 
 SelectedKeywordView::~SelectedKeywordView() {
 }
 
 void SelectedKeywordView::ResetImage() {
   SetImage(gfx::CreateVectorIcon(omnibox::kSearchIcon,
                                  LocationBarView::kIconWidth, GetTextColor()));
 }
 
 SkColor SelectedKeywordView::GetTextColor() const {
   return GetNativeTheme()->GetSystemColor(
       color_utils::IsDark(GetParentBackgroundColor())
           ? ui::NativeTheme::kColorId_TextfieldDefaultColor
           : ui::NativeTheme::kColorId_LinkEnabled);
 }
 
 gfx::Size SelectedKeywordView::GetPreferredSize() const {
   // Height will be ignored by the LocationBarView.
   return GetSizeForLabelWidth(full_label_.GetPreferredSize().width());
 }
 
-gfx::Size SelectedKeywordView::GetMinimumSize() const {

[Evan Stade, 2017/03/07 18:04:44]

I mis-spoke before, this actually is necessary and the implementation should be

return GetSizeForLabelWidth(0).width();

because the decoration will indeed refuse to go smaller than this even if it has
to take up >50% of the omnibox to do so. Of course the omnibox would have to be
teeny weeny for this to occur.

-  // Height will be ignored by the LocationBarView.
-  return GetSizeForLabelWidth(partial_label_.GetMinimumSize().width());
-}
+void SelectedKeywordView::Layout() {
+  // Keep showing the full label as long as there's more than enough width for
+  // the partial label. Otherwise there will be empty space displayed next to
+  // the partial label.
+  if (width() >
+      GetSizeForLabelWidth(partial_label_.GetPreferredSize().width()).width()) {
+    SetLabel(full_label_.text());
+  } else {
+    SetLabel(partial_label_.text());
+  }

[Peter Kasting, 2017/03/07 02:28:45]

Nit: Shorter:

  const int partial_label_width = partial_label_.GetPreferredSize().width();
  const bool show_full_label =
      width() > GetSizeForLabelWidth(partial_label_width).width();
  SetLabel((show_full_label ? full_label_ : partial_label_).text());

 
-void SelectedKeywordView::Layout() {
-  SetLabel(((width() == GetPreferredSize().width()) ?
-      full_label_ : partial_label_).text());
   IconLabelBubbleView::Layout();
 }
 
 void SelectedKeywordView::SetKeyword(const base::string16& keyword) {
   keyword_ = keyword;
   if (keyword.empty())
     return;
   DCHECK(profile_);
   TemplateURLService* model =
       TemplateURLServiceFactory::GetForProfile(profile_);
   if (!model)
     return;
 
   bool is_extension_keyword;
   const base::string16 short_name =
       model->GetKeywordShortName(keyword, &is_extension_keyword);
   const base::string16 full_name =
       is_extension_keyword
           ? short_name
           : l10n_util::GetStringFUTF16(IDS_OMNIBOX_KEYWORD_TEXT_MD, short_name);
   full_label_.SetText(full_name);
 
-  const base::string16 min_string(
-      location_bar_util::CalculateMinString(short_name));
-  const base::string16 partial_name =
-      is_extension_keyword
-          ? min_string
-          : l10n_util::GetStringFUTF16(IDS_OMNIBOX_KEYWORD_TEXT_MD, min_string);
-  partial_label_.SetText(min_string.empty() ?
-      full_label_.text() : partial_name);
+  partial_label_.SetText(location_bar_util::CalculateMinString(short_name));

[Peter Kasting, 2017/03/07 02:28:45]

This lost the protection the old code had against the min string being empty,
which seems like it could happen if the search engine short name begins with
whitespace or a period.  I would imagine that's worth preserving (and maybe
unittesting)

[Evan Stade, 2017/03/07 18:04:44]

hmmm, this is even better if I just remove CalculateMinString. The Label view
will already be doing elision so there's no need. (And in fact we can get rid of
the helper if Mac is also updated.)

One thing I just noticed from testing without the fade elision is that we can
get into a state where the label is nothing but an ellipsis. With fade elision,
that can't happen as you can at least see a partially opaque "ama" (for the
amazon example). I don't know how to fix this state. So, to open this can of
worms again, we can...

1) use fade tail always. Also, make the label extend all the way to the
separator. This actually gives us more room, e.g. you would see

[icon] Search Amazon.com| 

but the .com would be fading out. With ellipsis, we see something like

[icon] Search Amazo...  |

because it looks bad if we don't leave that space beside the ellipsis. This
effectively doubles the unusable space.

2) use fade tail only when the label is really small. Compare

[icon] Amazo|

(again with fading text) to

[icon] ...  |

3) like #2, but truncating instead of fading. We'd get

[icon] Am   |

[Peter Kasting, 2017/03/08 03:13:12]

Set the "minimum size" to the required width for several label characters?  Then
we can't shrink all the way down that small.  Actually I'm kind of surprised
that the omnibox min width is small enough we can get that small at all.
I reserve judgment until I see a screenshot.
Not a fan of these two.  (3) kinda feels broken.  If we were willing to do (2),
I'd just do (1), or some variant thereof.

 
   // Update the label now so ShouldShowLabel() works correctly when the parent
   // class is calculating the preferred size. It will be updated again in
   // Layout(), taking into account how much space has actually been allotted.
   SetLabel(full_name);
 }
 
 const char* SelectedKeywordView::GetClassName() const {
   return "SelectedKeywordView";
 }