Reland "[Editing] Fix canonical caret position at line break"

This is a reland of d46bdb4f

It was reverted because the new test could time out in Mac.
Fixed by adding '<meta name="timeout" content="long">'.

Original change's description:
> [Editing] Fix canonical caret position at line break
>
> Consider this testcase:
>     <style>div { width: min-content; padding: 5px; }</style>
>     <div contenteditable>line1<wbr>line2</div>
> which is rendered as
>     line1
>     line2
>
> Before this patch, when clicking at the beginning of the 2nd line, the
> caret would appear at the end of the 1st one, because CanonicalPosition
> would search backwards even with a downstream affinity.
>
> Also, when clicking at the beginning of the 1st line and pressing the
> down arrow key, the caret would move to the end of the 1st line instead
> of to the beginning of the 2nd one. And pressing the key again would
> have no effect, the caret would refuse to go down.
>
> This patch fixes these problems by making CanonicalPosition take a
> TextAffinity parameter which affects whether the canonical position is
> first searched backwards or forwards. If no suitable candidate is found,
> it will still search in the other direction.
>
> And then, VisiblePosition::Create takes care of deciding between the
> upstream and the downstream canonical positions, depending on the
> affinity and whether there is a line break.
>
> Bug: 1002937
>
> Web tests:
> TEST=external/wpt/editing/run/caret-navigation-around-line-break.html
>
> Unit tests:
> TEST=All/ParameterizedVisibleUnitsLineTest.inSameLine/0
> TEST=All/ParameterizedVisibleUnitsLineTest.inSameLine/1
> TEST=All/ParameterizedVisibleUnitsWordTest.StartOfWordShadowDOM/0
> TEST=All/ParameterizedVisibleUnitsWordTest.StartOfWordShadowDOM/1
> TEST=VisiblePositionTest.ShadowV0DistributedNodes
> TEST=VisibleUnitsLineTest.endOfLine
> TEST=VisibleUnitsLineTest.isEndOfLine
> TEST=VisibleUnitsLineTest.isLogicalEndOfLine
> TEST=VisibleUnitsLineTest.isStartOfLine
> TEST=VisibleUnitsLineTest.logicalEndOfLine
> TEST=VisibleUnitsLineTest.logicalStartOfLine
> TEST=VisibleUnitsLineTest.startOfLine
> TEST=VisibleUnitsTest.canonicalPositionOfWithHTMLHtmlElement
> TEST=VisibleUnitsTest.canonicalPositionOfWithInputElement
> TEST=VisibleUnitsTest.canonicalizationWithCollapsedSpaceAndIsolatedCombiningCharacter
>
> Change-Id: I82d86d40a87513b2e92c024735957e9f71154094
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2410404
> Commit-Queue: Oriol Brufau <obrufau@igalia.com>
> Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#813744}

