[LayoutNG] Fix NGInlineNode::MarkLineBoxesDirty

This patch fixes MarkLineBoxesDirty to mark correctly when:
1. The changed node has a parent inline box, and
2. is the first child of the parent, and
3. the parent has box fragment (not culled,) and
4. the parent wraps to multiple lines.

e.g.,
<span id="parent" style="background: yellow">
  <span id="changed"></span>
  <br>
  text
</span>

In this case, the old code marks the line box that contains
the last fragment of #parent because #parent is "previous"
of #changed in the pre-order DFS. The marked line box is the
one after #changed, and thus we try to reuse changed line.

This issue was found by a WIP to apply reusing line boxes in
more cases. From the stack, this looks the same as
crbug.com/900898.

Bug: 636993, 900898
Change-Id: I43d1d8150bfff7e1d9a6572be5a235cbe937caac
Reviewed-on: https://chromium-review.googlesource.com/c/1314086
Commit-Queue: Koji Ishii <kojii@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605422}

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

@@ -729,15 +729,26 @@ bool NGInlineNode::MarkLineBoxesDirty(LayoutBlockFlow* block_flow) {
   for (LayoutObject* layout_object = block_flow->NextInPreOrder(block_flow);
        layout_object;) {
     bool should_dirty_lines = false;
+    NGPaintFragment* fragment = nullptr;
     LayoutObject* next = nullptr;
-    if (layout_object->IsText()) {
-      should_dirty_lines =
-          !has_dirtied_lines && layout_object->SelfNeedsLayout();
+    if (LayoutText* layout_text = ToLayoutTextOrNull(layout_object)) {
+      if (!has_dirtied_lines) {
+        should_dirty_lines = layout_object->SelfNeedsLayout();
+        if (!should_dirty_lines)
+          fragment = layout_text->FirstInlineFragment();
+      }
       next = layout_object->NextInPreOrderAfterChildren(block_flow);
       layout_object->ClearNeedsLayout();
-    } else if (layout_object->IsLayoutInline()) {
-      should_dirty_lines =
-          !has_dirtied_lines && layout_object->SelfNeedsLayout();
+    } else if (LayoutInline* layout_inline =
+                   ToLayoutInlineOrNull(layout_object)) {
+      if (!has_dirtied_lines) {
+        should_dirty_lines = layout_object->SelfNeedsLayout();
+        // Do not keep fragments of LayoutInline unless it's a leaf, because
+        // the last fragment of LayoutInline is not the previous fragment of its
+        // descendants.
+        if (!should_dirty_lines && !layout_inline->FirstChild())
+          fragment = layout_inline->FirstInlineFragment();
+      }
       next = layout_object->NextInPreOrder(block_flow);
       layout_object->ClearNeedsLayout();
     } else if (UNLIKELY(layout_object->IsFloatingOrOutOfFlowPositioned())) {
@@ -751,7 +762,11 @@ bool NGInlineNode::MarkLineBoxesDirty(LayoutBlockFlow* block_flow) {
       // solution if any.
       return false;
     } else if (layout_object->IsAtomicInlineLevel()) {
-      should_dirty_lines = !has_dirtied_lines && layout_object->NeedsLayout();
+      if (!has_dirtied_lines) {
+        should_dirty_lines = layout_object->NeedsLayout();
+        if (!should_dirty_lines)
+          fragment = layout_object->FirstInlineFragment();
+      }
       next = layout_object->NextInPreOrderAfterChildren(block_flow);
     } else {
       NOTREACHED();
@@ -778,9 +793,8 @@ bool NGInlineNode::MarkLineBoxesDirty(LayoutBlockFlow* block_flow) {
             first_line->MarkLineBoxDirty();
         }
         has_dirtied_lines = true;
-      } else {
-        if (NGPaintFragment* fragment = layout_object->FirstInlineFragment())
-          last_fragment = fragment;
+      } else if (fragment) {
+        last_fragment = fragment;
       }
     }
 

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

@@ -830,4 +830,25 @@ TEST_F(NGInlineNodeTest, MarkLineBoxesDirtyOnNeedsLayoutWithBox) {
   EXPECT_TRUE(lines[1]->IsDirty());
 }
 
+// Test marking line boxes when a span inside a span has NeedsLayout.
+// The parent span has a box fragment, and wraps, so that its fragment
+// is seen earlier in pre-order DFS.
+TEST_F(NGInlineNodeTest, MarkLineBoxesDirtyOnChildOfWrappedBox) {
+  SetupHtml("container", R"HTML(
+    <div id=container style="font-size: 10px">
+      <span style="background: yellow">
+        <span id=t>target</span>
+        <br>
+        12345678
+      </span>
+    </div>
+  )HTML");
+
+  LayoutObject* span = GetLayoutObjectByElementId("t");
+  span->SetNeedsLayout("");
+
+  auto lines = MarkLineBoxesDirty();
+  EXPECT_TRUE(lines[0]->IsDirty());
+}
+
 }  // namespace blink

third_party/blink/renderer/core/paint/ng/ng_paint_fragment.cc

@@ -601,9 +601,6 @@ NGPaintFragment* NGPaintFragment::FirstLineBox() const {
 void NGPaintFragment::MarkLineBoxesDirtyFor(const LayoutObject& layout_object) {
   DCHECK(layout_object.IsInline()) << layout_object;
 
-  if (TryMarkFirstLineBoxDirtyFor(layout_object))
-    return;
-
   // Since |layout_object| isn't in fragment tree, check preceding siblings.
   // Note: Once we reuse lines below dirty lines, we should check next siblings.
   for (LayoutObject* previous = layout_object.PreviousSibling(); previous;

third_party/blink/renderer/core/paint/ng/ng_paint_fragment_test.cc

@@ -584,9 +584,10 @@ TEST_F(NGPaintFragmentTest, MarkLineBoxesDirtyByTextSetData) {
   Element& target = *GetDocument().getElementById("target");
   ToText(*target.firstChild()).setData("abc");
   const NGPaintFragment& container = *GetPaintFragmentByElementId("container");
-  EXPECT_FALSE(container.FirstChild()->IsDirty());
-  EXPECT_TRUE(ToList(container.Children())[1]->IsDirty());
-  EXPECT_FALSE(ToList(container.Children())[2]->IsDirty());
+  auto lines = ToList(container.Children());
+  // TODO(kojii): Currently we don't optimzie for <br>. We can do this, then
+  // lines[0] should not be dirty.
+  EXPECT_TRUE(lines[0]->IsDirty());
 }
 
 TEST_F(NGPaintFragmentTest, MarkLineBoxesDirtyWrappedLine) {