Elide text in the new Autofill UI

chrome/browser/ui/autofill/autofill_popup_controller_impl.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/autofill/autofill_popup_controller_impl.h"
 
 #include "base/logging.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/ui/autofill/autofill_popup_delegate.h"
 #include "chrome/browser/ui/autofill/autofill_popup_view.h"
 #include "content/public/browser/native_web_keyboard_event.h"
 #include "grit/webkit_resources.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebAutofillClient.h"
 #include "ui/base/events/event.h"
+#include "ui/base/text/text_elider.h"
+#include "ui/gfx/display.h"
+#include "ui/gfx/screen.h"
+#include "ui/gfx/vector2d.h"
 
 using WebKit::WebAutofillClient;
 
 namespace {
 
 // Used to indicate that no line is currently selected by the user.
 const int kNoSelection = -1;
 
 // Size difference between name and subtext in pixels.
 const int kLabelFontSizeDelta = -2;
@@ skipping 86 matching lines @@
     const std::vector<string16>& icons,
     const std::vector<int>& identifiers) {
   names_ = names;
   subtexts_ = subtexts;
   icons_ = icons;
   identifiers_ = identifiers;
 
 #if !defined(OS_ANDROID)
   // Android displays the long text with ellipsis using the view attributes.
 
-  // TODO(csharp): Fix crbug.com/156163 and use better logic when clipping.
+  // Don't let the popup have a smaller width then then element it is a popup

[Ilya Sherman, 2013/01/10 23:34:36]

nit: "then then" -> "than the"

[csharp, 2013/01/11 17:51:12]

Done.

+  // for. If the text field is already going of the screen, then the popup
+  // should follow.

[Ilya Sherman, 2013/01/10 23:34:36]

As I mention below, this is almost certainly wrong, for security reasons.

[csharp, 2013/01/11 17:51:12]

Done.

+  int max_popup_width = std::max(MaxVisiblePopupWidth(),
+                                 element_bounds().width());
+
   for (size_t i = 0; i < names_.size(); ++i) {

[Ilya Sherman, 2013/01/10 23:34:36]

nit: Please add a comment indicating that this loop is for eliding strings to
fit in the available width.

[csharp, 2013/01/11 17:51:12]

Done.

-    if (names_[i].length() > 15)
-      names_[i].erase(15);
-    if (subtexts[i].length() > 15)
-      subtexts_[i].erase(15);
+    int total_text_length = name_font().GetStringWidth(names[i])+

[Ilya Sherman, 2013/01/10 23:34:36]

nit: Add a space before the plus sign.

[csharp, 2013/01/11 17:51:12]

Done.

+        subtext_font().GetStringWidth(subtexts_[i]);
+    // The line can have no strings if it represents a UI element, such as
+    // a separator line.
+    if (total_text_length == 0)
+      continue;
+
+    int row_available_width = max_popup_width - RowWidthWithoutText(i);

[Ilya Sherman, 2013/01/10 23:34:36]

Optional nit: Drop the "row_" prefix IMO

[csharp, 2013/01/11 17:51:12]

Done.

+
+    // Each field recieves space in proportion to it length.

[Ilya Sherman, 2013/01/10 23:34:36]

nit: "it length" -> "its length"

[csharp, 2013/01/11 17:51:12]

Done.

+    int name_size = row_available_width *
+        name_font().GetStringWidth(names_[i]) / total_text_length;
+    names_[i] = ui::ElideText(names_[i],
+                              name_font(),
+                              name_size,
+                              ui::ELIDE_AT_END);
+
+    int subtext_size = row_available_width *
+        subtext_font().GetStringWidth(subtexts[i]) / total_text_length;
+    subtexts_[i] = ui::ElideText(subtexts_[i],
+                                 subtext_font(),
+                                 subtext_size,
+                                 ui::ELIDE_AT_END);
   }
 #endif
 
   if (!view_) {
     view_ = AutofillPopupView::Create(this);
     ShowView();
   } else {
     UpdateBoundsAndRedrawPopup();
   }
 }
