Handle properly CR character in RenderText

This CL is handling the \r character in RenderText.

The sequence "\r\n" is a grapheme and is a processed as
a newline by RenderText.

Bug: 1050607, 1019061
Change-Id: I161d1e08fe65225f78556b861847dd1b1ee22230
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2051972
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#741136}

ui/gfx/render_text.cc

@@ -207,8 +207,7 @@ typename BreakList<T>::const_iterator IncrementBreakListIteratorToPosition(
 
 // Replaces the unicode control characters, control characters and PUA (Private
 // Use Areas) codepoints.
-UChar32 ReplaceControlCharacter(bool multiline,
-                                UChar32 codepoint) {
+UChar32 ReplaceControlCharacter(UChar32 codepoint) {
   // 'REPLACEMENT CHARACTER' used to replace an unknown,
   // unrecognized or unrepresentable character.
   constexpr base::char16 kReplacementCodepoint = 0xFFFD;
@@ -217,17 +216,15 @@ UChar32 ReplaceControlCharacter(bool multiline,
   constexpr base::char16 kSymbolsCodepoint = 0x2400;
 
   if (codepoint >= 0 && codepoint <= 0x1F) {
-    // The newline character should be kept as-is when
-    // rendertext is multiline.
-    if (codepoint != '\n' || !multiline) {
-      // Replace codepoints with their visual symbols, which are
-      // at the same offset from kSymbolsCodepoint.
-      return kSymbolsCodepoint + codepoint;
-    }
-  } else if (codepoint == 0x7F) {
+    // Replace codepoints with their visual symbols, which are
+    // at the same offset from kSymbolsCodepoint.
+    return kSymbolsCodepoint + codepoint;
+  }
+  if (codepoint == 0x7F) {
     // Replace the 'del' codepoint by its symbol (u2421).
     return kSymbolsCodepoint + 0x21;
-  } else if (!U_IS_UNICODE_CHAR(codepoint)) {
+  }
+  if (!U_IS_UNICODE_CHAR(codepoint)) {
     // Unicode codepoint that can't be assigned a character.
     // This handles:
     // - single surrogate codepoints,
@@ -235,7 +232,8 @@ UChar32 ReplaceControlCharacter(bool multiline,
     // - invalid characters (e.g. u+fdd0..u+fdef)
     // - codepoints above u+10ffff
     return kReplacementCodepoint;
-  } else if (codepoint > 0x7F) {
+  }
+  if (codepoint > 0x7F) {
     // Private use codepoints are working with a pair of font
     // and codepoint, but they are not used in Chrome.
     const int8_t codepoint_category = u_charType(codepoint);
@@ -1285,8 +1283,11 @@ bool RenderText::IsNewlineSegment(const internal::LineSegment& segment) const {
 
 bool RenderText::IsNewlineSegment(const base::string16& text,
                                   const internal::LineSegment& segment) const {
-  DCHECK_LT(segment.char_range.start(), text.length());
-  return text[segment.char_range.start()] == '\n';
+  const size_t offset = segment.char_range.start();
+  const size_t length = segment.char_range.length();
+  DCHECK_LT(offset + length - 1, text.length());
+  return (length == 1 && (text[offset] == '\r' || text[offset] == '\n')) ||
+         (length == 2 && text[offset] == '\r' && text[offset + 1] == '\n');
 }
 
 Range RenderText::GetLineRange(const base::string16& text,
@@ -1501,12 +1502,18 @@ void RenderText::EnsureLayoutTextUpdated() const {
                                             layout_grapheme_start_position};
     text_to_display_indices_.push_back(mapping);
 
+    // Flag telling if the current grapheme is a newline control sequence.
+    const bool is_newline_grapheme =
+        (grapheme_codepoints.size() == 1 &&
+         (grapheme_codepoints[0] == '\r' || grapheme_codepoints[0] == '\n')) ||
+        (grapheme_codepoints.size() == 2 && grapheme_codepoints[0] == '\r' &&
+         grapheme_codepoints[1] == '\n');
+
     // Obscure the layout text by replacing the grapheme by a bullet.
     if (obscured_ &&
         (reveal_index < text_grapheme_start_position ||
          reveal_index >= text_grapheme_end_position) &&
-        (grapheme_codepoints.size() != 1 || grapheme_codepoints[0] != '\n' ||
-         !multiline_)) {
+        (!is_newline_grapheme || !multiline_)) {
       grapheme_codepoints.clear();
       grapheme_codepoints.push_back(RenderText::kPasswordReplacementChar);
     }
@@ -1514,8 +1521,10 @@ void RenderText::EnsureLayoutTextUpdated() const {
     // Rewrite each codepoint of the grapheme.
     for (uint32_t codepoint : grapheme_codepoints) {
       // Handle unicode control characters ISO 6429 (block C0). Range from 0 to
-      // 0x1F and 0x7F.
-      codepoint = ReplaceControlCharacter(multiline_, codepoint);
+      // 0x1F and 0x7F. The newline character should be kept as-is when
+      // rendertext is multiline.
+      if (!multiline_ || !is_newline_grapheme)
+        codepoint = ReplaceControlCharacter(codepoint);
 
       // Truncate the remaining codepoints if appending the codepoint to
       // |layout_text_| is making the text larger than |truncate_length_|.

ui/gfx/render_text_harfbuzz.cc

@@ -336,7 +336,11 @@ inline hb_script_t ICUScriptToHBScript(UScriptCode script) {
 // Whether |segment| corresponds to the newline character.
 bool IsNewlineSegment(const base::string16& text,
                       const internal::LineSegment& segment) {
-  return text[segment.char_range.start()] == '\n';
+  const size_t offset = segment.char_range.start();
+  const size_t length = segment.char_range.length();
+  DCHECK_LT(segment.char_range.start() + length - 1, text.length());
+  return (length == 1 && (text[offset] == '\r' || text[offset] == '\n')) ||
+         (length == 2 && text[offset] == '\r' && text[offset + 1] == '\n');
 }
 
 // Returns the line index considering the newline character. Line index is
@@ -1942,7 +1946,10 @@ void RenderTextHarfBuzz::ShapeRuns(
   // font fallbacks before reporting a missing glyph (see http://crbug/972090).
   std::vector<internal::TextRunHarfBuzz*> need_shaping_runs;
   for (internal::TextRunHarfBuzz*& run : runs) {
-    if (run->range.length() == 1 && text[run->range.start()] == '\n') {
+    if ((run->range.length() == 1 && (text[run->range.start()] == '\r' ||
+                                      text[run->range.start()] == '\n')) ||
+        (run->range.length() == 2 && text[run->range.start()] == '\r' &&
+         text[run->range.start() + 1] == '\n')) {
       // Newline runs can't be shaped. Shape this run as if the glyph is
       // missing.
       run->font_params = font_params;

ui/gfx/render_text_unittest.cc

@@ -1185,6 +1185,20 @@ TEST_F(RenderTextTest, ObscuredTextMultiline) {
   EXPECT_EQ(display_text[4], '\n');
 }
 
+TEST_F(RenderTextTest, ObscuredTextMultilineNewline) {
+  const base::string16 test = UTF8ToUTF16("\r\r\n");
+  RenderText* render_text = GetRenderText();
+  render_text->SetText(test);
+  render_text->SetObscured(true);
+  render_text->SetMultiline(true);
+
+  // Newlines should be kept in multiline mode.
+  base::string16 display_text = render_text->GetDisplayText();
+  EXPECT_EQ(display_text[0], '\r');
+  EXPECT_EQ(display_text[1], '\r');
+  EXPECT_EQ(display_text[2], '\n');
+}
+
 TEST_F(RenderTextTest, RevealObscuredText) {
   const base::string16 seuss = UTF8ToUTF16("hop on pop");
   const base::string16 no_seuss = GetObscuredString(seuss.length());
@@ -1421,6 +1435,7 @@ struct RunListCase {
   const char* test_name;
   const wchar_t* text;
   const char* expected;
+  const bool multiline = false;
 };
 
 class RenderTextTestWithRunListCase
@@ -1436,6 +1451,7 @@ class RenderTextTestWithRunListCase
 TEST_P(RenderTextTestWithRunListCase, ItemizeTextToRuns) {
   RunListCase param = GetParam();
   RenderTextHarfBuzz* render_text = GetRenderText();
+  render_text->SetMultiline(param.multiline);
   render_text->SetText(WideToUTF16(param.text));
   EXPECT_EQ(param.expected, GetRunListStructureString());
 }
@@ -1463,6 +1479,12 @@ const RunListCase kBasicsRunListCases[] = {
      "[0][1][2][3][4]"},  // http://crbug.com/396776
     {"jap_paren2", L"國哲(c)1",
      "[0->1][2][3][4][5]"},  // http://crbug.com/125792
+    {"newline1", L"\n\n", "[0->1]"},
+    {"newline2", L"\r\n\r\n", "[0->3]"},
+    {"newline3", L"\r\r\n", "[0->2]"},
+    {"multiline_newline1", L"\n\n", "[0][1]", true},
+    {"multiline_newline2", L"\r\n\r\n", "[0->1][2->3]", true},
+    {"multiline_newline3", L"\r\r\n", "[0][1->2]", true},
 };
 
 INSTANTIATE_TEST_SUITE_P(ItemizeTextToRunsBasics,
@@ -5403,7 +5425,7 @@ TEST_F(RenderTextTest, ControlCharacterReplacement) {
 
   // Setting multiline, the newline character will be back to the original text.
   render_text->SetMultiline(true);
-  EXPECT_EQ(WideToUTF16(L"␈␍␇␉\n␋␌"), render_text->GetDisplayText());
+  EXPECT_EQ(WideToUTF16(L"␈\r␇␉\n␋␌"), render_text->GetDisplayText());
 
   // The generic control characters should have been replaced by the replacement
   // codepoints.