[LayoutNG] Restore collapsed space if non-space was inserted

This patch fixes to restore collapsed spaces when non-space
was inserted before. This is seen in LayoutTest results.html,
when it updates the number of failures, once collapsed space
needs to be restored.

Also, fixing this case ended up optimizing the re-using
NGInlineItem conditions further. Allowing more cases to re-
use hits hidden bugs. This CL includes fixes for them too.

For instance, the re-use is possible when preserving white-
spaces (e.g., <pre>) with this CL. It was almost always
determined as not re-usable.

Bug: 636993
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I1d1224812b4e92e6582e3112f73372577a332bc3
Reviewed-on: https://chromium-review.googlesource.com/1154242Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579053}

third_party/blink/renderer/core/layout/layout_menu_list.cc

@@ -91,11 +91,11 @@ void LayoutMenuList::CreateInnerBlock() {
   inner_block_ =
       LayoutBlockFlow::CreateAnonymous(&GetDocument(), CreateInnerStyle());
 
-  button_text_ = LayoutText::CreateEmptyAnonymous(GetDocument());
+  button_text_ =
+      LayoutText::CreateEmptyAnonymous(GetDocument(), MutableStyle());
   // We need to set the text explicitly though it was specified in the
   // constructor because LayoutText doesn't refer to the text
   // specified in the constructor in a case of re-transforming.
-  button_text_->SetStyle(MutableStyle());
   inner_block_->AddChild(button_text_);
   LayoutFlexibleBox::AddChild(inner_block_);
 

third_party/blink/renderer/core/layout/layout_text.cc

@@ -50,6 +50,7 @@
 #include "third_party/blink/renderer/core/layout/line/glyph_overflow.h"
 #include "third_party/blink/renderer/core/layout/line/inline_text_box.h"
 #include "third_party/blink/renderer/core/layout/ng/geometry/ng_logical_rect.h"
+#include "third_party/blink/renderer/core/layout/ng/inline/layout_ng_text.h"
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_abstract_inline_text_box.h"
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_inline_fragment_traversal.h"
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.h"
@@ -141,9 +142,15 @@ LayoutText::LayoutText(Node* node, scoped_refptr<StringImpl> str)
     GetFrameView()->IncrementVisuallyNonEmptyCharacterCount(text_.length());
 }
 