Bug: 1002937
Change-Id: Ida01b6d838f03cfbe66e4cba86472ed90499af45
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2450292Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Cr-Commit-Position: refs/heads/master@{#814053}

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

@@ -82,36 +82,49 @@ VisiblePositionTemplate<Strategy> VisiblePositionTemplate<Strategy>::Create(
   DocumentLifecycle::DisallowTransitionScope disallow_transition(
       document.Lifecycle());
 
-  const PositionTemplate<Strategy> deep_position =
-      CanonicalPositionOf(position_with_affinity.GetPosition());
-  if (deep_position.IsNull())
+  // Find the canonical position with a backward preference.
+  const PositionWithAffinityTemplate<Strategy> backward_position =
+      SnapBackward(position_with_affinity.GetPosition());
+  if (backward_position.IsNull())
     return VisiblePositionTemplate<Strategy>();
-  const PositionWithAffinityTemplate<Strategy> downstream_position(
-      deep_position);
-  if (position_with_affinity.Affinity() == TextAffinity::kDownstream)
-    return VisiblePositionTemplate<Strategy>(downstream_position);
+  const PositionWithAffinityTemplate<Strategy> backward_position_upstream(
+      backward_position.GetPosition(), TextAffinity::kUpstream);
+  const PositionWithAffinityTemplate<Strategy> backward_position_downstream(
+      backward_position.GetPosition(), TextAffinity::kDownstream);
 
   if (RuntimeEnabledFeatures::BidiCaretAffinityEnabled() &&
-      NGInlineFormattingContextOf(deep_position)) {
+      NGInlineFormattingContextOf(backward_position.GetPosition())) {
+    if (position_with_affinity.Affinity() == TextAffinity::kDownstream)
+      return VisiblePositionTemplate<Strategy>(backward_position_downstream);
     // When not at a line wrap or bidi boundary, make sure to end up with
     // |TextAffinity::Downstream| affinity.
-    const PositionWithAffinityTemplate<Strategy> upstream_position(
-        deep_position, TextAffinity::kUpstream);
-
-    if (AbsoluteCaretBoundsOf(downstream_position) !=
-        AbsoluteCaretBoundsOf(upstream_position)) {
-      return VisiblePositionTemplate<Strategy>(upstream_position);
+    if (AbsoluteCaretBoundsOf(backward_position_upstream) ==
+        AbsoluteCaretBoundsOf(backward_position_downstream)) {
+      return VisiblePositionTemplate<Strategy>(backward_position_downstream);
     }
-    return VisiblePositionTemplate<Strategy>(downstream_position);
+    return VisiblePositionTemplate<Strategy>(backward_position_upstream);
   }
 
-  // When not at a line wrap, make sure to end up with
-  // |TextAffinity::Downstream| affinity.
-  const PositionWithAffinityTemplate<Strategy> upstream_position(
-      deep_position, TextAffinity::kUpstream);
-  if (InSameLine(downstream_position, upstream_position))
-    return VisiblePositionTemplate<Strategy>(downstream_position);
-  return VisiblePositionTemplate<Strategy>(upstream_position);
+  // Find the canonical position with a forward preference. If backward_position
+  // has a downstream affinity, it means that we couldn't find any backward
+  // candidate, so they must be equal and we can avoid calling SnapForward().
+  // The forward canonical position can't be null because we already checked
+  // that the backward one is not null.
+  const PositionWithAffinityTemplate<Strategy> forward_position =
+      backward_position.Affinity() == TextAffinity::kDownstream
+          ? backward_position
+          : SnapForward(position_with_affinity.GetPosition());
+  DCHECK(forward_position.IsNotNull());
+
+  // When not at a line wrap, make sure to end up with the backward canonical
+  // position with |TextAffinity::Downstream| affinity.
+  if (InSameLine(backward_position_upstream, forward_position))
+    return VisiblePositionTemplate<Strategy>(backward_position_downstream);
+  if (position_with_affinity.Affinity() == TextAffinity::kUpstream)
+    return VisiblePositionTemplate<Strategy>(backward_position_upstream);
+  if (StartOfLine(forward_position).IsNull())
+    return VisiblePositionTemplate<Strategy>(backward_position_downstream);
+  return VisiblePositionTemplate<Strategy>(forward_position);
 }
 
 template <typename Strategy>

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

@@ -156,19 +156,35 @@ TEST_F(VisiblePositionTest, ShadowV0DistributedNodes) {
 
   EXPECT_EQ(Position(one->firstChild(), 0),
             CanonicalPositionOf(Position(one, 0)));
+  EXPECT_EQ(Position(one->firstChild(), 0),
+            SnapBackward(Position(one, 0)).GetPosition());
+  EXPECT_EQ(Position(one->firstChild(), 0),
+            SnapForward(Position(one, 0)).GetPosition());
   EXPECT_EQ(Position(one->firstChild(), 0),
             CreateVisiblePosition(Position(one, 0)).DeepEquivalent());
   EXPECT_EQ(Position(one->firstChild(), 2),
             CanonicalPositionOf(Position(two, 0)));
+  EXPECT_EQ(Position(one->firstChild(), 2),
+            SnapBackward(Position(two, 0)).GetPosition());
+  EXPECT_EQ(Position(two->firstChild(), 0),
+            SnapForward(Position(two, 0)).GetPosition());
   EXPECT_EQ(Position(one->firstChild(), 2),
             CreateVisiblePosition(Position(two, 0)).DeepEquivalent());
 
   EXPECT_EQ(PositionInFlatTree(five->firstChild(), 2),
             CanonicalPositionOf(PositionInFlatTree(one, 0)));
+  EXPECT_EQ(PositionInFlatTree(five->firstChild(), 2),
+            SnapBackward(PositionInFlatTree(one, 0)).GetPosition());
+  EXPECT_EQ(PositionInFlatTree(one->firstChild(), 0),
+            SnapForward(PositionInFlatTree(one, 0)).GetPosition());
   EXPECT_EQ(PositionInFlatTree(five->firstChild(), 2),
             CreateVisiblePosition(PositionInFlatTree(one, 0)).DeepEquivalent());
   EXPECT_EQ(PositionInFlatTree(four->firstChild(), 2),
             CanonicalPositionOf(PositionInFlatTree(two, 0)));
+  EXPECT_EQ(PositionInFlatTree(four->firstChild(), 2),
+            SnapBackward(PositionInFlatTree(two, 0)).GetPosition());
+  EXPECT_EQ(PositionInFlatTree(two->firstChild(), 0),
+            SnapForward(PositionInFlatTree(two, 0)).GetPosition());
   EXPECT_EQ(PositionInFlatTree(four->firstChild(), 2),
             CreateVisiblePosition(PositionInFlatTree(two, 0)).DeepEquivalent());
 }

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

@@ -75,32 +75,7 @@ static PositionType CanonicalizeCandidate(const PositionType& candidate) {
 }
 
 template <typename PositionType>
-static PositionType CanonicalPosition(const PositionType& position) {
-  // Sometimes updating selection positions can be extremely expensive and
-  // occur frequently.  Often calling preventDefault on mousedown events can
-  // avoid doing unnecessary text selection work.  http://crbug.com/472258.
-  TRACE_EVENT0("input", "VisibleUnits::canonicalPosition");
-
-  // FIXME (9535):  Canonicalizing to the leftmost candidate means that if
-  // we're at a line wrap, we will ask layoutObjects to paint downstream
-  // carets for other layoutObjects. To fix this, we need to either a) add
-  // code to all paintCarets to pass the responsibility off to the appropriate
-  // layoutObject for VisiblePosition's like these, or b) canonicalize to the
-  // rightmost candidate unless the affinity is upstream.
-  if (position.IsNull())
-    return PositionType();
-
-  DCHECK(position.GetDocument());
-  DCHECK(!position.GetDocument()->NeedsLayoutTreeUpdate());
-
-  const PositionType& backward_candidate = MostBackwardCaretPosition(position);
-  if (IsVisuallyEquivalentCandidate(backward_candidate))
-    return backward_candidate;
-
-  const PositionType& forward_candidate = MostForwardCaretPosition(position);
-  if (IsVisuallyEquivalentCandidate(forward_candidate))
-    return forward_candidate;
-
+static PositionType SnapFallbackTemplate(const PositionType& position) {
   // When neither upstream or downstream gets us to a candidate
   // (upstream/downstream won't leave blocks or enter new ones), we search
   // forward and backward until we find one.
@@ -153,12 +128,93 @@ static PositionType CanonicalPosition(const PositionType& position) {
   return next;
 }
 
+template <typename Strategy>
+static PositionWithAffinityTemplate<Strategy> SnapBackwardTemplate(
+    const PositionTemplate<Strategy>& position) {
+  // Sometimes updating selection positions can be extremely expensive and
+  // occur frequently.  Often calling preventDefault on mousedown events can
+  // avoid doing unnecessary text selection work.  http://crbug.com/472258.
+  TRACE_EVENT0("input", "VisibleUnits::SnapBackward");
+
+  if (position.IsNull())
+    return PositionWithAffinityTemplate<Strategy>();
+
+  DCHECK(position.GetDocument());
+  DCHECK(!position.GetDocument()->NeedsLayoutTreeUpdate());
+
+  const PositionTemplate<Strategy>& candidate1 =
+      MostBackwardCaretPosition(position);
+  if (IsVisuallyEquivalentCandidate(candidate1)) {
+    return PositionWithAffinityTemplate<Strategy>(candidate1,
+                                                  TextAffinity::kUpstream);
+  }
+
+  const PositionTemplate<Strategy>& candidate2 =
+      MostForwardCaretPosition(position);
+  if (IsVisuallyEquivalentCandidate(candidate2)) {
+    return PositionWithAffinityTemplate<Strategy>(candidate2,
+                                                  TextAffinity::kDownstream);
+  }
+
+  return PositionWithAffinityTemplate<Strategy>(SnapFallbackTemplate(position),
+                                                TextAffinity::kDownstream);
+}
+
+PositionWithAffinity SnapBackward(const Position& position) {
+  return SnapBackwardTemplate(position);
+}
+
+PositionInFlatTreeWithAffinity SnapBackward(
+    const PositionInFlatTree& position) {
+  return SnapBackwardTemplate(position);
+}
+
+template <typename Strategy>
+static PositionWithAffinityTemplate<Strategy> SnapForwardTemplate(
+    const PositionTemplate<Strategy>& position) {
+  // Sometimes updating selection positions can be extremely expensive and
+  // occur frequently.  Often calling preventDefault on mousedown events can
+  // avoid doing unnecessary text selection work.  http://crbug.com/472258.
+  TRACE_EVENT0("input", "VisibleUnits::SnapForward");
+
+  if (position.IsNull())
+    return PositionWithAffinityTemplate<Strategy>();
+
+  DCHECK(position.GetDocument());
+  DCHECK(!position.GetDocument()->NeedsLayoutTreeUpdate());
+
+  const PositionTemplate<Strategy>& candidate1 =
+      MostForwardCaretPosition(position);
+  if (IsVisuallyEquivalentCandidate(candidate1)) {
+    return PositionWithAffinityTemplate<Strategy>(candidate1,
+                                                  TextAffinity::kDownstream);
+  }
+
+  const PositionTemplate<Strategy>& candidate2 =
+      MostBackwardCaretPosition(position);
+  if (IsVisuallyEquivalentCandidate(candidate2)) {
+    return PositionWithAffinityTemplate<Strategy>(candidate2,
+                                                  TextAffinity::kDownstream);
+  }
+
+  return PositionWithAffinityTemplate<Strategy>(SnapFallbackTemplate(position),
+                                                TextAffinity::kDownstream);
+}
+
+PositionWithAffinity SnapForward(const Position& position) {
+  return SnapForwardTemplate(position);
+}
+
+PositionInFlatTreeWithAffinity SnapForward(const PositionInFlatTree& position) {
+  return SnapForwardTemplate(position);
+}
+
 Position CanonicalPositionOf(const Position& position) {
-  return CanonicalPosition(position);
+  return SnapBackward(position).GetPosition();
 }
 
 PositionInFlatTree CanonicalPositionOf(const PositionInFlatTree& position) {
-  return CanonicalPosition(position);
+  return SnapBackward(position).GetPosition();
 }
 
 template <typename Strategy>

third_party/blink/renderer/core/editing/visible_units.h

@@ -92,6 +92,12 @@ CORE_EXPORT bool IsVisuallyEquivalentCandidate(const PositionInFlatTree&);
 // If true, adjacent candidates are visually distinct.
 CORE_EXPORT bool EndsOfNodeAreVisuallyDistinctPositions(const Node*);
 
+CORE_EXPORT PositionWithAffinity SnapBackward(const Position&);
+CORE_EXPORT PositionInFlatTreeWithAffinity
+SnapBackward(const PositionInFlatTree&);
+CORE_EXPORT PositionWithAffinity SnapForward(const Position&);
+CORE_EXPORT PositionInFlatTreeWithAffinity
+SnapForward(const PositionInFlatTree&);
 CORE_EXPORT Position CanonicalPositionOf(const Position&);
 CORE_EXPORT PositionInFlatTree CanonicalPositionOf(const PositionInFlatTree&);
 
@@ -156,11 +162,17 @@ EphemeralRange ExpandRangeToSentenceBoundary(const EphemeralRange&);
 CORE_EXPORT VisiblePosition StartOfLine(const VisiblePosition&);
 CORE_EXPORT VisiblePositionInFlatTree
 StartOfLine(const VisiblePositionInFlatTree&);
+CORE_EXPORT PositionWithAffinity StartOfLine(const PositionWithAffinity&);
+CORE_EXPORT PositionInFlatTreeWithAffinity
+StartOfLine(const PositionInFlatTreeWithAffinity&);
 // TODO(yosin) Return values of |VisiblePosition| version of |endOfLine()| with
 // shadow tree isn't defined well. We should not use it for shadow tree.
 CORE_EXPORT VisiblePosition EndOfLine(const VisiblePosition&);
 CORE_EXPORT VisiblePositionInFlatTree
 EndOfLine(const VisiblePositionInFlatTree&);
+CORE_EXPORT PositionWithAffinity EndOfLine(const PositionWithAffinity&);
+CORE_EXPORT PositionInFlatTreeWithAffinity
+EndOfLine(const PositionInFlatTreeWithAffinity&);
 CORE_EXPORT bool InSameLine(const VisiblePosition&, const VisiblePosition&);
 CORE_EXPORT bool InSameLine(const VisiblePositionInFlatTree&,
                             const VisiblePositionInFlatTree&);

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

@@ -313,6 +313,8 @@ PositionWithAffinityTemplate<Strategy> StartOfLineAlgorithm(
       vis_pos, c.GetPosition());
 }
 
+}  // namespace
+
 PositionWithAffinity StartOfLine(const PositionWithAffinity& current_position) {
   return StartOfLineAlgorithm<EditingStrategy>(current_position);
 }
@@ -322,8 +324,6 @@ PositionInFlatTreeWithAffinity StartOfLine(
   return StartOfLineAlgorithm<EditingInFlatTreeStrategy>(current_position);
 }
 
-}  // namespace
-
 // FIXME: Rename this function to reflect the fact it ignores bidi levels.
 VisiblePosition StartOfLine(const VisiblePosition& current_position) {
   DCHECK(current_position.IsValid()) << current_position;
@@ -394,11 +394,11 @@ static PositionWithAffinityTemplate<Strategy> EndOfLineAlgorithm(
       candidate_position, current_position.GetPosition());
 }
 
-static PositionWithAffinity EndOfLine(const PositionWithAffinity& position) {
+PositionWithAffinity EndOfLine(const PositionWithAffinity& position) {
   return EndOfLineAlgorithm<EditingStrategy>(position);
 }
 
-static PositionInFlatTreeWithAffinity EndOfLine(
+PositionInFlatTreeWithAffinity EndOfLine(
     const PositionInFlatTreeWithAffinity& position) {
   return EndOfLineAlgorithm<EditingInFlatTreeStrategy>(position);
 }

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

@@ -121,8 +121,12 @@ TEST_F(VisibleUnitsLineTest, endOfLine) {
       PositionInFlatTree(seven, 7),
       EndOfLine(CreateVisiblePositionInFlatTree(*one, 1)).DeepEquivalent());
 
+  EXPECT_EQ(Position(seven, 7), EndOfLine(CreateVisiblePositionInDOMTree(
+                                              *two, 0, TextAffinity::kUpstream))
+                                    .DeepEquivalent());
   EXPECT_EQ(
-      Position(seven, 7),
+      // The result on legacy layout is broken and not worth fixing.
+      LayoutNGEnabled() ? Position(two, 2) : Position(five, 5),
       EndOfLine(CreateVisiblePositionInDOMTree(*two, 0)).DeepEquivalent());
   EXPECT_EQ(
       PositionInFlatTree(two, 2),
@@ -139,6 +143,11 @@ TEST_F(VisibleUnitsLineTest, endOfLine) {
   EXPECT_EQ(
       // The result on legacy layout is broken and not worth fixing.
       LayoutNGEnabled() ? Position(two, 2) : Position(five, 5),
+      EndOfLine(
+          CreateVisiblePositionInDOMTree(*three, 0, TextAffinity::kUpstream))
+          .DeepEquivalent());
+  EXPECT_EQ(
+      Position(four, 4),
       EndOfLine(CreateVisiblePositionInDOMTree(*three, 0)).DeepEquivalent());
   EXPECT_EQ(
       PositionInFlatTree(four, 4),
@@ -204,11 +213,9 @@ TEST_F(VisibleUnitsLineTest, isEndOfLine) {
   EXPECT_FALSE(IsEndOfLine(CreateVisiblePositionInDOMTree(*one, 1)));
   EXPECT_FALSE(IsEndOfLine(CreateVisiblePositionInFlatTree(*one, 1)));
 
-  // The result on legacy layout is broken and not worth fixing.
-  if (LayoutNGEnabled())
-    EXPECT_TRUE(IsEndOfLine(CreateVisiblePositionInFlatTree(*two, 2)));
-  else
-    EXPECT_FALSE(IsEndOfLine(CreateVisiblePositionInDOMTree(*two, 2)));
+  EXPECT_TRUE(IsEndOfLine(
+      CreateVisiblePositionInFlatTree(*two, 2, TextAffinity::kUpstream)));
+  EXPECT_FALSE(IsEndOfLine(CreateVisiblePositionInDOMTree(*two, 2)));
   EXPECT_TRUE(IsEndOfLine(CreateVisiblePositionInFlatTree(*two, 2)));
 
   EXPECT_FALSE(IsEndOfLine(CreateVisiblePositionInDOMTree(*three, 3)));
@@ -262,10 +269,14 @@ TEST_F(VisibleUnitsLineTest, isLogicalEndOfLine) {
   EXPECT_FALSE(IsLogicalEndOfLine(CreateVisiblePositionInFlatTree(*one, 1)));
 
   // The result in legacy layout is broken and not worth fixing.
-  if (LayoutNGEnabled())
-    EXPECT_TRUE(IsLogicalEndOfLine(CreateVisiblePositionInDOMTree(*two, 2)));
-  else
-    EXPECT_FALSE(IsLogicalEndOfLine(CreateVisiblePositionInDOMTree(*two, 2)));
+  if (LayoutNGEnabled()) {
+    EXPECT_TRUE(IsLogicalEndOfLine(
+        CreateVisiblePositionInDOMTree(*two, 2, TextAffinity::kUpstream)));
+  } else {
+    EXPECT_FALSE(IsLogicalEndOfLine(
+        CreateVisiblePositionInDOMTree(*two, 2, TextAffinity::kUpstream)));
+  }
+  EXPECT_FALSE(IsLogicalEndOfLine(CreateVisiblePositionInDOMTree(*two, 2)));
   EXPECT_TRUE(IsLogicalEndOfLine(CreateVisiblePositionInFlatTree(*two, 2)));
 
   EXPECT_FALSE(IsLogicalEndOfLine(CreateVisiblePositionInDOMTree(*three, 3)));
@@ -315,15 +326,25 @@ TEST_P(ParameterizedVisibleUnitsLineTest, inSameLine) {
       InSameLine(PositionWithAffinityInDOMTree(*two->firstChild(), 0),
                  PositionWithAffinityInDOMTree(*four->firstChild(), 0)));
 
-  EXPECT_TRUE(InSameLine(CreateVisiblePositionInDOMTree(*one, 0),
-                         CreateVisiblePositionInDOMTree(*two, 0)));
-  EXPECT_TRUE(
+  EXPECT_TRUE(InSameLine(
+      CreateVisiblePositionInDOMTree(*one, 0),
+      CreateVisiblePositionInDOMTree(*two, 0, TextAffinity::kUpstream)));
+  EXPECT_FALSE(InSameLine(CreateVisiblePositionInDOMTree(*one, 0),
+                          CreateVisiblePositionInDOMTree(*two, 0)));
+  EXPECT_TRUE(InSameLine(CreateVisiblePositionInDOMTree(*one->firstChild(), 0),
+                         CreateVisiblePositionInDOMTree(
+                             *two->firstChild(), 0, TextAffinity::kUpstream)));
+  EXPECT_FALSE(
       InSameLine(CreateVisiblePositionInDOMTree(*one->firstChild(), 0),
                  CreateVisiblePositionInDOMTree(*two->firstChild(), 0)));
   EXPECT_FALSE(
       InSameLine(CreateVisiblePositionInDOMTree(*one->firstChild(), 0),
                  CreateVisiblePositionInDOMTree(*five->firstChild(), 0)));
   EXPECT_FALSE(
+      InSameLine(CreateVisiblePositionInDOMTree(*two->firstChild(), 0,
+                                                TextAffinity::kUpstream),
+                 CreateVisiblePositionInDOMTree(*four->firstChild(), 0)));
+  EXPECT_TRUE(
       InSameLine(CreateVisiblePositionInDOMTree(*two->firstChild(), 0),
                  CreateVisiblePositionInDOMTree(*four->firstChild(), 0)));
 
@@ -380,7 +401,9 @@ TEST_F(VisibleUnitsLineTest, isStartOfLine) {
   EXPECT_FALSE(IsStartOfLine(CreateVisiblePositionInDOMTree(*two, 0)));
   EXPECT_FALSE(IsStartOfLine(CreateVisiblePositionInFlatTree(*two, 0)));
 
-  EXPECT_FALSE(IsStartOfLine(CreateVisiblePositionInDOMTree(*three, 0)));
+  EXPECT_FALSE(IsStartOfLine(
+      CreateVisiblePositionInDOMTree(*three, 0, TextAffinity::kUpstream)));
+  EXPECT_TRUE(IsStartOfLine(CreateVisiblePositionInDOMTree(*three, 0)));
   EXPECT_TRUE(IsStartOfLine(CreateVisiblePositionInFlatTree(*three, 0)));
 
   EXPECT_FALSE(IsStartOfLine(CreateVisiblePositionInDOMTree(*four, 0)));
@@ -435,6 +458,11 @@ TEST_F(VisibleUnitsLineTest, logicalEndOfLine) {
                 .DeepEquivalent());
 
   EXPECT_EQ(Position(seven, 7),
+            LogicalEndOfLine(CreateVisiblePositionInDOMTree(
+                                 *two, 0, TextAffinity::kUpstream))
+                .DeepEquivalent());
+  // The result on legacy layout is broken and not worth fixing.
+  EXPECT_EQ(LayoutNGEnabled() ? Position(two, 2) : Position(five, 5),
             LogicalEndOfLine(CreateVisiblePositionInDOMTree(*two, 0))
                 .DeepEquivalent());
   EXPECT_EQ(PositionInFlatTree(two, 2),
@@ -452,6 +480,10 @@ TEST_F(VisibleUnitsLineTest, logicalEndOfLine) {
   // DOM VisiblePosition canonicalization moves input position to (two, 2),
   // which yields wrong results in both legacy layout and LayoutNG.
   EXPECT_EQ(LayoutNGEnabled() ? Position(two, 2) : Position(five, 5),
+            LogicalEndOfLine(CreateVisiblePositionInDOMTree(
+                                 *three, 0, TextAffinity::kUpstream))
+                .DeepEquivalent());
+  EXPECT_EQ(Position(four, 4),
             LogicalEndOfLine(CreateVisiblePositionInDOMTree(*three, 0))
                 .DeepEquivalent());
   EXPECT_EQ(PositionInFlatTree(four, 4),
@@ -522,6 +554,10 @@ TEST_F(VisibleUnitsLineTest, logicalStartOfLine) {
                 .DeepEquivalent());
 
   EXPECT_EQ(Position(one, 0),
+            LogicalStartOfLine(CreateVisiblePositionInDOMTree(
+                                   *two, 0, TextAffinity::kUpstream))
+                .DeepEquivalent());
+  EXPECT_EQ(Position(five, 0),
             LogicalStartOfLine(CreateVisiblePositionInDOMTree(*two, 0))
                 .DeepEquivalent());
   EXPECT_EQ(PositionInFlatTree(five, 0),
@@ -536,15 +572,21 @@ TEST_F(VisibleUnitsLineTest, logicalStartOfLine) {
                 .DeepEquivalent());
 
   EXPECT_EQ(Position(five, 0),
+            LogicalStartOfLine(CreateVisiblePositionInDOMTree(
+                                   *three, 0, TextAffinity::kUpstream))
+                .DeepEquivalent());
+  EXPECT_EQ(Position(three, 0),
             LogicalStartOfLine(CreateVisiblePositionInDOMTree(*three, 0))
                 .DeepEquivalent());
   EXPECT_EQ(PositionInFlatTree(three, 0),
             LogicalStartOfLine(CreateVisiblePositionInFlatTree(*three, 1))
                 .DeepEquivalent());
 
-  // TODO(yosin) logicalStartOfLine(four, 1) -> (two, 2) is a broken result.
-  // We keep it as a marker for future change.
-  EXPECT_EQ(Position(two, 2),
+  EXPECT_EQ(Position(three, 0),
+            LogicalStartOfLine(CreateVisiblePositionInDOMTree(
+                                   *four, 1, TextAffinity::kUpstream))
+                .DeepEquivalent());
+  EXPECT_EQ(Position(three, 0),
             LogicalStartOfLine(CreateVisiblePositionInDOMTree(*four, 1))
                 .DeepEquivalent());
   EXPECT_EQ(PositionInFlatTree(three, 0),
@@ -611,8 +653,11 @@ TEST_F(VisibleUnitsLineTest, startOfLine) {
       PositionInFlatTree(one, 0),
       StartOfLine(CreateVisiblePositionInFlatTree(*one, 1)).DeepEquivalent());
 
+  EXPECT_EQ(Position(one, 0), StartOfLine(CreateVisiblePositionInDOMTree(
+                                              *two, 0, TextAffinity::kUpstream))
+                                  .DeepEquivalent());
   EXPECT_EQ(
-      Position(one, 0),
+      Position(five, 0),
       StartOfLine(CreateVisiblePositionInDOMTree(*two, 0)).DeepEquivalent());
   EXPECT_EQ(
       PositionInFlatTree(five, 0),
@@ -625,17 +670,23 @@ TEST_F(VisibleUnitsLineTest, startOfLine) {
       PositionInFlatTree(five, 0),
       StartOfLine(CreateVisiblePositionInFlatTree(*two, 1)).DeepEquivalent());
 
+  EXPECT_EQ(Position(five, 0),
+            StartOfLine(CreateVisiblePositionInDOMTree(*three, 0,
+                                                       TextAffinity::kUpstream))
+                .DeepEquivalent());
   EXPECT_EQ(
-      Position(five, 0),
+      Position(three, 0),
       StartOfLine(CreateVisiblePositionInDOMTree(*three, 0)).DeepEquivalent());
   EXPECT_EQ(
       PositionInFlatTree(three, 0),
       StartOfLine(CreateVisiblePositionInFlatTree(*three, 1)).DeepEquivalent());
 
-  // TODO(yosin) startOfLine(four, 1) -> (two, 2) is a broken result. We keep
-  // it as a marker for future change.
+  EXPECT_EQ(Position(three, 0),
+            StartOfLine(CreateVisiblePositionInDOMTree(*four, 1,
+                                                       TextAffinity::kUpstream))
+                .DeepEquivalent());
   EXPECT_EQ(
-      Position(two, 2),
+      Position(three, 0),
       StartOfLine(CreateVisiblePositionInDOMTree(*four, 1)).DeepEquivalent());
   EXPECT_EQ(
       PositionInFlatTree(three, 0),

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

@@ -147,8 +147,10 @@ VisiblePositionTemplate<Strategy> StartOfParagraphAlgorithm(
     const VisiblePositionTemplate<Strategy>& visible_position,
     EditingBoundaryCrossingRule boundary_crossing_rule) {
   DCHECK(visible_position.IsValid()) << visible_position;
-  return CreateVisiblePosition(StartOfParagraphAlgorithm(
-      visible_position.DeepEquivalent(), boundary_crossing_rule));
+  return CreateVisiblePosition(
+      StartOfParagraphAlgorithm(visible_position.DeepEquivalent(),
+                                boundary_crossing_rule),
+      TextAffinity::kDownstream);
 }
 
 template <typename Strategy>
@@ -245,8 +247,10 @@ VisiblePositionTemplate<Strategy> EndOfParagraphAlgorithm(
     const VisiblePositionTemplate<Strategy>& visible_position,
     EditingBoundaryCrossingRule boundary_crossing_rule) {
   DCHECK(visible_position.IsValid()) << visible_position;
-  return CreateVisiblePosition(EndOfParagraphAlgorithm(
-      visible_position.DeepEquivalent(), boundary_crossing_rule));
+  return CreateVisiblePosition(
+      EndOfParagraphAlgorithm(visible_position.DeepEquivalent(),
+                              boundary_crossing_rule),
+      TextAffinity::kUpstream);
 }
 
 template <typename Strategy>

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

@@ -113,25 +113,66 @@ TEST_F(VisibleUnitsTest, canonicalPositionOfWithHTMLHtmlElement) {
 
   EXPECT_EQ(Position(),
             CanonicalPositionOf(Position(GetDocument().documentElement(), 0)));
+  EXPECT_EQ(
+      Position(),
+      SnapBackward(Position(GetDocument().documentElement(), 0)).GetPosition());
+  EXPECT_EQ(
+      Position(),
+      SnapForward(Position(GetDocument().documentElement(), 0)).GetPosition());
 
   EXPECT_EQ(Position(one->firstChild(), 0),
             CanonicalPositionOf(Position(one, 0)));
+  EXPECT_EQ(Position(one->firstChild(), 0),
+            SnapBackward(Position(one, 0)).GetPosition());
+  EXPECT_EQ(Position(one->firstChild(), 0),
+            SnapForward(Position(one, 0)).GetPosition());
+
   EXPECT_EQ(Position(one->firstChild(), 1),
             CanonicalPositionOf(Position(one, 1)));
+  EXPECT_EQ(Position(one->firstChild(), 1),
+            SnapBackward(Position(one, 1)).GetPosition());
+  EXPECT_EQ(Position(one->firstChild(), 1),
+            SnapForward(Position(one, 1)).GetPosition());
 
   EXPECT_EQ(Position(one->firstChild(), 0),
             CanonicalPositionOf(Position(one->firstChild(), 0)));
+  EXPECT_EQ(Position(one->firstChild(), 0),
+            SnapBackward(Position(one->firstChild(), 0)).GetPosition());
+  EXPECT_EQ(Position(one->firstChild(), 0),
+            SnapForward(Position(one->firstChild(), 0)).GetPosition());
+
   EXPECT_EQ(Position(one->firstChild(), 1),
             CanonicalPositionOf(Position(one->firstChild(), 1)));
+  EXPECT_EQ(Position(one->firstChild(), 1),
+            SnapBackward(Position(one->firstChild(), 1)).GetPosition());
+  EXPECT_EQ(Position(one->firstChild(), 1),
+            SnapForward(Position(one->firstChild(), 1)).GetPosition());
 
   EXPECT_EQ(Position(html, 0), CanonicalPositionOf(Position(html, 0)));
+  EXPECT_EQ(Position(html, 0), SnapBackward(Position(html, 0)).GetPosition());
+  EXPECT_EQ(Position(html, 0), SnapForward(Position(html, 0)).GetPosition());
+
   EXPECT_EQ(Position(html, 1), CanonicalPositionOf(Position(html, 1)));
+  EXPECT_EQ(Position(html, 1), SnapBackward(Position(html, 1)).GetPosition());
+  EXPECT_EQ(Position(html, 1), SnapForward(Position(html, 1)).GetPosition());
+
   EXPECT_EQ(Position(html, 2), CanonicalPositionOf(Position(html, 2)));
+  EXPECT_EQ(Position(html, 2), SnapBackward(Position(html, 2)).GetPosition());
+  EXPECT_EQ(Position(html, 2), SnapForward(Position(html, 2)).GetPosition());
 
   EXPECT_EQ(Position(two->firstChild(), 0),
             CanonicalPositionOf(Position(two, 0)));
+  EXPECT_EQ(Position(two->firstChild(), 0),
+            SnapBackward(Position(two, 0)).GetPosition());
+  EXPECT_EQ(Position(two->firstChild(), 0),
+            SnapForward(Position(two, 0)).GetPosition());
+
   EXPECT_EQ(Position(two->firstChild(), 2),
             CanonicalPositionOf(Position(two, 1)));
+  EXPECT_EQ(Position(two->firstChild(), 2),
+            SnapBackward(Position(two, 1)).GetPosition());
+  EXPECT_EQ(Position(two->firstChild(), 2),
+            SnapForward(Position(two, 1)).GetPosition());
 }
 
 // For http://crbug.com/695317
@@ -139,13 +180,20 @@ TEST_F(VisibleUnitsTest, canonicalPositionOfWithInputElement) {
   SetBodyContent("<input>123");
   Element* const input = GetDocument().QuerySelector("input");
 
-  EXPECT_EQ(Position::BeforeNode(*input),
-            CanonicalPositionOf(Position::FirstPositionInNode(
-                *GetDocument().documentElement())));
+  Position position =
+      Position::FirstPositionInNode(*GetDocument().documentElement());
+  EXPECT_EQ(Position::BeforeNode(*input), CanonicalPositionOf(position));
+  EXPECT_EQ(Position::BeforeNode(*input), SnapBackward(position).GetPosition());
+  EXPECT_EQ(Position::BeforeNode(*input), SnapForward(position).GetPosition());
 
+  PositionInFlatTree pos_in_flat_tree =
+      PositionInFlatTree::FirstPositionInNode(*GetDocument().documentElement());
+  EXPECT_EQ(PositionInFlatTree::BeforeNode(*input),
+            CanonicalPositionOf(pos_in_flat_tree));
+  EXPECT_EQ(PositionInFlatTree::BeforeNode(*input),
+            SnapBackward(pos_in_flat_tree).GetPosition());
   EXPECT_EQ(PositionInFlatTree::BeforeNode(*input),
-            CanonicalPositionOf(PositionInFlatTree::FirstPositionInNode(
-                *GetDocument().documentElement())));
+            SnapForward(pos_in_flat_tree).GetPosition());
 }
 
 TEST_F(VisibleUnitsTest, characterBefore) {
@@ -757,8 +805,12 @@ TEST_F(VisibleUnitsTest,
 
   Node* paragraph = GetDocument().QuerySelector("p");
   Node* text = paragraph->firstChild();
-  Position start = CanonicalPositionOf(Position::BeforeNode(*paragraph));
-  EXPECT_EQ(Position(text, 2), start);
+  EXPECT_EQ(Position(text, 2),
+            CanonicalPositionOf(Position::BeforeNode(*paragraph)));
+  EXPECT_EQ(Position(text, 2),
+            SnapBackward(Position::BeforeNode(*paragraph)).GetPosition());
+  EXPECT_EQ(Position(text, 2),
+            SnapForward(Position::BeforeNode(*paragraph)).GetPosition());
 }
 
 TEST_F(VisibleUnitsTest, MostForwardCaretPositionWithInvisibleFirstLetter) {

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

@@ -233,8 +233,13 @@ TEST_P(ParameterizedVisibleUnitsWordTest, StartOfWordShadowDOM) {
                 StartOfWordPosition(
                     CreateVisiblePositionInFlatTree(*two, 1).DeepEquivalent()))
                 .DeepEquivalent());
-  // DOM tree canonicalization moves the result to a wrong position.
-  EXPECT_EQ(Position(two, 2),
+  EXPECT_EQ(Position(three, 0),
+            CreateVisiblePosition(
+                StartOfWordPosition(CreateVisiblePositionInDOMTree(
+                                        *three, 1, TextAffinity::kUpstream)
+                                        .DeepEquivalent()))
+                .DeepEquivalent());
+  EXPECT_EQ(Position(three, 0),
             CreateVisiblePosition(
                 StartOfWordPosition(
                     CreateVisiblePositionInDOMTree(*three, 1).DeepEquivalent()))

third_party/blink/web_tests/external/wpt/editing/run/caret-navigation-around-line-break.html

+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>Caret navigation around line break</title>
+<link rel="author" title="Oriol Brufau" href="mailto:obrufau@igalia.com">
+<meta name="assert" content="This test checks that caret navigation works well around various kinds of line breaks." />
+<meta name="timeout" content="long">
+<style>
+.test {
+  font-size: 16px;
+  line-height: 20px;
+  padding: 4px;
+  width: 5.5ch;
+  padding: 5px;
+  font-family: monospace;
+  word-wrap: break-word;
+}
+</style>
+
+<div class="test" contenteditable data-title="no separator"
+    >line1line2</div>
+<div class="test" contenteditable data-title="<br> separator"
+    >line1<br>line2</div>
+<div class="test" contenteditable data-title="<wbr> separator"
+    >line1<wbr>line2</div>
+<div class="test" contenteditable data-title="<span> separator"
+    >line1<span></span>line2</div>
+<div class="test" contenteditable data-title="two <span> separators"
+    >line1<span></span><span></span>line2</div>
+
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="/resources/testdriver.js"></script>
+<script src="/resources/testdriver-vendor.js"></script>
+<script src="/resources/testdriver-actions.js"></script>
+<script>
+const KEY_CODE_MAP = {
+  'ArrowLeft':  '\uE012',
+  'ArrowUp':    '\uE013',
+  'ArrowRight': '\uE014',
+  'ArrowDown':  '\uE015',
+};
+
+function click(target, x, y) {
+  return new test_driver.Actions()
+    .pointerMove(x, y, {origin: target})
+    .pointerDown()
+    .pointerUp()
+    .send();
+}
+
+const s = getSelection();
+for (const test of document.querySelectorAll(".test")) {
+  const padding = 4;
+  const halfLineWidth = Math.floor((test.offsetWidth - padding) / 2);
+  const halfLineHeight = Math.floor(20 / 2);
+  const hasSeparator = test.firstChild !== test.lastChild;
+  const line1 = {
+    node: test.firstChild,
+    start: 0,
+    end: "line1".length,
+  };
+  const line2 = {
+    node: test.lastChild,
+    start: hasSeparator ? 0 : "line1".length,
+    end: hasSeparator ? "line2".length : "line1line2".length,
+  };
+
+  promise_test(async t => {
+    // Click at the start of line 1
+    await click(test, -halfLineWidth, -halfLineHeight);
+    assert_equals(s.anchorNode, line1.node, "Caret is in line 1");
+    assert_equals(s.anchorOffset, line1.start, "Caret is at the start of line 1");
+
+    // Move down, expect start of line 2
+    await test_driver.send_keys(test, KEY_CODE_MAP.ArrowDown);
+    assert_equals(s.anchorNode, line2.node, "Caret moved to line 2");
+    assert_equals(s.anchorOffset, line2.start, "Caret moved to the start of line 2");
+
+    // Click at the end of line 1
+    await click(test, +halfLineWidth, -halfLineHeight);
+    range = getSelection().getRangeAt(0);
+    assert_equals(s.anchorNode, line1.node, "Caret is in line 1");
+    assert_equals(s.anchorOffset, line1.end, "Caret is at the end of line 1");
+
+    // Move down, expect end of line 2
+    await test_driver.send_keys(test, KEY_CODE_MAP.ArrowDown);
+    assert_equals(s.anchorNode, line2.node, "Caret moved to line 2");
+    assert_equals(s.anchorOffset, line2.end, "Caret moved to the end of line 2");
+  }, test.dataset.title + " - move down");
+
+  promise_test(async t => {
+    // Click at the start of line 2
+    await click(test, -halfLineWidth, +halfLineHeight);
+    assert_equals(s.anchorNode, line2.node, "Caret is in line 2");
+    assert_equals(s.anchorOffset, line2.start, "Caret is at the start of line 2");
+
+    // Move up, expect start of line 1
+    await test_driver.send_keys(test, KEY_CODE_MAP.ArrowUp);
+    assert_equals(s.anchorNode, line1.node, "Caret moved to line 1");
+    assert_equals(s.anchorOffset, line1.start, "Caret moved to the start of line 1");
+
+    // Click at the end of line 2
+    await click(test, +halfLineWidth, +halfLineHeight);
+    assert_equals(s.anchorNode, line2.node, "Caret is in line 2");
+    assert_equals(s.anchorOffset, line2.end, "Caret is at the end of line 2");
+
+    // Move up, expect end of line 1
+    await test_driver.send_keys(test, KEY_CODE_MAP.ArrowUp);
+    assert_equals(s.anchorNode, line1.node, "Caret moved to line 1");
+    assert_equals(s.anchorOffset, line1.end, "Caret moved to the end of line 1");
+  }, test.dataset.title + " - move up");
+
+  promise_test(async t => {
+    // Click at the end of line 1
+    await click(test, +halfLineWidth, -halfLineHeight);
+    assert_equals(s.anchorNode, line1.node, "Caret is in line 1");
+    assert_equals(s.anchorOffset, line1.end, "Caret is at the end of line 1");
+
+    // Move right, expect start or start+1 of line 2
+    await test_driver.send_keys(test, KEY_CODE_MAP.ArrowRight);
+    assert_equals(s.anchorNode, line2.node, "Caret moved to line 2");
+    assert_in_array(s.anchorOffset, [line2.start, line2.start + 1], "Caret moved to the start or start+1 of line 2");
+  }, test.dataset.title + " - move right");
+
+  promise_test(async t => {
+    // Click at the start of line 2
+    await click(test, -halfLineWidth, +halfLineHeight);
+    assert_equals(s.anchorNode, line2.node, "Caret is in line 2");
+    assert_equals(s.anchorOffset, line2.start, "Caret is at the start of line 2");
+
+    // Move left, expect end or end-1 of line 1
+    await test_driver.send_keys(test, KEY_CODE_MAP.ArrowLeft);
+    assert_equals(s.anchorNode, line1.node, "Caret moved to line 1");
+    assert_in_array(s.anchorOffset, [line1.end, line1.end - 1], "Caret moved to the end or end-1 of line 1");
+  }, test.dataset.title + " - move left");
+}
+</script>