Implement fallback to element anchor

If we parse a TextFragmentAnchor but don't find a match, we should
fallback to matching the element anchor, if there is one.

Example:
https://spaceplace.nasa.gov/blue-sky/en/#similar##targetText=notamatch

Bug: 992522
Change-Id: Ie6dd2a4f5b35a612d563a3acb81392df756ce1d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1746210
Commit-Queue: Nick Burris <nburris@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690845}

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

@@ -15,6 +15,8 @@
 #include "third_party/blink/renderer/core/frame/local_frame.h"
 #include "third_party/blink/renderer/core/layout/layout_object.h"
 #include "third_party/blink/renderer/core/loader/document_loader.h"
+#include "third_party/blink/renderer/core/page/chrome_client.h"
+#include "third_party/blink/renderer/core/page/page.h"
 #include "third_party/blink/renderer/core/page/scrolling/text_fragment_selector.h"
 #include "third_party/blink/renderer/core/scroll/scroll_alignment.h"
 #include "third_party/blink/renderer/core/scroll/scrollable_area.h"
@@ -103,7 +105,8 @@ TextFragmentAnchor* TextFragmentAnchor::TryCreate(
   if (!ParseTargetTextIdentifier(url.FragmentIdentifier(), &selectors))
     return nullptr;
 
-  return MakeGarbageCollected<TextFragmentAnchor>(selectors, frame);
+  return MakeGarbageCollected<TextFragmentAnchor>(
+      selectors, frame, TextFragmentFormat::PlainFragment);
 }
 
 TextFragmentAnchor* TextFragmentAnchor::TryCreateFragmentDirective(
@@ -137,13 +140,16 @@ TextFragmentAnchor* TextFragmentAnchor::TryCreateFragmentDirective(
   stripped_url.SetFragmentIdentifier(stripped_fragment);
   frame.GetDocument()->SetURL(std::move(stripped_url));
 
-  return MakeGarbageCollected<TextFragmentAnchor>(selectors, frame);
+  return MakeGarbageCollected<TextFragmentAnchor>(
+      selectors, frame, TextFragmentFormat::FragmentDirective);
 }
 
 TextFragmentAnchor::TextFragmentAnchor(
     const Vector<TextFragmentSelector>& text_fragment_selectors,
-    LocalFrame& frame)
+    LocalFrame& frame,
+    const TextFragmentFormat fragment_format)
     : frame_(&frame),
