From 946a1f7fb237413d0524b07dc177d4f216ffd60f Mon Sep 17 00:00:00 2001 From: garyqian Date: Wed, 18 Sep 2019 12:06:00 -0400 Subject: [PATCH 1/9] Check for special case index out of bounds condition for leading space --- third_party/txt/src/txt/paragraph_txt.cc | 3 +- third_party/txt/tests/paragraph_unittests.cc | 51 ++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/third_party/txt/src/txt/paragraph_txt.cc b/third_party/txt/src/txt/paragraph_txt.cc index 122c1de39a988..5b3bbedbea247 100644 --- a/third_party/txt/src/txt/paragraph_txt.cc +++ b/third_party/txt/src/txt/paragraph_txt.cc @@ -469,7 +469,8 @@ bool ParagraphTxt::ComputeBidiRuns(std::vector* result) { continue; // Attach the final trailing whitespace as part of this run. - if (has_trailing_whitespace && bidi_run_index == bidi_run_count - 1) { + if (has_trailing_whitespace && bidi_run_index == bidi_run_count - 1 && + bidi_run_length + bidi_run_start + 1ull < text_.size()) { bidi_run_length++; } diff --git a/third_party/txt/tests/paragraph_unittests.cc b/third_party/txt/tests/paragraph_unittests.cc index 78e28d771ad84..f4b7e9de22991 100644 --- a/third_party/txt/tests/paragraph_unittests.cc +++ b/third_party/txt/tests/paragraph_unittests.cc @@ -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(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 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(), 1ull); + + // This test should crash if behavior regresses. +} + TEST_F(ParagraphTest, DecorationsParagraph) { txt::ParagraphStyle paragraph_style; paragraph_style.max_lines = 14; From a35fa718929b8cb69930fe303b19aa1eadb28175 Mon Sep 17 00:00:00 2001 From: garyqian Date: Wed, 18 Sep 2019 12:07:50 -0400 Subject: [PATCH 2/9] Add TODO --- third_party/txt/src/txt/paragraph_txt.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/third_party/txt/src/txt/paragraph_txt.cc b/third_party/txt/src/txt/paragraph_txt.cc index 5b3bbedbea247..0d06529060940 100644 --- a/third_party/txt/src/txt/paragraph_txt.cc +++ b/third_party/txt/src/txt/paragraph_txt.cc @@ -415,6 +415,9 @@ bool ParagraphTxt::ComputeBidiRuns(std::vector* 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 + // level. bool has_trailing_whitespace = false; int32_t bidi_run_start, bidi_run_length; if (bidi_run_count > 1) { From 326308fc48298c47b10f175fa776598cf3597fe2 Mon Sep 17 00:00:00 2001 From: garyqian Date: Wed, 18 Sep 2019 13:24:31 -0400 Subject: [PATCH 3/9] Rework to pass tests --- third_party/txt/src/txt/paragraph_txt.cc | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/third_party/txt/src/txt/paragraph_txt.cc b/third_party/txt/src/txt/paragraph_txt.cc index 0d06529060940..2fde4107717bb 100644 --- a/third_party/txt/src/txt/paragraph_txt.cc +++ b/third_party/txt/src/txt/paragraph_txt.cc @@ -419,6 +419,7 @@ bool ParagraphTxt::ComputeBidiRuns(std::vector* result) { // TODO(garyq): Handle this in the text editor caret code instead at layout // level. bool has_trailing_whitespace = false; + bool is_leading = false; // True if the whitespace is a leading whitespace. int32_t bidi_run_start, bidi_run_length; if (bidi_run_count > 1) { ubidi_getVisualRun(bidi.get(), bidi_run_count - 1, &bidi_run_start, @@ -432,6 +433,9 @@ bool ParagraphTxt::ComputeBidiRuns(std::vector* result) { if (u_hasBinaryProperty(last_char, UCHAR_WHITE_SPACE)) { has_trailing_whitespace = true; bidi_run_count--; + if (bidi_run_start == 0) { + is_leading = true; + } } } } @@ -472,9 +476,11 @@ bool ParagraphTxt::ComputeBidiRuns(std::vector* result) { continue; // Attach the final trailing whitespace as part of this run. - if (has_trailing_whitespace && bidi_run_index == bidi_run_count - 1 && - bidi_run_length + bidi_run_start + 1ull < text_.size()) { + if (has_trailing_whitespace && bidi_run_index == bidi_run_count - 1) { bidi_run_length++; + if (is_leading) { + bidi_run_start--; + } } size_t bidi_run_end = bidi_run_start + bidi_run_length; From a30248d65af7123b219cbea8bbb26293a186ef5e Mon Sep 17 00:00:00 2001 From: garyqian Date: Wed, 18 Sep 2019 14:28:22 -0400 Subject: [PATCH 4/9] More robust check for leading --- third_party/txt/src/txt/paragraph_txt.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/third_party/txt/src/txt/paragraph_txt.cc b/third_party/txt/src/txt/paragraph_txt.cc index 2fde4107717bb..a0b65a308b0a0 100644 --- a/third_party/txt/src/txt/paragraph_txt.cc +++ b/third_party/txt/src/txt/paragraph_txt.cc @@ -433,7 +433,13 @@ bool ParagraphTxt::ComputeBidiRuns(std::vector* result) { if (u_hasBinaryProperty(last_char, UCHAR_WHITE_SPACE)) { has_trailing_whitespace = true; bidi_run_count--; - if (bidi_run_start == 0) { + // 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) { is_leading = true; } } From aa9dbb8921751840f2a7e855eacb30d4cb5c3fe4 Mon Sep 17 00:00:00 2001 From: garyqian Date: Wed, 18 Sep 2019 15:10:51 -0400 Subject: [PATCH 5/9] Minor adjustment --- third_party/txt/src/txt/paragraph_txt.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/third_party/txt/src/txt/paragraph_txt.cc b/third_party/txt/src/txt/paragraph_txt.cc index a0b65a308b0a0..a986e312ae638 100644 --- a/third_party/txt/src/txt/paragraph_txt.cc +++ b/third_party/txt/src/txt/paragraph_txt.cc @@ -439,7 +439,7 @@ bool ParagraphTxt::ComputeBidiRuns(std::vector* result) { 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) { + if (bidi_run_start <= second_last_bidi_run_start) { is_leading = true; } } From 5e119a63b4a483b8eb4cde19ea7f09772f185453 Mon Sep 17 00:00:00 2001 From: garyqian Date: Wed, 18 Sep 2019 15:31:13 -0400 Subject: [PATCH 6/9] Fix order bug --- third_party/txt/src/txt/paragraph_txt.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/third_party/txt/src/txt/paragraph_txt.cc b/third_party/txt/src/txt/paragraph_txt.cc index a986e312ae638..97533f415e6c6 100644 --- a/third_party/txt/src/txt/paragraph_txt.cc +++ b/third_party/txt/src/txt/paragraph_txt.cc @@ -431,17 +431,17 @@ bool ParagraphTxt::ComputeBidiRuns(std::vector* result) { U16_GET(text_.data(), 0, bidi_run_start + bidi_run_length - 1, static_cast(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) { + if (bidi_run_start < second_last_bidi_run_start) { is_leading = true; } + has_trailing_whitespace = true; + bidi_run_count--; } } } From 5b330756dc2f7e4b81888d8448dc9d84df70a934 Mon Sep 17 00:00:00 2001 From: garyqian Date: Wed, 18 Sep 2019 16:03:51 -0400 Subject: [PATCH 7/9] Do not modify for leading space --- third_party/txt/src/txt/paragraph_txt.cc | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/third_party/txt/src/txt/paragraph_txt.cc b/third_party/txt/src/txt/paragraph_txt.cc index 97533f415e6c6..7d817c9b3ab88 100644 --- a/third_party/txt/src/txt/paragraph_txt.cc +++ b/third_party/txt/src/txt/paragraph_txt.cc @@ -419,7 +419,6 @@ bool ParagraphTxt::ComputeBidiRuns(std::vector* result) { // TODO(garyq): Handle this in the text editor caret code instead at layout // level. bool has_trailing_whitespace = false; - bool is_leading = false; // True if the whitespace is a leading whitespace. int32_t bidi_run_start, bidi_run_length; if (bidi_run_count > 1) { ubidi_getVisualRun(bidi.get(), bidi_run_count - 1, &bidi_run_start, @@ -437,11 +436,10 @@ bool ParagraphTxt::ComputeBidiRuns(std::vector* result) { 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) { - is_leading = true; + if (bidi_run_start >= second_last_bidi_run_start) { + has_trailing_whitespace = true; + bidi_run_count--; } - has_trailing_whitespace = true; - bidi_run_count--; } } } @@ -484,9 +482,6 @@ bool ParagraphTxt::ComputeBidiRuns(std::vector* result) { // Attach the final trailing whitespace as part of this run. if (has_trailing_whitespace && bidi_run_index == bidi_run_count - 1) { bidi_run_length++; - if (is_leading) { - bidi_run_start--; - } } size_t bidi_run_end = bidi_run_start + bidi_run_length; From 74eb10098258d8e7822e9e091c5e2b4bafe96e4e Mon Sep 17 00:00:00 2001 From: GaryQian Date: Mon, 23 Sep 2019 10:18:45 -0700 Subject: [PATCH 8/9] Fix test value --- third_party/txt/tests/paragraph_unittests.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/third_party/txt/tests/paragraph_unittests.cc b/third_party/txt/tests/paragraph_unittests.cc index f4b7e9de22991..afe70ef1a84e9 100644 --- a/third_party/txt/tests/paragraph_unittests.cc +++ b/third_party/txt/tests/paragraph_unittests.cc @@ -2373,7 +2373,7 @@ TEST_F(ParagraphTest, DISABLE_ON_WINDOWS(LeadingSpaceRTL)) { for (size_t i = 0; i < boxes.size(); ++i) { GetCanvas()->drawRect(boxes[i].rect, paint); } - ASSERT_EQ(boxes.size(), 1ull); + ASSERT_EQ(boxes.size(), 2ull); // This test should crash if behavior regresses. } From 443a61de80e083f433e139c5a35ba2f127c6429a Mon Sep 17 00:00:00 2001 From: GaryQian Date: Mon, 23 Sep 2019 12:36:10 -0700 Subject: [PATCH 9/9] Condition --- third_party/txt/src/txt/paragraph_txt.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/third_party/txt/src/txt/paragraph_txt.cc b/third_party/txt/src/txt/paragraph_txt.cc index 7d817c9b3ab88..e0c406de21361 100644 --- a/third_party/txt/src/txt/paragraph_txt.cc +++ b/third_party/txt/src/txt/paragraph_txt.cc @@ -436,7 +436,8 @@ bool ParagraphTxt::ComputeBidiRuns(std::vector* result) { 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) { + if (bidi_run_start == + second_last_bidi_run_start + second_last_bidi_run_length) { has_trailing_whitespace = true; bidi_run_count--; }