Make styled-label trimming less aggressive - now when wrapped on \n, only this...

Make styled-label trimming less aggressive - now when wrapped on \n, only this \n is removed, not all whitespace sequence. This allows whitespace
"formatting" like line indentation with spaces. Also makes double newline behave like expected - insert an empty line between text before and after \n\n.
Syled label layout loop refactored to de-duplicate code.
Added tests coverage for styled label.
Fixed LegalMessageLine comment - no longer have to explain that it is based on styled_label with some caveats.

R=sky@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2348433002
Cr-Commit-Position: refs/heads/master@{#438795}

components/autofill/core/browser/legal_message_line.h

@@ -63,9 +63,7 @@ class LegalMessageLine {
   //    expand correctly.
   // 3. "${" anywhere in the template string is invalid.
   // 4. "\n" embedded anywhere in the template string, or an empty template
-  //    string, can be used to separate paragraphs. It is not possible to create
-  //    a completely blank line by using two consecutive newlines (they will be
-  //    treated as a single newline by views::StyledLabel).
+  //    string, can be used to separate paragraphs.
   static bool Parse(const base::DictionaryValue& legal_message,
                     LegalMessageLines* out);
 

ui/views/controls/styled_label.cc

@@ -256,15 +256,24 @@ gfx::Size StyledLabel::CalculateAndDoLayout(int width, bool dry_run) {
   base::string16 remaining_string = text_;
   StyleRanges::const_iterator current_range = style_ranges_.begin();
 
+  bool first_loop_iteration = true;
+
   // Iterate over the text, creating a bunch of labels and links and laying them
   // out in the appropriate positions.
   while (!remaining_string.empty()) {
-    // Don't put whitespace at beginning of a line with an exception for the
-    // first line (so the text's leading whitespace is respected).
-    if (x == 0 && line > 0) {
-      base::TrimWhitespace(remaining_string, base::TRIM_LEADING,
-                           &remaining_string);
+    if (x == 0 && !first_loop_iteration) {
+      if (remaining_string.front() == L'\n') {
+        // Wrapped to the next line on \n, remove it. Other whitespace,
+        // eg, spaces to indent next line, are preserved.
+        remaining_string.erase(0, 1);
+      } else {
+        // Wrapped on whitespace character or characters in the middle of the
+        // line - none of them are needed at the beginning of the next line.
+        base::TrimWhitespace(remaining_string, base::TRIM_LEADING,
+                             &remaining_string);
+      }
     }
+    first_loop_iteration = false;
 
     gfx::Range range(gfx::Range::InvalidRange());
     if (current_range != style_ranges_.end())
@@ -290,22 +299,18 @@ gfx::Size StyledLabel::CalculateAndDoLayout(int width, bool dry_run) {
                             gfx::WRAP_LONG_WORDS,
                             &substrings);
 
-    if (substrings.empty() || substrings[0].empty()) {
+    if (substrings.empty()) {
+      // there is no room for anything; abort.
+      break;
+    }
+    if (substrings[0].empty()) {
+      x = 0;
       // Nothing fits on this line. Start a new line.
-      // If x is 0, first line may have leading whitespace that doesn't fit in a
-      // single line, so try trimming those. Otherwise there is no room for
-      // anything; abort.
-      if (x == 0) {
-        if (line == 0) {
-          base::TrimWhitespace(remaining_string, base::TRIM_LEADING,
-                               &remaining_string);
-          continue;
-        }
-        break;
+      // As for the first line, don't advance line number so that it will be
+      // handled again at the beginning of the loop.
+      if (line > 0) {
+        ++line;
       }
-
-      x = 0;
-      line++;
       continue;
     }
 
@@ -320,7 +325,7 @@ gfx::Size StyledLabel::CalculateAndDoLayout(int width, bool dry_run) {
         // If the chunk should not be wrapped, try to fit it entirely on the
         // next line.
         x = 0;
-        line++;
+        ++line;
         continue;
       }
 

ui/views/controls/styled_label_unittest.cc

@@ -89,6 +89,40 @@ TEST_F(StyledLabelTest, RespectLeadingWhitespace) {
             static_cast<Label*>(styled()->child_at(0))->text());
 }
 
+TEST_F(StyledLabelTest, RespectLeadingSpacesInNonFirstLine) {
+  const std::string indented_line = "  indented line";
+  const std::string text(std::string("First line\n") + indented_line);
+  InitStyledLabel(text);
+  styled()->SetBounds(0, 0, 1000, 1000);
+  styled()->Layout();
+  ASSERT_EQ(2, styled()->child_count());
+  ASSERT_EQ(std::string(Label::kViewClassName),
+            styled()->child_at(0)->GetClassName());
+  EXPECT_EQ(ASCIIToUTF16(indented_line),
+            static_cast<Label*>(styled()->child_at(1))->text());
+}
+
+TEST_F(StyledLabelTest, CorrectWrapAtNewline) {
+  const std::string first_line = "Line one";
+  const std::string second_line = "  two";
+  const std::string multiline_text(first_line + "\n" + second_line);
+  InitStyledLabel(multiline_text);
+  Label label(ASCIIToUTF16(first_line));
+  gfx::Size label_preferred_size = label.GetPreferredSize();
+  // Correct handling of \n and label width limit encountered at the same place
+  styled()->SetBounds(0, 0, label_preferred_size.width(), 1000);
+  styled()->Layout();
+  ASSERT_EQ(2, styled()->child_count());
+  ASSERT_EQ(std::string(Label::kViewClassName),
+            styled()->child_at(1)->GetClassName());
+  EXPECT_EQ(ASCIIToUTF16(first_line),
+            static_cast<Label*>(styled()->child_at(0))->text());
+  EXPECT_EQ(ASCIIToUTF16(second_line),
+            static_cast<Label*>(styled()->child_at(1))->text());
+  EXPECT_EQ(styled()->GetHeightForWidth(1000),
+            styled()->child_at(1)->bounds().bottom());
+}
+
 TEST_F(StyledLabelTest, FirstLineNotEmptyWhenLeadingWhitespaceTooLong) {
   const std::string text("                                     a");
   InitStyledLabel(text);
@@ -104,6 +138,8 @@ TEST_F(StyledLabelTest, FirstLineNotEmptyWhenLeadingWhitespaceTooLong) {
             styled()->child_at(0)->GetClassName());
   EXPECT_EQ(ASCIIToUTF16("a"),
             static_cast<Label*>(styled()->child_at(0))->text());
+  EXPECT_EQ(label_preferred_size.height(),
+            styled()->GetHeightForWidth(label_preferred_size.width() / 2));
 }
 
 TEST_F(StyledLabelTest, BasicWrapping) {
@@ -128,6 +164,20 @@ TEST_F(StyledLabelTest, BasicWrapping) {
   EXPECT_EQ(styled()->height() - 3, styled()->child_at(1)->bounds().bottom());
 }
 
+TEST_F(StyledLabelTest, AllowEmptyLines) {
+  const std::string text("one");
+  InitStyledLabel(text);
+  int default_height = styled()->GetHeightForWidth(1000);
+  const std::string multiline_text("one\n\nthree");
+  InitStyledLabel(multiline_text);
+  styled()->SetBounds(0, 0, 1000, 1000);
+  styled()->Layout();
+  EXPECT_EQ(3 * default_height, styled()->GetHeightForWidth(1000));
+  ASSERT_EQ(2, styled()->child_count());
+  EXPECT_EQ(styled()->GetHeightForWidth(1000),
+            styled()->child_at(1)->bounds().bottom());
+}
+
 TEST_F(StyledLabelTest, WrapLongWords) {
   const std::string text("ThisIsTextAsASingleWord");
   InitStyledLabel(text);