fix: do not crop notification titles

If a notification title is between 22 and 26 characters long, it might
get truncated in the view. This is because we calculate different line
counts for a title in the layout and draw phases.
This is related to crbug.com/682266.

Instead of relying on the layout phase, we now just set a wider
right-border on the first element in the view, to make space for the
control buttons.

Bug: 785589
Change-Id: I4c6e157d732065224612766dbe02a3f21f377b2f
Reviewed-on: https://chromium-review.googlesource.com/c/1341924
Commit-Queue: Richard Knoll <knollr@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Yoshiki Iguchi <yoshiki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610225}

ui/message_center/views/bounded_label.cc

@@ -303,6 +303,10 @@ int BoundedLabel::GetLineLimit() const {
   return line_limit_;
 }
 
+const gfx::FontList& BoundedLabel::font_list() const {
+  return label_->font_list();
+}
+
 int BoundedLabel::GetLinesForWidthAndLimit(int width, int limit) {
   return visible() ? label_->GetLinesForWidthAndLimit(width, limit) : 0;
 }

ui/message_center/views/bounded_label.h

@@ -47,6 +47,9 @@ class MESSAGE_CENTER_EXPORT BoundedLabel : public views::View {
   int GetLineHeight() const;
   int GetLineLimit() const;
 
+  // Gets the FontList used by this label.
+  const gfx::FontList& font_list() const;
+
   // Pass in a -1 width to use the preferred width, a -1 limit to skip limits.
   int GetLinesForWidthAndLimit(int width, int limit);
   gfx::Size GetSizeForWidthAndLines(int width, int lines);

ui/message_center/views/notification_view.cc

@@ -94,6 +94,17 @@ std::unique_ptr<views::Border> MakeSeparatorBorder(int top,
   return views::CreateSolidSidedBorder(top, left, 0, 0, color);
 }
 
+#if !defined(OS_CHROMEOS)
+// static
+void SetBorderRight(views::View* view, int right) {
+  const gfx::Insets& insets = view->GetInsets();
+  if (insets.right() != right) {
+    view->SetBorder(
+        MakeEmptyBorder(insets.top(), insets.left(), insets.bottom(), right));
+  }
+}
+#endif
+
 // NotificationItemView ////////////////////////////////////////////////////////
 
 // NotificationItemViews are responsible for drawing each list notification
@@ -245,7 +256,14 @@ int NotificationView::GetHeightForWidth(int width) const {
 }
 
 void NotificationView::Layout() {
+  // |ShrinkTopmostLabel| updates the borders of views to make space for the
+  // control buttons. We have to update the borders before calling
+  // |MessageView::Layout| so that the latest values get taken into account
+  // while layouting. As |ShrinkTopmostLabel| only depends on the PreferredSize,
+  // this is valid to do before calling |MessageView::Layout|.
+  ShrinkTopmostLabel();
   MessageView::Layout();
+
   gfx::Insets insets = GetInsets();
   int content_width = width() - insets.width();
   gfx::Rect content_bounds = GetContentsBounds();
@@ -262,7 +280,6 @@ void NotificationView::Layout() {
   // Top views.
   int top_height = top_view_->GetHeightForWidth(content_width);
   top_view_->SetBounds(insets.left(), insets.top(), content_width, top_height);
-  ShrinkTopmostLabel();
 
   // Icon.
   icon_view_->SetBounds(insets.left(), insets.top(), kNotificationIconSize,
@@ -678,12 +695,12 @@ void NotificationView::ShrinkTopmostLabel() {
 // Reduce width of the topmost label not to be covered by the control buttons
 // only on non Chrome OS platform.
 #if !defined(OS_CHROMEOS)
-  const int content_width = width() - GetInsets().width();
-  const int buttons_width = control_buttons_view_->GetPreferredSize().width();
-  if (top_view_->child_count() > 0) {
-    gfx::Rect bounds = top_view_->child_at(0)->bounds();
-    bounds.set_width(content_width - buttons_width);
-    top_view_->child_at(0)->SetBoundsRect(bounds);
+  const int child_count = top_view_->child_count();
+  if (child_count > 0) {
+    const int buttons_width = control_buttons_view_->GetPreferredSize().width();
+    SetBorderRight(top_view_->child_at(0), kTextRightPadding + buttons_width);
+    for (int i = 1; i < child_count; ++i)
+      SetBorderRight(top_view_->child_at(i), kTextRightPadding);
   }
 #endif
 }

ui/message_center/views/notification_view.h

@@ -60,6 +60,7 @@ class MESSAGE_CENTER_EXPORT NotificationView : public MessageView,
   FRIEND_TEST_ALL_PREFIXES(NotificationViewTest, TestLineLimits);
   FRIEND_TEST_ALL_PREFIXES(NotificationViewTest, TestIconSizing);
   FRIEND_TEST_ALL_PREFIXES(NotificationViewTest, TestImageSizing);
+  FRIEND_TEST_ALL_PREFIXES(NotificationViewTest, TitleWrappingTest);
   FRIEND_TEST_ALL_PREFIXES(NotificationViewTest, UpdateButtonsStateTest);
   FRIEND_TEST_ALL_PREFIXES(NotificationViewTest, UpdateButtonCountTest);
   FRIEND_TEST_ALL_PREFIXES(NotificationViewTest, UpdateViewsOrderingTest);

ui/message_center/views/notification_view_unittest.cc

@@ -22,6 +22,7 @@
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/image/image.h"
+#include "ui/gfx/text_utils.h"
 #include "ui/message_center/message_center.h"
 #include "ui/message_center/message_center_style.h"
 #include "ui/message_center/notification_list.h"
@@ -35,6 +36,7 @@
 #include "ui/message_center/views/padded_button.h"
 #include "ui/message_center/views/proportional_image_view.h"
 #include "ui/views/controls/button/image_button.h"
+#include "ui/views/controls/label.h"
 #include "ui/views/layout/fill_layout.h"
 #include "ui/views/test/views_test_base.h"
 #include "ui/views/test/widget_test.h"
@@ -167,6 +169,49 @@ class NotificationViewTest : public views::ViewsTestBase {
         .x();
   }
 
+  int GetTitleWidth() {
+    return notification_view()->title_view_->GetContentsBounds().width();
+  }
+
+  int GetTitleHeight() {
+    return notification_view()->title_view_->GetContentsBounds().height();
+  }
+
+  int GetTitleCharactersPerLine(base::char16 character) {
+    int available_width = GetTitleWidth();
+#if !defined(OS_CHROMEOS)
+    // On non-ChromeOS systems, we expect the available width to be reduced by
+    // the width of the control buttons view.
+    available_width -=
+        notification_view()->control_buttons_view_->GetPreferredSize().width();
+#endif
+    const gfx::FontList& font_list =
+        notification_view()->title_view_->font_list();
+
+    // To get the number of characters that fit into one line of text with a
+    // given width, we first get the width of one character.
+    int char_width =
+        gfx::GetStringWidth(base::string16(1, character), font_list);
+
+    // We then assume that multiple of these characters next to each other have
+    // a total width of N * char_width. This is usually a very good estimation,
+    // but based on the platform, it may vary due to font shaping.
+    int characters_per_line = available_width / char_width;
+
+    // These while loops account for any unexpected font shaping and are not
+    // expected to be expensive.
+    while (gfx::GetStringWidth(base::string16(characters_per_line, character),
+                               font_list) <= available_width) {
+      characters_per_line++;
+    }
+    while (gfx::GetStringWidth(base::string16(characters_per_line, character),
+                               font_list) > available_width) {
+      characters_per_line--;
+    }
+
+    return characters_per_line;
+  }
+
   bool IsRemovedAfterIdle(const std::string& notification_id) const {
     base::RunLoop().RunUntilIdle();
     return !MessageCenter::Get()->FindVisibleNotificationById(notification_id);
@@ -570,6 +615,50 @@ TEST_F(NotificationViewTest, ViewOrderingTest) {
   CheckVerticalOrderInNotification();
 }
 
+TEST_F(NotificationViewTest, TitleWrappingTest) {
+  data()->settings_button_handler = SettingsButtonHandler::INLINE;
+  Notification notf(
+      NOTIFICATION_TYPE_BASE_FORMAT, std::string("notification id"),
+      base::UTF8ToUTF16(""), base::UTF8ToUTF16("message"),
+      CreateTestImage(80, 80), base::UTF8ToUTF16("display source"),
+      GURL("https://hello.com"),
+      NotifierId(NotifierType::APPLICATION, "extension_id"), *data(), nullptr);
+
+  const base::char16 character = '1';
+  const int characters_per_line = GetTitleCharactersPerLine(character);
+
+  // Test a very short title
+  notf.set_title(base::string16(1, character));
+  notification_view()->UpdateWithNotification(notf);
+  int one_line_height = GetTitleHeight();
+
+  // Test a title that exactly fits into one line
+  notf.set_title(base::string16(characters_per_line, character));
+  notification_view()->UpdateWithNotification(notf);
+  EXPECT_EQ(one_line_height, GetTitleHeight());
+
+  // Test a title that breaks into 2 lines with only 1 char in the second line
+  notf.set_title(base::string16(characters_per_line + 1, character));
+  notification_view()->UpdateWithNotification(notf);
+  int two_line_height = GetTitleHeight();
+  EXPECT_GT(two_line_height, one_line_height);
+
+  // Test a title that clearly breaks into 2 lines
+  notf.set_title(base::string16(characters_per_line + 10, character));
+  notification_view()->UpdateWithNotification(notf);
+  EXPECT_EQ(two_line_height, GetTitleHeight());
+
+  // Test a title that fits exactly into 2 lines
+  notf.set_title(base::string16(characters_per_line * 2, character));
+  notification_view()->UpdateWithNotification(notf);
+  EXPECT_EQ(two_line_height, GetTitleHeight());
+
+  // Test a title that would break into 3 lines but has ellipsis in the 2nd line
+  notf.set_title(base::string16(characters_per_line * 2 + 1, character));
+  notification_view()->UpdateWithNotification(notf);
+  EXPECT_EQ(two_line_height, GetTitleHeight());
+}
+
 TEST_F(NotificationViewTest, FormatContextMessageTest) {
   const std::string kRegularContextText = "Context Text";
   const std::string kVeryLongContextText =