+      fragment_format_(fragment_format),
       metrics_(MakeGarbageCollected<TextFragmentAnchorMetrics>(
           frame_->GetDocument())) {
   DCHECK(!text_fragment_selectors.IsEmpty());
@@ -157,6 +163,13 @@ TextFragmentAnchor::TextFragmentAnchor(
 }
 
 bool TextFragmentAnchor::Invoke() {
+  if (element_fragment_anchor_) {
+    DCHECK(search_finished_);
+    // We need to keep this TextFragmentAnchor alive if we're proxying an
+    // element fragment anchor.
+    return true;
+  }
+
   if (search_finished_)
     return false;
 
@@ -178,12 +191,12 @@ bool TextFragmentAnchor::Invoke() {
       finder.FindMatch(*frame_->GetDocument());
   }
 
-  search_finished_ = frame_->GetDocument()->IsLoadCompleted();
+  if (frame_->GetDocument()->IsLoadCompleted())
+    DidFinishSearch();
 
-  if (search_finished_)
-    metrics_->ReportMetrics();
-
-  return !search_finished_;
+  // We need another Invoke if we're not finished searching or if we're proxying
+  // an element fragment anchor.
+  return !search_finished_ || element_fragment_anchor_;
 }
 
 void TextFragmentAnchor::Installed() {}
@@ -195,21 +208,27 @@ void TextFragmentAnchor::DidScroll(ScrollType type) {
   user_scrolled_ = true;
 }
 
-void TextFragmentAnchor::PerformPreRafActions() {}
+void TextFragmentAnchor::PerformPreRafActions() {
+  if (element_fragment_anchor_) {
+    element_fragment_anchor_->Installed();
+    element_fragment_anchor_->Invoke();
+    element_fragment_anchor_->PerformPreRafActions();
+    element_fragment_anchor_ = nullptr;
+  }
+}
 
 void TextFragmentAnchor::DidCompleteLoad() {
   if (search_finished_)
     return;
 
   // If there is a pending layout we'll finish the search from Invoke.
-  if (!frame_->View()->NeedsLayout()) {
-    metrics_->ReportMetrics();
-    search_finished_ = true;
-  }
+  if (!frame_->View()->NeedsLayout())
+    DidFinishSearch();
 }
 
 void TextFragmentAnchor::Trace(blink::Visitor* visitor) {
   visitor->Trace(frame_);
+  visitor->Trace(element_fragment_anchor_);
   visitor->Trace(metrics_);
   FragmentAnchor::Trace(visitor);
 }
@@ -231,6 +250,7 @@ void TextFragmentAnchor::DidFindMatch(const EphemeralRangeInFlatTree& range) {
   }
 
   metrics_->DidFindMatch(PlainText(range));
+  did_find_match_ = true;
 
   if (first_match_needs_scroll_) {
     first_match_needs_scroll_ = false;
@@ -280,4 +300,23 @@ void TextFragmentAnchor::DidFindAmbiguousMatch() {
   metrics_->DidFindAmbiguousMatch();
 }
 
+void TextFragmentAnchor::DidFinishSearch() {
+  DCHECK(!search_finished_);
+  search_finished_ = true;
+
+  metrics_->ReportMetrics();
+
+  if (!did_find_match_ &&
+      fragment_format_ == TextFragmentFormat::FragmentDirective) {
+    DCHECK(!element_fragment_anchor_);
+    element_fragment_anchor_ =
+        ElementFragmentAnchor::TryCreate(frame_->GetDocument()->Url(), *frame_);
+    if (element_fragment_anchor_) {
+      // Schedule a frame so we can invoke the element anchor in
+      // PerformPreRafActions.
+      frame_->GetPage()->GetChromeClient().ScheduleAnimation(frame_->View());
+    }
+  }
+}
+
 }  // namespace blink

third_party/blink/renderer/core/page/scrolling/text_fragment_anchor.h

@@ -7,6 +7,7 @@
 
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/editing/forward.h"
+#include "third_party/blink/renderer/core/page/scrolling/element_fragment_anchor.h"
 #include "third_party/blink/renderer/core/page/scrolling/fragment_anchor.h"
 #include "third_party/blink/renderer/core/page/scrolling/text_fragment_anchor_metrics.h"
 #include "third_party/blink/renderer/core/page/scrolling/text_fragment_finder.h"
@@ -19,6 +20,8 @@ namespace blink {
 class LocalFrame;
 class KURL;
 
+enum class TextFragmentFormat { PlainFragment, FragmentDirective };
+
 class CORE_EXPORT TextFragmentAnchor final : public FragmentAnchor,
                                              public TextFragmentFinder::Client {
  public:
@@ -33,7 +36,8 @@ class CORE_EXPORT TextFragmentAnchor final : public FragmentAnchor,
 
   TextFragmentAnchor(
       const Vector<TextFragmentSelector>& text_fragment_selectors,
-      LocalFrame& frame);
+      LocalFrame& frame,
+      const TextFragmentFormat fragment_format);
   ~TextFragmentAnchor() override = default;
 
   bool Invoke() override;
@@ -53,6 +57,10 @@ class CORE_EXPORT TextFragmentAnchor final : public FragmentAnchor,
   void DidFindAmbiguousMatch() override;
 
  private:
+  // Called when the search is finished. Reports metrics and activates the
+  // element fragment anchor if we didn't find a match.
+  void DidFinishSearch();
+
   Vector<TextFragmentFinder> text_fragment_finders_;
 
   Member<LocalFrame> frame_;
@@ -66,6 +74,16 @@ class CORE_EXPORT TextFragmentAnchor final : public FragmentAnchor,
   // Whether we successfully scrolled into view a match at least once, used for
   // metrics reporting.
   bool did_scroll_into_view_ = false;
+  // Whether we found a match. Used to determine if we should activate the
+  // element fragment anchor at the end of searching.
+  bool did_find_match_ = false;
+  // Whether the text fragment anchor is specified as a regular URL fragment or
+  // a fragment directive. Used to determine if we should activate the element
+  // fragment anchor in the case where we don't find a match.
+  const TextFragmentFormat fragment_format_;
+  // If the text fragment anchor is defined as a fragment directive and we don't
+  // find a match, we fall back to the element anchor if it is present.
+  Member<ElementFragmentAnchor> element_fragment_anchor_;
 
   Member<TextFragmentAnchorMetrics> metrics_;
 

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

@@ -169,7 +169,7 @@ TEST_F(TextFragmentAnchorTest, IdFragmentTakesPrecedence) {
       }
     </style>
     <p id="text">This is a test page</p>
-    <div id="targetText=test"></div>
+    <div id="targetText=test">Some text</div>
   )HTML");
   Compositor().BeginFrame();
 
@@ -191,7 +191,7 @@ TEST_F(TextFragmentAnchorTest, MultipleMatches) {
     <!DOCTYPE html>
     <style>
       body {
-        height: 1200px;
+        height: 2200px;
       }
       #first {
         position: absolute;
@@ -262,7 +262,7 @@ TEST_F(TextFragmentAnchorTest, MultipleTextFragments) {
     <!DOCTYPE html>
     <style>
       body {
-        height: 1200px;
+        height: 2200px;
       }
       #first {
         position: absolute;
@@ -299,7 +299,7 @@ TEST_F(TextFragmentAnchorTest, FirstTextFragmentNotFound) {
     <!DOCTYPE html>
     <style>
       body {
-        height: 1200px;
+        height: 2200px;
       }
       #first {
         position: absolute;
@@ -1146,9 +1146,9 @@ TEST_F(TextFragmentAnchorTest, DoubleHashSyntax) {
   EXPECT_EQ(GetDocument().Url(), "https://example.com/test.html#");
 }
 
-// Test that the ##targetText fragment directive is stripped from the URL when
-// there's also non-directive fragment contents.
-TEST_F(TextFragmentAnchorTest, DoubleHashStrippedWithRemainingFragment) {
+// Test that the ##targetText fragment directive is scrolled into view and is
+// stripped from the URL when there's also a valid element fragment.
+TEST_F(TextFragmentAnchorTest, DoubleHashWithElementFragment) {
   SimRequest request("https://example.com/test.html#element##targetText=test",
                      "text/html");
   LoadURL("https://example.com/test.html#element##targetText=test");
@@ -1156,7 +1156,7 @@ TEST_F(TextFragmentAnchorTest, DoubleHashStrippedWithRemainingFragment) {
     <!DOCTYPE html>
     <style>
       body {
-        height: 1200px;
+        height: 2200px;
       }
       #text {
         position: absolute;
@@ -1168,7 +1168,7 @@ TEST_F(TextFragmentAnchorTest, DoubleHashStrippedWithRemainingFragment) {
       }
     </style>
     <p id="text">This is a test page</p>
-    <div id="element"></div>
+    <div id="element">Some text</div>
   )HTML");
   Compositor().BeginFrame();
 
@@ -1204,7 +1204,7 @@ TEST_F(TextFragmentAnchorTest, IdFragmentWithDoubleHash) {
       }
     </style>
     <p id="element">This is a test page</p>
-    <div id="element##id"></div>
+    <div id="element##id">Some text</div>
   )HTML");
   Compositor().BeginFrame();
 
@@ -1280,6 +1280,45 @@ TEST_F(TextFragmentAnchorTest, CSSTextTransform) {
   EXPECT_EQ(1u, GetDocument().Markers().Markers().size());
 }
 
+// Test that we scroll the element fragment into view if we don't find a match.
+TEST_F(TextFragmentAnchorTest, NoMatchFoundFallsBackToElementFragment) {
+  SimRequest request("https://example.com/test.html#element##targetText=cats",
+                     "text/html");
+  LoadURL("https://example.com/test.html#element##targetText=cats");
+  request.Complete(R"HTML(
+    <!DOCTYPE html>
+    <style>
+      body {
+        height: 2200px;
+      }
+      #text {
+        position: absolute;
+        top: 1000px;
+      }
+      #element {
+        position: absolute;
+        top: 2000px;
+      }
+    </style>
+    <p>This is a test page</p>
+    <div id="element">Some text</div>
+  )HTML");
+  Compositor().BeginFrame();
+  RunAsyncMatchingTasks();
+
+  // The TextFragmentAnchor needs another frame to invoke the element anchor
+  Compositor().BeginFrame();
+  RunAsyncMatchingTasks();
+
+  EXPECT_EQ(GetDocument().Url(), "https://example.com/test.html#element");
+
+  Element& p = *GetDocument().getElementById("element");
+
+  EXPECT_TRUE(ViewportRect().Contains(BoundingRectInFrame(p)))
+      << "<p> Element wasn't scrolled into view, viewport's scroll offset: "
+      << LayoutViewport()->GetScrollOffset().ToString();
+}
+
 }  // namespace
 
 }  // namespace blink

third_party/blink/web_tests/external/wpt/scroll-to-text-fragment/scroll-to-text-fragment-target.html

@@ -16,7 +16,12 @@ function checkScroll() {
     position: absolute;
     top: 3000px;
   }
+  #element {
+    position: absolute;
+    top: 2000px;
+  }
 </style>
 <body onload="checkScroll()">
+  <div id="element">Element</div>
   <p id="text">This is a test page</p>
 </body>

third_party/blink/web_tests/external/wpt/scroll-to-text-fragment/scroll-to-text-fragment.html

@@ -18,6 +18,7 @@ let test_cases = [
   { fragment: '##targetText=this&targetText=test,page', expect_scroll: true },
   { fragment: '#pagestate##targetText=test', expect_scroll: true },
   { fragment: '#pagestate##targetText=nomatch', expect_scroll: false },
+  { fragment: '#element##targetText=nomatch', expect_scroll: true },
 ];
 
 for (const test_case of test_cases) {