Fix for cases when previous text is incorrectly skipped.

Fix for cases when previous text is incorrectly skipped due to
PreviousSkippingChildren.

For following example:

<div id=1>
 <div id=2></div>
</div>
<div id=3></div>

PreviousSkippingChildren(div3) = div1
but
Previous(div3) = div2

that's why Previous should be used when we are searching for a previous
text.

Bug: 1154308
Change-Id: I043bf8f5a7db4e361afbe34d2d854566e5bacd87
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2565812
Commit-Queue: Gayane Petrosyan <gayane@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Reviewed-by: Mason Freed <masonfreed@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833381}

third_party/blink/renderer/core/editing/finder/find_buffer.cc

@@ -85,7 +85,7 @@ Node* GetVisibleTextNode(Node& start_node) {
   while (Node* ancestor = GetNonSearchableAncestor(*node)) {
     if (!ancestor)
       return nullptr;
-    node = Direction::NextSkippingChildren(*ancestor);
+    node = Direction::NextSkippingSubtree(*ancestor);
     if (!node)
       return nullptr;
   }
@@ -95,7 +95,7 @@ Node* GetVisibleTextNode(Node& start_node) {
     if (ShouldIgnoreContents(*node) ||
         (style && style->Display() == EDisplay::kNone)) {
       // This element and its descendants are not visible, skip it.
-      node = Direction::NextSkippingChildren(*node);
+      node = Direction::NextSkippingSubtree(*node);
       continue;
     }
     if (style && style->Visibility() == EVisibility::kVisible &&
@@ -201,7 +201,7 @@ Node* FindBuffer::ForwardVisibleTextNode(Node& start_node) {
     static Node* Next(const Node& node) {
       return FlatTreeTraversal::Next(node);
     }
-    static Node* NextSkippingChildren(const Node& node) {
+    static Node* NextSkippingSubtree(const Node& node) {
       return FlatTreeTraversal::NextSkippingChildren(node);
     }
   };
@@ -213,8 +213,10 @@ Node* FindBuffer::BackwardVisibleTextNode(Node& start_node) {
     static Node* Next(const Node& node) {
       return FlatTreeTraversal::Previous(node);
     }
-    static Node* NextSkippingChildren(const Node& node) {
-      return FlatTreeTraversal::PreviousSkippingChildren(node);
+    static Node* NextSkippingSubtree(const Node& node) {
+      // Unlike |NextSkippingChildren|, |Previous| already skips given nodes
+      // subtree.
+      return FlatTreeTraversal::Previous(node);
     }
   };
   return GetVisibleTextNode<BackwardDirection>(start_node);

third_party/blink/renderer/core/page/scrolling/text_fragment_selector_generator.cc

@@ -224,8 +224,10 @@ void TextFragmentSelectorGenerator::AdjustSelection() {
   int corrected_end_offset =
       ephemeral_range.EndPosition().ComputeOffsetInContainerNode();
   if (IsFirstVisiblePosition(corrected_end, corrected_end_offset)) {
+    // Here, |Previous()| already skips the children of the given node,
+    // because we're doing pre-order traversal.
     corrected_end = BackwardNonEmptyVisibleTextNode(
-        FlatTreeTraversal::PreviousSkippingChildren(*corrected_end));
+        FlatTreeTraversal::Previous(*corrected_end));
     if (corrected_end)
       corrected_end_offset = corrected_end->textContent().length();
   } else {
@@ -563,7 +565,7 @@ String TextFragmentSelectorGenerator::GetPreviousTextBlock(
   // use the preceding visible node for the suffix.
   if (IsFirstVisiblePosition(prefix_end, prefix_end_offset)) {
     prefix_end = BackwardNonEmptyVisibleTextNode(
-        FlatTreeTraversal::PreviousSkippingChildren(*prefix_end));
+        FlatTreeTraversal::Previous(*prefix_end));
 
     if (!prefix_end)
       return "";

third_party/blink/renderer/core/page/scrolling/text_fragment_selector_generator_test.cc

@@ -872,6 +872,76 @@ TEST_F(TextFragmentSelectorGeneratorTest, EndIsStartofNextBlock) {
   VerifySelector(start, end, "First%20paragraph,-Second");
 }
 
+// Check the case when parent of selection start is a sibling of a node where
+// selection ends.
+//   :root
+//  /      \
+// div      p
+//  |       |
+//  p      "]Second"
+//  |
+// "[First..."
+// Where [] indicate selection. In this case, when the selection is adjusted, we
+// want to ensure it correctly traverses the tree back to the previous text node
+// and not to the <div>(sibling of second <p>).
+//
+// crbug.com/1154308 - checks the use of Previous instead of
+// PreviousSkippingChildren in TextFragmentSelectorGenerator::AdjustSelection
+TEST_F(TextFragmentSelectorGeneratorTest, PrevNodeIsSiblingsChild) {
+  SimRequest request("https://example.com/test.html", "text/html");
+  LoadURL("https://example.com/test.html");
+
+  // HTML is intentionally not formatted. Adding new lines and itendation
+  // creates empty text nodes which changes the dom tree.
+  request.Complete(R"HTML(
+    <!DOCTYPE html>
+  <div><p id='start'>First paragraph</p></div><p id='end'>Second paragraph</p>
+  )HTML");
+  GetDocument().UpdateStyleAndLayoutTree();
+  Node* first_paragraph = GetDocument().getElementById("start")->firstChild();
+  Node* second_paragraph = GetDocument().getElementById("end");
+  const auto& start = Position(first_paragraph, 0);
+  const auto& end = Position(second_paragraph, 0);
+  ASSERT_EQ("First paragraph\n\n", PlainText(EphemeralRange(start, end)));
+
+  VerifySelector(start, end, "First%20paragraph,-Second");
+}
+
+// Check the case when parent of selection start is a sibling of a node where
+// selection ends.
+//    :root
+//  /    |     \
+// div  div     p
+//  |    |       \
+//  p   "test"   "]Second"
+//  |
+//"[First..."
+//
+// Where [] indicate selection. In this case, when the selection is adjusted, we
+// want to ensure it correctly traverses the tree back to the previous text by
+// correctly skipping the invisible div but not skipping the second <p>.
+//
+// crbug.com/1154308 - checks the use of Previous instead of
+// PreviousSkippingChildren in FindBuffer::BackwardVisibleTextNode
+TEST_F(TextFragmentSelectorGeneratorTest, PrevPrevNodeIsSiblingsChild) {
+  SimRequest request("https://example.com/test.html", "text/html");
+  LoadURL("https://example.com/test.html");
+  // HTML is intentionally not formatted. Adding new lines and itendation
+  // creates empty text nodes which changes the dom tree.
+  request.Complete(R"HTML(
+    <!DOCTYPE html>
+  <div><p id='start'>First paragraph</p></div><div style='display:none'>test</div><p id='end'>Second paragraph</p>
+  )HTML");
+  GetDocument().UpdateStyleAndLayoutTree();
+  Node* first_paragraph = GetDocument().getElementById("start")->firstChild();
+  Node* second_paragraph = GetDocument().getElementById("end");
+  const auto& start = Position(first_paragraph, 0);
+  const auto& end = Position(second_paragraph, 0);
+  ASSERT_EQ("First paragraph\n\n", PlainText(EphemeralRange(start, end)));
+
+  VerifySelector(start, end, "First%20paragraph,-Second");
+}
+
 // Checks that for short selection that have nested block element range selector
 // is used.
 TEST_F(TextFragmentSelectorGeneratorTest, RangeSelector_SameNode_Interrupted) {