[LayoutNG] Make ComputeNGCaretPosition() to work correctly for RTL inline boxes

This patch changes |ComputeNGCaretPosition()| to return valid |NGCaretPosition|
for inline boxes in RTL and pass the test[1] for enabling EditnigNG.

Before this patch, |ComputeNGCaretPosition()| to return position relative to
left of inline box of specified DOM position.

Example:
<div dir=rtl><bdo id=box1 dir=rtl>ABC</bdo>><bdo id=box2 dir=rtl>DEF</bdo>
=> ComputeNGCaretPosition("ABC"@3)
Before: "DEF"@0
After: "ABC@3"

[1] editing/caret/caret-height-multi-line.html

Bug: 707656
Change-Id: I785f87ca7b18f05a55bd809b6b390d68dac89c8c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2275742
Commit-Queue: Kent Tamura <tkent@chromium.org>
Auto-Submit: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#784259}

third_party/blink/renderer/core/editing/local_caret_rect_bidi_test.cc

@@ -140,9 +140,7 @@ TEST_P(ParameterizedLocalCaretRectBidiTest,
       "dir=rtl>ABC</bdo></bdo></bdo>|jkl</bdo></div>");
   const PositionWithAffinity position_with_affinity(position,
                                                     TextAffinity::kDownstream);
-  // TODO(xiaochengh): Decide if the behavior difference is worth to fix.
-  EXPECT_EQ(LayoutNGEnabled() ? PhysicalRect(60, 0, 1, 10)
-                              : PhysicalRect(90, 0, 1, 10),
+  EXPECT_EQ(PhysicalRect(90, 0, 1, 10),
             LocalCaretRectOfPosition(position_with_affinity).rect);
 }
 
@@ -176,9 +174,7 @@ TEST_P(ParameterizedLocalCaretRectBidiTest,
       "dir=rtl>DEF<bdo dir=ltr>abc</bdo></bdo></bdo></bdo>|mno</bdo></div>");
   const PositionWithAffinity position_with_affinity(position,
                                                     TextAffinity::kDownstream);
-  // TODO(xiaochengh): Decide if the behavior difference is worth to fix.
-  EXPECT_EQ(LayoutNGEnabled() ? PhysicalRect(90, 0, 1, 10)
-                              : PhysicalRect(120, 0, 1, 10),
+  EXPECT_EQ(PhysicalRect(120, 0, 1, 10),
             LocalCaretRectOfPosition(position_with_affinity).rect);
 }
 
@@ -321,9 +317,7 @@ TEST_P(ParameterizedLocalCaretRectBidiTest,
       "dir=rtl>|ABC</bdo>def</bdo>GHI</bdo></bdo></div>");
   const PositionWithAffinity position_with_affinity(position,
                                                     TextAffinity::kDownstream);
-  // TODO(xiaochengh): Decide if the behavior difference is worth to fix.
-  EXPECT_EQ(LayoutNGEnabled() ? PhysicalRect(30, 0, 1, 10)
-                              : PhysicalRect(60, 0, 1, 10),
+  EXPECT_EQ(PhysicalRect(60, 0, 1, 10),
             LocalCaretRectOfPosition(position_with_affinity).rect);
 }
 
@@ -357,9 +351,7 @@ TEST_P(ParameterizedLocalCaretRectBidiTest,
       "dir=rtl><bdo dir=ltr>|abc</bdo>DEF</bdo>ghi</bdo>JKL</bdo></bdo></div>");
   const PositionWithAffinity position_with_affinity(position,
                                                     TextAffinity::kDownstream);
-  // TODO(xiaochengh): Decide if the behavior difference is worth to fix.
-  EXPECT_EQ(LayoutNGEnabled() ? PhysicalRect(30, 0, 1, 10)
-                              : PhysicalRect(60, 0, 1, 10),
+  EXPECT_EQ(PhysicalRect(60, 0, 1, 10),
             LocalCaretRectOfPosition(position_with_affinity).rect);
 }
 
@@ -514,7 +506,8 @@ TEST_P(ParameterizedLocalCaretRectBidiTest,
       "dir=ltr>def<bdo dir=rtl>ABC|</bdo></bdo></bdo></bdo>MNO</bdo></div>");
   const PositionWithAffinity position_with_affinity(position,
                                                     TextAffinity::kDownstream);
-  EXPECT_EQ(PhysicalRect(150, 0, 1, 10),
+  EXPECT_EQ(LayoutNGEnabled() ? PhysicalRect(120, 0, 1, 10)
+                              : PhysicalRect(150, 0, 1, 10),
             LocalCaretRectOfPosition(position_with_affinity).rect);
 }
 
