[layout] Stop reusing ShapeResult ends with ligature

This patch changes `NeedsShapeText()` to return false when `ShapeResult`
ends with ligature spans into multiple `Text` node to paint corret glyph
after text change, e.g.
    <div>abf<span>i</span><div>, we may have a ligature "fi".
change to
    <div>abf<div>

This patch also changes `NGInlineItem` and `NGInlineNode::ShapeText()`
to record whether `ShapeResult` ends with ligature or not.

Bug: 1409702
Change-Id: Id9f497f1af4c92100faf984c30f93db9f19621f8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4206952
Commit-Queue: Yoshifumi Inoue <yosin@chromium.org>
Auto-Submit: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1099639}

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

@@ -86,7 +86,8 @@ NGInlineItem::NGInlineItem(NGInlineItemType type,
       is_empty_item_(false),
       is_block_level_(false),
       is_end_collapsible_newline_(false),
-      is_generated_for_line_break_(false) {
+      is_generated_for_line_break_(false),
+      is_unsafe_to_reuse_shape_result_(false) {
   DCHECK_GE(end, start);
   ComputeBoxProperties();
 }
@@ -110,7 +111,8 @@ NGInlineItem::NGInlineItem(const NGInlineItem& other,
       is_empty_item_(other.is_empty_item_),
       is_block_level_(other.is_block_level_),
       is_end_collapsible_newline_(other.is_end_collapsible_newline_),
-      is_generated_for_line_break_(other.is_generated_for_line_break_) {
+      is_generated_for_line_break_(other.is_generated_for_line_break_),
+      is_unsafe_to_reuse_shape_result_(other.is_unsafe_to_reuse_shape_result_) {
   DCHECK_GE(end, start);
 }
 

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

@@ -85,6 +85,12 @@ class CORE_EXPORT NGInlineItem {
   }
 
   const ShapeResult* TextShapeResult() const { return shape_result_.get(); }
+  bool IsUnsafeToReuseShapeResult() const {
+    return is_unsafe_to_reuse_shape_result_;
+  }
+  void SetUnsafeToReuseShapeResult() {
+    is_unsafe_to_reuse_shape_result_ = true;
+  }
 
   // If this item is "empty" for the purpose of empty block calculation.
   // Note: for block-in-inlines, this can't be determined until this is laid
@@ -274,6 +280,7 @@ class CORE_EXPORT NGInlineItem {
   unsigned is_block_level_ : 1;
   unsigned is_end_collapsible_newline_ : 1;
   unsigned is_generated_for_line_break_ : 1;
+  unsigned is_unsafe_to_reuse_shape_result_ : 1;
   friend class NGInlineNode;
   friend class NGInlineNodeDataEditor;
 };

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

@@ -407,8 +407,12 @@ inline bool NeedsShaping(const NGInlineItem& item) {
     return false;
   // Text item with length==0 exists to maintain LayoutObject states such as
   // ClearNeedsLayout, but not needed to shape.
-  if (!item.Length())
+  if (!item.Length()) {
     return false;
+  }
+  if (item.IsUnsafeToReuseShapeResult()) {
+    return true;
+  }
   const ShapeResult* shape_result = item.TextShapeResult();
   if (!shape_result)
     return true;
@@ -1451,6 +1455,11 @@ void NGInlineNode::ShapeText(NGInlineItemsData* data,
     // "32" is heuristic, most major sites are up to 8 or so, wikipedia is 21.
     Vector<ShapeResult::ShapeRange, 32> text_item_ranges;
     text_item_ranges.ReserveInitialCapacity(num_text_items);
+    const bool has_ligatures =
+        shape_result->NumGlyphs() < shape_result->NumCharacters();
+    if (has_ligatures) {
+      shape_result->EnsurePositionData();
+    }
     for (; index < end_index; index++) {
       NGInlineItem& item = (*items)[index];
       if (item.Type() != NGInlineItem::kText || !item.Length())
@@ -1466,6 +1475,14 @@ void NGInlineNode::ShapeText(NGInlineItemsData* data,
           ShapeResult::CreateEmpty(*shape_result.get());
       text_item_ranges.emplace_back(item.StartOffset(), item.EndOffset(),
                                     item_result.get());
+      if (has_ligatures && item.EndOffset() < shape_result->EndIndex() &&
+          shape_result->CachedNextSafeToBreakOffset(item.EndOffset()) !=
+              item.EndOffset()) {
+        // Note: We should not reuse `ShapeResult` ends with ligature glyph.
+        // e.g. <div>f<span>i</div> to <div>f</div> with ligature "fi".
+        // See http://crbug.com/1409702
+        item.SetUnsafeToReuseShapeResult();
+      }
       item.shape_result_ = std::move(item_result);
     }
     DCHECK_EQ(text_item_ranges.size(), num_text_items);

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

@@ -26,6 +26,7 @@
 #include "third_party/blink/renderer/core/style/computed_style.h"
 #include "third_party/blink/renderer/core/svg_names.h"
 #include "third_party/blink/renderer/core/testing/core_unit_test_helper.h"
+#include "third_party/blink/renderer/platform/testing/unit_test_helpers.h"
 
 namespace blink {
 
@@ -159,6 +160,13 @@ class NGInlineNodeTest : public RenderingTest {
     EXPECT_FALSE(expected);
   }
 
+  // "Google Sans" has ligatures, e.g. "fi", "tt", etc.
+  void LoadGoogleSans() {
+    LoadFontFromFile(GetFrame(),
+                     test::CoreTestDataPath("GoogleSans-Regular.ttf"),
+                     "Google Sans");
+  }
+
   Persistent<LayoutNGBlockFlow> layout_block_flow_;
   Persistent<LayoutObject> layout_object_;
   FontCachePurgePreventer purge_preventer_;
@@ -1535,6 +1543,35 @@ TEST_F(NGInlineNodeTest, ReuseFirstNonSafeRtl) {
   EXPECT_TRUE(NGInlineNode::NeedsShapingForTesting(item_v));
 }
 
+// http://crbug.com/1409702
+TEST_F(NGInlineNodeTest, ShouldNotResueLigature) {
+  LoadGoogleSans();
+  InsertStyleElement("#sample { font-family: 'Google Sans'; }");
+  SetBodyContent("<div id=sample>abf<span>i</span></div>");
+  Element& sample = *GetElementById("sample");
+
+  // `shape_result_before` has a ligature "fi".
+  const LayoutText& layout_text =
+      *To<Text>(sample.firstChild())->GetLayoutObject();
+  const ShapeResult& shape_result_before =
+      *layout_text.InlineItems().begin()->TextShapeResult();
+  ASSERT_EQ(3u, shape_result_before.NumGlyphs());
+
+  const LayoutText& layout_text_i =
+      *To<Text>(sample.lastChild()->firstChild())->GetLayoutObject();
+  const ShapeResult& shape_result_i =
+      *layout_text_i.InlineItems().begin()->TextShapeResult();
+  ASSERT_EQ(0u, shape_result_i.NumGlyphs());
+
+  // To <div id=sample>abf</div>
+  sample.lastChild()->remove();
+  UpdateAllLifecyclePhasesForTest();
+
+  const ShapeResult& shape_result_after =
+      *layout_text.InlineItems().begin()->TextShapeResult();
+  EXPECT_NE(&shape_result_before, &shape_result_after);
+}
+
 TEST_F(NGInlineNodeTest, InitialLetter) {
   ScopedCSSInitialLetterForTest enable_initial_letter_scope(true);
   LoadAhem();

third_party/blink/renderer/core/testing/page_test_base.cc

@@ -192,13 +192,18 @@ void PageTestBase::LoadAhem() {
 }
 
 void PageTestBase::LoadAhem(LocalFrame& frame) {
+  LoadFontFromFile(frame, test::CoreTestDataPath("Ahem.ttf"), "Ahem");
+}
+
+void PageTestBase::LoadFontFromFile(LocalFrame& frame,
+                                    String font_path,
+                                    const AtomicString& family_name) {
   Document& document = *frame.DomWindow()->document();
-  scoped_refptr<SharedBuffer> shared_buffer =
-      test::ReadFromFile(test::CoreTestDataPath("Ahem.ttf"));
+  scoped_refptr<SharedBuffer> shared_buffer = test::ReadFromFile(font_path);
   auto* buffer =
       MakeGarbageCollected<V8UnionArrayBufferOrArrayBufferViewOrString>(
           DOMArrayBuffer::Create(shared_buffer));
-  FontFace* ahem = FontFace::Create(frame.DomWindow(), "Ahem", buffer,
+  FontFace* ahem = FontFace::Create(frame.DomWindow(), family_name, buffer,
                                     FontFaceDescriptors::Create());
 
   ScriptState* script_state = ToScriptStateForMainWorld(&frame);
@@ -212,20 +217,10 @@ void PageTestBase::LoadNoto() {
 }
 
 void PageTestBase::LoadNoto(LocalFrame& frame) {
-  Document& document = *frame.DomWindow()->document();
-  scoped_refptr<SharedBuffer> shared_buffer =
-      test::ReadFromFile(blink::test::PlatformTestDataPath(
-          "third_party/Noto/NotoNaskhArabic-regular.woff2"));
-  auto* buffer =
-      MakeGarbageCollected<V8UnionArrayBufferOrArrayBufferViewOrString>(
-          DOMArrayBuffer::Create(shared_buffer));
-  FontFace* noto = FontFace::Create(frame.DomWindow(), "NotoArabic", buffer,
-                                    FontFaceDescriptors::Create());
-
-  ScriptState* script_state = ToScriptStateForMainWorld(&frame);
-  DummyExceptionStateForTesting exception_state;
-  FontFaceSetDocument::From(document)->addForBinding(script_state, noto,
-                                                     exception_state);
+  LoadFontFromFile(frame,
+                   blink::test::PlatformTestDataPath(
+                       "third_party/Noto/NotoNaskhArabic-regular.woff2"),
+                   "NotoArabic");
 }
 
 // Both sets the inner html and runs the document lifecycle.

third_party/blink/renderer/core/testing/page_test_base.h

@@ -102,6 +102,11 @@ class PageTestBase : public testing::Test, public ScopedMockOverlayScrollbars {
   // See external/wpt/css/fonts/ahem/README for more about the 'Ahem' font.
   static void LoadAhem(LocalFrame&);
 
+  // Install the font specified by `font_path` as `family_name` in `frame`.
+  static void LoadFontFromFile(LocalFrame& fame,
+                               String font_path,
+                               const AtomicString& family_name);
+
   static void LoadNoto(LocalFrame&);
 
   static std::string ToSimpleLayoutTree(const LayoutObject& layout_object);