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.
diff --git a/third_party/txt/src/txt/paragraph_txt.cc b/third_party/txt/src/txt/paragraph_txt.cc
index 780fd61..1e4bcb8 100644
--- a/third_party/txt/src/txt/paragraph_txt.cc
+++ b/third_party/txt/src/txt/paragraph_txt.cc
@@ -194,11 +194,9 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
}
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);
diff --git a/third_party/txt/src/txt/paragraph_txt.h b/third_party/txt/src/txt/paragraph_txt.h
index 4941bb8..0919130 100644
--- a/third_party/txt/src/txt/paragraph_txt.h
+++ b/third_party/txt/src/txt/paragraph_txt.h
@@ -260,17 +260,11 @@
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);
};
diff --git a/third_party/txt/tests/paragraph_unittests.cc b/third_party/txt/tests/paragraph_unittests.cc
index 8c806d7..8f94d54 100644
--- a/third_party/txt/tests/paragraph_unittests.cc
+++ b/third_party/txt/tests/paragraph_unittests.cc
@@ -5211,26 +5211,6 @@
}
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 @@
}
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 @@
}
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 @@
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());
}
