Modify LocalCaretRectOfPositionTemplate for line break.

This patch modifies the function so that it returns
valid LayoutRect for line break.

The function didn't work if (LayoutObject, offset) points after line break:
1. (LayoutBR, 1)
2. ("foo\n", 4) on white-space: pre.
Because ComputeInlineBoxPositionForTextNode didn't receive such position.

Historically, VisualPosition canonicalization updated Position not to point
end of Node and LocalCaretRectOfPosition and ComputeInlineBoxPosition assumed
that.

This patch updates LocalCaretRectOfPositionTemplate
to check if Position is after line break(NeedsLineEndAdjustment)
and return the first InlineBoxPosition of next line(NextLinePositionOf).

* Notes: This kind of adjustment should be done inside
ComputeInlineBoxPosition.
However, SelectionModifierCharacter and SelectionModifierWord
depend on the current behavior of ComputeInlineBoxPosition
that it returns null InlineBoxPosition for such position of line end
(see L261 and L332 for each function).
Since regression is about LocalCaretRectOfPositionTemplate,
this patch only fixes the function.

Bug: 807930
Change-Id: I46630d86401c165e0bb908d748adc069e03a5374
Reviewed-on: https://chromium-review.googlesource.com/906263
Commit-Queue: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Reviewed-by: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547639}

third_party/WebKit/Source/core/editing/InlineBoxPosition.cpp

@@ -153,7 +153,7 @@ InlineBoxPosition AdjustInlineBoxPositionForPrimaryDirection(
   return InlineBoxPosition(result_box, result_box->CaretLeftmostOffset());
 }
 