-LayoutText* LayoutText::CreateEmptyAnonymous(Document& doc) {
-  LayoutText* text = new LayoutText(nullptr, StringImpl::empty_);
+LayoutText* LayoutText::CreateEmptyAnonymous(
+    Document& doc,
+    scoped_refptr<ComputedStyle> style) {
+  LayoutText* text =
+      RuntimeEnabledFeatures::LayoutNGEnabled() && !style->ForceLegacyLayout()
+          ? new LayoutNGText(nullptr, StringImpl::empty_)
+          : new LayoutText(nullptr, StringImpl::empty_);
   text->SetDocumentForAnonymous(&doc);
+  text->SetStyle(std::move(style));
   return text;
 }
 

third_party/blink/renderer/core/layout/layout_text.h

@@ -77,7 +77,8 @@ class CORE_EXPORT LayoutText : public LayoutObject {
   // doesn't re-transform the string.
   LayoutText(Node*, scoped_refptr<StringImpl>);
 
-  static LayoutText* CreateEmptyAnonymous(Document&);
+  static LayoutText* CreateEmptyAnonymous(Document&,
+                                          scoped_refptr<ComputedStyle>);
 
   const char* GetName() const override { return "LayoutText"; }
 

third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder.cc

@@ -6,6 +6,7 @@
 
 #include "third_party/blink/renderer/core/layout/layout_object.h"
 #include "third_party/blink/renderer/core/layout/layout_text.h"
+#include "third_party/blink/renderer/core/layout/ng/inline/layout_ng_text.h"
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_offset_mapping_builder.h"
 #include "third_party/blink/renderer/core/style/computed_style.h"
 
@@ -193,8 +194,8 @@ NGInlineItem* LastItemToCollapseWith(Vector<NGInlineItem>* items) {
   return nullptr;
 }
 
-inline bool MayCollapseWithLast(const NGInlineItem* item) {
-  return item && item->EndMayCollapse();
+inline bool MayCollapseWithLast(const NGInlineItem& item) {
+  return item.EndMayCollapse();
 }
 
 }  // anonymous namespace
@@ -202,14 +203,37 @@ inline bool MayCollapseWithLast(const NGInlineItem* item) {
 template <typename OffsetMappingBuilder>
 bool NGInlineItemsBuilderTemplate<OffsetMappingBuilder>::Append(
     const String& original_string,
-    LayoutObject* layout_object,
+    LayoutNGText* layout_text,
     const Vector<NGInlineItem*>& items) {
   // Don't reuse existing items if they might be affected by whitespace
   // collapsing.
   // TODO(layout-dev): This could likely be optimized further.
   // TODO(layout-dev): Handle cases where the old items are not consecutive.
-  if (MayCollapseWithLast(LastItemToCollapseWith(items_)) ||
-      IsCollapsibleSpace(original_string[items[0]->StartOffset()]))
+  const ComputedStyle& new_style = layout_text->StyleRef();
+  const ComputedStyle& old_style = *items[0]->Style();
+  bool collapse_spaces = new_style.CollapseWhiteSpace();
+  if (collapse_spaces != old_style.CollapseWhiteSpace())
+    return false;
+
+  NGInlineItem* last_item = LastItemToCollapseWith(items_);
+  if (collapse_spaces) {
+    if (!last_item || MayCollapseWithLast(*last_item)) {
+      // If the original string starts with a collapsible space, it may be
+      // collapsed.
+      if (original_string[items[0]->StartOffset()] == kSpaceCharacter)
+        return false;
+    } else {
+      // If the start of the original string was collapsed, it may be
+      // restored.
+      const String& source_text = layout_text->GetText();
+      if (source_text.length() && IsCollapsibleSpace(source_text[0]) &&
+          original_string[items[0]->StartOffset()] != kSpaceCharacter)
+        return false;
+    }
+  }
+
+  // On nowrap -> wrap boundary, a break opporunity may be inserted.
+  if (last_item && !last_item->Style()->AutoWrap() && new_style.AutoWrap())
     return false;
 
   for (const NGInlineItem* item : items) {
@@ -227,16 +251,28 @@ bool NGInlineItemsBuilderTemplate<OffsetMappingBuilder>::Append(
     // If the position has shifted the item and the shape result needs to be
     // adjusted to reflect the new start and end offsets.
     unsigned end = start + item->Length();
-    DCHECK(item->TextShapeResult());
-    NGInlineItem adjusted_item(
-        *item, start, end, item->TextShapeResult()->CopyAdjustedOffset(start));
+    scoped_refptr<ShapeResult> adjusted_shape_result;
+    if (item->TextShapeResult()) {
+      DCHECK_EQ(item->Type(), NGInlineItem::kText);
+      adjusted_shape_result =
+          item->TextShapeResult()->CopyAdjustedOffset(start);
+      DCHECK(adjusted_shape_result);
+    } else {
+      // The following should be true, but some unit tests fail.
+      // DCHECK_EQ(item->Type(), NGInlineItem::kControl);
+    }
+    NGInlineItem adjusted_item(*item, start, end,
+                               std::move(adjusted_shape_result));
 
-    DCHECK(adjusted_item.TextShapeResult());
+#if DCHECK_IS_ON()
     DCHECK_EQ(start, adjusted_item.StartOffset());
-    DCHECK_EQ(start, adjusted_item.TextShapeResult()->StartIndexForResult());
     DCHECK_EQ(end, adjusted_item.EndOffset());
-    DCHECK_EQ(end, adjusted_item.TextShapeResult()->EndIndexForResult());
+    if (adjusted_item.TextShapeResult()) {
+      DCHECK_EQ(start, adjusted_item.TextShapeResult()->StartIndexForResult());
+      DCHECK_EQ(end, adjusted_item.TextShapeResult()->EndIndexForResult());
+    }
     DCHECK_EQ(item->IsEmptyItem(), adjusted_item.IsEmptyItem());
+#endif
 
     items_->push_back(adjusted_item);
     is_empty_inline_ &= adjusted_item.IsEmptyItem();
@@ -247,7 +283,7 @@ bool NGInlineItemsBuilderTemplate<OffsetMappingBuilder>::Append(
 template <>
 bool NGInlineItemsBuilderTemplate<NGOffsetMappingBuilder>::Append(
     const String&,
-    LayoutObject*,
+    LayoutNGText*,
     const Vector<NGInlineItem*>&) {
   NOTREACHED();
   return false;

third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder.h

@@ -17,6 +17,7 @@
 namespace blink {
 
 class ComputedStyle;
+class LayoutNGText;
 class LayoutObject;
 class LayoutText;
 
@@ -57,7 +58,7 @@ class NGInlineItemsBuilderTemplate {
   // Returns whether the existing items could be reused.
   // NOTE: The state of the builder remains unchanged if the append operation
   // fails (i.e. if it returns false).
-  bool Append(const String&, LayoutObject*, const Vector<NGInlineItem*>&);
+  bool Append(const String&, LayoutNGText*, const Vector<NGInlineItem*>&);
 
   // Append a string.
   // When appending, spaces are collapsed according to CSS Text, The white space
@@ -163,7 +164,7 @@ NGInlineItemsBuilderTemplate<NGOffsetMappingBuilder>::ToString();
 template <>
 CORE_EXPORT bool NGInlineItemsBuilderTemplate<NGOffsetMappingBuilder>::Append(
     const String&,
-    LayoutObject*,
+    LayoutNGText*,
     const Vector<NGInlineItem*>&);
 
 extern template class CORE_EXTERN_TEMPLATE_EXPORT

third_party/blink/renderer/core/layout/ng/inline/ng_inline_items_builder_test.cc

@@ -6,9 +6,10 @@
 
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/renderer/core/layout/layout_inline.h"
+#include "third_party/blink/renderer/core/layout/ng/inline/layout_ng_text.h"
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_offset_mapping_builder.h"
+#include "third_party/blink/renderer/core/layout/ng/ng_layout_test.h"
 #include "third_party/blink/renderer/core/style/computed_style.h"
-#include "third_party/blink/renderer/core/testing/page_test_base.h"
 
 namespace blink {
 
@@ -41,10 +42,10 @@ static String GetCollapsed(const NGOffsetMappingBuilder& builder) {
   return result.ToString();
 }
 
-class NGInlineItemsBuilderTest : public PageTestBase {
+class NGInlineItemsBuilderTest : public NGLayoutTest {
  protected:
   void SetUp() override {
-    PageTestBase::SetUp();
+    NGLayoutTest::SetUp();
     style_ = ComputedStyle::Create();
   }
 
@@ -72,11 +73,11 @@ class NGInlineItemsBuilderTest : public PageTestBase {
     NGInlineItemsBuilderForOffsetMapping builder(&items_);
     for (Input& input : inputs) {
       if (!input.layout_text) {
-        input.layout_text = LayoutText::CreateEmptyAnonymous(GetDocument());
+        input.layout_text = LayoutText::CreateEmptyAnonymous(
+            GetDocument(), GetStyle(input.whitespace));
         anonymous_objects.push_back(input.layout_text);
       }
-      builder.Append(input.text, GetStyle(input.whitespace).get(),
-                     input.layout_text);
+      builder.Append(input.text, input.layout_text->Style(), input.layout_text);
     }
     text_ = builder.ToString();
     collapsed_ = GetCollapsed(builder.GetOffsetMappingBuilder());
@@ -126,10 +127,12 @@ class NGInlineItemsBuilderTest : public PageTestBase {
       }
 
       // Try to re-use previous items, or Append if it was not re-usable.
-      bool reused = !previous_items.IsEmpty() &&
-                    reuse_builder.Append(text_, nullptr, previous_items);
+      bool reused =
+          !previous_items.IsEmpty() &&
+          reuse_builder.Append(text_, ToLayoutNGText(input.layout_text),
+                               previous_items);
       if (!reused)
-        reuse_builder.Append(input.text, style_.get());
+        reuse_builder.Append(input.text, input.layout_text->Style());
     }
 
     // Currently, NGInlineItemsBuilder does not strip trailing spaces while

third_party/blink/renderer/core/layout/ng/inline/ng_inline_node_test.cc

@@ -5,6 +5,7 @@
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_inline_node.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/renderer/core/dom/text.h"
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_inline_layout_algorithm.h"
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_physical_line_box_fragment.h"
 #include "third_party/blink/renderer/core/layout/ng/inline/ng_physical_text_fragment.h"
@@ -116,6 +117,12 @@ class NGInlineNodeTest : public NGLayoutTest {
     }
   }
 
+  const String& GetText() const {
+    NGInlineNodeData* data = layout_block_flow_->GetNGInlineNodeData();
+    CHECK(data);
+    return data->text_content;
+  }
+
   Vector<NGInlineItem>& Items() {
     NGInlineNodeData* data = layout_block_flow_->GetNGInlineNodeData();
     CHECK(data);
@@ -615,4 +622,19 @@ TEST_F(NGInlineNodeTest, InvalidateRemoveFloat) {
   EXPECT_TRUE(layout_block_flow_->NeedsCollectInlines());
 }
 
+TEST_F(NGInlineNodeTest, SpaceRestoredByInsertingWord) {
+  SetupHtml("t", "<div id=t>before <span id=x></span> after</div>");
+  EXPECT_FALSE(layout_block_flow_->NeedsCollectInlines());
+  EXPECT_EQ(String("before after"), GetText());
+
+  Element* span = GetElementById("x");
+  ASSERT_TRUE(span);
+  Text* text = Text::Create(GetDocument(), "mid");
+  span->appendChild(text);
+  // EXPECT_TRUE(layout_block_flow_->NeedsCollectInlines());
+
+  ForceLayout();
+  EXPECT_EQ(String("before mid after"), GetText());
+}
+
 }  // namespace blink

third_party/blink/renderer/core/layout/ng/list/layout_ng_list_item.cc

@@ -293,8 +293,8 @@ void LayoutNGListItem::UpdateMarkerContentIfNeeded() {
       }
     }
     if (!child) {
-      text = LayoutText::CreateEmptyAnonymous(GetDocument());
-      text->SetStyle(marker_->MutableStyle());
+      text = LayoutText::CreateEmptyAnonymous(GetDocument(),
+                                              marker_->MutableStyle());
       marker_->AddChild(text);
       is_marker_text_updated_ = false;
     }