Fix the offset passed to minikin::GraphemeBreak::isGraphemeBreak (#21706)

The character offset passed to isGraphemeBreak is relative to the beginning
of the string (not relative to the text_start parameter).  This caused bad
results when searching for grapheme breaks beyond the first line of text
(see https://github.com/flutter/flutter/issues/24802).

This PR fixes the offset value.  It also reverts the workaround applied in
https://github.com/flutter/engine/pull/10063, which caused incorrect
calculation of boundaries between graphemes within ligatures.

third_party/txt/src/txt/paragraph_txt.cc

@@ -194,11 +194,9 @@ static const float kDoubleDecorationSpacing = 3.0f;
 ParagraphTxt::GlyphPosition::GlyphPosition(double x_start,
                                            double x_advance,
                                            size_t code_unit_index,
-                                           size_t code_unit_width,
-                                           size_t cluster)
+                                           size_t code_unit_width)
     : code_units(code_unit_index, code_unit_index + code_unit_width),
-      x_pos(x_start, x_start + x_advance),
-      cluster(cluster) {}
+      x_pos(x_start, x_start + x_advance) {}
 
 void ParagraphTxt::GlyphPosition::Shift(double delta) {
   x_pos.Shift(delta);
@@ -797,7 +795,6 @@ void ParagraphTxt::Layout(double width) {
 
     double run_x_offset = 0;
     double justify_x_offset = 0;
-    size_t cluster_unique_id = 0;
     std::vector<PaintRecord> paint_records;
 
     for (auto line_run_it = line_runs.begin(); line_run_it != line_runs.end();
@@ -945,7 +942,7 @@ void ParagraphTxt::Layout(double width) {
                  offset < glyph_code_units.end; ++offset) {
               if (minikin::GraphemeBreak::isGraphemeBreak(
                       layout_advances.data(), text_ptr, text_start, text_count,
-                      offset)) {
+                      text_start + offset)) {
                 grapheme_code_unit_counts.push_back(code_unit_count);
                 code_unit_count = 1;
               } else {
@@ -958,10 +955,10 @@ void ParagraphTxt::Layout(double width) {
           float grapheme_advance =
               glyph_advance / grapheme_code_unit_counts.size();
 
-          glyph_positions.emplace_back(
-              run_x_offset + glyph_x_offset, grapheme_advance,
-              run.start() + glyph_code_units.start,
-              grapheme_code_unit_counts[0], cluster_unique_id);
+          glyph_positions.emplace_back(run_x_offset + glyph_x_offset,
+                                       grapheme_advance,
+                                       run.start() + glyph_code_units.start,
+                                       grapheme_code_unit_counts[0]);
 
           // Compute positions for the additional graphemes in the ligature.
           for (size_t i = 1; i < grapheme_code_unit_counts.size(); ++i) {
@@ -969,9 +966,8 @@ void ParagraphTxt::Layout(double width) {
                 glyph_positions.back().x_pos.end, grapheme_advance,
                 glyph_positions.back().code_units.start +
                     grapheme_code_unit_counts[i - 1],
-                grapheme_code_unit_counts[i], cluster_unique_id);
+                grapheme_code_unit_counts[i]);
           }
-          cluster_unique_id++;
 
           bool at_word_start = false;
           bool at_word_end = false;
@@ -1876,28 +1872,12 @@ Paragraph::PositionWithAffinity ParagraphTxt::GetGlyphPositionAtCoordinate(
 
   size_t x_index;
   const GlyphPosition* gp = nullptr;
-  const GlyphPosition* gp_cluster = nullptr;
-  bool is_cluster_corection = false;
   for (x_index = 0; x_index < line_glyph_position.size(); ++x_index) {
     double glyph_end = (x_index < line_glyph_position.size() - 1)
                            ? line_glyph_position[x_index + 1].x_pos.start
                            : line_glyph_position[x_index].x_pos.end;
-    if (gp_cluster == nullptr ||
-        gp_cluster->cluster != line_glyph_position[x_index].cluster) {
-      gp_cluster = &line_glyph_position[x_index];
-    }
     if (dx < glyph_end) {
-      // Check if the glyph position is part of a cluster. If it is,
-      // we assign the cluster's root GlyphPosition to represent it.
-      if (gp_cluster->cluster == line_glyph_position[x_index].cluster) {
-        gp = gp_cluster;
-        // Detect if the matching GlyphPosition was non-root for the cluster.
-        if (gp_cluster != &line_glyph_position[x_index]) {
-          is_cluster_corection = true;
-        }
-      } else {
-        gp = &line_glyph_position[x_index];
-      }
+      gp = &line_glyph_position[x_index];
       break;
     }
   }
@@ -1918,13 +1898,8 @@ Paragraph::PositionWithAffinity ParagraphTxt::GetGlyphPositionAtCoordinate(
   }
 
   double glyph_center = (gp->x_pos.start + gp->x_pos.end) / 2;
-  // We want to use the root cluster's start when the cluster
-  // was corrected.
-  // TODO(garyq): Detect if the position is in the middle of the cluster
-  // and properly assign the start/end positions.
   if ((direction == TextDirection::ltr && dx < glyph_center) ||
-      (direction == TextDirection::rtl && dx >= glyph_center) ||
-      is_cluster_corection) {
+      (direction == TextDirection::rtl && dx >= glyph_center)) {
     return PositionWithAffinity(gp->code_units.start, DOWNSTREAM);
   } else {
     return PositionWithAffinity(gp->code_units.end, UPSTREAM);

third_party/txt/src/txt/paragraph_txt.h

@@ -260,17 +260,11 @@ class ParagraphTxt : public Paragraph {
   struct GlyphPosition {
     Range<size_t> code_units;
     Range<double> x_pos;
-    // Tracks the cluster that this glyph position belongs to. For example, in
-    // extended emojis, multiple glyph positions will have the same cluster. The
-    // cluster can be used as a key to distinguish between codepoints that
-    // contribute to the drawing of a single glyph.
-    size_t cluster;
 
     GlyphPosition(double x_start,
                   double x_advance,
                   size_t code_unit_index,
-                  size_t code_unit_width,
-                  size_t cluster);
+                  size_t code_unit_width);
 
     void Shift(double delta);
   };

third_party/txt/tests/paragraph_unittests.cc

@@ -5211,26 +5211,6 @@ TEST_F(ParagraphTest, LINUX_ONLY(EmojiMultiLineRectsParagraph)) {
   }
   EXPECT_EQ(boxes.size(), 0ull);
 
-  // Ligature style indexing.
-  boxes =
-      paragraph->GetRectsForRange(0, 119, rect_height_style, rect_width_style);
-  for (size_t i = 0; i < boxes.size(); ++i) {
-    GetCanvas()->drawRect(boxes[i].rect, paint);
-  }
-  EXPECT_EQ(boxes.size(), 2ull);
-  EXPECT_FLOAT_EQ(boxes[1].rect.left(), 0);
-  EXPECT_FLOAT_EQ(boxes[1].rect.right(), 334.61475);
-
-  boxes = paragraph->GetRectsForRange(122, 132, rect_height_style,
-                                      rect_width_style);
-  paint.setColor(SK_ColorBLUE);
-  for (size_t i = 0; i < boxes.size(); ++i) {
-    GetCanvas()->drawRect(boxes[i].rect, paint);
-  }
-  EXPECT_EQ(boxes.size(), 1ull);
-  EXPECT_FLOAT_EQ(boxes[0].rect.left(), 357.95996);
-  EXPECT_FLOAT_EQ(boxes[0].rect.right(), 418.79901);
-
   // GetPositionForCoordinates should not return inter-emoji positions.
   boxes = paragraph->GetRectsForRange(
       0, paragraph->GetGlyphPositionAtCoordinate(610, 100).position,
@@ -5241,9 +5221,7 @@ TEST_F(ParagraphTest, LINUX_ONLY(EmojiMultiLineRectsParagraph)) {
   }
   EXPECT_EQ(boxes.size(), 2ull);
   EXPECT_FLOAT_EQ(boxes[1].rect.left(), 0);
-  // The following is expected to change to a higher value when
-  // rounding up is added to getGlyphPositionForCoordinate.
-  EXPECT_FLOAT_EQ(boxes[1].rect.right(), 560.28516);
+  EXPECT_FLOAT_EQ(boxes[1].rect.right(), 622.53906);
 
   boxes = paragraph->GetRectsForRange(
       0, paragraph->GetGlyphPositionAtCoordinate(580, 100).position,
@@ -5265,13 +5243,45 @@ TEST_F(ParagraphTest, LINUX_ONLY(EmojiMultiLineRectsParagraph)) {
   }
   EXPECT_EQ(boxes.size(), 2ull);
   EXPECT_FLOAT_EQ(boxes[1].rect.left(), 0);
-  // The following is expected to change to the 560.28516 value when
-  // rounding up is added to getGlyphPositionForCoordinate.
-  EXPECT_FLOAT_EQ(boxes[1].rect.right(), 498.03125);
+  EXPECT_FLOAT_EQ(boxes[1].rect.right(), 560.28516);
 
   ASSERT_TRUE(Snapshot());
 }
 
+TEST_F(ParagraphTest, LINUX_ONLY(LigatureCharacters)) {
+  const char* text = "Office";
+  auto icu_text = icu::UnicodeString::fromUTF8(text);
+  std::u16string u16_text(icu_text.getBuffer(),
+                          icu_text.getBuffer() + icu_text.length());
+
+  txt::ParagraphStyle paragraph_style;
+  txt::ParagraphBuilderTxt builder(paragraph_style, GetTestFontCollection());
+
+  txt::TextStyle text_style;
+  text_style.font_families = std::vector<std::string>(1, "Roboto");
+  text_style.color = SK_ColorBLACK;
+  builder.PushStyle(text_style);
+  builder.AddText(u16_text);
+
+  builder.Pop();
+
+  auto paragraph = BuildParagraph(builder);
+  paragraph->Layout(GetTestCanvasWidth());
+
+  // The "ffi" characters will be combined into one glyph in the Roboto font.
+  // Verify that the graphemes within the glyph have distinct boxes.
+  std::vector<txt::Paragraph::TextBox> boxes =
+      paragraph->GetRectsForRange(1, 2, Paragraph::RectHeightStyle::kTight,
+                                  Paragraph::RectWidthStyle::kTight);
+  EXPECT_FLOAT_EQ(boxes[0].rect.left(), 9.625);
+  EXPECT_FLOAT_EQ(boxes[0].rect.right(), 13.608073);
+
+  boxes = paragraph->GetRectsForRange(2, 4, Paragraph::RectHeightStyle::kTight,
+                                      Paragraph::RectWidthStyle::kTight);
+  EXPECT_FLOAT_EQ(boxes[0].rect.left(), 13.608073);
+  EXPECT_FLOAT_EQ(boxes[0].rect.right(), 21.574219);
+}
+
 TEST_F(ParagraphTest, HyphenBreakParagraph) {
   const char* text =
       "A "
@@ -6409,8 +6419,8 @@ TEST_F(ParagraphTest, KhmerLineBreaker) {
 
   ASSERT_EQ(paragraph->glyph_lines_.size(), 3ull);
   EXPECT_EQ(paragraph->glyph_lines_[0].positions.size(), 7ul);
-  EXPECT_EQ(paragraph->glyph_lines_[1].positions.size(), 12ul);
-  EXPECT_EQ(paragraph->glyph_lines_[2].positions.size(), 7ul);
+  EXPECT_EQ(paragraph->glyph_lines_[1].positions.size(), 7ul);
+  EXPECT_EQ(paragraph->glyph_lines_[2].positions.size(), 3ul);
 
   ASSERT_TRUE(Snapshot());
 }