Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Check for index bounds in RTL handling for trailing whitespace runs. #12336

Merged
merged 9 commits into from
Sep 23, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions third_party/txt/src/txt/paragraph_txt.cc
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,9 @@ bool ParagraphTxt::ComputeBidiRuns(std::vector<BidiRun>* result) {
//
// This only applies to the final whitespace at the end as other whitespace is
// no longer ambiguous when surrounded by additional text.

// TODO(garyq): Handle this in the text editor caret code instead at layout
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still prefer to revert this instead of adding more complex post-processing to the output of the bidi algorithm.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is viable for the RTL users of flutter if we revert this without a complete solution ready to go, as it causes all RTL input to look completely wrong. Reverting would make RTL essentially unusable until the final solution is rolled.

// level.
bool has_trailing_whitespace = false;
int32_t bidi_run_start, bidi_run_length;
if (bidi_run_count > 1) {
Expand All @@ -427,8 +430,17 @@ bool ParagraphTxt::ComputeBidiRuns(std::vector<BidiRun>* result) {
U16_GET(text_.data(), 0, bidi_run_start + bidi_run_length - 1,
static_cast<int>(text_.size()), last_char);
if (u_hasBinaryProperty(last_char, UCHAR_WHITE_SPACE)) {
has_trailing_whitespace = true;
bidi_run_count--;
// Check if the trailing whitespace occurs before the previous run or
// not. If so, this trailing whitespace was a leading whitespace.
int32_t second_last_bidi_run_start, second_last_bidi_run_length;
ubidi_getVisualRun(bidi.get(), bidi_run_count - 2,
&second_last_bidi_run_start,
&second_last_bidi_run_length);
if (bidi_run_start ==
second_last_bidi_run_start + second_last_bidi_run_length) {
has_trailing_whitespace = true;
bidi_run_count--;
}
}
}
}
Expand Down
51 changes: 51 additions & 0 deletions third_party/txt/tests/paragraph_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2327,6 +2327,57 @@ TEST_F(ParagraphTest, DISABLE_ON_WINDOWS(JustifyRTLNewLine)) {
}
}

TEST_F(ParagraphTest, DISABLE_ON_WINDOWS(LeadingSpaceRTL)) {
const char* text = " leading space";

auto icu_text = icu::UnicodeString::fromUTF8(text);
std::u16string u16_text(icu_text.getBuffer(),
icu_text.getBuffer() + icu_text.length());

txt::ParagraphStyle paragraph_style;
paragraph_style.max_lines = 14;
paragraph_style.text_align = TextAlign::justify;
paragraph_style.text_direction = TextDirection::rtl;
txt::ParagraphBuilderTxt builder(paragraph_style, GetTestFontCollection());

txt::TextStyle text_style;
text_style.font_families = std::vector<std::string>(1, "Ahem");
text_style.font_size = 26;
text_style.color = SK_ColorBLACK;
text_style.height = 1;
builder.PushStyle(text_style);

builder.AddText(u16_text);

builder.Pop();

auto paragraph = BuildParagraph(builder);
size_t paragraph_width = GetTestCanvasWidth() - 100;
paragraph->Layout(paragraph_width);

paragraph->Paint(GetCanvas(), 0, 0);

SkPaint paint;
paint.setStyle(SkPaint::kStroke_Style);
paint.setAntiAlias(true);
paint.setStrokeWidth(1);

// Tests for GetRectsForRange()
Paragraph::RectHeightStyle rect_height_style =
Paragraph::RectHeightStyle::kMax;
Paragraph::RectWidthStyle rect_width_style =
Paragraph::RectWidthStyle::kTight;
paint.setColor(SK_ColorRED);
std::vector<txt::Paragraph::TextBox> boxes =
paragraph->GetRectsForRange(0, 100, rect_height_style, rect_width_style);
for (size_t i = 0; i < boxes.size(); ++i) {
GetCanvas()->drawRect(boxes[i].rect, paint);
}
ASSERT_EQ(boxes.size(), 2ull);

// This test should crash if behavior regresses.
}

TEST_F(ParagraphTest, DecorationsParagraph) {
txt::ParagraphStyle paragraph_style;
paragraph_style.max_lines = 14;
Expand Down