[LayoutNG] Fix whitespace collapsing when a node is a newline

This patch fixes to handle a text node that contains only a collapsible
newline.

Before this change, collapsible newlines were lazily appended, when
following text is appended. When a text node contains only collapsible
newlines, NGLayoutInlineItemsBuilder does not keep the node and thus
unable to append lazily.

This patch changes to append pending collapsible newlines first, then
remove later if it needed to be collapsed.

BUG=636993

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

third_party/WebKit/Source/core/layout/ng/ng_layout_inline_items_builder.cc

@@ -22,7 +22,7 @@ String NGLayoutInlineItemsBuilder::ToString() {
   // [1] https://drafts.csswg.org/css-text-3/#line-break-transform
   // [2] https://drafts.csswg.org/css-text-3/#white-space-phase-2
   unsigned next_start_offset = text_.length();
-  RemoveTrailingCollapsibleSpace(&next_start_offset);
+  RemoveTrailingCollapsibleSpaceIfExists(&next_start_offset);
 
   return text_.toString();
 }
@@ -55,8 +55,9 @@ static bool ShouldRemoveNewlineSlow(const StringBuilder& before,
                                     const ComputedStyle* after_style) {
   // Remove if either before/after the newline is zeroWidthSpaceCharacter.
   UChar32 last = 0;
-  if (!before.isEmpty()) {
-    last = before[before.length() - 1];
+  DCHECK(!before.isEmpty() && before[before.length() - 1] == ' ');
+  if (before.length() >= 2) {
+    last = before[before.length() - 2];
     if (last == zeroWidthSpaceCharacter)
       return true;
   }
@@ -111,15 +112,17 @@ static void AppendItem(Vector<NGLayoutInlineItem>* items,
   items->push_back(NGLayoutInlineItem(start, end, style, layout_object));
 }
 
+static inline bool IsCollapsibleSpace(UChar c, bool preserve_newline) {
+  return c == spaceCharacter || c == tabulationCharacter ||
+         (!preserve_newline && c == newlineCharacter);
+}
+
 void NGLayoutInlineItemsBuilder::Append(const String& string,
                                         const ComputedStyle* style,
                                         LayoutObject* layout_object) {
   if (string.isEmpty())
     return;
 
-  if (has_pending_newline_)
-    ProcessPendingNewline(string, style);
-
   EWhiteSpace whitespace = style->whiteSpace();
   bool preserve_newline =
       ComputedStyle::preserveNewline(whitespace) && !is_svgtext_;
@@ -128,38 +131,49 @@ void NGLayoutInlineItemsBuilder::Append(const String& string,
 
   if (!collapse_whitespace) {
     text_.append(string);
-    is_last_collapsible_space_ = false;
+    last_collapsible_space_ = CollapsibleSpace::None;
   } else {
     text_.reserveCapacity(string.length());
-    for (unsigned i = 0; i < string.length(); i++) {
+    for (unsigned i = 0; i < string.length();) {
       UChar c = string[i];
-      bool is_collapsible_space;
       if (c == newlineCharacter) {
-        RemoveTrailingCollapsibleSpace(&start_offset);
         if (preserve_newline) {
+          RemoveTrailingCollapsibleSpaceIfExists(&start_offset);
           text_.append(c);
           // Remove collapsible spaces immediately following a newline.
-          is_last_collapsible_space_ = true;
+          last_collapsible_space_ = CollapsibleSpace::Space;
+          i++;
           continue;
         }
-        if (i + 1 == string.length()) {
-          // If at the end of string, process this newline on the next Append.
-          has_pending_newline_ = true;
-          break;
+
+        if (last_collapsible_space_ == CollapsibleSpace::None)
+          text_.append(spaceCharacter);
+        last_collapsible_space_ = CollapsibleSpace::Newline;
+        i++;
+        continue;
+      }
+
+      if (c == spaceCharacter || c == tabulationCharacter) {
+        if (last_collapsible_space_ == CollapsibleSpace::None) {
+          text_.append(spaceCharacter);
+          last_collapsible_space_ = CollapsibleSpace::Space;
         }
-        if (ShouldRemoveNewline(text_, style, string, i + 1, style))
-          continue;
-        is_collapsible_space = true;
-      } else {
-        is_collapsible_space = c == spaceCharacter || c == tabulationCharacter;
+        i++;
+        continue;
+      }
+
+      if (last_collapsible_space_ == CollapsibleSpace::Newline) {
+        RemoveTrailingCollapsibleNewlineIfNeeded(&start_offset, string, i,
+                                                 style);
       }
-      if (!is_collapsible_space) {
-        text_.append(c);
-        is_last_collapsible_space_ = false;
-      } else if (!is_last_collapsible_space_) {
-        text_.append(spaceCharacter);
-        is_last_collapsible_space_ = true;
+
+      unsigned start_of_non_space = i;
+      for (i++; i < string.length(); i++) {
+        if (IsCollapsibleSpace(string[i], false))
+          break;
       }
+      text_.append(string, start_of_non_space, i - start_of_non_space);
+      last_collapsible_space_ = CollapsibleSpace::None;
     }
   }
 
@@ -172,49 +186,49 @@ void NGLayoutInlineItemsBuilder::Append(UChar character,
                                         LayoutObject* layout_object) {
   DCHECK(character != spaceCharacter && character != tabulationCharacter &&
          character != newlineCharacter && character != zeroWidthSpaceCharacter);
-  if (has_pending_newline_)
-    ProcessPendingNewline(emptyString, nullptr);
 
   text_.append(character);
   unsigned end_offset = text_.length();
   AppendItem(items_, end_offset - 1, end_offset, style, layout_object);
-  is_last_collapsible_space_ = false;
+  last_collapsible_space_ = CollapsibleSpace::None;
 }
 
-void NGLayoutInlineItemsBuilder::AppendAsOpaqueToSpaceCollapsing(
-    UChar character) {
-  if (has_pending_newline_)
-    ProcessPendingNewline(emptyString, nullptr);
+void NGLayoutInlineItemsBuilder::RemoveTrailingCollapsibleNewlineIfNeeded(
+    unsigned* next_start_offset,
+    const String& after,
+    unsigned after_index,
+    const ComputedStyle* after_style) {
+  DCHECK_EQ(last_collapsible_space_, CollapsibleSpace::Newline);
 
-  text_.append(character);
-  unsigned end_offset = text_.length();
-  AppendItem(items_, end_offset - 1, end_offset, nullptr);
-}
+  if (text_.isEmpty() || text_[text_.length() - 1] != spaceCharacter)
+    return;
 
-void NGLayoutInlineItemsBuilder::ProcessPendingNewline(
-    const String& string,
-    const ComputedStyle* style) {
-  DCHECK(has_pending_newline_);
+  const ComputedStyle* before_style = after_style;
   if (!items_->isEmpty()) {
     NGLayoutInlineItem& item = items_->back();
-    if (!ShouldRemoveNewline(text_, item.Style(), string, 0, style)) {
-      text_.append(spaceCharacter);
-      item.SetEndOffset(text_.length());
-    }
+    if (text_.length() < item.EndOffset() + 2)
+      before_style = item.Style();
   }
-  // Remove spaces following a newline even when the newline was removed.
-  is_last_collapsible_space_ = true;
-  has_pending_newline_ = false;
+
+  if (ShouldRemoveNewline(text_, before_style, after, after_index, after_style))
+    RemoveTrailingCollapsibleSpace(next_start_offset);
+}
+
+void NGLayoutInlineItemsBuilder::RemoveTrailingCollapsibleSpaceIfExists(
+    unsigned* next_start_offset) {
+  if (last_collapsible_space_ != CollapsibleSpace::None && !text_.isEmpty() &&
+      text_[text_.length() - 1] == spaceCharacter)
+    RemoveTrailingCollapsibleSpace(next_start_offset);
 }
 
 void NGLayoutInlineItemsBuilder::RemoveTrailingCollapsibleSpace(
     unsigned* next_start_offset) {
-  if (!is_last_collapsible_space_ || text_.isEmpty())
-    return;
-  DCHECK_EQ(spaceCharacter, text_[text_.length() - 1]);
+  DCHECK_NE(last_collapsible_space_, CollapsibleSpace::None);
+  DCHECK(!text_.isEmpty() && text_[text_.length() - 1] == spaceCharacter);
+
   unsigned new_size = text_.length() - 1;
   text_.resize(new_size);
-  is_last_collapsible_space_ = false;
+  last_collapsible_space_ = CollapsibleSpace::None;
 
   // Adjust the last item if the removed space is already appended.
   if (*next_start_offset > new_size) {
@@ -233,8 +247,7 @@ void NGLayoutInlineItemsBuilder::RemoveTrailingCollapsibleSpace(
 void NGLayoutInlineItemsBuilder::AppendBidiControl(const ComputedStyle* style,
                                                    UChar ltr,
                                                    UChar rtl) {
-  AppendAsOpaqueToSpaceCollapsing(
-      style->direction() == TextDirection::kRtl ? rtl : ltr);
+  Append(style->direction() == TextDirection::kRtl ? rtl : ltr);
 }
 
 void NGLayoutInlineItemsBuilder::EnterBlock(const ComputedStyle* style) {
@@ -273,11 +286,11 @@ void NGLayoutInlineItemsBuilder::EnterInline(LayoutObject* node) {
       Enter(node, popDirectionalIsolateCharacter);
       break;
     case UnicodeBidi::kPlaintext:
-      AppendAsOpaqueToSpaceCollapsing(firstStrongIsolateCharacter);
+      Append(firstStrongIsolateCharacter);
       Enter(node, popDirectionalIsolateCharacter);
       break;
     case UnicodeBidi::kIsolateOverride:
-      AppendAsOpaqueToSpaceCollapsing(firstStrongIsolateCharacter);
+      Append(firstStrongIsolateCharacter);
       AppendBidiControl(style, leftToRightOverrideCharacter,
                         rightToLeftOverrideCharacter);
       Enter(node, popDirectionalIsolateCharacter);
@@ -303,7 +316,7 @@ void NGLayoutInlineItemsBuilder::ExitInline(LayoutObject* node) {
 
 void NGLayoutInlineItemsBuilder::Exit(LayoutObject* node) {
   while (!exits_.isEmpty() && exits_.back().node == node) {
-    AppendAsOpaqueToSpaceCollapsing(exits_.back().character);
+    Append(exits_.back().character);
     exits_.pop_back();
   }
 }

third_party/WebKit/Source/core/layout/ng/ng_layout_inline_items_builder.h

@@ -61,11 +61,7 @@ class CORE_EXPORT NGLayoutInlineItemsBuilder {
   // LayoutObject.
   void Append(UChar, const ComputedStyle* = nullptr, LayoutObject* = nullptr);
 
-  // Append a character.
-  // The character is opaque to space collapsing that spaces before this
-  // character and after this character can collapse as if this character does
-  // not exist.
-  void AppendAsOpaqueToSpaceCollapsing(UChar);
+  // Append a Bidi control character, for LTR or RTL depends on the style.
   void AppendBidiControl(const ComputedStyle*, UChar ltr, UChar rtl);
 
   void EnterBlock(const ComputedStyle*);
@@ -83,8 +79,9 @@ class CORE_EXPORT NGLayoutInlineItemsBuilder {
   } OnExitNode;
   Vector<OnExitNode> exits_;
 
-  bool is_last_collapsible_space_ = true;
-  bool has_pending_newline_ = false;
+  enum class CollapsibleSpace { None, Space, Newline };
+
+  CollapsibleSpace last_collapsible_space_ = CollapsibleSpace::Space;
   bool is_svgtext_ = false;
   bool has_bidi_controls_ = false;
 
@@ -95,8 +92,14 @@ class CORE_EXPORT NGLayoutInlineItemsBuilder {
   void ProcessPendingNewline(const String&, const ComputedStyle*);
 
   // Removes the collapsible space at the end of |text_| if exists.
+  void RemoveTrailingCollapsibleSpaceIfExists(unsigned*);
   void RemoveTrailingCollapsibleSpace(unsigned*);
 
+  void RemoveTrailingCollapsibleNewlineIfNeeded(unsigned*,
+                                                const String&,
+                                                unsigned,
+                                                const ComputedStyle*);
+
   void Enter(LayoutObject*, UChar);
   void Exit(LayoutObject*);
 };

third_party/WebKit/Source/core/layout/ng/ng_layout_inline_items_builder_test.cc

@@ -97,15 +97,22 @@ TEST_F(NGLayoutInlineItemsBuilderTest, CollapseTabs) {
 }
 
 TEST_F(NGLayoutInlineItemsBuilderTest, CollapseNewLines) {
-  String input("text\ntext \n text");
-  String collapsed("text text text");
+  String input("text\ntext \n text\n\ntext");
+  String collapsed("text text text text");
   TestWhitespaceValue(collapsed, input, EWhiteSpace::kNormal);
   TestWhitespaceValue(collapsed, input, EWhiteSpace::kNowrap);
-  TestWhitespaceValue("text\ntext\ntext", input, EWhiteSpace::kPreLine);
+  TestWhitespaceValue("text\ntext\ntext\n\ntext", input, EWhiteSpace::kPreLine);
   TestWhitespaceValue(input, input, EWhiteSpace::kPre);
   TestWhitespaceValue(input, input, EWhiteSpace::kPreWrap);
 }
 
+TEST_F(NGLayoutInlineItemsBuilderTest, CollapseNewlinesAsSpaces) {
+  EXPECT_EQ("text text", TestAppend("text\ntext"));
+  EXPECT_EQ("text text", TestAppend("text\n\ntext"));
+  EXPECT_EQ("text text", TestAppend("text \n\n text"));
+  EXPECT_EQ("text text", TestAppend("text \n \n text"));
+}
+
 TEST_F(NGLayoutInlineItemsBuilderTest, CollapseAcrossElements) {
   EXPECT_EQ("text text", TestAppend("text ", " text"))
       << "Spaces are collapsed even when across elements.";
@@ -175,9 +182,6 @@ TEST_F(NGLayoutInlineItemsBuilderTest,
 }
 
 TEST_F(NGLayoutInlineItemsBuilderTest, CollapseZeroWidthSpaces) {
-  EXPECT_EQ("text text", TestAppend("text\ntext"))
-      << "Newline is converted to a space.";
-
   EXPECT_EQ(String(u"text\u200Btext"), TestAppend(u"text\u200B\ntext"))
       << "Newline is removed if the character before is ZWS.";
   EXPECT_EQ(String(u"text\u200Btext"), TestAppend(u"text\n\u200Btext"))
@@ -224,12 +228,16 @@ TEST_F(NGLayoutInlineItemsBuilderTest, CollapseAroundReplacedElement) {
   EXPECT_EQ(String(u"Hello \uFFFC World"), builder.ToString());
 }
 
-TEST_F(NGLayoutInlineItemsBuilderTest, AppendAsOpaqueToSpaceCollapsing) {
+TEST_F(NGLayoutInlineItemsBuilderTest, CollapseNewlineAfterObject) {
   NGLayoutInlineItemsBuilder builder(&items_);
-  builder.Append("Hello ", style_.get());
-  builder.AppendAsOpaqueToSpaceCollapsing(firstStrongIsolateCharacter);
-  builder.Append(" World", style_.get());
-  EXPECT_EQ(String(u"Hello \u2068World"), builder.ToString());
+  builder.Append(objectReplacementCharacter);
+  builder.Append("\n", style_.get());
+  builder.Append(objectReplacementCharacter);
+  EXPECT_EQ(String(u"\uFFFC \uFFFC"), builder.ToString());
+  EXPECT_EQ(3u, items_.size());
+  EXPECT_EQ(nullptr, items_[0].Style());
+  EXPECT_EQ(style_.get(), items_[1].Style());
+  EXPECT_EQ(nullptr, items_[2].Style());
 }
 
 TEST_F(NGLayoutInlineItemsBuilderTest, AppendEmptyString) {