@@ skipping 65 matching lines @@
   if (name_font_.platform_font() == NULL ||
       subtext_font_.platform_font() == NULL) {
     // We can't calculate the size of the popup if the fonts
     // aren't present.
     return 0;
   }
 
   int popup_width = element_bounds().width();
   DCHECK_EQ(names().size(), subtexts().size());
   for (size_t i = 0; i < names().size(); ++i) {
-    int row_size = kEndPadding +
-        name_font_.GetStringWidth(names()[i]) +
-        kNamePadding +
-        subtext_font_.GetStringWidth(subtexts()[i]);
-
-    // Add the Autofill icon size, if required.
-    if (!icons()[i].empty())
-      row_size += kAutofillIconWidth + kIconPadding;
-
-    // Add delete icon, if required.
-    if (CanDelete(i))
-      row_size += kDeleteIconWidth + kIconPadding;
-
-    // Add the padding at the end
-    row_size += kEndPadding;
+    int row_size = name_font_.GetStringWidth(names()[i]) +
+        subtext_font_.GetStringWidth(subtexts()[i]) +
+        RowWidthWithoutText(i);
 
     popup_width = std::max(popup_width, row_size);
   }
 
   return popup_width;
 }
 
 int AutofillPopupControllerImpl::GetPopupRequiredHeight() {
   int popup_height = 0;
 
@@ skipping 260 matching lines @@
        identifiers_[0] == WebAutofillClient::MenuItemIDDataListEntry);
 }
 
 void AutofillPopupControllerImpl::ShowView() {
   view_->Show();
 }
 
 void AutofillPopupControllerImpl::InvalidateRow(size_t row) {
   view_->InvalidateRow(row);
 }
+
+int AutofillPopupControllerImpl::RowWidthWithoutText(int row) {
+  int row_size = kEndPadding + kNamePadding;
+
+  // Add the Autofill icon size, if required.
+  if (!icons_[row].empty())
+    row_size += kAutofillIconWidth + kIconPadding;
+
+  // Add delete icon, if required.

[Ilya Sherman, 2013/01/10 23:34:36]

nit: "Add the delete icon size", or perhaps "Add space for the delete icon"?

[csharp, 2013/01/11 17:51:12]

Done.

+  if (CanDelete(row))
+    row_size += kDeleteIconWidth + kIconPadding;
+
+  // Add the padding at the end
+  row_size += kEndPadding;
+
+  return row_size;
+}
+
+

[Ilya Sherman, 2013/01/10 23:34:36]

nit: Extra newline; please remove.

[csharp, 2013/01/11 17:51:12]

Done.

+int AutofillPopupControllerImpl::MaxVisiblePopupWidth() {
+  gfx::Point right_corner_of_popup = element_bounds().origin() +

[Ilya Sherman, 2013/01/10 23:34:36]

When anchoring to the left edge of the field, we should never anchor beyond the
left-most on-screen pixel of the text field.  That is, we shouldn't assume that
element_bounds.origin() is visible on the screen.  This is important to get
right for security reasons: a malicious website designer shouldn't be able to
force us to show any part of the popup off screen.

[csharp, 2013/01/11 17:51:12]

Ah, I hadn't been aware of that issue. Fixed.

+      gfx::Vector2d(GetPopupRequiredWidth(), 0);
+  gfx::Screen* screen =
+      gfx::Screen::GetScreenFor(container_view());
+  gfx::Display display = screen->GetDisplayNearestPoint(right_corner_of_popup);

[Ilya Sherman, 2013/01/10 23:34:36]

In addition to anchoring to the left edge of the field, we should be able to
anchor to the right edge.  If the window happens to be positioned so that the
field is near the right edge of the screen, the popup should grow to the left
rather than to the right.  Again, this is important for security reasons, so
that an attacker can't force us to display a popup with all of the text elided
by simply placing a field super near the edge of the screen.

[csharp, 2013/01/11 17:51:12]

Done.

+
+  int rightmost_display_x =  display.GetSizeInPixel().width() +

[Ilya Sherman, 2013/01/10 23:34:36]

nit: Extra space after the equals sign.

[csharp, 2013/01/11 17:51:12]

Done.

+      display.bounds().x() * display.device_scale_factor();
+
+  return rightmost_display_x - element_bounds().origin().x();
+}