[LayoutNG] Fix multi-glyph-cluster case for PositionData

This patch fixes ShapeResult::ComputePositionData,
CachedPositionForOffset, and CachedOffsetForPosition for when
multiple glyphs have a same character (cluster) index.

Bug: 636993
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_layout_ng
Change-Id: I862117d1ac19d7950306edc574be8f3e9b03ccc3
Reviewed-on: https://chromium-review.googlesource.com/1153068
Commit-Queue: Koji Ishii <kojii@chromium.org>
Commit-Queue: Emil A Eklund <eae@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578871}

third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG

@@ -28,11 +28,6 @@ crbug.com/854889 accessibility/canvas-fallback-content-2.html [ Timeout ]
 crbug.com/854840 fast/table/backgr_border-table-quirks-collapsed-border.html [ Failure ]
 crbug.com/854840 fast/table/backgr_border-table-quirks.html [ Failure ]
 
-# Fails due to incorrect offset mapping using cache.
-crbug.com/636993 external/wpt/css/css-text/word-break/word-break-break-all-005.html [ Failure ]
-crbug.com/636993 external/wpt/css/css-text/word-break/word-break-normal-hi-000.html [ Failure ]
-crbug.com/636993 transforms/2d/hindi-rotated.html [ Failure ]
-
 # Superscript text off by 1px
 crbug.com/636993 external/wpt/css/css-text-decor/text-decoration-color.html [ Failure ]
 

third_party/blink/renderer/platform/fonts/shaping/harfbuzz_shaper_test.cc

@@ -744,7 +744,7 @@ class IncludePartialGlyphsTest
       public ::testing::WithParamInterface<IncludePartialGlyphsOption> {};
 
 INSTANTIATE_TEST_CASE_P(
-    OffsetForPositionTest,
+    HarfBuzzShaperTest,
     IncludePartialGlyphsTest,
     ::testing::Values(IncludePartialGlyphsOption::OnlyFullGlyphs,
                       IncludePartialGlyphsOption::IncludePartialGlyphs));
@@ -882,6 +882,46 @@ TEST_F(HarfBuzzShaperTest, CachedOffsetPositionMappingMixed) {
   EXPECT_EQ(6u, sr->CachedOffsetForPosition(sr->CachedPositionForOffset(6)));
 }
 