-InlineBoxPosition AdjustInlineBoxPositionForTextDirection(
+InlineBoxPosition AdjustInlineBoxPositionForTextDirectionInternal(
     InlineBox* inline_box,
     int caret_offset,
     UnicodeBidi unicode_bidi) {
@@ -414,4 +414,12 @@ InlineBoxPosition ComputeInlineBoxPositionForInlineAdjustedPosition(
   return ComputeInlineBoxPositionForInlineAdjustedPositionAlgorithm(position);
 }
 
+InlineBoxPosition AdjustInlineBoxPositionForTextDirection(
+    InlineBox* inline_box,
+    int caret_offset,
+    UnicodeBidi unicode_bidi) {
+  return AdjustInlineBoxPositionForTextDirectionInternal(
+      inline_box, caret_offset, unicode_bidi);
+}
+
 }  // namespace blink

third_party/WebKit/Source/core/editing/InlineBoxPosition.h

@@ -38,6 +38,7 @@
 namespace blink {
 
 class InlineBox;
+enum class UnicodeBidi : unsigned;
 
 struct InlineBoxPosition {
   STACK_ALLOCATED();
@@ -63,6 +64,13 @@ struct InlineBoxPosition {
   }
 };
 
+// TODO(yoichio): ComputeInlineBoxPosition returns null if position is at the
+// end of line and We fixed LocalCaretRectOfPosition for such position with
+// NeedsLineEndAdjustment and NextLinePositionOf.
+// We should include the fix into ComputeInlineBoxPosition however
+// SelectionModifierCharacter and SelectionModifierWord
+// depend on the null-line-end behavior of CIBP.
+// Move the fix into the CIBP while fixing the modifier functions.
 CORE_EXPORT InlineBoxPosition
 ComputeInlineBoxPosition(const PositionWithAffinity&);
 CORE_EXPORT InlineBoxPosition
@@ -79,6 +87,10 @@ InlineBoxPosition ComputeInlineBoxPositionForInlineAdjustedPosition(
 InlineBoxPosition ComputeInlineBoxPositionForInlineAdjustedPosition(
     const PositionInFlatTreeWithAffinity&);
 
+InlineBoxPosition AdjustInlineBoxPositionForTextDirection(InlineBox*,
+                                                          int,
+                                                          UnicodeBidi);
+
 // The print for |InlineBoxPosition| is available only for testing
 // in "webkit_unit_tests", and implemented in
 // "core/editing/InlineBoxPositionTest.cpp".

third_party/WebKit/Source/core/editing/LocalCaretRect.cpp

@@ -36,6 +36,7 @@
 #include "core/editing/PositionWithAffinity.h"
 #include "core/editing/VisiblePosition.h"
 #include "core/layout/api/LineLayoutAPIShim.h"
+#include "core/layout/line/InlineTextBox.h"
 #include "core/layout/line/RootInlineBox.h"
 #include "core/layout/ng/inline/ng_caret_rect.h"
 #include "core/layout/ng/inline/ng_offset_mapping.h"
@@ -44,6 +45,50 @@ namespace blink {
 
 namespace {
 
+// Returns true if |layout_object| and |offset| points after line end.
+template <typename Strategy>
+bool NeedsLineEndAdjustment(
+    const PositionWithAffinityTemplate<Strategy>& adjusted) {
+  const PositionTemplate<Strategy>& position = adjusted.GetPosition();
+  const LayoutObject& layout_object = *position.AnchorNode()->GetLayoutObject();
+  if (!layout_object.IsText())
+    return false;
+  const LayoutText& layout_text = ToLayoutText(layout_object);
+  if (layout_text.IsBR())
+    return position.IsAfterAnchor();
+  // For normal text nodes.
+  if (!layout_text.Style()->PreserveNewline())
+    return false;
+  if (!layout_text.TextLength() ||
+      layout_text.CharacterAt(layout_text.TextLength() - 1) != '\n')
+    return false;
+  if (position.IsAfterAnchor())
+    return true;
+  return position.IsOffsetInAnchor() &&
+         position.OffsetInContainerNode() ==
+             static_cast<int>(layout_text.TextLength());
+}
+
+// Returns the first InlineBoxPosition at next line of last InlineBoxPosition
+// in |layout_object| if it exists to avoid making InlineBoxPosition at end of
+// line.
+template <typename Strategy>
+InlineBoxPosition NextLinePositionOf(
+    const PositionWithAffinityTemplate<Strategy>& adjusted) {
+  const PositionTemplate<Strategy>& position = adjusted.GetPosition();
+  const LayoutText& layout_text =
+      ToLayoutTextOrDie(*position.AnchorNode()->GetLayoutObject());
+  InlineTextBox* const last = layout_text.LastTextBox();
+  const RootInlineBox& root = last->Root();
+  const RootInlineBox* const next_root = root.NextRootBox();
+  if (!next_root)
+    return InlineBoxPosition();
+  InlineBox* const inline_box = next_root->FirstLeafChild();
+  return AdjustInlineBoxPositionForTextDirection(
+      inline_box, inline_box->CaretMinOffset(),
+      layout_text.Style()->GetUnicodeBidi());
+}
+
 template <typename Strategy>
 LocalCaretRect LocalCaretRectOfPositionTemplate(
     const PositionWithAffinityTemplate<Strategy>& position,
@@ -72,7 +117,9 @@ LocalCaretRect LocalCaretRectOfPositionTemplate(
     DCHECK_EQ(PrimaryDirectionOf(*position.AnchorNode()),
               PrimaryDirectionOf(*adjusted.AnchorNode()));
     const InlineBoxPosition& box_position =
-        ComputeInlineBoxPositionForInlineAdjustedPosition(adjusted);
+        NeedsLineEndAdjustment(adjusted)
+            ? NextLinePositionOf(adjusted)
+            : ComputeInlineBoxPositionForInlineAdjustedPosition(adjusted);
 
     if (box_position.inline_box) {
       const LayoutObject* box_layout_object =

third_party/WebKit/Source/core/editing/LocalCaretRectTest.cpp

@@ -702,13 +702,10 @@ TEST_P(ParameterizedLocalCaretRectTest, AfterLineBreak) {
   EXPECT_EQ(LocalCaretRect(foo->GetLayoutObject(), LayoutRect(30, 0, 1, 10)),
             LocalCaretRectOfPosition(PositionWithAffinity(
                 Position::AfterNode(*foo), TextAffinity::kDownstream)));
-  // TODO(yoichio): Legacy should return valid rect: crbug.com/812535.
-  EXPECT_EQ(LayoutNGEnabled() ? LocalCaretRect(second_br->GetLayoutObject(),
-                                               LayoutRect(0, 10, 1, 10))
-                              : LocalCaretRect(first_br->GetLayoutObject(),
-                                               LayoutRect(0, 0, 0, 0)),
-            LocalCaretRectOfPosition(PositionWithAffinity(
-                Position::AfterNode(*first_br), TextAffinity::kDownstream)));
+  EXPECT_EQ(
+      LocalCaretRect(second_br->GetLayoutObject(), LayoutRect(0, 10, 1, 10)),
+      LocalCaretRectOfPosition(PositionWithAffinity(
+          Position::AfterNode(*first_br), TextAffinity::kDownstream)));
   EXPECT_EQ(LocalCaretRect(second_br->GetLayoutObject(),
                            LayoutNGEnabled() ? LayoutRect(0, 10, 1, 10)
                                              : LayoutRect(0, 0, 0, 0)),
@@ -746,13 +743,9 @@ TEST_P(ParameterizedLocalCaretRectTest, AfterLineBreakInPre2) {
   EXPECT_EQ(LocalCaretRect(foo->GetLayoutObject(), LayoutRect(30, 0, 1, 10)),
             LocalCaretRectOfPosition(PositionWithAffinity(
                 Position(foo, 3), TextAffinity::kDownstream)));
-  // TODO(yoichio): Legacy should return valid rect: crbug.com/812535.
-  EXPECT_EQ(
-      LayoutNGEnabled()
-          ? LocalCaretRect(br->GetLayoutObject(), LayoutRect(0, 10, 1, 10))
-          : LocalCaretRect(foo->GetLayoutObject(), LayoutRect(0, 0, 0, 0)),
-      LocalCaretRectOfPosition(
-          PositionWithAffinity(Position(foo, 4), TextAffinity::kDownstream)));
+  EXPECT_EQ(LocalCaretRect(br->GetLayoutObject(), LayoutRect(0, 10, 1, 10)),
+            LocalCaretRectOfPosition(PositionWithAffinity(
+                Position(foo, 4), TextAffinity::kDownstream)));
   EXPECT_EQ(LocalCaretRect(br->GetLayoutObject(), LayoutNGEnabled()
                                                       ? LayoutRect(0, 10, 1, 10)
                                                       : LayoutRect(0, 0, 0, 0)),
@@ -773,9 +766,9 @@ TEST_P(ParameterizedLocalCaretRectTest, AfterLineBreakTextArea) {
       LocalCaretRect(inner_text->GetLayoutObject(), LayoutRect(0, 10, 1, 10)),
       LocalCaretRectOfPosition(PositionWithAffinity(
           Position(inner_text, 4), TextAffinity::kDownstream)));
-  // TODO(yoichio): Following should return valid rect: crbug.com/812535.
+  const Node* hidden_br = inner_text->nextSibling();
   EXPECT_EQ(
-      LocalCaretRect(inner_text->GetLayoutObject(), LayoutRect(0, 0, 0, 0)),
+      LocalCaretRect(hidden_br->GetLayoutObject(), LayoutRect(0, 20, 1, 10)),
       LocalCaretRectOfPosition(PositionWithAffinity(
           Position(inner_text, 5), TextAffinity::kDownstream)));
 }
@@ -870,8 +863,9 @@ TEST_P(ParameterizedLocalCaretRectTest, AfterLineBreakInPreBlockLTRLineLTR) {
       SetCaretTextToBody("<pre dir='ltr'>foo\n|<bdo dir='ltr'>abc</bdo></pre>");
   LayoutRect position_rect, visible_position_rect;
   std::tie(position_rect, visible_position_rect) = GetLayoutRects(caret);
+  // TODO(xiaochengh): Should return the same result for legacy and LayoutNG.
   EXPECT_EQ(
-      LayoutNGEnabled() ? LayoutRect(30, 0, 1, 10) : LayoutRect(0, 0, 0, 0),
+      LayoutNGEnabled() ? LayoutRect(30, 0, 1, 10) : LayoutRect(0, 10, 1, 10),
       position_rect);
   EXPECT_EQ(LayoutRect(0, 10, 1, 10), visible_position_rect);
 };
@@ -885,7 +879,7 @@ TEST_P(ParameterizedLocalCaretRectTest, AfterLineBreakInPreBlockLTRLineRTL) {
   std::tie(position_rect, visible_position_rect) = GetLayoutRects(caret);
   // TODO(xiaochengh): Should return the same result for legacy and LayoutNG.
   EXPECT_EQ(
-      LayoutNGEnabled() ? LayoutRect(30, 0, 1, 10) : LayoutRect(0, 0, 0, 0),
+      LayoutNGEnabled() ? LayoutRect(30, 0, 1, 10) : LayoutRect(0, 10, 1, 10),
       position_rect);
   EXPECT_EQ(
       LayoutNGEnabled() ? LayoutRect(30, 10, 1, 10) : LayoutRect(0, 10, 1, 10),
@@ -899,10 +893,10 @@ TEST_P(ParameterizedLocalCaretRectTest, AfterLineBreakInPreBlockRTLLineLTR) {
       SetCaretTextToBody("<pre dir='rtl'>foo\n|<bdo dir='ltr'>abc</bdo></pre>");
   LayoutRect position_rect, visible_position_rect;
   std::tie(position_rect, visible_position_rect) = GetLayoutRects(caret);
-  EXPECT_EQ(
-      LayoutNGEnabled() ? LayoutRect(270, 0, 1, 10) : LayoutRect(0, 0, 0, 0),
-      position_rect);
   // TODO(xiaochengh): Should return the same result for legacy and LayoutNG.
+  EXPECT_EQ(LayoutNGEnabled() ? LayoutRect(270, 0, 1, 10)
+                              : LayoutRect(299, 10, 1, 10),
+            position_rect);
   EXPECT_EQ(LayoutNGEnabled() ? LayoutRect(270, 10, 1, 10)
                               : LayoutRect(299, 10, 1, 10),
             visible_position_rect);
@@ -915,9 +909,10 @@ TEST_P(ParameterizedLocalCaretRectTest, AfterLineBreakInPreBlockRTLLineRTL) {
       SetCaretTextToBody("<pre dir='rtl'>foo\n|<bdo dir='rtl'>abc</bdo></pre>");
   LayoutRect position_rect, visible_position_rect;
   std::tie(position_rect, visible_position_rect) = GetLayoutRects(caret);
-  EXPECT_EQ(
-      LayoutNGEnabled() ? LayoutRect(270, 0, 1, 10) : LayoutRect(0, 0, 0, 0),
-      position_rect);
+  // TODO(xiaochengh): Should return the same result for legacy and LayoutNG.
+  EXPECT_EQ(LayoutNGEnabled() ? LayoutRect(270, 0, 1, 10)
+                              : LayoutRect(299, 10, 1, 10),
+            position_rect);
   EXPECT_EQ(LayoutRect(299, 10, 1, 10), visible_position_rect);
 };
 }  // namespace blink

third_party/WebKit/Source/core/editing/RenderedPositionTest.cpp

@@ -10,6 +10,7 @@
 #include "core/editing/testing/EditingTestBase.h"
 #include "core/frame/Settings.h"
 #include "core/html/forms/HTMLInputElement.h"
+#include "core/html/forms/TextControlElement.h"
 #include "core/layout/LayoutBox.h"
 #include "core/paint/PaintLayerScrollableArea.h"
 #include "core/paint/compositing/CompositedSelection.h"
@@ -181,4 +182,72 @@ TEST_P(RenderedPositionTest, PositionInScroller) {
             composited_selection.end.edge_bottom_in_layer);
 }
 
+// TODO(yoichio): These helper static functions are introduced to avoid
+// conflicting while merging this change into M66.
+// Refactor them into RenderedPositionTest member.
+namespace rendered_position_test {
+static void SetUpinternal(RenderedPositionTest* test) {
+  // Enable compositing.
+  test->GetPage().GetSettings().SetAcceleratedCompositingEnabled(true);
+  test->GetDocument().View()->SetParentVisible(true);
+  test->GetDocument().View()->SetSelfVisible(true);
+  test->GetDocument().View()->UpdateAllLifecyclePhases();
+}
+
+static void FocusAndSelect(RenderedPositionTest* test,
+                           Element* focus,
+                           const Node& select) {
+  DCHECK(focus);
+  focus->focus();
+  test->Selection().SetSelection(
+      SelectionInDOMTree::Builder().SelectAllChildren(select).Build(),
+      SetSelectionOptions::Builder().SetShouldShowHandle(true).Build());
+  test->UpdateAllLifecyclePhases();
+}
+}  // namespace rendered_position_test
+
+// crbug.com/807930
+TEST_P(RenderedPositionTest, ContentEditableLinebreak) {
+  rendered_position_test::SetUpinternal(this);
+  LoadAhem();
+  SetBodyContent(
+      "<div style='font: 10px/10px Ahem;' contenteditable>"
+      "test<br><br></div>");
+  Element* target = GetDocument().QuerySelector("div");
+  rendered_position_test::FocusAndSelect(this, target, *target);
+  const CompositedSelection& composited_selection =
+      RenderedPosition::ComputeCompositedSelection(Selection());
+  EXPECT_EQ(composited_selection.start.edge_top_in_layer,
+            FloatPoint(8.0f, 8.0f));
+  EXPECT_EQ(composited_selection.start.edge_bottom_in_layer,
+            FloatPoint(8.0f, 18.0f));
+  EXPECT_EQ(composited_selection.end.edge_top_in_layer,
+            FloatPoint(8.0f, 18.0f));
+  EXPECT_EQ(composited_selection.end.edge_bottom_in_layer,
+            FloatPoint(8.0f, 28.0f));
+}
+
+// crbug.com/807930
+TEST_P(RenderedPositionTest, TextAreaLinebreak) {
+  rendered_position_test::SetUpinternal(this);
+  LoadAhem();
+  SetBodyContent(
+      "<textarea style='font: 10px/10px Ahem;'>"
+      "test\n</textarea>");
+  TextControlElement* target =
+      ToTextControl(GetDocument().QuerySelector("textarea"));
+  rendered_position_test::FocusAndSelect(this, target,
+                                         *target->InnerEditorElement());
+  const CompositedSelection& composited_selection =
+      RenderedPosition::ComputeCompositedSelection(Selection());
+  EXPECT_EQ(composited_selection.start.edge_top_in_layer,
+            FloatPoint(11.0f, 11.0f));
+  EXPECT_EQ(composited_selection.start.edge_bottom_in_layer,
+            FloatPoint(11.0f, 21.0f));
+  EXPECT_EQ(composited_selection.end.edge_top_in_layer,
+            FloatPoint(11.0f, 21.0f));
+  EXPECT_EQ(composited_selection.end.edge_bottom_in_layer,
+            FloatPoint(11.0f, 31.0f));
+}
+
 }  // namespace blink