Revert "Custom RTL handling for ghost runs, NotoNaskhArabic test font"

third_party/txt/src/txt/paragraph.cc

@@ -17,15 +17,14 @@
 #include "paragraph.h"
 
 #include <hb.h>
-#include <minikin/Layout.h>
-
 #include <algorithm>
 #include <limits>
 #include <map>
 #include <numeric>
 #include <utility>
 #include <vector>
 
+#include <minikin/Layout.h>
 #include "flutter/fml/logging.h"
 #include "font_collection.h"
 #include "font_skia.h"
@@ -35,6 +34,9 @@
 #include "minikin/LayoutUtils.h"
 #include "minikin/LineBreaker.h"
 #include "minikin/MinikinFont.h"
+#include "unicode/ubidi.h"
+#include "unicode/utf16.h"
+
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkFont.h"
 #include "third_party/skia/include/core/SkFontMetrics.h"
@@ -44,8 +46,6 @@
 #include "third_party/skia/include/core/SkTypeface.h"
 #include "third_party/skia/include/effects/SkDashPathEffect.h"
 #include "third_party/skia/include/effects/SkDiscretePathEffect.h"
-#include "unicode/ubidi.h"
-#include "unicode/utf16.h"
 
 namespace txt {
 namespace {
@@ -552,39 +552,28 @@ void Paragraph::Layout(double width, bool force) {
     // Find the runs comprising this line.
     std::vector<BidiRun> line_runs;
     for (const BidiRun& bidi_run : bidi_runs) {
+      if (bidi_run.start() < line_end_index &&
+          bidi_run.end() > line_range.start) {
+        line_runs.emplace_back(std::max(bidi_run.start(), line_range.start),
+                               std::min(bidi_run.end(), line_end_index),
+                               bidi_run.direction(), bidi_run.style());
+      }
       // A "ghost" run is a run that does not impact the layout, breaking,
-      // alignment, width, etc but is still "visible" through getRectsForRange.
+      // alignment, width, etc but is still "visible" though getRectsForRange.
       // For example, trailing whitespace on centered text can be scrolled
       // through with the caret but will not wrap the line.
       //
       // Here, we add an additional run for the whitespace, but dont
       // let it impact metrics. After layout of the whitespace run, we do not
       // add its width into the x-offset adjustment, effectively nullifying its
       // impact on the layout.
-      std::unique_ptr<BidiRun> ghost_run = nullptr;
       if (paragraph_style_.ellipsis.empty() &&
           line_range.end_excluding_whitespace < line_range.end &&
           bidi_run.start() <= line_range.end &&
           bidi_run.end() > line_end_index) {
-        ghost_run = std::make_unique<BidiRun>(
-            std::max(bidi_run.start(), line_end_index),
-            std::min(bidi_run.end(), line_range.end), bidi_run.direction(),
-            bidi_run.style(), true);
-      }
-      // Include the ghost run before normal run if RTL
-      if (bidi_run.direction() == TextDirection::rtl && ghost_run != nullptr) {
-        line_runs.push_back(*ghost_run);
-      }
-      // Emplace a normal line run.
-      if (bidi_run.start() < line_end_index &&
-          bidi_run.end() > line_range.start) {
-        line_runs.emplace_back(std::max(bidi_run.start(), line_range.start),
-                               std::min(bidi_run.end(), line_end_index),
-                               bidi_run.direction(), bidi_run.style());
-      }
-      // Include the ghost run after normal run if LTR
-      if (bidi_run.direction() == TextDirection::ltr && ghost_run != nullptr) {
-        line_runs.push_back(*ghost_run);
+        line_runs.emplace_back(std::max(bidi_run.start(), line_end_index),
+                               std::min(bidi_run.end(), line_range.end),
+                               bidi_run.direction(), bidi_run.style(), true);
       }
     }
     bool line_runs_all_rtl =
@@ -672,17 +661,6 @@ void Paragraph::Layout(double width, bool force) {
       if (layout.nGlyphs() == 0)
         continue;
 
-      // When laying out RTL ghost runs, shift the run_x_offset here by the
-      // advance so that the ghost run is positioned to the left of the first
-      // real run of text in the line. However, since we do not want it to
-      // impact the layout of real text, this advance is subsequently added
-      // back into the run_x_offset after the ghost run positions have been
-      // calcuated and before the next real run of text is laid out, ensuring
-      // later runs are laid out in the same position as if there were no ghost
-      // run.
-      if (run.is_ghost() && run.is_rtl())
-        run_x_offset -= layout.getAdvance();
-
       std::vector<float> layout_advances(text_count);
       layout.getAdvances(layout_advances.data());
 
@@ -830,25 +808,21 @@ void Paragraph::Layout(double width, bool force) {
                   [](const GlyphPosition& a, const GlyphPosition& b) {
                     return a.code_units.start < b.code_units.start;
                   });
-
         line_code_unit_runs.emplace_back(
             std::move(code_unit_positions),
             Range<size_t>(run.start(), run.end()),
             Range<double>(glyph_positions.front().x_pos.start,
                           glyph_positions.back().x_pos.end),
             line_number, metrics, run.direction());
 
-        if (!run.is_ghost()) {
-          min_left_ = std::min(min_left_, glyph_positions.front().x_pos.start);
-          max_right_ = std::max(max_right_, glyph_positions.back().x_pos.end);
-        }
+        min_left_ = std::min(min_left_, glyph_positions.front().x_pos.start);
+        max_right_ = std::max(max_right_, glyph_positions.back().x_pos.end);
       }  // for each in glyph_blobs
-      // Do not increase x offset for LTR trailing ghost runs as it should not
-      // impact the layout of visible glyphs. RTL tailing ghost runs have the
-      // advance subtracted, so we do add the advance here to reset the
-      // run_x_offset. We do keep the record though so GetRectsForRange() can
-      // find metrics for trailing spaces.
-      if (!run.is_ghost() || run.is_rtl()) {
+
+      // Do not increase x offset for trailing ghost runs as it should not
+      // impact the layout of visible glyphs. We do keep the record though so
+      // GetRectsForRange() can find metrics for trailing spaces.
+      if (!run.is_ghost()) {
         run_x_offset += layout.getAdvance();
       }
     }  // for each in line_runs

third_party/txt/tests/paragraph_unittests.cc

@@ -706,7 +706,6 @@ TEST_F(ParagraphTest, DISABLE_ON_WINDOWS(JustifyRTL)) {
   txt::ParagraphStyle paragraph_style;
   paragraph_style.max_lines = 14;
   paragraph_style.text_align = TextAlign::justify;
-  paragraph_style.text_direction = TextDirection::rtl;
   txt::ParagraphBuilder builder(paragraph_style, GetTestFontCollection());
 
   txt::TextStyle text_style;
@@ -726,33 +725,16 @@ TEST_F(ParagraphTest, DISABLE_ON_WINDOWS(JustifyRTL)) {
 
   paragraph->Paint(GetCanvas(), 0, 0);
 
+  ASSERT_TRUE(Snapshot());
+
   auto glyph_line_width = [&paragraph](int index) {
     size_t second_to_last_position_index =
-        paragraph->glyph_lines_[index].positions.size() - 1;
+        paragraph->glyph_lines_[index].positions.size() - 2;
     return paragraph->glyph_lines_[index]
         .positions[second_to_last_position_index]
         .x_pos.end;
   };
 
-  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(), 5ull);
-  ASSERT_TRUE(Snapshot());
-
   // All lines except the last should be justified to the width of the
   // paragraph.
   for (size_t i = 0; i < paragraph->glyph_lines_.size() - 1; ++i) {
@@ -989,74 +971,6 @@ TEST_F(ParagraphTest, DISABLED_ArabicParagraph) {
   ASSERT_TRUE(Snapshot());
 }
 
-// Checks if the rects are in the correct positions after typing spaces in
-// Arabic.
-TEST_F(ParagraphTest, DISABLE_ON_WINDOWS(ArabicRectsParagraph)) {
-  const char* text = "بمباركة التقليدية قام عن. تصفح يد    ";
-  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::right;
-  paragraph_style.text_direction = TextDirection::rtl;
-  txt::ParagraphBuilder builder(paragraph_style, GetTestFontCollection());
-
-  txt::TextStyle text_style;
-  text_style.font_families = std::vector<std::string>(1, "Noto Naskh Arabic");
-  text_style.font_size = 26;
-  text_style.letter_spacing = 1;
-  text_style.word_spacing = 5;
-  text_style.color = SK_ColorBLACK;
-  text_style.height = 1;
-  text_style.decoration = TextDecoration::kUnderline;
-  text_style.decoration_color = SK_ColorBLACK;
-  builder.PushStyle(text_style);
-
-  builder.AddText(u16_text);
-
-  builder.Pop();
-
-  auto paragraph = builder.Build();
-  paragraph->Layout(GetTestCanvasWidth() - 100);
-
-  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);
-  }
-  EXPECT_EQ(boxes.size(), 2ull);
-
-  EXPECT_FLOAT_EQ(boxes[0].rect.left(), 556.54688);
-  EXPECT_FLOAT_EQ(boxes[0].rect.top(), -0.26855469);
-  EXPECT_FLOAT_EQ(boxes[0].rect.right(), 900);
-  EXPECT_FLOAT_EQ(boxes[0].rect.bottom(), 44);
-
-  EXPECT_FLOAT_EQ(boxes[1].rect.left(), 510.09375);
-  EXPECT_FLOAT_EQ(boxes[1].rect.top(), -0.26855469);
-  EXPECT_FLOAT_EQ(boxes[1].rect.right(), 557.04688);
-  EXPECT_FLOAT_EQ(boxes[1].rect.bottom(), 44);
-
-  ASSERT_EQ(paragraph_style.text_align,
-            paragraph->GetParagraphStyle().text_align);
-
-  ASSERT_TRUE(Snapshot());
-}
-
 TEST_F(ParagraphTest, GetGlyphPositionAtCoordinateParagraph) {
   const char* text =
       "12345 67890 12345 67890 12345 67890 12345 67890 12345 67890 12345 "