@@ -653,7 +646,8 @@ TEST_P(ParameterizedLocalCaretRectBidiTest,
       "dir=ltr><bdo dir=rtl>ABC</bdo>def</bdo>GHI</bdo>jkl</bdo></bdo></div>");
   const PositionWithAffinity position_with_affinity(position,
                                                     TextAffinity::kDownstream);
-  EXPECT_EQ(PhysicalRect(30, 0, 1, 10),
+  EXPECT_EQ(LayoutNGEnabled() ? PhysicalRect(0, 0, 1, 10)
+                              : PhysicalRect(30, 0, 1, 10),
             LocalCaretRectOfPosition(position_with_affinity).rect);
 }
 
@@ -808,9 +802,7 @@ TEST_P(ParameterizedLocalCaretRectBidiTest,
       "dir=rtl>DEF<bdo dir=ltr>abc</bdo></bdo></bdo></bdo>|mno</bdo></div>");
   const PositionWithAffinity position_with_affinity(position,
                                                     TextAffinity::kDownstream);
-  // TODO(xiaochengh): Decide if the behavior difference is worth to fix.
-  EXPECT_EQ(LayoutNGEnabled() ? PhysicalRect(180, 0, 1, 10)
-                              : PhysicalRect(150, 0, 1, 10),
+  EXPECT_EQ(PhysicalRect(150, 0, 1, 10),
             LocalCaretRectOfPosition(position_with_affinity).rect);
 }
 
@@ -985,9 +977,7 @@ TEST_P(ParameterizedLocalCaretRectBidiTest,
       "dir=rtl><bdo dir=ltr>|abc</bdo>DEF</bdo>ghi</bdo>JKL</bdo></bdo></div>");
   const PositionWithAffinity position_with_affinity(position,
                                                     TextAffinity::kDownstream);
-  // TODO(xiaochengh): Decide if the behavior difference is worth to fix.
-  EXPECT_EQ(LayoutNGEnabled() ? PhysicalRect(299, 0, 1, 10)
-                              : PhysicalRect(270, 0, 1, 10),
+  EXPECT_EQ(PhysicalRect(270, 0, 1, 10),
             LocalCaretRectOfPosition(position_with_affinity).rect);
 }
 
@@ -1110,7 +1100,8 @@ TEST_P(ParameterizedLocalCaretRectBidiTest,
       "dir=ltr>abc|</bdo></bdo></bdo>JKL</bdo></div>");
   const PositionWithAffinity position_with_affinity(position,
                                                     TextAffinity::kDownstream);
-  EXPECT_EQ(PhysicalRect(210, 0, 1, 10),
+  EXPECT_EQ(LayoutNGEnabled() ? PhysicalRect(240, 0, 1, 10)
+                              : PhysicalRect(210, 0, 1, 10),
             LocalCaretRectOfPosition(position_with_affinity).rect);
 }
 
@@ -1142,7 +1133,8 @@ TEST_P(ParameterizedLocalCaretRectBidiTest,
       "dir=ltr>def<bdo dir=rtl>ABC|</bdo></bdo></bdo></bdo>MNO</bdo></div>");
   const PositionWithAffinity position_with_affinity(position,
                                                     TextAffinity::kDownstream);
-  EXPECT_EQ(PhysicalRect(180, 0, 1, 10),
+  EXPECT_EQ(LayoutNGEnabled() ? PhysicalRect(210, 0, 1, 10)
+                              : PhysicalRect(180, 0, 1, 10),
             LocalCaretRectOfPosition(position_with_affinity).rect);
 }
 
@@ -1249,7 +1241,8 @@ TEST_P(ParameterizedLocalCaretRectBidiTest,
       "dir=ltr>abc</bdo>DEF</bdo>ghi</bdo></bdo></div>");
   const PositionWithAffinity position_with_affinity(position,
                                                     TextAffinity::kDownstream);
-  EXPECT_EQ(PhysicalRect(240, 0, 1, 10),
+  EXPECT_EQ(LayoutNGEnabled() ? PhysicalRect(270, 0, 1, 10)
+                              : PhysicalRect(240, 0, 1, 10),
             LocalCaretRectOfPosition(position_with_affinity).rect);
 }
 
@@ -1281,7 +1274,8 @@ TEST_P(ParameterizedLocalCaretRectBidiTest,
       "dir=ltr><bdo dir=rtl>ABC</bdo>def</bdo>GHI</bdo>jkl</bdo></bdo></div>");
   const PositionWithAffinity position_with_affinity(position,
                                                     TextAffinity::kDownstream);
