[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}

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." />
+<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>