+TEST_F(HarfBuzzShaperTest, PositionForOffsetMultiGlyphClusterLtr) {
+  // In this Hindi text, each code unit produces a glyph, and the first 3 glyphs
+  // form a grapheme cluster, and the last 2 glyphs form another.
+  String string(u"\u0930\u093F\u0902\u0926\u0940");
+  TextDirection direction = TextDirection::kLtr;
+  HarfBuzzShaper shaper(string);
+  scoped_refptr<ShapeResult> sr = shaper.Shape(&font, direction);
+  sr->EnsurePositionData();
+
+  // The first 3 code units should be at position 0.
+  EXPECT_EQ(0, sr->CachedPositionForOffset(0));
+  EXPECT_EQ(0, sr->CachedPositionForOffset(1));
+  EXPECT_EQ(0, sr->CachedPositionForOffset(2));
+  // The last 2 code units should be > 0, and the same position.
+  EXPECT_GT(sr->CachedPositionForOffset(3), 0);
+  EXPECT_EQ(sr->CachedPositionForOffset(3), sr->CachedPositionForOffset(4));
+}
+
+TEST_F(HarfBuzzShaperTest, PositionForOffsetMultiGlyphClusterRtl) {
+  // In this Hindi text, each code unit produces a glyph, and the first 3 glyphs
+  // form a grapheme cluster, and the last 2 glyphs form another.
+  String string(u"\u0930\u093F\u0902\u0926\u0940");
+  TextDirection direction = TextDirection::kRtl;
+  HarfBuzzShaper shaper(string);
+  scoped_refptr<ShapeResult> sr = shaper.Shape(&font, direction);
+  sr->EnsurePositionData();
+
+  // The first 3 code units should be at position 0, but since this is RTL, the
+  // position is the right edgef of the character, and thus > 0.
+  float pos0 = sr->CachedPositionForOffset(0);
+  EXPECT_GT(pos0, 0);
+  EXPECT_EQ(pos0, sr->CachedPositionForOffset(1));
+  EXPECT_EQ(pos0, sr->CachedPositionForOffset(2));
+  // The last 2 code units should be > 0, and the same position.
+  float pos3 = sr->CachedPositionForOffset(3);
+  EXPECT_GT(pos3, 0);
+  EXPECT_LT(pos3, pos0);
+  EXPECT_EQ(pos3, sr->CachedPositionForOffset(4));
+}
+
 TEST_F(HarfBuzzShaperTest, PositionForOffsetMissingGlyph) {
   String string(u"\u0633\u0644\u0627\u0645");
   HarfBuzzShaper shaper(string);

third_party/blink/renderer/platform/fonts/shaping/shape_result.cc

@@ -1330,6 +1330,7 @@ void ShapeResult::ComputePositionData() const {
   unsigned start_offset = StartIndexForResult();
   unsigned next_character_index = 0;
   float run_advance = 0;
+  float last_x_position = 0;
 
   // Iterate runs/glyphs in the visual order; i.e., from the left edge
   // regardless of the directionality, so that |x_position| is always in
@@ -1355,26 +1356,27 @@ void ShapeResult::ComputePositionData() const {
       if (rtl)
         character_index = num_characters_ - character_index - 1;
 
-      // Multiple glyphs may have the same character index and not all character
-      // indices may have glyphs.
-      // For character indices without glyps set the x-position to that of the
-      // nearest preceding glyph.
-      for (unsigned i = next_character_index; i < character_index; i++) {
-        DCHECK_LT(i, num_characters_);
-        data[i] = {total_advance, false};
-      }
-
-      // For glyphs with the same character index the last logical one wins.
-      // This is the last visual one in LTR, no need to do anything speical.
-
-      // For glyphs with the same character index in LTR take the advance from
-      // the last one but the safe to break flag from the first.
+      // If this glyph is the first glyph of a new cluster, set the data.
+      // Otherwise, |data[character_index]| is already set. Do not overwrite.
       DCHECK_LT(character_index, num_characters_);
-      bool safe_to_break =
-          next_character_index > character_index
-              ? data[next_character_index - 1].safe_to_break_before
-              : glyph_data.safe_to_break_before;
-      data[character_index] = {total_advance, safe_to_break};
+      if (next_character_index <= character_index) {
+        if (next_character_index < character_index) {
+          // Multiple glyphs may have the same character index and not all
+          // character indices may have glyphs. For character indices without
+          // glyphs set the x-position to that of the nearest preceding glyph in
+          // the logical order; i.e., the last position for LTR or this position
+          // for RTL.
+          float x_position = !rtl ? last_x_position : total_advance;
+          for (unsigned i = next_character_index; i < character_index; i++) {
+            DCHECK_LT(i, num_characters_);
+            data[i] = {x_position, false, false};
+          }
+        }
+
+        data[character_index] = {total_advance, true,
+                                 glyph_data.safe_to_break_before};
+        last_x_position = total_advance;
+      }
 
       total_advance += glyph_data.advance;
       next_character_index = character_index + 1;
@@ -1384,8 +1386,11 @@ void ShapeResult::ComputePositionData() const {
 
   // Fill |x_position| for the rest of characters, when they don't have
   // corresponding glyphs.
-  for (unsigned i = next_character_index; i < num_characters_; i++) {
-    data[i] = {run_advance, false};
+  if (next_character_index < num_characters_) {
+    float x_position = !rtl ? last_x_position : run_advance;
+    for (unsigned i = next_character_index; i < num_characters_; i++) {
+      data[i] = {x_position, false, false};
+    }
   }
 
   character_position_->start_offset_ = start_offset;
@@ -1405,37 +1410,11 @@ void ShapeResult::EnsurePositionData() const {
 
 unsigned ShapeResult::CachedOffsetForPosition(float x) const {
   DCHECK(character_position_);
-  unsigned offset;
-  // At or before start, return offset *of* the first character.
-  // At or beyond the end, return offset *after* the last character.
-  if (!Rtl()) {
-    if (x <= 0) {
-      offset = 0;
-    } else if (x >= width_) {
-      offset = num_characters_;
-    } else {
-      offset = character_position_->OffsetForPosition(x);
-      DCHECK_LE(offset, num_characters_);
-    }
+  unsigned offset = character_position_->OffsetForPosition(x, Rtl());
 #if 0
-    // TODO(kojii): This DCHECK fails in ~10 tests. Needs investigations.
-    DCHECK_EQ(OffsetForPosition(x, BreakGlyphsOption::DontBreakGlyphs), offset);
+  // TODO(kojii): This DCHECK fails in ~10 tests. Needs investigations.
+  DCHECK_EQ(OffsetForPosition(x, BreakGlyphsOption::DontBreakGlyphs), offset) << x;
 #endif
-  } else {
-    if (x <= 0) {
-      offset = num_characters_;
-    } else if (x >= width_) {
-      offset = 0;
-    } else {
-      unsigned visual_offset = character_position_->OffsetForPosition(x);
-      DCHECK_LT(visual_offset, num_characters_);
-      if (visual_offset &&
-          x == character_position_->data_[visual_offset].x_position)
-        --visual_offset;
-      offset = num_characters_ - visual_offset - 1;
-    }
-    DCHECK_EQ(OffsetForPosition(x, BreakGlyphsOption::DontBreakGlyphs), offset);
-  }
   return offset;
 }
 
@@ -1443,30 +1422,11 @@ float ShapeResult::CachedPositionForOffset(unsigned offset) const {
   DCHECK_GE(offset, 0u);
   DCHECK_LE(offset, num_characters_);
   DCHECK(character_position_);
-  float position;
-  if (!Rtl()) {
-    position = character_position_->PositionForOffset(offset);
+  float position = character_position_->PositionForOffset(offset, Rtl());
 #if 0
-    // TODO(kojii): This DCHECK fails in several tests. Needs investigations.
-    DCHECK_EQ(PositionForOffset(offset), position);
+  // TODO(kojii): This DCHECK fails in several tests. Needs investigations.
+  DCHECK_EQ(PositionForOffset(offset), position) << offset;
 #endif
-  } else {
-    if (offset >= num_characters_) {
-      position = 0;
-    } else {
-      unsigned visual_offset = num_characters_ - offset;
-      position = character_position_->PositionForOffset(visual_offset);
-    }
-#if DCHECK_IS_ON()
-    if (!offset && !runs_.back()->glyph_data_.IsEmpty() &&
-        runs_.back()->glyph_data_.front().character_index) {
-      // TODO(kojii): |PositionForOffset| incorrectly returns 0 if offset == 0
-      // and the glyph is missing.
-    } else {
-      DCHECK_EQ(PositionForOffset(offset), position);
-    }
-#endif
-  }
   return position;
 }
 
@@ -1489,11 +1449,14 @@ unsigned ShapeResult::CachedPreviousSafeToBreakOffset(unsigned offset) const {
 // TODO(eae): Might be worth trying to set midpoint to ~50% more than the number
 // of characters in the previous line for the first try. Would cut the number
 // of tries in the majority of cases for long strings.
-unsigned ShapeResult::CharacterPositionData::OffsetForPosition(float x) const {
-  // Caller should handle before-start and beyond-end because their results are
-  // not symmetric for LTR and RTL.
-  DCHECK_GT(x, 0);
-  DCHECK_LT(x, width_);
+unsigned ShapeResult::CharacterPositionData::OffsetForPosition(float x,
+                                                               bool rtl) const {
+  // At or before start, return offset *of* the first character.
+  // At or beyond the end, return offset *after* the last character.
+  if (x <= 0)
+    return !rtl ? 0 : data_.size();
+  if (x >= width_)
+    return !rtl ? data_.size() : 0;
 
   // Do a binary search to find the largest x-position that is less than or
   // equal to the supplied x value.
@@ -1504,7 +1467,11 @@ unsigned ShapeResult::CharacterPositionData::OffsetForPosition(float x) const {
     unsigned midpoint = low + (high - low) / 2;
     if (data_[midpoint].x_position <= x &&
         (midpoint + 1 == length || data_[midpoint + 1].x_position > x)) {
-      return midpoint;
+      if (!rtl)
+        return midpoint;
+      // The border belongs to the logical next character.
+      return data_[midpoint].x_position == x ? data_.size() - midpoint
+                                             : data_.size() - midpoint - 1;
     }
     if (x < data_[midpoint].x_position)
       high = midpoint - 1;
@@ -1515,12 +1482,27 @@ unsigned ShapeResult::CharacterPositionData::OffsetForPosition(float x) const {
   return 0;
 }
 
-float ShapeResult::CharacterPositionData::PositionForOffset(
-    unsigned offset) const {
+float ShapeResult::CharacterPositionData::PositionForOffset(unsigned offset,
+                                                            bool rtl) const {
   DCHECK_GT(data_.size(), 0u);
-  if (offset >= data_.size())
-    return width_;
-  return data_[offset].x_position;
+  if (!rtl) {
+    if (offset < data_.size())
+      return data_[offset].x_position;
+  } else {
+    if (offset >= data_.size())
+      return 0;
+    // Return the left edge of the next character because in RTL, the position
+    // is the right edge of the character.
+    for (unsigned visual_offset = data_.size() - offset - 1;
+         visual_offset < data_.size(); visual_offset++) {
+      if (data_[visual_offset].is_cluster_base) {
+        return visual_offset + 1 < data_.size()
+                   ? data_[visual_offset + 1].x_position
+                   : width_;
+      }
+    }
+  }
+  return width_;
 }
 
 unsigned ShapeResult::CharacterPositionData::NextSafeToBreakOffset(

third_party/blink/renderer/platform/fonts/shaping/shape_result.h

@@ -64,6 +64,8 @@ enum class AdjustMidCluster {
 struct ShapeResultCharacterData {
   DISALLOW_NEW_EXCEPT_PLACEMENT_NEW();
   float x_position;
+  // Set for the logical first character of a cluster.
+  unsigned is_cluster_base : 1;
   unsigned safe_to_break_before : 1;
 };
 
@@ -293,10 +295,10 @@ class PLATFORM_EXPORT ShapeResult : public RefCounted<ShapeResult> {
 
     // Returns the offset of the last character that fully fits before the given
     // x-position.
-    unsigned OffsetForPosition(float x) const;
+    unsigned OffsetForPosition(float x, bool rtl) const;
 
     // Returns the x-position for a given offset.
-    float PositionForOffset(unsigned offset) const;
+    float PositionForOffset(unsigned offset, bool rtl) const;
 
    private:
     // This vector is indexed by visual-offset; the character offset from the