-  EXPECT_EQ(PhysicalRect(240, 0, 1, 10),
+  EXPECT_EQ(LayoutNGEnabled() ? PhysicalRect(270, 0, 1, 10)
+                              : PhysicalRect(240, 0, 1, 10),
             LocalCaretRectOfPosition(position_with_affinity).rect);
 }
 

third_party/blink/renderer/core/editing/local_caret_rect_test.cc

@@ -654,8 +654,8 @@ TEST_P(ParameterizedLocalCaretRectTest, TextAndImageMixedHeight) {
                 Position::AfterNode(img), TextAffinity::kDownstream)));
 
   // TODO(xiaochengh): Should return the same result for legacy and LayoutNG.
-  EXPECT_EQ(LayoutNGEnabled() ? LocalCaretRect(img.GetLayoutObject(),
-                                               PhysicalRect(9, -5, 1, 10))
+  EXPECT_EQ(LayoutNGEnabled() ? LocalCaretRect(text2->GetLayoutObject(),
+                                               PhysicalRect(20, 0, 1, 10))
                               : LocalCaretRect(text2->GetLayoutObject(),
                                                PhysicalRect(20, 5, 1, 10)),
             LocalCaretRectOfPosition(PositionWithAffinity(

third_party/blink/renderer/core/layout/ng/inline/ng_caret_position.cc

@@ -257,7 +257,6 @@ NGCaretPosition BetterCandidateBetween(const NGCaretPosition& current,
     DCHECK(!IsUpstreamAfterLineBreak(other));
     return other;
   }
-  DCHECK(IsUpstreamAfterLineBreak(other));
   return current;
 }
 
@@ -267,7 +266,8 @@ NGCaretPosition BetterCandidateBetween(const NGCaretPosition& current,
 // of this file for details.
 NGCaretPosition ComputeNGCaretPosition(const LayoutBlockFlow& context,
                                        unsigned offset,
-                                       TextAffinity affinity) {
+                                       TextAffinity affinity,
+                                       const LayoutText* layout_text) {
   NGInlineCursor cursor(context);
 
   NGCaretPosition candidate;
@@ -281,8 +281,13 @@ NGCaretPosition ComputeNGCaretPosition(const LayoutBlockFlow& context,
     // TODO(xiaochengh): Handle caret poisition in empty container (e.g. empty
     // line box).
 
-    if (resolution.type == ResolutionType::kResolved)
-      return AdjustCaretPositionForBidiText(resolution.caret_position);
+    if (resolution.type == ResolutionType::kResolved) {
+      candidate = resolution.caret_position;
+      if (!layout_text ||
+          candidate.cursor.Current().GetLayoutObject() == layout_text)
+        return AdjustCaretPositionForBidiText(resolution.caret_position);
+      continue;
+    }
 
     DCHECK_EQ(ResolutionType::kFoundCandidate, resolution.type);
     candidate = BetterCandidateBetween(candidate, resolution.caret_position,
@@ -292,25 +297,31 @@ NGCaretPosition ComputeNGCaretPosition(const LayoutBlockFlow& context,
   return AdjustCaretPositionForBidiText(candidate);
 }
 
-NGCaretPosition ComputeNGCaretPosition(const PositionWithAffinity& position) {
-  LayoutBlockFlow* context =
-      NGInlineFormattingContextOf(position.GetPosition());
+NGCaretPosition ComputeNGCaretPosition(
+    const PositionWithAffinity& position_with_affinity) {
+  const Position& position = position_with_affinity.GetPosition();
+  LayoutBlockFlow* context = NGInlineFormattingContextOf(position);
   if (!context)
     return NGCaretPosition();
 
   const NGOffsetMapping* mapping = NGInlineNode::GetOffsetMapping(context);
   DCHECK(mapping);
   const base::Optional<unsigned> maybe_offset =
-      mapping->GetTextContentOffset(position.GetPosition());
+      mapping->GetTextContentOffset(position);
   if (!maybe_offset.has_value()) {
     // TODO(xiaochengh): Investigate if we reach here.
     NOTREACHED();
     return NGCaretPosition();
   }
 
+  const LayoutText* const layout_text =
+      position.IsOffsetInAnchor() && IsA<Text>(position.AnchorNode())
+          ? To<Text>(position.AnchorNode())->GetLayoutObject()
+          : nullptr;
+
   const unsigned offset = *maybe_offset;
-  const TextAffinity affinity = position.Affinity();
-  return ComputeNGCaretPosition(*context, offset, affinity);
+  const TextAffinity affinity = position_with_affinity.Affinity();
+  return ComputeNGCaretPosition(*context, offset, affinity, layout_text);
 }
 
 Position NGCaretPosition::ToPositionInDOMTree() const {

third_party/blink/renderer/core/layout/ng/inline/ng_caret_position.h

@@ -44,13 +44,17 @@ struct NGCaretPosition {
 // affinity, returns the corresponding NGCaretPosition, or null if not found.
 // Note that in many cases, null result indicates that we have reached an
 // unexpected case that is not properly handled.
-CORE_EXPORT NGCaretPosition ComputeNGCaretPosition(const LayoutBlockFlow&,
-                                                   unsigned,
-                                                   TextAffinity);
+// When |layout_text| isn't |nullptr|, this functions returns position for
+// |layout_text| in multiple candidates.
+CORE_EXPORT NGCaretPosition
+ComputeNGCaretPosition(const LayoutBlockFlow& context,
+                       unsigned offset,
+                       TextAffinity affinity,
+                       const LayoutText* layout_text = nullptr);
 
 // Shorthand of the above when the input is a position instead of a
 // (context, offset) pair.
-NGCaretPosition ComputeNGCaretPosition(const PositionWithAffinity&);
+CORE_EXPORT NGCaretPosition ComputeNGCaretPosition(const PositionWithAffinity&);
 
 }  // namespace blink
 

third_party/blink/renderer/core/layout/ng/inline/ng_caret_position_test.cc

@@ -308,4 +308,54 @@ TEST_F(NGCaretPositionTest, InlineBlockBeforeContent) {
              base::Optional<unsigned>(text_offset));
 }
 
+TEST_F(NGCaretPositionTest, InlineBoxesLTR) {
+  SetBodyInnerHTML(
+      "<div dir=ltr>"
+      "<bdo id=box1 dir=ltr>ABCD</bdo>"
+      "<bdo id=box2 dir=ltr style='font-size: 150%'>EFG</bdo></div>");
+
+  // text_content:
+  //    [0] U+202D LEFT-TO_RIGHT_OVERRIDE
+  //    [1:4] "ABCD"
+  //    [5] U+202C POP DIRECTIONAL FORMATTING
+  //    [6] U+202D LEFT-TO_RIGHT_OVERRIDE
+  //    [7:8] "EF"
+  //    [9] U+202C POP DIRECTIONAL FORMATTING
+  const Node& box1 = *GetElementById("box1")->firstChild();
+  const Node& box2 = *GetElementById("box1")->firstChild();
+
+  TEST_CARET(
+      blink::ComputeNGCaretPosition(PositionWithAffinity(Position(box1, 4))),
+      FragmentOf(&box1), kAtTextOffset, base::Optional<unsigned>(5));
+
+  TEST_CARET(
+      blink::ComputeNGCaretPosition(PositionWithAffinity(Position(box2, 0))),
+      FragmentOf(&box2), kAtTextOffset, base::Optional<unsigned>(1));
+}
+
+TEST_F(NGCaretPositionTest, InlineBoxesRTL) {
+  SetBodyInnerHTML(
+      "<div dir=rtl>"
+      "<bdo id=box1 dir=rtl>ABCD</bdo>"
+      "<bdo id=box2 dir=rtl style='font-size: 150%'>EFG</bdo></div>");
+
+  // text_content:
+  //    [0] U+202E RIGHT-TO_LEFT _OVERRIDE
+  //    [1:4] "ABCD"
+  //    [5] U+202C POP DIRECTIONAL FORMATTING
+  //    [6] U+202E RIGHT-TO_LEFT _OVERRIDE
+  //    [7:8] "EF"
+  //    [9] U+202C POP DIRECTIONAL FORMATTING
+  const Node& box1 = *GetElementById("box1")->firstChild();
+  const Node& box2 = *GetElementById("box1")->firstChild();
+
+  TEST_CARET(
+      blink::ComputeNGCaretPosition(PositionWithAffinity(Position(box1, 4))),
+      FragmentOf(&box1), kAtTextOffset, base::Optional<unsigned>(5));
+
+  TEST_CARET(
+      blink::ComputeNGCaretPosition(PositionWithAffinity(Position(box2, 0))),
+      FragmentOf(&box2), kAtTextOffset, base::Optional<unsigned>(1));
+}
+
 }  // namespace blink