Highlight text fragment directives on page reload

Fix the behavior where fragment anchors were only kept alive if a scroll
was needed, as is the behavior of element fragment anchors. This caused
text fragment anchors not to be kept alive and we didn't highlight on
reload.

Note that *sometimes* we would highlight on reload, due to a race
condition with fragment anchors, where we process the fragment before
restoring scroll position (which is where should_scroll becomes false),
see crbug/839292. This change makes it so we highlight on reload
independent of whether a scroll is needed.

Bug: 932551
Change-Id: I38cc82c4920255d8bcae9f8b55ac80f24d1465da
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1864714
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706847}

third_party/blink/renderer/core/frame/local_frame_view.cc

@@ -1362,10 +1362,10 @@ void LocalFrameView::ProcessUrlFragment(const KURL& url,
                                         bool should_scroll) {
   // We want to create the anchor even if we don't need to scroll. This ensures
   // all the side effects like setting CSS :target are correctly set.
-  FragmentAnchor* anchor =
-      FragmentAnchor::TryCreate(url, *frame_, same_document_navigation);
+  FragmentAnchor* anchor = FragmentAnchor::TryCreate(
+      url, *frame_, same_document_navigation, should_scroll);
 
-  if (anchor && should_scroll) {
+  if (anchor) {
     fragment_anchor_ = anchor;
     fragment_anchor_->Installed();
 

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

@@ -34,7 +34,8 @@ Node* FindAnchorFromFragment(const String& fragment, Document& doc) {
 }  // namespace
 
 ElementFragmentAnchor* ElementFragmentAnchor::TryCreate(const KURL& url,
-                                                        LocalFrame& frame) {
+                                                        LocalFrame& frame,
+                                                        bool should_scroll) {
   DCHECK(frame.GetDocument());
   Document& doc = *frame.GetDocument();
 
@@ -83,6 +84,10 @@ ElementFragmentAnchor* ElementFragmentAnchor::TryCreate(const KURL& url,
   if (!anchor_node)
     return nullptr;
 
+  // Element fragment anchors only need to be kept alive if they need scrolling.
+  if (!should_scroll)
+    return nullptr;
+
   return MakeGarbageCollected<ElementFragmentAnchor>(*anchor_node, frame);
 }
 

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

@@ -29,7 +29,9 @@ class CORE_EXPORT ElementFragmentAnchor final : public FragmentAnchor {
   // Parses the URL fragment and, if possible, creates and returns a fragment
   // based on an Element in the page. Returns nullptr otherwise. Produces side
   // effects related to fragment targeting in the page in either case.
-  static ElementFragmentAnchor* TryCreate(const KURL& url, LocalFrame& frame);
+  static ElementFragmentAnchor* TryCreate(const KURL& url,
+                                          LocalFrame& frame,
+                                          bool should_scroll);
 
   ElementFragmentAnchor(Node& anchor_node, LocalFrame& frame);
   ~ElementFragmentAnchor() override = default;

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

@@ -15,7 +15,8 @@ namespace blink {
 
 FragmentAnchor* FragmentAnchor::TryCreate(const KURL& url,
                                           LocalFrame& frame,
-                                          bool same_document_navigation) {
+                                          bool same_document_navigation,
+                                          bool should_scroll) {
   DCHECK(frame.GetDocument());
 
   FragmentAnchor* anchor = nullptr;
@@ -28,7 +29,7 @@ FragmentAnchor* FragmentAnchor::TryCreate(const KURL& url,
   bool text_fragment_anchor_created = false;
   if (text_fragment_identifiers_enabled) {
     anchor = TextFragmentAnchor::TryCreateFragmentDirective(
-        url, frame, same_document_navigation);
+        url, frame, same_document_navigation, should_scroll);
     text_fragment_anchor_created = anchor;
   }
 
@@ -60,7 +61,7 @@ FragmentAnchor* FragmentAnchor::TryCreate(const KURL& url,
 
   bool element_id_anchor_found = false;
   if (!anchor) {
-    anchor = ElementFragmentAnchor::TryCreate(url, frame);
+    anchor = ElementFragmentAnchor::TryCreate(url, frame, should_scroll);
     element_id_anchor_found = anchor;
   }
 

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

@@ -32,7 +32,8 @@ class CORE_EXPORT FragmentAnchor : public GarbageCollected<FragmentAnchor> {
   // will be performed, for example, setting/clearing :target and svgView().
   static FragmentAnchor* TryCreate(const KURL& url,
                                    LocalFrame& frame,
-                                   bool same_document_navigation);
+                                   bool same_document_navigation,
+                                   bool should_scroll);
 
   FragmentAnchor() = default;
   virtual ~FragmentAnchor() = default;

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

@@ -77,7 +77,8 @@ bool CheckSecurityRestrictions(LocalFrame& frame,
 TextFragmentAnchor* TextFragmentAnchor::TryCreateFragmentDirective(
     const KURL& url,
     LocalFrame& frame,
-    bool same_document_navigation) {
+    bool same_document_navigation,
+    bool should_scroll) {
   DCHECK(RuntimeEnabledFeatures::TextFragmentIdentifiersEnabled(
       frame.GetDocument()));
 
@@ -96,13 +97,16 @@ TextFragmentAnchor* TextFragmentAnchor::TryCreateFragmentDirective(
     return nullptr;
   }
 
-  return MakeGarbageCollected<TextFragmentAnchor>(selectors, frame);
+  return MakeGarbageCollected<TextFragmentAnchor>(selectors, frame,
+                                                  should_scroll);
 }
 
 TextFragmentAnchor::TextFragmentAnchor(
     const Vector<TextFragmentSelector>& text_fragment_selectors,
-    LocalFrame& frame)
+    LocalFrame& frame,
+    bool should_scroll)
     : frame_(&frame),
+      should_scroll_(should_scroll),
       metrics_(MakeGarbageCollected<TextFragmentAnchorMetrics>(
           frame_->GetDocument())) {
   DCHECK(!text_fragment_selectors.IsEmpty());
@@ -134,7 +138,7 @@ bool TextFragmentAnchor::Invoke() {
   if (user_scrolled_ && !did_scroll_into_view_)
     metrics_->ScrollCancelled();
 
-  first_match_needs_scroll_ = !user_scrolled_;
+  first_match_needs_scroll_ = should_scroll_ && !user_scrolled_;
 
   {
     // FindMatch might cause scrolling and set user_scrolled_ so reset it when
@@ -263,8 +267,8 @@ void TextFragmentAnchor::DidFinishSearch() {
     dismissed_ = true;
 
     DCHECK(!element_fragment_anchor_);
-    element_fragment_anchor_ =
-        ElementFragmentAnchor::TryCreate(frame_->GetDocument()->Url(), *frame_);
+    element_fragment_anchor_ = ElementFragmentAnchor::TryCreate(
+        frame_->GetDocument()->Url(), *frame_, should_scroll_);
     if (element_fragment_anchor_) {
       // Schedule a frame so we can invoke the element anchor in
       // PerformPreRafActions.
@@ -284,7 +288,7 @@ bool TextFragmentAnchor::Dismiss() {
   if (!did_find_match_ || dismissed_)
     return true;
 
-  DCHECK(did_scroll_into_view_ || user_scrolled_);
+  DCHECK(!should_scroll_ || did_scroll_into_view_ || user_scrolled_);
 
   frame_->GetDocument()->Markers().RemoveMarkersOfTypes(
       DocumentMarker::MarkerTypes::TextFragment());

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

@@ -36,11 +36,13 @@ class CORE_EXPORT TextFragmentAnchor final : public FragmentAnchor,
   static TextFragmentAnchor* TryCreateFragmentDirective(
       const KURL& url,
       LocalFrame& frame,
-      bool same_document_navigation);
+      bool same_document_navigation,
+      bool should_scroll);
 
   TextFragmentAnchor(
       const Vector<TextFragmentSelector>& text_fragment_selectors,
-      LocalFrame& frame);
+      LocalFrame& frame,
+      bool should_scroll);
   ~TextFragmentAnchor() override = default;
 
   bool Invoke() override;
@@ -89,6 +91,10 @@ class CORE_EXPORT TextFragmentAnchor final : public FragmentAnchor,
   // Whether the text fragment anchor has been dismissed yet. This should be
   // kept alive until dismissed so we can remove text highlighting.
   bool dismissed_ = false;
+  // Whether we should scroll the anchor into view. This will be false for
+  // history navigations and reloads, where we want to restore the highlight but
+  // not scroll into view again.
+  bool should_scroll_ = false;
 
   Member<TextFragmentAnchorMetrics> metrics_;
 

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

@@ -12,6 +12,7 @@
 #include "third_party/blink/renderer/core/frame/local_frame.h"
 #include "third_party/blink/renderer/core/frame/local_frame_view.h"
 #include "third_party/blink/renderer/core/frame/location.h"
+#include "third_party/blink/renderer/core/frame/web_local_frame_impl.h"
 #include "third_party/blink/renderer/core/html/html_element.h"
 #include "third_party/blink/renderer/core/html/html_frame_owner_element.h"
 #include "third_party/blink/renderer/core/input/event_handler.h"
@@ -1545,6 +1546,46 @@ TEST_F(TextFragmentAnchorTest, TextDirectiveInSvg) {
   EXPECT_EQ(1u, GetDocument().Markers().Markers().size());
 }
 
+// Ensure we restore the text highlight on page reload
+TEST_F(TextFragmentAnchorTest, HighlightOnReload) {
+  SimRequest request("https://example.com/test.html#:~:text=test", "text/html");
+  LoadURL("https://example.com/test.html#:~:text=test");
+  const String& html = R"HTML(
+    <!DOCTYPE html>
+    <style>
+      body {
+        height: 1200px;
+      }
+      p {
+        position: absolute;
+        top: 1000px;
+      }
+    </style>
+    <p id="text">This is a test page</p>
+  )HTML";
+  request.Complete(html);
+
+  Compositor().BeginFrame();
+  RunAsyncMatchingTasks();
+
+  EXPECT_EQ(1u, GetDocument().Markers().Markers().size());
+
+  // Tap to dismiss the highlight.
+  SimulateClick(10, 10);
+  EXPECT_EQ(0u, GetDocument().Markers().Markers().size());
+
+  // Reload the page and expect the highlight to be restored.
+  SimRequest reload_request("https://example.com/test.html#:~:text=test",
+                            "text/html");
+  MainFrame().StartReload(WebFrameLoadType::kReload);
+  reload_request.Complete(html);
+
+  Compositor().BeginFrame();
+  RunAsyncMatchingTasks();
+
+  EXPECT_EQ(1u, GetDocument().Markers().Markers().size());
+}
+
 }  // namespace
 
 }  // namespace blink